Fix C aliasing issue in Python/dtoa.c to use strict aliasing on Clang 4.0

[vstinner]

BPO	30124
Nosy	@mdickinson, @vstinner, @ericvsmith, @benjaminp, @serhiy-storchaka, @DimitryAndric
PRs	bpo-30104: Compile dtoa.c without strict aliasing #1340

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2017-05-02.14:11:13.299>
created_at = <Date 2017-04-21.09:28:55.758>
labels = ['build', '3.7', 'performance']
title = 'Fix C aliasing issue in Python/dtoa.c to use strict aliasing on Clang 4.0'
updated_at = <Date 2017-05-02.14:11:13.298>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2017-05-02.14:11:13.298>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2017-05-02.14:11:13.299>
closer = 'vstinner'
components = ['Build']
creation = <Date 2017-04-21.09:28:55.758>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 30124
keywords = []
message_count = 25.0
messages = ['292018', '292020', '292031', '292034', '292037', '292038', '292039', '292040', '292047', '292048', '292054', '292203', '292204', '292211', '292223', '292224', '292242', '292243', '292280', '292281', '292412', '292415', '292418', '292525', '292761']
nosy_count = 6.0
nosy_names = ['mark.dickinson', 'vstinner', 'eric.smith', 'benjamin.peterson', 'serhiy.storchaka', 'dim']
pr_nums = ['1340']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue30124'
versions = ['Python 3.7']

[vstinner]

My change 28205b2 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

[vstinner]

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

[mdickinson]

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.

[mdickinson]

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 ...

[vstinner]

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

[mdickinson]

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.

[mdickinson]

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.

[vstinner]

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).

[mdickinson]

