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

Thu Jun 29 12:56:34 PDT 2023

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

This is starting to look really good!

================
Comment at: libcxx/CMakeLists.txt:70-81
+option(LIBCXX_ENABLE_HARDENED_MODE
+  "Enable the hardened mode inside the compiled library and make it the default
+   when compiling user code. Note that users can override this setting in their
+   own code. Setting this to 'ON' implicitly enables assertions, unless
+   `LIBCXX_ENABLE_ASSERTIONS` has been explicitly set to 'OFF'. This does not
+   affect the ABI." OFF)
+option(LIBCXX_ENABLE_HARDENED_DEBUG_MODE
----------------
Do we want to make this a multi-valued option instead? Like `LIBCXX_HARDENING_MODE="unsafe|hardened|debug"` and then tie that into the `__config_site` using the same macros you added in this patch? This would make it easier to add a new mode and also it avoids the issue of mutual exclusion of CMake options. It also mirrors a bit more closely what we're trying to do with e.g. the ABI library choice.

================
Comment at: libcxx/docs/DesignDocs/HardenedMode.rst:23-25
+Vendors can enable the hardened mode by building the library with the
+``LIBCXX_ENABLE_HARDENED_MODE`` option, and similarly enable the debug mode by
+building with the ``LIBCXX_ENABLE_HARDENED_DEBUG_MODE`` option.
----------------
Can you mention how users can enable it on a per-TU basis? Basically, the setting that vendors select is only used as the *default* value for user code (and also used for building the dylib itself). We also need to make sure that we're clearly identifying what is a CMake option and what is a compiler define, otherwise casual users pointed to this documentation will be extremely confused. I generally use some wording like "the `LIBCXX_ENABLE_HARDENED_MODE` option at CMake configuration-time", or something like that. And when I talk about a macro, I generally say "the `_LIBCPP_ENABLE_FOO` macro". That makes things less ambiguous.

================
Comment at: libcxx/docs/DesignDocs/HardenedMode.rst:27-31
+The hardened mode requires ``LIBCXX_ENABLE_ASSERTIONS`` to work. If
+``LIBCXX_ENABLE_ASSERTIONS`` was not set explicitly, enabling
+``LIBCXX_ENABLE_HARDENED_MODE`` will implicitly enable
+``LIBCXX_ENABLE_ASSERTIONS``. If ``LIBCXX_ENABLE_ASSERTIONS`` was explicitly
+disabled, this will effectively disable the hardened mode.
----------------
You're talking about `LIBCXX_ENABLE_ASSERTIONS` in specific terms here, and I think that adds a lot of complexity if you want to be precise. Instead, I would say:

```
The hardened and debug modes require assertions <LINK TO THE DOCUMENTATION FOR ASSERTIONS> to work. If assertions were explicitly disabled, none of the checks of the hardened/debug mode will trigger assertions.
```

(also note that I wouldn't document that we enable assertions automatically cause we might not want to maintain that, we only do that to be nice to early adopters)

With a wording like this, you don't have to be precise about `LIBCXX_ENABLE_ASSERTIONS` vs `_LIBCPP_ENABLE_ASSERTIONS=1`.

================
Comment at: libcxx/docs/DesignDocs/HardenedMode.rst:33
+
+Enabling the hardened mode or the debug mode has no impact on the ABI.
+
----------------
We want to expand a bit more on the relationship between ABI settings and these modes. Basically, you can say that the "mode" is separate from the ABI, i.e. it never modifies the ABI. However, libc++ provides alternate ABIs under which checks from the hardened/debug mode become a lot more potent (for example bounded iterators).

================
Comment at: libcxx/docs/ReleaseNotes.rst:74-76
+- It is now possible to use CMake to enable the hardened mode by setting the ``LIBCXX_ENABLE_HARDENED_MODE`` variable,
+  and similarly to enable the debug mode using the ``LIBCXX_ENABLE_HARDENED_DEBUG_MODE`` variable (note that this is the
+  new debug mode distinct from the legacy debug mode that is being removed in this release).
----------------

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

----------------
var-const wrote:
> 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)
I would move to `docs/`. I'm not really fond of `DesignDocs/` anymore, since we do most of our design work in Discourse / Discord nowadays.

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

+// HARDENING {
+
----------------
var-const wrote:
> ldionne wrote:
> > 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.
> I think the original idea for the split was that `__config` will include everything from `__config_dir`, so that other files can simply include `__config` and get everything. If that's the case, `__config_dir/hardening.h` cannot include `__config`. It could include `__config_site` directly, but I'm concerned it will need other includes in the future (for other macros it might rely upon), necessitating more granularization, and I'm not sure we want to go that way right now.
> 
> OTOH, it's straightforward to include `__config` from `__config_dir/hardening.h`, but then all the other files that use assertions would need to include `__config_dir/hardening.h` (or I guess we could include it in `__assert` and then those files would just include `__assert`?). What do you think?
Yeah, I didn't really think about that. I think it's fine to leave as-is for this patch. I might take a stab at moving it to another header at some point, but for now this seems fine.

================
Comment at: libcxx/utils/ci/buildkite-pipeline.yml:523-540
+  - label: "Debug mode"
+    command: "libcxx/utils/ci/run-buildbot generic-debug-mode"
+    artifact_paths:
+      - "**/test-results.xml"
+      - "**/*.abilist"
+    env:
+        CC: "clang-${LLVM_HEAD_VERSION}"
----------------
I think this is a leftover

================
Comment at: libcxx/utils/libcxx/test/params.py:298
+    Parameter(
+        name="enable_hardened_mode",
+        choices=[True, False],
----------------
Using a multi-valued option here would also be nicer!

================
Comment at: libcxx/utils/libcxx/test/params.py:306
+            AddCompileFlag("-D_LIBCPP_ENABLE_HARDENED_MODE=1"),
+            AddFeature("libcpp-has-hardened-mode"),
+        ],
----------------
You'll need to make sure that we pass `--param enable_hardened_mode=True` when `LIBCXX_ENABLE_HARDENED_MODE=ON`. We do this for assertions in `libcxx/test/CMakeLists.txt` for example:

```
if (LIBCXX_ENABLE_ASSERTIONS)
  serialize_lit_param(enable_assertions True)
endif()
```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153902