This patch effectively maintains the status quo, making sure that the safe mode keeps working the same way as before. Hardening will target the next major release, allowing it to go through RFC and for the implementation to stabilize and mature.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/__config | ||
---|---|---|
221 | This might allow catching a few curious users that would be tempted to start depending on this despite the lack of documentation and release note. | |
273 | We should also have a check (+ test) that _LIBCPP_ENABLE_ASSERTIONS is mutually exclusive with the new hardening modes. | |
libcxx/test/libcxx/assertions/assertions_disabled.pass.cpp | ||
12 | You'll need an XFAIL: hardening-mode={{hardened|debug}} here. | |
libcxx/utils/libcxx/test/params.py | ||
295 | We have a bunch of assert.pass.cpp tests (like libcxx/test/libcxx/containers/sequences/vector/assert.cfront.empty.pass.cpp) that currently do stuff like: // UNSUPPORTED: !libcpp-has-hardened-mode && !libcpp-has-debug-mode We should change those to // UNSUPPORTED: !libcpp-has-hardened-mode && !libcpp-has-debug-mode && !libcpp-has-assertions That way, the assertions will be tested when _LIBCPP_ENABLE_ASSERTIONS=1 is passed. |
The patch is quite large, I assume most of it's just reverts and only little new code. Is that right?
libcxx/CMakeLists.txt | ||
---|---|---|
760 | Note in main we should update this to LLVM 19 to keep the courtesy for vendors. |
The biggest part of the patch is the changes to the test suite. Those are making sure that we test the safe mode again like we used to. I went over these changes and they look OK to me, but CI will help validate that.
libcxx/test/libcxx/assertions/assertions_disabled.pass.cpp | ||
---|---|---|
12 | Actually this needs to be XFAIL: libcpp-has-hardened-mode || libcpp-has-debug-mode because libcpp-hardening-mode=FOO doesn't exist yet on release/17.x. While you're at it, can you grep your patch for libcpp-hardening-mode? Any reference to it is basically a mistake. |
Address feedback
libcxx/test/libcxx/assertions/assertions_disabled.pass.cpp | ||
---|---|---|
12 | Thanks! Fixed and manually tested now. |
Yes, it mostly restores bits removed by https://reviews.llvm.org/D154997. Some REQUIRES/UNSUPPORTED annotations are new (but very repetitive), and there are a couple of new test files.
LGTM with green CI, I think the remaining should be just nitpicks. I will let Mark give final approval.
In terms of landing this, this will need to be landed directly on release/17.x since there is no corresponding patch on main. Then we will need another patch on main that fixes up the release notes for LLVM 18 and also a few other things like the deprecation period for LIBCXX_ENABLE_ASSERTIONS (if my memory serves me right).
libcxx/test/libcxx/assertions/modes/debug.pass.cpp | ||
---|---|---|
12 | IMO we should just say REQUIRES: libcpp-has-debug-mode here since the !libcpp-has-assertions bit doesn't add anything. | |
libcxx/test/libcxx/assertions/modes/debug_mode_disabled_in_tu.pass.cpp | ||
11 | Same here. | |
libcxx/utils/ci/run-buildbot | ||
498 |
libcxx/CMakeLists.txt | ||
---|---|---|
760 | Thanks! Addressed in https://reviews.llvm.org/D159454. |
a5e5eb17fd9743ea39de35f2779c6dc10729cf9f doesn't really explain the rationale here. Anyway, we picked up that _ENABLE_ASSERTIONS was deprecated thanks to this warning during the RCs (https://bugs.gentoo.org/912223). This change reverting to the old behaviour wasn't in any of the RCs and have now become "experimental" again.
I'll adjust our config files again but I feel like this would've been better to not do at all if it wasn't going to be tested in a final RC given it actively requires changes for downstreams.
Note in main we should update this to LLVM 19 to keep the courtesy for vendors.