[libcxx-commits] [PATCH] D158823: [libc++][hardening] Add back the safe mode.

Mon Aug 28 22:15:32 PDT 2023

var-const added inline comments.

================
Comment at: libcxx/cmake/caches/Generic-safe-mode.cmake:1
+set(LIBCXX_HARDENING_MODE "safe" CACHE STRING "")
----------------
Mordante wrote:
> I really would like to enable modules in this build too. Maybe it would be better to do in a followup when we want to backport this.
Thanks! I'll do it in a follow-up like you suggest.

================
Comment at: libcxx/include/__config:287-289
+#  if (_LIBCPP_ENABLE_HARDENED_MODE && _LIBCPP_ENABLE_SAFE_MODE) ||                                                    \
+      (_LIBCPP_ENABLE_HARDENED_MODE && _LIBCPP_ENABLE_DEBUG_MODE) ||                                                   \
+      (_LIBCPP_ENABLE_SAFE_MODE && _LIBCPP_ENABLE_DEBUG_MODE)
----------------
var-const wrote:
> ldionne wrote:
> > No strong preference, but we could also do `#if HARDENED_MODE + SAFE_MODE + DEBUG_MODE > 1`.
> Thanks! I think it's better (I don't really expect another mode at this point, but if that ever happens, the new form of this check is much easier to modify).
Update: unfortunately, with this approach the error is also triggered when setting one of the macros to a bad value (e.g. `2`). Reverted to the previous state.

================
Comment at: libcxx/test/libcxx/assertions/modes/debug_mode_disabled_in_tu.pass.cpp:11

-// UNSUPPORTED: libcpp-hardening-mode=hardened
+// REQUIRES: libcpp-hardening-mode=unchecked || libcpp-hardening-mode=debug
 // ADDITIONAL_COMPILE_FLAGS: -Wno-macro-redefined -D_LIBCPP_ENABLE_DEBUG_MODE=0
----------------
ldionne wrote:
> I don't really understand this `REQUIRES` anymore. Can you explain?
If another hardening mode is enabled (e.g. the safe mode), the assertions will still fire -- basically, we would end up in a situation where `_LIBCPP_ENABLE_DEBUG_MODE` is `0` but `_LIBCPP_ENABLE_SAFE_MODE` is `1`.

(I changed the assertion type from "uncategorized" to "valid-element-access", btw -- thanks for calling out this test!)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158823