This is an archive of the discontinued LLVM Phabricator instance.

[libc++][hardening] Remove hardening from release notes, undeprecate safe mode
ClosedPublic

Authored by var-const on Aug 29 2023, 11:38 PM.

Details

Reviewers
ldionne
Mordante
Group Reviewers
Restricted Project
Summary

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

Event Timeline

var-const created this revision.Aug 29 2023, 11:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2023, 11:38 PM
Herald added a subscriber: arichardson. · View Herald Transcript
ldionne added inline comments.
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.

var-const edited the summary of this revision. (Show Details)Aug 31 2023, 1:32 AM
var-const updated this revision to Diff 554940.Aug 31 2023, 2:07 AM
var-const marked 4 inline comments as done.

Address feedback

var-const published this revision for review.Aug 31 2023, 2:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2023, 2:08 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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.

Fix formatting

The patch is quite large, I assume most of it's just reverts and only little new code. Is that right?

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.

var-const updated this revision to Diff 555210.Aug 31 2023, 5:56 PM
var-const marked an inline comment as done.

Address feedback

libcxx/test/libcxx/assertions/assertions_disabled.pass.cpp
12

Thanks! Fixed and manually tested now.

The patch is quite large, I assume most of it's just reverts and only little new code. Is that right?

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.

ldionne accepted this revision as: ldionne.Sep 1 2023, 11:27 AM

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
var-const updated this revision to Diff 555447.Sep 1 2023, 11:40 AM

Fix the CI

Mordante accepted this revision.Sep 4 2023, 9:18 AM

LGTM after the open comments are addressed.

This revision is now accepted and ready to land.Sep 4 2023, 9:18 AM
var-const updated this revision to Diff 555831.Sep 5 2023, 1:59 AM
var-const marked 3 inline comments as done.

Address feedback

var-const marked an inline comment as done.Sep 7 2023, 8:55 PM
var-const added inline comments.
libcxx/CMakeLists.txt
760

Thanks! Addressed in https://reviews.llvm.org/D159454.

ldionne closed this revision.Sep 11 2023, 6:46 PM

Closing since this has been merged to release/17.x.

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.

thesamesam added a subscriber: Restricted Project.
libcxx/test/libcxx/assertions/modes/enable_assertions_and_hardened_mutually_exclusive.verify.cpp