(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.

[mdickinson]

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.

[ericvsmith]

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.

[mdickinson]

@Haypo: for your strict-aliasing notes collection, I highly recommend the recent paper "Detecting Strict Aliasing Violations" by P. Cuoq et. al.

http://trust-in-soft.com/wp-content/uploads/2017/01/vmcai.pdf

[serhiy-storchaka]

Could we use Clang specific pragma in dtoa.c rather than a compiler option?

[vstinner]

Could we use Clang specific pragma in dtoa.c rather than a compiler option?

If we decide to go for the -fno-strict-aliasing only for dtoa.c, I suggest to use it also for GCC. GCC might decide to also optimize dtoa.c further in the future. I don't think that the flag has a major impact on performance if it's restricted to dtoa.c, and it would simplify the build system to only have "per compiler" flags. (Ex: Does ICC also "miscompile" dtoa?)

FreeBSD uses the following syntax to only add the flag on a specific C file. Does it work with GNU and BSD make? (is it a "portable" syntax?)

CFLAGS.gdtoa_${src}+=-fno-strict-aliasing

See https://svnweb.freebsd.org/changeset/base/313706 (linked from http://bugs.python.org/issue30104#msg292001).

[DimitryAndric]

Note that gcc has documented accessing union members in this way as an "implementation defined" feature:
https://gcc.gnu.org/onlinedocs/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html#Structures-unions-enumerations-and-bit-fields-implementation

They specifically mention an example that is very similar to the dtoa pattern as being allowed under *their* implementation, here:
https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#Type-punning

However, whether that means that it is generally allowed by all standards is unfortunately still rather vague. I have seen more than one complaint that the standards committees should make this very explicit, instead of weaseling it into post-release footnotes.

That said, I'd like to reply to some other posters here.

Eric V. Smith writes:

At most, I think Mark's idea to use -fno-strict-aliasing only on dtoa.c and nowhere else would be the better approach.

Indeed, this is exactly the solution I chose for FreeBSD. Just the one file that needs it is compiled with -fno-strict-aliasing. The caveat is that this might not work when link-time optimization is in effect.

Serhiy Storchaka writes:

Could we use Clang specific pragma in dtoa.c rather than a compiler option?

Unfortunately, as far as I know, clang still does not support function-level optimization pragmas. Maybe it was implemented recently, but then you would still have to have a workaround for older versions.

STINNER Victor writes:

If we decide to go for the -fno-strict-aliasing only for dtoa.c, I suggest to use it also for GCC. GCC might decide to also optimize dtoa.c further in the future.

Theoretically they could, but as I pointed out above, they explicitly document this as a feature of their union implementation. I estimate the probability of them dropping this in the future to be near zero.

I don't think that the flag has a major impact on performance if it's restricted to dtoa.c, and it would simplify the build system to only have "per compiler" flags. (Ex: Does ICC also "miscompile" dtoa?)

Disabling strict aliasing for just that file, or even just the one function it applies to (by splitting it off to a separate file, for instance) should not result in different assembly output, unless the compiler is very old and/or dumb.

FreeBSD uses the following syntax to only add the flag on a specific C file. Does it work with GNU and BSD make? (is it a "portable" syntax?)

CFLAGS.gdtoa_${src}+=-fno-strict-aliasing

No, this is specifically a feature of BSD's bsd.sys.mk infrastructure. It's most likely not compatible with GNU make.

[ericvsmith]

Unfortunately, as far as I know, clang still does not support
function-level optimization pragmas. Maybe it was implemented
recently, but then you would still have to have a workaround>
for older versions.

I realize the answer is probably "no", but I'll ask anyway. Do you know if they support whole-module optimization pragmas? That would still be better than mucking with makefiles to get a per-module flag.

[DimitryAndric]

There is a "#pragma clang optimize", but it only has the options "on" or "off", for specific source locations. I guess that would defeat the purpose a little bit. :(

As an experiment, and to show what would be needed (at minimum), I have attempted to make Python/dtoa.c completely aliasing-safe here:

DimitryAndric@29c3f6f

This allowed to remove the -fno-strict-aliasing flag again, and it succeeded all tests, even with clang 5.0.0.

It basically replaces the direct union member accesses with getters and setters, which do the right thing for clang. Note that even though those getters and setters use memcpy(), this is actually completely optimized away in the resulting assembly. (Old compilers might not fare that well, though.)

In any case, while these are mostly mechanical changes, it is still a lot of code churn, and it should really be reviewed by the original maintainer of dtoa, David M. Gay. I have no idea whether he is still active, though...

[vstinner]

As an experiment, and to show what would be needed (at minimum), I have attempted to make Python/dtoa.c completely aliasing-safe here:
DimitryAndric@29c3f6f

Would it be technically possible to completely get ride of the union? For example, it would allow to replace:

set_dval(rv, get_dval(rv) + sulp(rv, bc));
with:
rv += sulp(rv, bc);

Is it possible to replace Big0 and Big1 with Big double?

etc.

[DimitryAndric]

STINNER Victor writes:

Would it be technically possible to completely get ride of the union?

Probably, it's just more work, and it has to be done pretty carefully to avoid regressions. It seems Python already does some exercising of these dtoa functions in its test suite, but ideally you would want to check against upstream's full tests, if those exist.

There are probably many edge cases for a set of tricky functions like this...

[serhiy-storchaka]

It seems to me that the purpose of using unions was avoiding aliasing issues. But something went wrong.

[mdickinson]

It seems Python already does some exercising of these dtoa functions in its test suite, but ideally you would want to check against upstream's full tests, if those exist.

They barely do: Python's tests for dtoa.c are much more comprehensive than the upstream tests. Or at least they were at the time when I was adapting dtoa.c for use in Python and communicating with Gay about issues, and I doubt that that situation has changed.

[vstinner]

Mark Dickinson: "It would also take us even further away from the upstream sources, making it harder to integrate bugfixes from upstream."

We have two main options:

• Use -fno-strict-aliasing on clang (solution currently used), maybe restrict the option to dtoa.c
• Avoid union to avoid any risk of aliasing issue: option experimented by dim

According to Mark, rewriting the code without union is not only more risky but would also be a major shift from upstream. I disagree with it's so risky, there is a risk yes, but we can take our timeto review it and test it on many platforms with the Python extensive test suite. For example, the aliasing issue on clang 4 was catched quicky on our FreeBSD CURRENT buildbot.

But the current blocker point is upstream: Mark doesn't want diverge from upstream, so Mark: can you (or someone else?) please contact "Gay (or other maintainers)" to take a decision with him/them?

[mdickinson]

Victor: I don't think that's necessary. We simply need to add -fno-strict-aliasing for this file.

[vstinner]

I would like to fix FreeBSD CURRENT buildbots of Python 2.7, 3.5 and 3.6, so here my attempt to restrict the -fno-strict-aliasing option to the dtoa.c file: #1340

I chose to add the flag for any C compiler. If you think that a C compiler may not support that option, we can start by only adding the option on clang, and maybe add it on other C compilers if someone complains.

[vstinner]

dtoa.c is now compiled with -fno-string-aliasing (for any clang version, not only clang 4.0) on Python 3.5, 3.6 and 3.7.

It was decided to not touch dtoa.c to not diverge from upstream.

Thanks Dimitry Andric and Mark Dickinson for your reviews and support in this cryptic issue!

Note 1: We consider that Clang 4.0 is wrong, whereas GCC respects the C99 standard for aliasing on unions. But I don't think that it matters much who is wrong or not :-)

Note 2: Python 2.7 already used -fno-strict-aliasing on all .c files because of its design of PyObject structures, fixed in Python 3.