[libcxx] reorganises the hardening documentation

[cjdb]

The reorganisation assists with identifying information that's relevant to the reader by using sections, note/warning blocks, and highlighted lists.

Some rewording was necessary to fit the new structure and some to improve flow. Changes to the intention of the documentation have not been made.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Christopher Di Bella (cjdb)

Changes

The reorganisation assists with identifying information that's relevant to the reader by using sections, note/warning blocks, and highlighted lists.

Some rewording was necessary to fit the new structure and some to improve flow. Changes to the intention of the documentation have not been made.

---

Full diff: https://github.com/llvm/llvm-project/pull/73159.diff

1 Files Affected:

• (modified) libcxx/docs/Hardening.rst (+64-54)

diff --git a/libcxx/docs/Hardening.rst b/libcxx/docs/Hardening.rst
index 7692f2a2c788725..2cc0f557c2736d5 100644
--- a/libcxx/docs/Hardening.rst
+++ b/libcxx/docs/Hardening.rst
@@ -15,61 +15,71 @@ assertions that prevent undefined behavior caused by violating preconditions of
 the standard library. Different hardening modes make different trade-offs
 between the amount of checking and runtime performance. The available hardening
 modes are:
-- fast mode;
-- extensive mode;
-- debug mode.
-
-The fast mode contains a set of security-critical checks that can be done with
-relatively little overhead in constant time and are intended to be used in
-production. We recommend most projects to adopt the fast mode.
-
-The extensive mode contains all the checks from the fast mode and additionally
-some checks for undefined behavior that incur relatively little overhead but
-aren't security-critical. While the performance penalty is somewhat more
-significant compared to the fast mode, the extensive mode is still intended to
-be usable in production.
-
-The debug mode enables all the available checks in the library, including
-internal assertions, some of which might be very expensive. This mode is
-intended to be used for testing, not in production.
-
-Vendors can set the default hardening mode by using the
-``LIBCXX_HARDENING_MODE`` variable at CMake configuration time with the possible
-values of ``none``, ``fast``, ``extensive`` and ``debug``. The default value is
-``none`` which doesn't enable any hardening checks (this mode is sometimes
-called the ``unchecked`` mode).
-
-When hardening is enabled, the compiled library is built with the corresponding
-mode enabled, **and** user code will be built with the same mode enabled by
-default. If the mode is set to "none" at the CMake 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 which level of hardening is enabled by default.
-
-Furthermore, independently of any vendor-selected default, users can always
-control which level of hardening is enabled in their code by defining the macro
-``_LIBCPP_HARDENING_MODE`` before including any libc++ headers (preferably by
-passing ``-D_LIBCPP_HARDENING_MODE=X`` to the compiler). The macro can be
-set to one of the following possible values:
-
-- ``_LIBCPP_HARDENING_MODE_NONE``;
-- ``_LIBCPP_HARDENING_MODE_FAST``;
-- ``_LIBCPP_HARDENING_MODE_EXTENSIVE``;
-- ``_LIBCPP_HARDENING_MODE_DEBUG``.
-
-The exact numeric values of these macros are unspecified and users should not
-rely on them (e.g. expect the values to be sorted in any way).
-
-Note that if the compiled library was built by the vendor with the hardening
-mode set to "none", functions compiled inside the static or shared library won't
-have any hardening enabled even if the user compiles with hardening enabled (the
-same is true for the inverse case where the static or shared library was
-compiled **with** hardening enabled but the user tries to disable it). However,
-most of the code in libc++ is in the headers, so the user-selected value for
-``_LIBCPP_HARDENING_MODE``, if any, will usually be respected.
-
-Enabling hardening has no impact on the ABI.
+
+- **Unchecked mode/none**, which disables all hardening checks.
+- **Fast mode**, which contains a set of security-critical checks that can be
+  done with relatively little overhead in constant time and are intended to be
+  used in production. We recommend most projects to adopt the fast mode.
+- **Extensive mode**, which contains all the checks from the fast mode and
+  additionally some checks for undefined behavior that incur relatively little
+  overhead but aren't security-critical. While the performance penalty is
+  somewhat more significant compared to the fast mode, the extensive mode is
+  still intended to be usable in production.
+- **Debug mode**, which enables all the available checks in the library,
+  including internal assertions, some of which might be very expensive. This
+  mode is intended to be used for testing, not in production.
+
+.. note::
+
+   Enabling hardening has no impact on the ABI.
+
+Notes for users
+---------------
+
+As a user, you can consult your vendor to know which level of hardening is
+enabled by default.
+
+Users wishing for a different hardening level to their vendor default are able
+to control the level by passing **one** of the following options to the compiler:
+
+- ``-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_NONE``
+- ``-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_FAST``
+- ``-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_EXTENSIVE``
+- ``-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG``
+
+.. warning::
+
+   The exact numeric values of these macros are unspecified and users should not
+   rely on them (e.g. expect the values to be sorted in any way).
+
+.. warning::
+
+   If you would prefer to override the hardening level on a per-translation-unit
+   basis, you must do so **before** including any headers to avoid `ODR issues`_.
+
+.. _`ODR issues`: https://en.cppreference.com/w/cpp/language/definition#:~:text=is%20ill%2Dformed.-,One%20Definition%20Rule,-Only%20one%20definition
+
+.. note::
+
+   Since the static and shared library components of libc++ are built by the
+   vendor, setting this macro will have no impact on the hardening mode for the
+   pre-built components. Most libc++ code is header-based, so a user-provided
+   value for ``_LIBCPP_HARDENING_MODE`` will be mostly respected.
+
+Notes for vendors
+-----------------
+
+Vendors can set the default hardening mode by providing ``LIBCXX_HARDENING_MODE``
+as a configuration option, with the possible values of ``none``, ``fast``,
+``extensive`` and ``debug``. The default value is ``none`` which doesn't enable
+any hardening checks (this mode is sometimes called the ``unchecked`` mode).
+
+This option controls both the hardening mode that the precompiled library is
+built with and the default hardening mode that users will build with. If set to
+``none``, the precompiled library will not contain any assertions, and user code
+will default to building without assertions.
 
 Iterator bounds checking
 ------------------------
