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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/CMakeLists.txt:71
 endif()
+option(LIBCXX_ENABLE_ASSERTIONS
+  "This is a deprecated option that is only provided for backward compatibility.
----------------
I would remove that option, since it's only used by vendors. I think this is what we want to do:

```
# TODO(LLVM 18): Remove this after branching for LLVM 17, this is a simple courtesy for vendors to be notified about this change.
if (LIBCXX_ENABLE_ASSERTIONS)
  message(FATAL_ERROR "LIBCXX_ENABLE_ASSERTIONS has been replaced by LIBCXX_HARDENING_MODE=hardened")
endif()
```

Then we also release note (for vendors) that we removed `LIBCXX_ENABLE_ASSERTIONS`.

The reason for the temporary `FATAL_ERROR` is that I want to avoid the case where a vendor is currently specifying `LIBCXX_ENABLE_ASSERTIONS` and then suddenly after this change they would build the library in unchecked mode, silently.


================
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()
----------------
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()
```


================
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)
 
----------------
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
```


================
Comment at: libcxx/include/__config:211-212
 
+// This is for backward compatibility -- make enabling `_LIBCPP_ENABLE_ASSERTIONS` (which predates hardening modes)
+// equivalent to setting the hardened mode.
+#    ifdef(_LIBCPP_ENABLE_ASSERTIONS)
----------------
We can add a `TODO(LLVM 18)` here to remove this I guess.


================
Comment at: libcxx/include/__config:216-218
+#    ifndef _LIBCPP_ENABLE_ASSERTIONS
+#      define _LIBCPP_ENABLE_ASSERTIONS _LIBCPP_ENABLE_ASSERTIONS_DEFAULT
+#    endif
----------------
`_LIBCPP_ENABLE_ASSERTIONS_DEFAULT` wouldn't be needed anymore since we're removing the CMake flag.


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


================
Comment at: libcxx/utils/ci/run-buildbot:491
 ;;
 apple-system-backdeployment-assertions-*)
     clean
----------------
Let's make this `apple-system-backdeployment-hardened-*` and make the corresponding changes so this uses the hardened mode instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154997