-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
BLD: Add clang -ftrapping-math
also for compiler_so
#19479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I somewhat expected this would fail, but I don't understand it. The mac CI build uses Clang 11.0.0, my local build uses |
-ffp-exception-behavior=strict
also for _so
-ftrapping-math
also for _so
Apparently, there is a second `compiler_so` that is actually the main compiler being used, and modifying only the first one is just futile. The old try was `-ffp-exception-behavior=strict`, but that seems to be a fairly new addition (and doesn't even work on MacOS with 10.0) `-ftrapping-math` seems to be the older, equivalent option.
7eae81e
to
e7d1841
Compare
I also confirmed that it does indeed fix gh-18005 for my clang version, so changing that comment. I expect the EDIT: But, this probably needs a release note. And I am not sure what to do about the 1.21 release notes being incorrect... |
So for those checking this: https://clang.llvm.org/docs/UsersManual.html#cmdoption-ffp-exception-behavior explains that
and
I think the main question is basically the old question: Is modifying |
-ftrapping-math
also for _so
-ftrapping-math
also for compiler_so
I think we should put this PR in. @charris, thoughts?
Well, this PR does not add a new modification, it fixes the current one. |
I don't have a problem putting this in. What is the question about 1.21.x release notes? |
@charris my question was whether we should do something about them being incorrect? |
I tend to regard release notes as history, flaws and all, after the release. We could backport this change if you wish and mention that in a later release note. |
OK, sounds good. I am hesitant about backporting, since I have no idea if this may have side-effects for some. |
Thanks @seberg |
This comment was marked as outdated.
This comment was marked as outdated.
The distutils build also uses this flag, and it avoids some problems with `floor_divide` and similar functions (xref numpygh-19479).
The distutils build also uses this flag, and it avoids some problems with `floor_divide` and similar functions (xref numpygh-19479). For older macOS arm64 Clang versions, the flag does get accepted, but then gets overridden because it's not actually supported - which yields these warnings: ``` warning: overriding currently unsupported use of floating point exceptions on this target [-Wunsupported-floating-point-opt] ``` Since they're a little annoying to suppress and will go away when updating to the latest XCode version, we'll ignore these warnings.
The distutils build also uses this flag, and it avoids some problems with `floor_divide` and similar functions (xref numpygh-19479). For older macOS arm64 Clang versions, the flag does get accepted, but then gets overridden because it's not actually supported - which yields these warnings: ``` warning: overriding currently unsupported use of floating point exceptions on this target [-Wunsupported-floating-point-opt] ``` Since they're a little annoying to suppress and will go away when updating to the latest XCode version, we'll ignore these warnings.
Apparently, there is a second
compiler_so
that is actually themain compiler being used, and modifying only the first one is just
futile.
Closes gh-19427 in theory. In practice, I somewhat expect that clang only decided that warning flags are worth any consideration so recently, that this will just break old clang versions
I have tested that this works: gh-19476 passes with this. What is the minimum clang version we actually support? I expect this needs at least version 10 (which is fairly new), but I can't compile locally on version 9. (I guess the CI run will tell me that this won't actually work :/)