[libcxx-commits] [PATCH] D154997: DRAFT [libc++][hardening] Deprecate `_LIBCPP_ENABLE_ASSERTIONS`.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/CMakeLists.txt:602-606
 define_if(LIBCXX_DEBUG_BUILD -D_DEBUG)
-if (LIBCXX_ENABLE_ASSERTIONS AND NOT LIBCXX_DEBUG_BUILD)
+if (NOT LIBCXX_HARDENING_MODE STREQUAL "unchecked" AND NOT LIBCXX_DEBUG_BUILD)
   # MSVC doesn't like _DEBUG on release builds. See PR 4379.
   define_if_not(LIBCXX_TARGETING_MSVC -D_DEBUG)
 endif()
----------------
ldionne wrote:
> Let's remove `LIBCXX_DEBUG_BUILD` in a pre-requisite patch. I don't think that's needed anymore, since we use our own internal assertion mechanism.
> 
> I think the only place where you'll still want to have a similar check is in `cxx_link_system_libraries` where you can do something like
> 
> ```
> if (CMAKE_BUILD_TYPE STREQUAL "Debug")
>   set(LIB_SUFFIX "d")
> else()
>   set(LIB_SUFFIX "")
> endif()
> ```
Done: https://reviews.llvm.org/D155038


================
Comment at: libcxx/include/__assert:37-38
 
+// TODO(hardening): only enable uncategorized assertions in the debug mode.
 #define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSERT(expression, message)
 
----------------
ldionne wrote:
> I think what I would do instead is this:
> 
> 
> ```
> // Always define _LIBCPP_ASSERT to _LIBCPP_VERBOSE_ABORT(whatever)
> // Always define _SOME_MACRO_THAT_MIGHT_EXPAND_TO_BUILTIN_ASSUME_ONE_DAY
> 
> #if _LIBCPP_ENABLE_HARDENED_MODE
>     // TODO(hardening): Once we categorize assertions, only enable the ones that we really want here.
> #  define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSERT(expression, message)
> #elif _LIBCPP_ENABLE_DEBUG_MODE
> #  define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _LIBCPP_ASSERT(expression, message)
> #else
> #  define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message) _SOME_MACRO_THAT_MIGHT_EXPAND_TO_BUILTIN_ASSUME_ONE_DAY(...)
> #endif
> ```
Done. I moved the part that deals with `_LIBCPP_ASSERT_UNCATEGORIZED` to `__config`, please let me know what you think.


================
Comment at: libcxx/utils/ci/run-buildbot:384
 ;;
-generic-assertions)
-    clean
----------------
ldionne wrote:
> `buildkite-pipeline.yml` changes?
Thanks!


================
Comment at: libcxx/utils/ci/run-buildbot:491
 ;;
 apple-system-backdeployment-assertions-*)
     clean
----------------
ldionne wrote:
> Let's make this `apple-system-backdeployment-hardened-*` and make the corresponding changes so this uses the hardened mode instead.
Done. Do we also need a new configuration for the debug mode?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154997