inline functions in math.h should be static - by Bruce Dawson2

Status : 

 


6
0
Sign in
to vote
ID 3114220 Comments
Status Active Workarounds
Type Bug Repros 0
Opened 12/4/2016 4:41:32 PM
Access Restriction Public

Description

Because math.h defines some inline functions and because these functions are not static it is impossible to safely
use math.h in a binary that uses a mixture of normal and /arch:AVX compiled translation units.

If functions like floorf are not inlined then multiple copies of the function will be compiled and the linker will
choose one. If the linker chooses the one that was compiled by a /arch:AVX translation unit then the binary
is likely to crash on non-AVX capable computers, even if calls to the /arch:AVX functions are carefully gated
behind AVX checks. That is because the AVX and non-AVX code will share a version of floorf.

The alternate consequence is more insidious. If the non-AVX version of floorf gets chosen then performance will
suffer on AVX machines due to the mixing of AVX and non-AVX FP instructions. This degradation will likely not be
noticed.

All of this complication and misery can be avoided by following gcc's lead and marking inline functions in math.h
(and other system header files) as static. Chrome has been forced to do this (using the preprocessor to do the
dirty work - see https://skia-review.googlesource.com/c/5089/) in order to continue safely using /arch:AVX.
Sign in to post a comment.
Posted by Steve [MSFT] on 1/30/2017 at 3:24 PM
The Universal CRT team has discussed this and will not be declaring the functions in math.h as static inline. The C standard is clear that all library functions should have external linkage.

In order to solve this scenario, we recommend using a separate binary for /arch:AVX compiled code.

Thanks for reporting this!

Steve Wishnousky
Software Engineer II - Visual C++ Libraries
stwish@microsoft.com
Posted by Bruce Dawson on 12/6/2016 at 9:12 PM
On twitter it was suggested - https://twitter.com/dascandy42/status/806175509130477577 - that name mangling should include modifiers for /arch: options, so /arch:AVX would produce a floorf:AVX symbol instead of floorf. If this was done for all non-inlined inline functions then the entire problem would disappear. No linker changes would be needed, just a simple compiler change.

Maybe I'm missing something but this seems like an ideal fix. It is simple, it has very low overhead, it works well with the existing linker, it doesn't require /OPT:ICF in order to work efficiently, it is automatic, it handles all inline functions, both in system headers and in developer specific projects.

Win, win, win. Ship it!
Posted by Microsoft on 12/4/2016 at 5:22 PM
Thank you for your feedback, we are currently reviewing the issue you have submitted.

Microsoft Visual Studio Connect Support Team
Posted by Bruce Dawson2 on 12/4/2016 at 4:45 PM
Comments #80 and #81 in the relevant Chrome bug discuss whether std::min and std::max also need to be marked as inline.

https://bugs.chromium.org/p/chromium/issues/detail?id=666707#c81

One suggestion was that marking them as static would be unreasonable or perhaps non-conformant. Another suggestion was that marking them as static is probably the only way to get correct code. Rock, hard place.