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

Wed Jul 12 13:04:14 PDT 2023

ldionne added inline comments.

================
Comment at: libcxx/CMakeLists.txt:68
 endif()
+option(LIBCXX_ENABLE_ASSERTIONS
+  "This is a deprecated option that is only temporarily retained for backward
----------------
I think it would be easier to just remove the whole thing here, since we're not really retaining for backwards compat.

================
Comment at: libcxx/cmake/caches/AIX.cmake:9
 set(LIBCXX_USE_COMPILER_RT ON CACHE BOOL "")
-set(LIBCXX_ENABLE_ASSERTIONS OFF CACHE BOOL "")
+set(LIBCXX_HARDENING_MODE "unchecked" CACHE STRING "")
 set(LIBCXX_ABI_VERSION "1" CACHE STRING "")
----------------
Since this is the default, I think I would just remove it. I think this was probably a leftover from copying the Apple cache, and the reason why it's in the Apple cache is that I wasn't sure what the default was (this went through various states, all of which were more complicated than the other).

================
Comment at: libcxx/cmake/caches/Apple.cmake:5
 set(LIBCXX_USE_COMPILER_RT ON CACHE BOOL "")
-set(LIBCXX_ENABLE_ASSERTIONS OFF CACHE BOOL "")
+set(LIBCXX_HARDENING_MODE "unchecked" CACHE STRING "")
 set(LIBCXX_ABI_VERSION "1" CACHE STRING "")
----------------
I would remove since this is default.

================
Comment at: libcxx/cmake/caches/Apple.cmake:15

-set(LIBCXXABI_ENABLE_ASSERTIONS OFF CACHE BOOL "")
 set(LIBCXXABI_ENABLE_FORGIVING_DYNAMIC_CAST ON CACHE BOOL "")
----------------
This is a bit of a can of worms. I think I would avoid touching this in this patch and handle it as a cleanup later. While `LIBCXX_ENABLE_ASSERTIONS` is related to the hardened mode, `LIBCXXABI_ENABLE_ASSERTIONS` is not (for now) -- yes that is confusing.

================
Comment at: libcxx/cmake/caches/FreeBSD.cmake:4

-set(LIBCXX_ENABLE_ASSERTIONS OFF CACHE BOOL "")
+set(LIBCXX_HARDENING_MODE "unchecked" CACHE STRING "")
 set(LIBCXX_ABI_VERSION "1" CACHE STRING "")
----------------
Same here.

================
Comment at: libcxx/docs/HardenedMode.rst:23

 Vendors can set the default hardened mode by using the ``LIBCXX_HARDENING_MODE``
+variable at CMake configuration time. Setting ``LIBCXX_HARDENING_MODE`` to
----------------

================
Comment at: libcxx/docs/HardenedMode.rst:27
+``debug`` enables the debug mode. The default value is ``unchecked`` which
+doesn't enable the hardened mode.
+
----------------

================
Comment at: libcxx/docs/HardenedMode.rst:29
+
+When the hardened mode (or the debug mode) is enabled, the compiled library is
+built with the corresponding mode enabled, **and** user code will be built with
----------------

================
Comment at: libcxx/docs/HardenedMode.rst:33-34
+configuration time, the compiled library will not contain any assertions and the
+default when building user code will be to have assertions disabled. As a user,
+you can consult your vendor to know whether assertions are enabled by default.
+
----------------

================
Comment at: libcxx/docs/HardenedMode.rst:36-37
+
+Furthermore, independently of any vendor-selected default, users can always
+control whether the hardened mode (or the debug mode) is enabled in their code
+by defining ``_LIBCPP_ENABLE_HARDENED_MODE=0|1`` (or
----------------

================
Comment at: libcxx/docs/HardenedMode.rst:43-44
+library was built by the vendor in the unchecked mode, functions compiled inside
+the static or shared library won't have hardening assertions enabled even if the
+user defines ``_LIBCPP_ENABLE_HARDENED_MODE=1`` (the same is true for the
+inverse case where the static or shared library was compiled **with** hardening
----------------

================
Comment at: libcxx/docs/ReleaseNotes.rst:94
+  ``_LIBCPP_ENABLE_ASSERTIONS`` macro is deprecated and setting it to ``1`` now enables the hardened mode. See
+  ``libcxx/docs/HardenedMode.rst`` for more details.
+
----------------
TODO in a follow-up: Rename the document from `HardenedMode.rst` to `Hardening.rst`?

================
Comment at: libcxx/docs/UsingLibcxx.rst:146

 When an assertion fails, the program is aborted through a special verbose termination function. The
 library provides a default function that prints an error message and calls ``std::abort()``. Note
----------------

================
Comment at: libcxx/docs/UsingLibcxx.rst:193
     std::vector<int> v;
-    int& x = v[0]; // Your termination function will be called here if _LIBCPP_ENABLE_ASSERTIONS=1
+    int& x = v[0]; // Your termination function will be called here if the hardened mode or the debug mode is enabled.
   }
----------------

================
Comment at: libcxx/docs/UsingLibcxx.rst:208
 Libc++ provides a number of configuration macros which can be used to enable
-or disable extended libc++ behavior, including enabling "debug mode" or
+or disable extended libc++ behavior, including enabling the hardened mode or
 thread safety annotations.
----------------

================
Comment at: libcxx/include/__config:258

 // Hardened mode checks.
 #  if _LIBCPP_ENABLE_HARDENED_MODE
----------------
I think we might need to move the categorization to `__assert`, otherwise we're depending on `_LIBCPP_ASSERT` and `_LIBCPP_ASSUME` that are defined in `__assert` (and there's a circular dep if you try to include `__assert` from `__config`).

================
Comment at: libcxx/utils/ci/buildkite-pipeline.yml:932-933

     - label: "Apple back-deployment with assertions enabled"
       command: "libcxx/utils/ci/run-buildbot apple-system-backdeployment-assertions-11.0"
       artifact_paths:
----------------

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154997