Skip to content

bpo-30104: Compile dtoa.c without strict aliasing#1340

Merged
vstinner merged 1 commit into
python:masterfrom
vstinner:dtoa_aliasing
Apr 28, 2017
Merged

bpo-30104: Compile dtoa.c without strict aliasing#1340
vstinner merged 1 commit into
python:masterfrom
vstinner:dtoa_aliasing

Conversation

@vstinner
Copy link
Copy Markdown
Member

dtoa.c uses union to cast double to unsigned long[2]. clang 4.0 with
-O2 or higher and strict aliasing miscompiles the ratio() function
causing rounding issues. Always disable strict aliasing on dtoa.c to
prevent any risk of bug caused by optimizations.

@vstinner
Copy link
Copy Markdown
Member Author

I tested manually the patch on Linux using clang 4.0: dtoa.c is compiled with -fno-strict-aliasing, test_strtod test pass, and only dtoa.c is compiled with -fno-strict-aliasing so other C optimized are optimized with strict aliasing rules.

@vstinner vstinner requested a review from mdickinson April 28, 2017 11:02
@vstinner
Copy link
Copy Markdown
Member Author

The main question is if the -no-strict-aliasing option is supported by all UNIX and BSD compilers? What about Solaris and HP-UX compilers?

I propose to push the change and fix it later if someone complains. What do you think?

@vstinner
Copy link
Copy Markdown
Member Author

cc @DimitryAndric

@vstinner
Copy link
Copy Markdown
Member Author

I don't know well autotools, so I also tested to compile CPython from a different directory:

cd # $HOME
mkdir build_cpython
cd build_cpython
 ~/prog/python/master/configure CC=/opt/llvm-4.0.0/bin/clang
make
./python -m test -v test_strtod

dtoa.o is written into build_cpython/Python, not in ~/prog/python/master/Python: it works :-)

@vstinner
Copy link
Copy Markdown
Member Author

The main question is if the -no-strict-aliasing option is supported by all UNIX and BSD compilers? What about Solaris and HP-UX compilers?

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. configure is now able to detect correctly the clang compiler.

@DimitryAndric
Copy link
Copy Markdown

Originally, -fstrict-aliasing and -fno-strict-aliasing were gcc specific optimization flags (see https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-fstrict-aliasing). But of course, other compilers that are meant as drop-in replacements for gcc, such as Intel's icc and LLVM's clang, support this flag to, with the same meaning.

However, there are definitely compilers out there which will not support this flag, for example the Microsoft C/C++ compiler. Does this commit affect the flags passed to Microsoft C/C++?

@vstinner
Copy link
Copy Markdown
Member Author

Does this commit affect the flags passed to Microsoft C/C++?

No, we use the PCbuild directory for Microsoft Visual Studio and its C compiler. It's a completely different build system.

However, there are definitely compilers out there which will not support this flag,

Do you suggest to first only define the flag on clang?

@DimitryAndric
Copy link
Copy Markdown

Do you suggest to first only define the flag on clang?

Yes, that should the most minimal way of fixing the original issue, since gcc itself won't need it.

On clang, only compile dtoa.c with -fno-strict-aliasing, use strict
aliasing to compile all other C files.
@vstinner
Copy link
Copy Markdown
Member Author

Yes, that should the most minimal way of fixing the original issue, since gcc itself won't need it.

Ok, done. The new change now only impacts clang and only use -fno-strict-aliasing on dtoa.c to optimize correctly all other C files using strict aliasing, as decided in bpo-30124.

@vstinner
Copy link
Copy Markdown
Member Author

I tested that clang uses -fno-strict-aliasing but only on dtoa.c and gcc never uses -fno-strict-aliasing.

@vstinner vstinner merged commit 826f83f into python:master Apr 28, 2017
@vstinner vstinner deleted the dtoa_aliasing branch April 28, 2017 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants