classification
Title: Fix C aliasing issue in Python/dtoa.c to use strict aliasing on Clang 4.0
Type: performance Stage:
Components: Build Versions: Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, eric.smith, haypo, mark.dickinson
Priority: normal Keywords:

Created on 2017-04-21 09:28 by haypo, last changed 2017-04-21 15:15 by eric.smith.

Messages (11)
msg292018 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-04-21 09:28
My change 28205b203a4742c40080b4a2b4b2dcd800716edc added -fno-strict-aliasing on clang to fix the compilation of Python/dtoa.c on clang 4.0. But it's only a temporary workaround until dtoa.c is fixed to respect C99 strict aliasing.

Strict aliasing allows the compiler to enable more optimization, and so should make Python a little bit faster. It would only fix a regression, before my change Python was already build with strict aliasing 

More info about the issue:

* bpo-30104
* https://bugs.llvm.org//show_bug.cgi?id=31928
msg292020 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-04-21 09:41
I hope that you understand aliasing issues, because I don't undertand them well :-D So I started to create my collection of links on the topic:
https://haypo-notes.readthedocs.io/misc.html#c-aliasing

The most interesting link is:
http://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-aliasing.html
msg292031 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2017-04-21 12:26
dtoa.c *does* (arguably) respect C99+TC3 strict aliasing. The union trick has long been used as a safe and supported way to do this kind of thing, though it appears there's still disagreement about exactly what the standard intends.

I'd strongly prefer not to modify dtoa.c here unless we really have to.
msg292034 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2017-04-21 12:31
In case anyone wants to have a go at interpreting the standard, the most immediately relevant part is the footnote to C99+TC3 6.5.2.3p3:

"""
If the member used to access the contents of a union object is not the same as the member last used to store a value in the object, the appropriate part of the object representation of the value is reinterpreted as an object representation in the new type as described in 6.2.6 (a process sometimes called "type punning"). This might be a trap representation.
"""

Trap representations could potentially cause undefined behaviour, but are highly unlikely to occur in practice for us for this particular case, so I don't think they can possibly serve as the excuse for clang's behaviour.

The "reinterpreted as ..." language here strongly suggests that the union trick should be supported.

OTOH, this is just a footnote, so not normative ...
msg292037 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-04-21 12:42
> I'd strongly prefer not to modify dtoa.c here unless we really have to.

See the clang bug report:
https://bugs.llvm.org//show_bug.cgi?id=31928
msg292038 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2017-04-21 12:46
And the basic rule that bans aliasing in the first place is C99 6.5p7:
"""
An object shall have its stored value accessed only by an lvalue expression that has one of the following types ...
"""
But I'd argue that the "an aggregate or union type" subclause of 6.5p7 covers this case.
msg292039 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2017-04-21 12:48
> See the clang bug report:

Yes, I've read it. And I think the Clang folks are wrong in their interpretation of the standard. And even if they're not, they're going to break a lot of code with this change: the union trick has been widely accepted as a valid way to do things.
msg292040 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-04-21 12:59
> Yes, I've read it. And I think the Clang folks are wrong in their interpretation of the standard. And even if they're not, they're going to break a lot of code with this change: the union trick has been widely accepted as a valid way to do things.

If Clang decides or not to handle union will decide if Python has or has no to change dtoa.c :-) (I would prefer to keep -fstrict-aliasing).
msg292047 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2017-04-21 14:21
> (I would prefer to keep -fstrict-aliasing).

Updating dtoa.c would be a large and error-prone task. It may be simpler to adjust the buildsystem so that we can specify -fno-strict-aliasing just for dtoa.c.
msg292048 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2017-04-21 14:23
> Updating dtoa.c would be a large and error-prone task.

It would also take us even further away from the upstream sources, making it harder to integrate bugfixes from upstream.
msg292054 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2017-04-21 15:15
I agree we shouldn't do anything heroic to "fix" dtoa.c. I'd wait and see if Gay (or other maintainers) will chose an approach if Clang keeps this behavior.

At most, I think Mark's idea to use -fno-strict-aliasing only on dtoa.c and nowhere else would be the better approach.
History
Date User Action Args
2017-04-21 15:15:24eric.smithsetmessages: + msg292054
2017-04-21 14:23:00mark.dickinsonsetmessages: + msg292048
2017-04-21 14:21:42mark.dickinsonsetmessages: + msg292047
2017-04-21 12:59:46hayposetmessages: + msg292040
2017-04-21 12:48:10mark.dickinsonsetmessages: + msg292039
2017-04-21 12:46:54mark.dickinsonsetmessages: + msg292038
2017-04-21 12:42:07hayposetmessages: + msg292037
2017-04-21 12:31:52mark.dickinsonsetnosy: + eric.smith
2017-04-21 12:31:32mark.dickinsonsetmessages: + msg292034
2017-04-21 12:26:13mark.dickinsonsetmessages: + msg292031
2017-04-21 09:41:44hayposetmessages: + msg292020
2017-04-21 09:28:55haypocreate