[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/CMakeLists.txt:67-70
+  "Specify the hardening mode to use. This mode will be used inside the
+   compiled library and will be the default when compiling user code. Note that
+   users can override this setting in their own code. This does not affect the
+   ABI. Supported values are ${LIBCXX_SUPPORTED_HARDENING_MODES}.")
----------------



================
Comment at: libcxx/CMakeLists.txt:807-808
+else()
+  message(FATAL_ERROR "LIBCXX_HARDENING_MODE is set to ${LIBCXX_HARDENING_MODE}, which is not a valid value.
+                       Valid values are: ${LIBCXX_SUPPORTED_HARDENING_MODES}")
+endif()
----------------
This check is duplicated with above, I don't think it is needed.


================
Comment at: libcxx/docs/ReleaseNotes.rst:90-92
+- The legacy debug mode has been removed in this release. Enabling the macro `_LIBCPP_ENABLE_DEBUG_MODE` 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/include/__algorithm/comp_ref_type.h:68-69
 // debugging wrapper that stores a reference.
 // TODO(varconst): update to be used in the new debug mode (or delete entirely).
-#ifdef _LIBCPP_ENABLE_DEBUG_MODE
+#if _LIBCPP_ENABLE_DEBUG_MODE
 template <class _Comp>
----------------
The `TODO` comment isn't relevant anymore.


================
Comment at: libcxx/include/__algorithm/three_way_comp_ref_type.h:61
 // debugging wrapper that stores a reference.
 // TODO(varconst): update to be used in the new debug mode (or delete entirely).
+#  if _LIBCPP_ENABLE_DEBUG_MODE
----------------
The `TODO` comment isn't relevant anymore.


================
Comment at: libcxx/include/__algorithm/three_way_comp_ref_type.h:74
 
 #endif // _LIBCPP___ALGORITHM_THREE_WAY_COMP_REF_TYPE_H
----------------
There's a similar TODO comment in `__hash_table` I think, look for `_LIBCPP_DEBUG_ASSERT` after rebasing onto `main`.


================
Comment at: libcxx/test/support/container_debug_tests.h:17
 
-#ifndef _LIBCPP_ENABLE_DEBUG_MODE
+#if !_LIBCPP_ENABLE_DEBUG_MODE
 #error The library must be built with the debug mode enabled in order to use this header
----------------
Let's rename `libcpp-has-debug-mode` to `libcpp-has-legacy-debug-mode` (preferably in a separate patch before this one). That'll clear up a lot of the confusion.


================
Comment at: libcxx/utils/ci/buildkite-pipeline.yml:496
+        CC: "clang-${LLVM_HEAD_VERSION}"
+        CXX: "clang++-${LLVM_HEAD_VERSION}"
+    agents:
----------------
Let's add `ENABLE_CLANG_TIDY: "On"` here and below.


================
Comment at: libcxx/utils/libcxx/test/params.py:290
         default=False,
         help="Whether to enable assertions when compiling the test suite. This is only meaningful when "
         "running the tests against libc++.",
----------------
We need tests for these macros along these lines: https://godbolt.org/z/sEKa1W3sn


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153902