[libcxx-commits] [PATCH] D153902: [libc++][hardening][NFC] Add macros to enable hardened mode.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/docs/ReleaseNotes.rst:91-93
+- The legacy debug mode has been removed in this release. Setting the macro `_LIBCPP_ENABLE_DEBUG_MODE` to `1` now
+  enables the new debug mode which is part of hardening (see the "Improvements and New Features" section above). The
+  `LIBCXX_ENABLE_DEBUG_MODE` CMake variable has been removed. For additional context, refer to the `Discourse post
----------------



================
Comment at: libcxx/test/libcxx/assertions/modes/debug.pass.cpp:15-18
+  _LIBCPP_ASSERT(true, "Should not fire");
+  TEST_LIBCPP_ASSERT_FAILURE([] {
+    _LIBCPP_ASSERT(false, "Should fire");
+  }(), "Should fire");
----------------



================
Comment at: libcxx/test/libcxx/assertions/modes/debug_no_assertions.pass.cpp:9-10
+
+// UNSUPPORTED: !libcpp-has-debug-mode
+// ADDITIONAL_COMPILE_FLAGS: -Wno-macro-redefined -D_LIBCPP_ENABLE_ASSERTIONS=0
+
----------------



================
Comment at: libcxx/test/libcxx/assertions/modes/hardened_and_debug_mutually_exclusive.verify.cpp:13-17
+int main(int, char**) {
+  // expected-error@*:*  {{Only one of _LIBCPP_ENABLE_HARDENED_MODE and _LIBCPP_ENABLE_DEBUG_MODE can be enabled.}}
+
+  return 0;
+}
----------------
No need to define a `main` function! It makes it more obvious that we're not running this test.


================
Comment at: libcxx/test/libcxx/assertions/modes/hardened_no_assertions.pass.cpp:15-16
+int main(int, char**) {
+  _LIBCPP_ASSERT(true, "Should not fire");
+  _LIBCPP_ASSERT(false, "Also should not fire");
+
----------------



================
Comment at: libcxx/test/libcxx/assertions/modes/unchecked.pass.cpp:15
+int main(int, char**) {
+  _LIBCPP_ASSERT(true, "Should not fire");
+  _LIBCPP_ASSERT(false, "Also should not fire");
----------------
ldionne wrote:
> 
I would guard this with `#if !_LIBCPP_ENABLE_ASSERTIONS` because the test will fail otherwise. Also add a TODO comment to remove this once we "remove" `_LIBCPP_ENABLE_ASSERTIONS` (in reality we'll keep it just for backwards compat).


================
Comment at: libcxx/test/libcxx/assertions/modes/unchecked.pass.cpp:15-16
+int main(int, char**) {
+  _LIBCPP_ASSERT(true, "Should not fire");
+  _LIBCPP_ASSERT(false, "Also should not fire");
+
----------------



================
Comment at: libcxx/test/libcxx/assertions/modes/unchecked.pass.cpp:18
+
+  assert(false);
+  return 0;
----------------
Leftover!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153902