+
 TODO(hardening)

[cjdb]

@var-const if you think there are changes to the intention, please flag them so they can be fixed.

[firewave]

In the past I mixed up defines used to build libc++ itself and ones I can specify when building against libc++. It seems this is no longer possible but reading it I still didn't feel confident this is the documentation that relates to me until I read the very last paragraph. Maybe that could be a bit more emphasized with an introduction paragraph for the vendor/user section instead - maybe even using the phrases I highlighted in my first sentence.

[firewave]

I guess my confusion also stems a bit from the page which links to the hardening settings: https://libcxx.llvm.org/UsingLibcxx.html#libc-configuration-macros. That also contains some ambiguity with macros that extend to both cases. Maybe that also needs to better differentiate between those.

As that refers to "using libc++" I wonder if it should even mention anything related to the vendor/building libc++ itself case at all. Maybe that should be moved to a separate (and possibly already existing) page.

In the past it was why more confusing with assertions, annotations, debug mode, safe mode, etc. The new hardening stuff is definitely more straight forward. 👍

[cjdb]

@firewave would a table of contents help in that regard? Readers can then see that there's a section for them regardless of whether they're a user or a vendor.

Perhaps we need to reorganise the documentation so that it is bifurcated for users, maintainers, and vendors, but I'd prefer to do that in a separate, more general CL.

[hawkinsw]

Thank you, @cjdb !! This documentation is very well written, succinct and helpful! Having read it I know much more about the different hardening levels. I hope that these suggestions are somewhat helpful!

[hawkinsw]

Suggested change

	overhead but aren't security-critical. While the performance penalty is
	overhead but aren't security-critical. While the performance penalty in Extensive mode is

[cjdb]

I think this one is already apparent from context.

[hawkinsw]

Suggested change

	used in production. We recommend most projects to adopt the fast mode.
	used in production. We recommend most projects to adopt the Fast mode.

[cjdb]

These aren't proper nouns, so I don't think that they need to be captialised.

[philnik777]

I'm not a native speaker, but I think you use it as a proper noun in this context. The name of the mode is "Fast". It's not about the mode being fast.

[hawkinsw]

That was exactly what I was trying to say. Thanks for chiming in, @philnik777 !!

[cjdb]

It's a compound noun, not a proper noun.

Perhaps a different context will help: if we were recommending trees instead of hardening modes, it would be "We recommend most projects adopt the gum tree", not "We recommend most projects adopt the Gum tree".

[philnik777]

My understanding is that that still depends on the context. If we have an individual tree named "Gum", we would recommend the Gum tree, not the gum tree. This can probably be debated, but in the end I don't actually care that much. I haven't studied English and don't plan to. I can't even speak my native language properly.

[hawkinsw]

My understanding is that that still depends on the context. If we have an individual tree named "Gum", we would recommend the Gum tree, not the gum tree. This can probably be debated, but in the end I don't actually care that much. I haven't studied English and don't plan to. I can't even speak my native language properly.

I'm with @philnik777 100%. We've already "wasted" way too much time here!

It's a compound noun, not a proper noun.

Perhaps a different context will help: if we were recommending trees instead of hardening modes, it would be "We recommend most projects adopt the gum tree", not "We recommend most projects adopt the Gum tree".

I agree with this particular example. However, I think that it's slightly inapplicable to the example here. But, again, we've already done way too much bikeshedding here!

Thanks for the work, @cjdb !

[var-const]

(also not a native speaker) FWIW, I think this is purely a matter of style whether to capitalize the names of the modes. I personally prefer to mark them as code using Markdown when supported, while I find capitalization distracting.

[hawkinsw]

Thanks for weighing in @var-const -- that was the reason I suggested capitalization. I just wanted them to stand out a little!

[hawkinsw]

Suggested change

	- **Extensive mode**, which contains all the checks from the fast mode and
	- **Extensive mode**, which contains all the checks from the Fast mode and

[hawkinsw]

Suggested change

	somewhat more significant compared to the fast mode, the extensive mode is
	somewhat more significant compared to the Fast mode, the Extensive mode is

[firewave]

@firewave would a table of contents help in that regard? Readers can then see that there's a section for them regardless of whether they're a user or a vendor.

Not sure. I guess I have to look at the documentation from the building side.

The hardening page should probably just contains all the information on that (since that is generic) and be linked from the "using" and "building" (if there is one) pages with all information on vendor/user stuff possibly contained in those.

Perhaps we need to reorganise the documentation so that it is bifurcated for users, maintainers, and vendors, but I'd prefer to do that in a separate, more general CL.

Just reducing any ambiguity is always helpful. I did not expect this to happen in this PR. Just saw the changes and remembered that I had issues with the documentation so I thought I drop my two cents.

[var-const]

@cjdb Thank you for working on this! Sorry I'm slow with the review (a combination of holidays and being OOO sick). LGTM modulo one comment (feel free to ping me on Discord if you'd like to discuss it further).