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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__config:186
 // iostreams by providing a single strong definition in the shared library.
 #    define _LIBCPP_ABI_ENABLE_ADDITIONAL_IOSTREAM_EXPLICIT_INSTANTIATIONS_1
 
----------------
ldionne wrote:
> We should also add a `.rst` document that explains what this mode is. This can be similar to `DebugMode.rst` but it should explain the various hardening modes instead (there will be TODOs at first).
Took a stab. Should the doc be under `docs/`, or `docs/DesignDocs/`? (`DebugMode.rst` is under the latter)


================
Comment at: libcxx/include/__config:213
+#ifndef _LIBCPP_ENABLE_HARDENED_MODE
+#  define _LIBCPP_ENABLE_HARDENED_MODE _LIBCPP_ENABLE_HARDENED_MODE_DEFAULT
+#endif
----------------
Mordante wrote:
> `_LIBCPP_ENABLE_HARDENED_MODE_DEFAULT` is undefined, or is that in one another patch?
> The same for `_LIBCPP_ENABLE_HARDENED_DEBUG_MODE_DEFAULT`.
It comes from `__config_site.in`.


================
Comment at: libcxx/include/__config_site.in:30
 #cmakedefine _LIBCPP_HAS_NO_LOCALIZATION
 #cmakedefine _LIBCPP_HAS_NO_WIDE_CHARACTERS
 #cmakedefine01 _LIBCPP_ENABLE_ASSERTIONS_DEFAULT
----------------
ldionne wrote:
> We'll want to add a release note explaining that we have these two new options and what they do.
Added a very short note. I'm not sure we want to expand it in this patch given that these variables don't currently do anything. What do you think? 


================
Comment at: libcxx/include/__config_site.in:39
+// Hardening.
+#cmakedefine01 _LIBCPP_ENABLE_HARDENED_MODE_DEFAULT
+#cmakedefine01 _LIBCPP_ENABLE_HARDENED_DEBUG_MODE_DEFAULT
----------------
ldionne wrote:
> This needs to be tied into CMake and into the CI as well. For now I'll suggest we add one configuration for each of them, however that will be offset by the fact that we're removing the legacy debug mode, and we can also remove the `With Assertions Enabled` CI configuration in a separate patch since it'll be covered by those two.
> 
> #### Tying into CMake
> You can look for this in `libcxx/CMakeLists.txt`
> 
> ```
> if (LIBCXX_ENABLE_ASSERTIONS)
>   config_define(1 _LIBCPP_ENABLE_ASSERTIONS_DEFAULT)
> else()
>   config_define(0 _LIBCPP_ENABLE_ASSERTIONS_DEFAULT)
> endif()
> ```
> 
> 
> #### Tying into the CI
> 
> You'll want to add new CMake caches similar to `libcxx/cmake/caches/Generic-assertions.cmake` and then modify `run-buildbot` and also `buildkite-pipeline.yml` to add new jobs.
Thanks for explaining the steps!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153902