[libcxx-commits] [PATCH] D153902: [libc++][hardening][NFC] Add macros to enable hardened mode.

Thu Jul 6 10:58:58 PDT 2023

var-const added inline comments.

================
Comment at: libcxx/docs/HardenedMode.rst:2
+=============
+Hardened Mode
+=============
----------------
Mordante wrote:
> Is this name future-proof with all the other pre-condition violation checks you want to do?
Pretty much. Hardening is the name of the whole effort, and I expect that the name of the main mode will always be "hardened".

================
Comment at: libcxx/docs/HardenedMode.rst:10-11
+
+Using the hardened mode
+====================
+
----------------
Mordante wrote:
> 
Thanks!

================
Comment at: libcxx/docs/HardenedMode.rst:15-16
+undefined behavior caused by violating preconditions of the standard library.
+These assertions can be done with relatively little overhead in constant time
+and are intended to be used in production by security-conscious projects.
+
----------------
Mordante wrote:
> I feel "by *security-conscious projects" is opinionated.
> Even if I would not care about security since my device has no connection with the outside world I still may want to avoid undefined behavior.
> 
> I think the text is better without the last part. It already mentions "security" and "undefined behavior" before giving the reader enough information to whether or not they like this feature.
> 
This is very useful feedback, thanks!

================
Comment at: libcxx/docs/index.rst:187
    DesignDocs/FileTimeType
+   DesignDocs/HardenedMode
    DesignDocs/HeaderRemovalPolicy
----------------
Mordante wrote:
> This file is not in design docs, so it does not belong here.
Thanks!

================
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
----------------
ldionne wrote:
> 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.
Pushed `e61764c32ff77ae85509e56234a13a47b2d36cec`.

================
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++.",
----------------
ldionne wrote:
> We need tests for these macros along these lines: https://godbolt.org/z/sEKa1W3sn
Added some tests -- do we want any additional test cases?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153902