[libcxx-commits] [PATCH] D155866: [libc++][hardening] Don't trigger uncategorized assertions in the hardened mode.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM w/ `UNSUPPORTED` and green CI! Thanks!



================
Comment at: libcxx/include/__config:287
 #    define _LIBCPP_ASSERT_INTERNAL(expression, message)               _LIBCPP_ASSUME(expression)
+#    define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message)          _LIBCPP_ASSUME(expression)
 
----------------
Mordante wrote:
> I noticed there are quite a number of cheap tests in this category. For example `nullptr` guards in `std::string`. Would it make sense to put them in a "cheap-to-test-category". Maybe a `_LIBCPP_ASSERT_PRECONDITION`?
I suggest we discuss this in D155873.


================
Comment at: libcxx/test/libcxx/algorithms/alg.sorting/assert.min.max.pass.cpp:13
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 // UNSUPPORTED: !libcpp-has-hardened-mode && !libcpp-has-debug-mode
 // XFAIL: availability-verbose_abort-missing
----------------
As a follow-up patch, I think we could do this:

```
# This,
AddFeature("libcpp-has-hardened-mode")             if hardening_mode == "hardened" else None,
AddFeature("libcpp-has-debug-mode")                if hardening_mode == "debug" else None,
AddFeature("libcpp-has-unchecked-mode")            if hardening_mode == "unchecked" else None,

# Or that
AddFeature(f"libcpp-hardening-mode={hardening_mode}")
```

And then we could switch to `// UNSUPPORTED: libcpp-has-unchecked-mode` (or the other syntax). This would be a bit easier to understand.



================
Comment at: libcxx/test/libcxx/algorithms/alg.sorting/assert.min.max.pass.cpp:15
 // XFAIL: availability-verbose_abort-missing
+// XFAIL: libcpp-has-hardened-mode
 
----------------
In this patch, I would use `UNSUPPORTED` here. Using `XFAIL` is really nice in theory because it will notify us if we forget to un-XFAIL a test after adding an assertion to the hardened mode, but unfortunately the test is UB when it's run outside of the debug mode. So in practice I think the only thing we can use consistently is `UNSUPPORTED`. For example you mentioned one of the `barrier` tests timing out.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155866/new/

https://reviews.llvm.org/D155866