-
Notifications
You must be signed in to change notification settings - Fork 20.2k
AP_Param: work around Clang -ftrapping-math issue
#31854
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
The other lines don't need it. No compiler-output change.
|
Godbolt playground: https://godbolt.org/z/fs8v7h1hx |
timtuxworth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's some pretty amazing detective work @tpwrules !
This fixes the problem. Tested on my Mac M1 Pro running Tahoe 26.2 which is where I had seen the original problem.
srmainwaring
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good fix thanks.
Tested on macOS Sequoia 15.2 (Apple clang version 16.0.0 (clang-1600.0.26.6).
Resolves issue discussed on https://discord.com/channels/674039678562861068/721931537809014885/1455512219121877003 and below.
Wonder as a follow up if the double to float conversions noted on discord could be handled in constexpr using if constexpr syntax (https://en.cppreference.com/w/cpp/language/if.html)?
tridge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice fix, thanks!
libraries/AP_Param/AP_Param.h
Outdated
| // on non-standard-layout C++ classes, and works in constexpr. as that isn't | ||
| // standard-compliant, this raises a warning we have to suppress. | ||
| // https://github.com/llvm/llvm-project/blob/5fa5ffeb6cb5bc9aa414c02513e44b8405f0e7cc/libcxx/include/__type_traits/datasizeof.h#L51 | ||
| #pragma diagnostic push |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be "#pragma GCC diagnostic push" ?, same for the pop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, turns out this was ignored. I had some memory of it being like this in our codebase but could not find it.
I had to just globally disable the warning since it's impossible to make it specifically surround our next hack. The warning is rather pointless as it simply warns that we are doing something not standard compliant. In the case the operation couldn't be done, the compiler will fail to compile the code.
3f1e66a to
10e99ee
Compare
Impossible to make it specifically surround our next hack. The warning is rather pointless as it simply warns that we are doing something not standard compliant. In the case the operation couldn't be done, the compiler will fail to compile the code.
In Clang (independent of OS or architecture), if three factors are true about a static variable initialization (const or not): * `-ftrapping-math` is turned on * a double literal needs to be rounded and doesn't exactly equal a float * the initialization involves something which isn't constexpr then Clang will decline to do constant initialization, unlike GCC, or Clang if any one of these factors is not true. Constant initialization is where the object is initialized as part of the memory image and is ready to go when the program is loaded. Instead, Clang will generate code at runtime to do the initialization. This is fine in a vacuum, but it makes lots more variables suddenly vulnerable to the static initialization order fiasco. In particular, many of the `AP_Param::Info` tables for the parameter system meet all the factors. The use for these tables, `AP_Param::setup_object_defaults`, is part of constructors of objects which are usually constructed in a different file than the table. Due to the new runtime initialization, there is no longer a guarantee that the table has meaningful data at the time of that setup call. This causes many parameter defaults to not be loaded and the system generally crashes and burns. This PR addresses the issue by fixing the non-constexpr casts in `AP_VAROFFSET` and `AP_CLASSTYPE` to use things permissible in constexpr. The compiler then constant initializes these tables again, parameters get their defaults, and everybody is happy. This is probably easier than fixing all the double literals to be floats. This fix could be "locked in" and be made to cause a compiler error in case of a problem by declaring all tables `constexpr`, but that is a lot of effort. Examination of the binary symbol tables shows that all `var_info` tables are correctly constant initialized now.
10e99ee to
2058352
Compare
In Clang (independent of OS or architecture), if three factors are true about a static variable initialization (const or not):
-ftrapping-mathis turned onthen Clang will decline to do constant initialization, unlike GCC, or Clang if any one of these factors is not true.
Constant initialization is where the object is initialized as part of the memory image and is ready to go when the program is loaded. Instead, Clang will generate code at runtime to do the initialization. This is fine in a vacuum, but it makes lots more variables suddenly vulnerable to the static initialization order fiasco.
In particular, many of the
AP_Param::Infotables for the parameter system meet all the factors. The use for these tables,AP_Param::setup_object_defaults, is part of constructors of objects which are usually constructed in a different file than the table. Due to the new runtime initialization, there is no longer a guarantee that the table has meaningful data at the time of that setup call. This causes many parameter defaults to not be loaded and the system generally crashes and burns.This PR addresses the issue by fixing the non-constexpr casts in
AP_VAROFFSETandAP_CLASSTYPEto use things permissible in constexpr. The compiler then constant initializes these tables again, parameters get their defaults, and everybody is happy.This is probably easier than fixing all the double literals to be floats. This fix could be "locked in" and be made to cause a compiler error in case of a problem by declaring all tables
constexpr, but that is a lot of effort. Examination of the binary symbol tables shows that allvar_infotables are correctly constant initialized now.However, that examination also shows that other things are being initialized differently (unsure if they create an ordering problem):
Given that the vehicle starts up now instead of crashing, I'm choosing to believe these latter problems are minor and can be fixed later. This is also better than removing the flag and letting random crashes from ignored checks come back. But the ultimate fix may still be something different. The new macros are also probably more defined and safer than the old ones so we should keep this change in regardless.
The whole situation is super weird so I also filed an issue with LLVM, but if there's something to fix I expect a very long time before we can remove any workarounds.
Tested that there is no compiler output change on CubeOrange, so the new definitions do not trip up GCC. I also ran the Copter test suite on macOS and many things pass so it's no longer completely broken there. Before EKF3 wouldn't even start because its enabled default didn't load, and turning on terrain would instantly crash. It's unclear why it wasn't so bad on Linux Clang, maybe the version or linker behavior is significantly different.