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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__config:194
 
+// HARDENING {
+
----------------
We don't have a good story for splitting up the whole `__config` header, however we could pretty easily add all the hardening configuration macros to something like `__hardening` or `__config_dir/hardening.h`. IMO it would make sure that this configuration is nicely contained in a single place.

You'll want to make sure to include `__config` in that header since that includes `__config_site`, and that's where you get some of the defaults from.


================
Comment at: libcxx/include/__config:219
+
+#ifndef _LIBCPP_ENABLE_HARDENED_DEBUG_MODE
+#  define _LIBCPP_ENABLE_HARDENED_DEBUG_MODE _LIBCPP_ENABLE_HARDENED_DEBUG_MODE_DEFAULT
----------------
IMO `_LIBCPP_ENABLE_HARDENED_DEBUG_MODE` is fine as a temporary name until we remove the legacy debug mode, but it's pretty confusing so we want to rename it to `_LIBCPP_ENABLE_DEBUG_MODE` when we can.


================
Comment at: libcxx/include/__config:231
+// Hardened mode checks.
+#if _LIBCPP_ENABLE_HARDENED_MODE
+
----------------
Mordante wrote:
> The comment on line 238 `// TODO(hardening): more checks to be added here...` seem to suggest we get quite a bit of duplication in the `#if` and `#elif` branch. So I propose
> 
> ```
> #if _LIBCPP_ENABLE_HARDENED_MODE || _LIBCPP_ENABLE_HARDENED_DEBUG_MODE
> // do stuff for hardened and hardened debug
> #endif // _LIBCPP_ENABLE_HARDENED_MODE || _LIBCPP_ENABLE_HARDENED_DEBUG_MODE
> 
> #if _LIBCPP_ENABLE_HARDENED_DEBUG_MODE
> // do stuff only for hardened debug
> #endif // _LIBCPP_ENABLE_HARDENED_DEBUG_MODE
> ```
Considering there will be only a small number of categories (at least at first), IMO it will end up being easier to read by listing all of them explicitly in each mode. We can also cross that bridge once we get to it in an upcoming patch.


================
Comment at: libcxx/include/__config:248-251
+// Always enable ABI-breaking checks in debug mode since it's not intended to be ABI-stable.
+#if !defined(_LIBCPP_ABI_BOUNDED_ITERATORS)
+#  define _LIBCPP_ABI_BOUNDED_ITERATORS
+#endif
----------------
I think the separation between hardening/debug mode selection and ABI flags should be kept. Otherwise, we lose the property of having a simple and consistent usage model for the mode selection. For example, we can't claim anymore that you can change modes between translation units (since that would affect ABI).

Similarly, I think it would be sensible (although a bit weird) for someone to enable the debug mode without wanting to break the ABI. For example you'd get the unspecified behavior randomization but you wouldn't get bounded iterators. That sounds like something we shouldn't preclude from the get go.

So essentially I'd just remove these lines and we can figure out how to enable the bounded iterator ABI later (soon).


================
Comment at: libcxx/include/__config:258
+
+// TODO: more checks to be added here...
+
----------------
Mordante wrote:
> Do we really want to disable things? If I only want to define `_LIBCPP_ABI_BOUNDED_ITERATORS` is that prohibited?
> If I only want to define `_LIBCPP_ABI_BOUNDED_ITERATORS` is that prohibited?

No, that's allowed. ABI flags are disjoint from the hardening checks, so you could enable bounded iterators but if you don't enable any hardening, the in-bounds assertions inside `__bounded_iter` would be disabled. The intent is that vendors would decide on those ABI flags at configuration time, since users can't change them on a per-TU basis.


================
Comment at: libcxx/include/__config_site.in:30
 #cmakedefine _LIBCPP_HAS_NO_LOCALIZATION
 #cmakedefine _LIBCPP_HAS_NO_WIDE_CHARACTERS
 #cmakedefine01 _LIBCPP_ENABLE_ASSERTIONS_DEFAULT
----------------
We'll want to add a release note explaining that we have these two new options and what they do.


================
Comment at: libcxx/include/__config_site.in:39
+// Hardening.
+#cmakedefine01 _LIBCPP_ENABLE_HARDENED_MODE_DEFAULT
+#cmakedefine01 _LIBCPP_ENABLE_HARDENED_DEBUG_MODE_DEFAULT
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153902