Simplify the __assertion_handler build logic. Be friendly to IDEs.

[EricWF]

Instead of having no default __assertion_handler in-tree and depending
on the build system to produce it, this change makes the default
assertion handler just be __assertion_handler.

Users can still override the file in the same way, by providing the
LIBCXX_ASSERTION_HANDLER_FILE Cmake variable, and everything still works
the same.

However, with this change we've

1. Simplified people reading and writing our headers. Having out-of-tree
   generated headers make development harder for everyone.

2. Made the in-tree headers, which are the files developers edit,
   actually compileable so IDE's can (with a little more help) actually
   consume these headers.

3. Decoupled the validity of the in-tree headers from the build system.

Ideally, we should be building using the in-tree headers with a
generated directory, similar to the target generated include dir, for
storing configure-time generated files. Historically there has been
push-back against this that has been addressed by better CI and testing.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Eric (EricWF)

Changes

Instead of having no default __assertion_handler in-tree and depending
on the build system to produce it, this change makes the default
assertion handler just be __assertion_handler.

Users can still override the file in the same way, by providing the
LIBCXX_ASSERTION_HANDLER_FILE Cmake variable, and everything still works
the same.

However, with this change we've

1. Simplified people reading and writing our headers. Having out-of-tree
   generated headers make development harder for everyone.

2. Made the in-tree headers, which are the files developers edit,
   actually compileable so IDE's can (with a little more help) actually
   consume these headers.

3. Decoupled the validity of the in-tree headers from the build system.

Ideally, we should be building using the in-tree headers with a
generated directory, similar to the target generated include dir, for
storing configure-time generated files. Historically there has been
push-back against this that has been addressed by better CI and testing.

---

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

4 Files Affected:

• (modified) libcxx/CMakeLists.txt (+1-1)
• (modified) libcxx/docs/BuildingLibcxx.rst (+7-6)
• (renamed) libcxx/include/__assertion_handler ()
• (modified) libcxx/include/module.modulemap (+1)

diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
index f34cb178e076e..6c80dc2f66070 100644
--- a/libcxx/CMakeLists.txt
+++ b/libcxx/CMakeLists.txt
@@ -70,7 +70,7 @@ if (NOT "${LIBCXX_HARDENING_MODE}" IN_LIST LIBCXX_SUPPORTED_HARDENING_MODES)
     "Unsupported hardening mode: '${LIBCXX_HARDENING_MODE}'. Supported values are ${LIBCXX_SUPPORTED_HARDENING_MODES}.")
 endif()
 set(LIBCXX_ASSERTION_HANDLER_FILE
-  "${CMAKE_CURRENT_SOURCE_DIR}/vendor/llvm/default_assertion_handler.in"
+  "${CMAKE_CURRENT_SOURCE_DIR}/include/__assertion_handler"
   CACHE STRING
   "Specify the path to a header that contains a custom implementation of the
    assertion handler that gets invoked when a hardening assertion fails. If
diff --git a/libcxx/docs/BuildingLibcxx.rst b/libcxx/docs/BuildingLibcxx.rst
index e425b9dadfe7d..b4fed5ddec0db 100644
--- a/libcxx/docs/BuildingLibcxx.rst
+++ b/libcxx/docs/BuildingLibcxx.rst
@@ -417,7 +417,7 @@ libc++ Feature Options
 
 .. option:: LIBCXX_ASSERTION_HANDLER_FILE:PATH
 
-  **Default**:: ``"${CMAKE_CURRENT_SOURCE_DIR}/vendor/llvm/default_assertion_handler.in"``
+  **Default**:: ``"${CMAKE_CURRENT_SOURCE_DIR}/include/__assertion_handler"``
 
   Specify the path to a header that contains a custom implementation of the
   assertion handler that gets invoked when a hardening assertion fails. If
@@ -505,11 +505,12 @@ Under the hood, a hardening assertion will invoke the
 that contains a custom definition of this macro and specify the path to the
 header via the ``LIBCXX_ASSERTION_HANDLER_FILE`` CMake variable. If provided,
 this header will be included by the library and replace the default
-implementation. The header must not include any standard library headers
-(directly or transitively) because doing so will almost always create a circular
-dependency. The ``_LIBCPP_ASSERTION_HANDLER(message)`` macro takes a single
-parameter that contains an error message explaining the hardening failure and
-some details about the source location that triggered it.
+implementation. The header may only include `__config`, `__config_site`,
+and `__verbose_abort`, The custom header must not include any other standard
+library headers (directly or transitively) because doing so will almost always
+create a circular dependency. The ``_LIBCPP_ASSERTION_HANDLER(message)`` macro
+takes a single parameter that contains an error message explaining the hardening
+failure and some details about the source location that triggered it.
 
 When a hardening assertion fails, it means that the program is about to invoke
 library undefined behavior. For this reason, the custom assertion handler is
diff --git a/libcxx/vendor/llvm/default_assertion_handler.in b/libcxx/include/__assertion_handler
similarity index 100%
rename from libcxx/vendor/llvm/default_assertion_handler.in
rename to libcxx/include/__assertion_handler
diff --git a/libcxx/include/module.modulemap b/libcxx/include/module.modulemap
index 8bc94d71391ec..887ccc475a0fd 100644
--- a/libcxx/include/module.modulemap
+++ b/libcxx/include/module.modulemap
@@ -568,6 +568,7 @@ module std [system] {
 // must not be directly imported.
 module std_private_assert            [system] {
   header "__assert"
+  header "__assertion_handler"
   export *
 }
 module std_private_availability      [system] {

[ldionne]

I think we are coming from fundamentally different angles here. I think this nicely captures where I disagree:

Decoupled the validity of the in-tree headers from the build system.

Our headers are fundamentally coupled to the build system because of things like __config_site. I think we're making everyone a disservice if we pretend that libcxx/include can be used for anything but generating an actual <install>/include directory. IMO it is better to instead have a clear cut between libcxx/include and <install>/include which doesn't cause confusion because of subtle differences. For example, I find it very surprising that if you pass -D LIBCXX_ASSERTION_HANDLER_FILE =/path/to/foo, <INSTALL_DIR>/include/c++/v1/__assertion_handler would contain the contents of /path/to/foo, yet libcxx/include would still contain the default implementation. Instead, not having a header in libcxx/include at all seems to more clearly capture the subtlety that's going on under the hood.

[EricWF]

This change respects your position entirely. I was careful to do that. Could you please do the same for me?

To respect your views, I made sure that this change had no effect on how the library is built and tested.
This change makes all configurations a little simpler. It's clearer where the default file comes from, while allowing users to override it.

Here's my issue. Developers read, edit, and work with the in-tree headers. Developers like myself often rely on IDE's and other tooling to highlight, autocomplete, diagnose, and otherwise transform the source. I find these tool assists invaluable. However, almost all tools give up after encountering a missing header. Because the build overwrites the out-of-tree includes every build, editing out of tree is not a viable solution.

This change alone is enough to better support developer tooling in almost all cases.

Where can we find consensus?

[ldionne]

Let's say you configure libc++ with a custom assertion handler. Will your IDE assume that libcxx/include/__assertion_handler is being used even though the configuration says otherwise?

If so, then isn't this the wrong way to fix this issue? Couldn't we instead teach the IDE what we're really doing? What IDE are you using, by the way?

[EricWF]

Let's say you configure libc++ with a custom assertion handler. Will your IDE assume that libcxx/include/__assertion_handler is being used even though the configuration says otherwise?

Very good point. Since you're open to a more holistic solution, this would be a problem.

If so, then isn't this the wrong way to fix this issue? Couldn't we instead teach the IDE what we're really doing? What IDE are you using, by the way?

I use CLion, but you can think of the problem very generally. In order for a tool to understand a file, it needs to know how to compile with it. For example, if you can compile a file using clang++ <whatever-flags-you-want> -I libcxx/include <whatever-other-flags-you-want>, then you can make tooling work.

I think the most appropriate solution given our recent discussion is to simply generate the __assertion_handler into the target include directory rather than the one containing the header copies. This would work everywhere except on Apple platforms.

(Which would allow the command line clang++ -nostdinc++ -I build/include/<target>/c++/v1 -I libcxx/include ...)

[EricWF]

I was wrong. Moving it to the target include directory doesn't work because it introduces a layering violation.

I'll do some digging into why __assertion_handler as a separate configurable file is needed at all.
There are likely other idiomatic means to allow overriding the handler.

Where are the requirements on the vendor override documented?
Can it include other C++ or C headers? Must the implementation be header only?
What vendors are currently using configuration knob?

[EricWF]

@var-const ^^^

[ldionne]

I was wrong. Moving it to the target include directory doesn't work because it introduces a layering violation.

I'll do some digging into why __assertion_handler as a separate configurable file is needed at all. There are likely other idiomatic means to allow overriding the handler.

You're welcome to, but TBH we've been there before and we basically tried all the possible different ways of customizing this before we ended up where we are. We tried link-time overriding, but that is too heavy when you need to generate a single instruction to trap (see below). We tried compile-time overriding via a macro (by defining _LIBCPP_VERBOSE_ABORT), but that doesn't scale if you want to override the handler for a whole platform/company/environment/stack/whatever, since everyone now has to define that macro.

So we ended up where we are. Note that any kind of configuration-time tweakability is going to throw a wrench into the IDE, I think. It's not the fact that it's a separate header that is problematic, it's the fact that there is some configuration-time tweaking of the headers at all. So even if __assertion_handler were to be a .in file that gets configured via CMake, it still wouldn't be valid C++ until it gets configured, which is what the IDE needs.

Where are the requirements on the vendor override documented?

The current design is the result of various iterations on __libcpp_verbose_abort, and the reason for the handler being overridable at configuration time is captured in https://discourse.llvm.org/t/rfc-hardening-in-libc/73925/23:

Our current thinking is to only provide two ways of customizing verbose abort:

A new way that would use __config_site.in and essentially allow a vendor to patch the code in __assert that defines the verbose abort function.

The current way that allows redefining the macro at compile time (aimed at users).
We’d prefer to not support link-time overriding. The reason is that we would like the default implementation to compile to a single instruction. The mechanism for link-time overriding would necessarily incur a function call that would be impossible to optimize out.

What vendors are currently using configuration knob?

We are, and Chrome is too. There may be others, I'm not certain.

[var-const]

@EricWF

Where are the requirements on the vendor override documented?

The best current documentation is at #92021 , but I don't think it goes into the level of detail you mention.

Can it include other C++ or C headers? Must the implementation be header only?

It can include other headers as long as it doesn't create circular dependencies (which most standard includes would create). It doesn't need to be header-only.

What vendors are currently using configuration knob?

For documentation purposes, I think that would go into too much detail. This was certainly welcomed by vendors when RFC was discussed.

There are likely other idiomatic means to allow overriding the handler.

I can't (at least immediately) see a better way rather than providing an overridable macro; if that's the case, it becomes a question of finding a good way to override a macro. Using a custom header for the purpose is convenient because it allows doing includes, providing helper inline functions, etc., in a pretty natural way.

Taking a step back, how significant is the issue? This is a pretty low-level header that is not configurable by users, only vendors. I feel the vast majority of users would never have to interact with it (or even be aware of its existence). Even for vendors and library developers, I don't expect this to be a part of code that is being interacted with often.

[cjdb]

Would it be possible to add a CMake option to inform the build system that libc++ is being worked on (e.g. -DLIBCXX_ENABLE_DEVELOPER_MODE=Yes), that autogenerates the __assertion_handler header for in-tree use?

IIUC, this would solve @EricWF's problem without affecting the concerns raised by @ldionne and @var-const.

[EricWF]

I'm going to close this while I look into better solutions.