_FORTIFY_SOURCE if using MSan
#30140
This PR shifts responsibility to disable _FORTIFY_SOURCE from the user to the build system, which:
APPEND_CPPFLAGS in such a corner case
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept NACK | fanquake |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Concept NACK. If anything should be changed, it would be the implementation of applying the hardening flags in the CMake branch, so it’s possible to override them using standard CMake variables. Not push more complexity into the build system, and hardcode specific things.
Removing APPEND_*FLAGS shouldn’t be a goal, as it needs to exist, to do what it exists for (actually give a mechanism for always overriding a flag, regardless of what CMake try’s to do). Otherwise in future, if we (or any other user) needs to do something, there may not be a (easy, working) way to do so, depending on the implementation details of the CMake build system.
Concept NACK. If anything should be changed, it would be the implementation of applying the hardening flags in the CMake branch, so it’s possible to override them using standard CMake variables.
I disagree. The current implementation is undocumented and error-prone on the user side.
Not push more complexity into the build system, and hardcode specific things.
This statement seems quite arbitrary. Considering that the build system is full of “hardcode specific things” that “push more complexity into”, what are explicit criteria for such a reasoning?
Removing APPEND_*FLAGS shouldn’t be a goal
It is not a goal of this PR. Rather removing the need to use them when building with MSan.
I disagree. The current implementation is undocumented and error-prone on the user side.
In the CMake branch? If this is the case, then it needs to be reworked anyway…?
Another reason undefining a flag like this needs to be easily possible, is if you want to set the fortify level to anything other than what we pick, it needs to be undefined first. So this all needs to be possible outside of MSAN. Given that, we should also use the same mechanisms, rather than hardcoding things here.
I disagree. The current implementation is undocumented and error-prone on the user side.
In the CMake branch? If this is the case, then it needs to be reworked anyway…?
In the master branch.
./configure CC=clang CXX=clang++ --with-sanitizers=memory just leads to MSan warnings forcing the user for further actions.
./configure CC=clang CXX=clang++ –with-sanitizers=memory just leads to MSan warnings forcing the user for further actions.
Your PR here doesn’t change that. Anyone wanting to use MSAN needs to do extensive setup, and build a fully instrumented toolchain. It can’t be expected to work out of the box.
Anyone wanting to use MSAN needs to do extensive setup, and build a fully instrumented toolchain. It can’t be expected to work out of the box.
Right. I didn’t mean the latter. Just quoted the main system configuration stage for brevity.
Your PR here doesn’t change that.
It does. The second commit proves it, doesn’t it?
It does. The second commit proves it, doesn’t it?
No. I don’t see how that commit sets up a fully instrumented MSAN toolchain.
It does. The second commit proves it, doesn’t it?
No. I don’t see how that commit sets up a fully instrumented MSAN toolchain.
Sure. It “shifts responsibility to disable _FORTIFY_SOURCE from the user to the build system”.
Sure. It “shifts responsibility to disable _FORTIFY_SOURCE from the user to the build system”.
Setting a single preprocessor flag isn’t instrumenting a toolchain?
I still haven’t seen a good reason to not use the mechanisms that exist to set flags, given those mechanisms need to exist (i.e undefining and redefining fortify source), and should be tested so they are known to be working.
Labels
Build system
Tests