Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions libcxx/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,13 @@ if (NOT "${LIBCXX_HARDENING_MODE}" IN_LIST LIBCXX_SUPPORTED_HARDENING_MODES)
message(FATAL_ERROR
"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"
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
provided, the contents of this header will get injected into the library code
and override the default assertion handler.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code it feels somewhat clunky to me.
Why would the following approach not work?

set(LIBCXX_ASSERTION_HANDLER_FILE
  "${CMAKE_CURRENT_SOURCE_DIR}/vendor/llvm/default_assertion_handler.h"
  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
   provided, this header will be used instead of libc++'s default handler.")

Then in libcxx/include/CMakeLists.txt you copy this file and no configuring at all.

The current approach would result in a file below or am I missing something.

// -*- C++ -*-
#ifndef _LIBCPP___ASSERTION_HANDLER
#define _LIBCPP___ASSERTION_HANDLER

#include <__config>

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
#  pragma GCC system_header
#endif

// -*- C++ -*-
#include <__config>
#include <__verbose_abort>

#if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG

#define _LIBCPP_ASSERTION_HANDLER(error_message, ...) ((void)error_message, _LIBCPP_VERBOSE_ABORT(__VA_ARGS__))

#else

// TODO(hardening): in production, trap rather than abort.
#define _LIBCPP_ASSERTION_HANDLER(error_message, ...) ((void)error_message, _LIBCPP_VERBOSE_ABORT(__VA_ARGS__))

#endif // #if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG

#endif // _LIBCPP___ASSERTION_HANDLER

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Injecting the contents of the header should give us a little more control over the custom handler. In particular, it is currently up to the vendor whether they want to allow users to override the macro. To support overriding, a custom implementation might first check that the macro isn't already defined, e.g. on the command line:

// In a vendor-provided `./custom_assertion_handler.h`
#ifndef _LIBCPP_ASSERTION_HANDLER
#define _LIBCPP_ASSERTION_HANDLER(...) platform::assert(__VA_ARGS__)
#endif

That way, a user can compile with -D_LIBCPP_ASSERTION_HANDLER(...)=my_project::special_assert(__VA_ARGS__) and it will be honored. Or the vendor could do the opposite and explicitly forbid overriding:

#ifdef _LIBCPP_ASSERTION_HANDLER
#error "You should not attempt to override the platform-provided assertion handler"
#endif

If in a future release we wanted to make sure that the handler can always be overridden by users, we could change our header to do something like:

#ifndef _LIBCPP_ASSERTION_HANDLER
@_LIBCPP_ASSERTION_HANDLER_OVERRIDE@
#endif

We don't necessarily want to do that, but I think leaving us some degree of flexibility in this regard is useful.

Also, this allows us to provide some internal includes for the custom header in a supported manner. E.g. we can make <__config> available without having the vendor include that internal header explicitly (which we generally don't support), and if in the future we decided to granularize <__config>, we could update the includes without the vendor having to update their code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mordante After writing this, I realized that we can just as well do something like:

#ifndef _LIBCPP_ASSERTION_HANDLER
#include "__custom_assertion_handler.h"
#endif

which alleviates my concern. In fact, I think both approaches have equal expressive power and are not visible to the user/vendor. I think they're very similar, but having a single header seems to be a little simpler/cleaner, so I'll give it a shot based on your suggestion.

option(LIBCXX_ENABLE_RANDOM_DEVICE
"Whether to include support for std::random_device in the library. Disabling
this can be useful when building the library for platforms that don't have
Expand Down
4 changes: 4 additions & 0 deletions libcxx/docs/ReleaseNotes/18.rst
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,10 @@ Deprecations and Removals
macro is provided to restore the previous behavior, and it will be supported in the LLVM 18 release only.
In LLVM 19 and beyond, ``_LIBCPP_ENABLE_NARROWING_CONVERSIONS_IN_VARIANT`` will not be honored anymore.

- The only supported way to customize the assertion handler that gets invoked when a hardening assertion fails
is now by setting the ``LIBCXX_ASSERTION_HANDLER_FILE`` CMake variable and providing a custom header. See
the documentation on overriding the default assertion handler for details.

- The ``_LIBCPP_AVAILABILITY_CUSTOM_VERBOSE_ABORT_PROVIDED`` macro is not honored anymore in LLVM 18.
Please see the updated documentation about the hardening modes in libc++ and in particular the
``_LIBCPP_VERBOSE_ABORT`` macro for details.
Expand Down
86 changes: 33 additions & 53 deletions libcxx/docs/UsingLibcxx.rst
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,9 @@ IWYU, you should run the tool like so:
If you would prefer to not use that flag, then you can replace ``/path/to/include-what-you-use/share/libcxx.imp``
file with the libc++-provided ``libcxx.imp`` file.

.. _termination-handler:
.. _assertion-handler:

Overriding the default termination handler
Overriding the default assertion handler
==========================================

When the library wants to terminate due to an unforeseen condition (such as a hardening assertion
Expand All @@ -158,57 +158,37 @@ provided by the static or shared library, so it is only available when deploying
the compiled library is sufficiently recent. On older platforms, the program will terminate in an
unspecified unsuccessful manner, but the quality of diagnostics won't be great.

However, users can also override that mechanism at two different levels. First, the mechanism can be
overridden at compile time by defining the ``_LIBCPP_VERBOSE_ABORT(format, args...)`` variadic macro.
When that macro is defined, it will be called with a format string as the first argument, followed by
a series of arguments to format using printf-style formatting. Compile-time customization may be
useful to get precise control over code generation, however it is also inconvenient to use in
some cases. Indeed, compile-time customization of the verbose termination function requires that all
translation units be compiled with a consistent definition for ``_LIBCPP_VERBOSE_ABORT`` to avoid ODR
violations, which can add complexity in the build system of users.

Otherwise, if compile-time customization is not necessary, link-time customization of the handler is also
possible, similarly to how replacing ``operator new`` works. This mechanism trades off fine-grained control
over the call site where the termination is initiated in exchange for better ergonomics. Link-time
customization is done by simply defining the following function in exactly one translation unit of your
program:

.. code-block:: cpp

void __libcpp_verbose_abort(char const* format, ...)

This mechanism is similar to how one can replace the default definition of ``operator new``
and ``operator delete``. For example:

.. code-block:: cpp

// In HelloWorldHandler.cpp
#include <version> // must include any libc++ header before defining the function (C compatibility headers excluded)

void std::__libcpp_verbose_abort(char const* format, ...) {
std::va_list list;
va_start(list, format);
std::vfprintf(stderr, format, list);
va_end(list);

std::abort();
}

// In HelloWorld.cpp
#include <vector>

int main() {
std::vector<int> v;
int& x = v[0]; // Your termination function will be called here if hardening is enabled.
}

Also note that the verbose termination function should never return. Since assertions in libc++
catch undefined behavior, your code will proceed with undefined behavior if your function is called
and does return.

Furthermore, exceptions should not be thrown from the function. Indeed, many functions in the
library are ``noexcept``, and any exception thrown from the termination function will result
in ``std::terminate`` being called.
However, vendors can also override that mechanism at CMake configuration time. When a hardening
assertion fails, the library invokes the ``_LIBCPP_ASSERTION_HANDLER`` macro. A vendor may provide
a header 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, the contents of this header will be
injected into library configuration headers, replacing 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.

``_LIBCPP_ASSERTION_HANDLER(error_message, format, args...)`` is a variadic macro that takes the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should just be _LIBCPP_ASSERTION_HANDLER(message) unless we want to be stuck with this interface forever.

Right now, we basically use this exclusively to pass the following arguments: "%s:%d: assertion %s failed: %s\n", __builtin_FILE(), __builtin_LINE(), #expression, message. However, we can do this instead:

_LIBCPP_ASSERTION_HANDLER(__FILE__ ":" _LIBCPP_STRINGIZE(__LINE__) ": assertion " _LIBCPP_STRINGIZE(expression) " failed: " message)

Basically we don't really need the full power of printf-style formatting in our current use case, and anyway __builtin_verbose_trap(...) will require a compile-time string, so that wouldn't work in that world.

For example: https://godbolt.org/z/1r75K7jo5

We don't want to encourage using variadic functions for this, as shown in the godbolt the codegen for calling variadic functions is very poor.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. (This required tweaking how we parse the assertion message in tests a little bit, but that was pretty minor)

I was originally a little concerned that it would make every assertion message slightly larger (since e.g. the assertion failed: part is now "inlined" into every message), but got convinced otherwise during the review:

  • it only matters when the string literal is actually referenced by the program. With the new approach we're aiming for with __builtin_verbose_trap, the string will end up not being referenced by the application and will simply not be stored in the binary (only in the debug metadata), making this a non-issue for all projects using the default assertion handler;
  • even if a project overrides the default handler and references the string, it looks like switching to passing a single parameter to the macro instead of variadics results in noticeably less code being generated (well, the difference is minor, but it affects every single assertion so it adds up). While I haven't measured, it looks like it should offset if not exceed the extra size coming from the string (which is already pretty small). Moreover, for projects using the default handler, this is strictly a binary size improvement (no extra instructions needed for passing variadic arguments).

So all in all, I really like this suggestion! Not doing runtime formatting also removes one more potential point of failure.

following parameters:

* ``error_message`` -- the original error message that explains the hardening failure. In general, it
does not contain information about the source location that triggered the failure.
* ``format`` -- a printf-style format string that contains a general description of the failure with
placeholders for the error message as well as details about the source location.
* ``args...`` -- arguments to substitute in the ``format`` string. The exact order and meaning of the
arguments is unspecified and subject to change (but is always in sync with the format string). Note
that for convenience, ``args`` contain the error message as well.

Programs that wish to terminate as fast as possible may use the ``error_message`` parameter that
doesn't require any formatting. Programs that prefer having more information about the failure (such as
the filename and the line number of the code that triggered the assertion) should use the printf-style
formatting with ``format`` and ``args...``.

When a hardening assertion fails, it means that the program is about to invoke undefined behavior. For
this reason, the custom assertion handler is generally expected to terminate the program. If a custom
assertion handler decides to avoid doing so (e.g. it chooses to log and continue instead), it does so
at its own risk -- this approach should only be used in non-production builds and with an understanding
of potential consequences. Furthermore, the custom assertion handler should not throw any exceptions as
it may be invoked from standard library functions that are marked ``noexcept`` (so throwing will result
in ``std::terminate`` being called).

Libc++ Configuration Macros
===========================
Expand Down
9 changes: 9 additions & 0 deletions libcxx/include/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1019,9 +1019,12 @@ foreach(feature LIBCXX_ENABLE_FILESYSTEM LIBCXX_ENABLE_LOCALIZATION LIBCXX_ENABL
endforeach()

configure_file("__config_site.in" "${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}/__config_site" @ONLY)
file(READ ${LIBCXX_ASSERTION_HANDLER_FILE} _LIBCPP_ASSERTION_HANDLER_OVERRIDE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this changes to something like

diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index 0fe3ab44d246..7546d8f2aed9 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -1020,9 +1020,11 @@ endforeach()
 
 configure_file("__config_site.in" "${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}/__config_site" @ONLY)
 configure_file("module.modulemap.in" "${LIBCXX_GENERATED_INCLUDE_DIR}/module.modulemap" @ONLY)
+file(COPY "${LIBCXX_ASSERTION_HANDLER_FILE}" "${LIBCXX_GENERATED_INCLUDE_DIR}/__assertion_handler")
 
 set(_all_includes "${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}/__config_site"
-                  "${LIBCXX_GENERATED_INCLUDE_DIR}/module.modulemap")
+                  "${LIBCXX_GENERATED_INCLUDE_DIR}/module.modulemap"
+                  "${LIBCXX_GENERATED_INCLUDE_DIR}/__assertion_handler")
 foreach(f ${files})
   set(src "${CMAKE_CURRENT_SOURCE_DIR}/${f}")
   set(dst "${LIBCXX_GENERATED_INCLUDE_DIR}/${f}")

after @mordante 's suggestion. It's a bit simpler indeed, we should probably go for that.

configure_file("__assertion_handler.in" "${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}/__assertion_handler" @ONLY)
configure_file("module.modulemap.in" "${LIBCXX_GENERATED_INCLUDE_DIR}/module.modulemap" @ONLY)

set(_all_includes "${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}/__config_site"
"${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}/__assertion_handler"
"${LIBCXX_GENERATED_INCLUDE_DIR}/module.modulemap")
foreach(f ${files})
set(src "${CMAKE_CURRENT_SOURCE_DIR}/${f}")
Expand Down Expand Up @@ -1059,6 +1062,12 @@ if (LIBCXX_INSTALL_HEADERS)
PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
COMPONENT cxx-headers)

# Install the generated __assertion_handler file to the per-target include dir.
install(FILES "${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}/__assertion_handler"
DESTINATION "${LIBCXX_INSTALL_INCLUDE_TARGET_DIR}"
PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
COMPONENT cxx-headers)

# Install the generated modulemap file to the generic include dir.
install(FILES "${LIBCXX_GENERATED_INCLUDE_DIR}/module.modulemap"
DESTINATION "${LIBCXX_INSTALL_INCLUDE_DIR}"
Expand Down
5 changes: 3 additions & 2 deletions libcxx/include/__assert
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
#ifndef _LIBCPP___ASSERT
#define _LIBCPP___ASSERT

#include <__assertion_handler>
#include <__config>
#include <__verbose_abort>

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
# pragma GCC system_header
Expand All @@ -20,7 +20,8 @@
#define _LIBCPP_ASSERT(expression, message) \
(__builtin_expect(static_cast<bool>(expression), 1) \
? (void)0 \
: _LIBCPP_VERBOSE_ABORT( \
: _LIBCPP_ASSERTION_HANDLER( \
message, \
"%s:%d: assertion %s failed: %s\n", __builtin_FILE(), __builtin_LINE(), #expression, message))

// TODO: __builtin_assume can currently inhibit optimizations. Until this has been fixed and we can add
Expand Down
21 changes: 21 additions & 0 deletions libcxx/include/__assertion_handler.in
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// -*- C++ -*-
//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#ifndef _LIBCPP___ASSERTION_HANDLER
#define _LIBCPP___ASSERTION_HANDLER

#include <__config>

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
# pragma GCC system_header
#endif

@_LIBCPP_ASSERTION_HANDLER_OVERRIDE@

#endif // _LIBCPP___ASSERTION_HANDLER
1 change: 1 addition & 0 deletions libcxx/test/libcxx/lint/lint_headers.sh.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ def exclude_from_consideration(path):
or path.endswith(".modulemap.in")
or os.path.basename(path) == "__config"
or os.path.basename(path) == "__config_site.in"
or os.path.basename(path) == "__assertion_handler.in"
or os.path.basename(path) == "libcxx.imp"
or os.path.basename(path).startswith("__pstl")
or not os.path.isfile(path) # TODO: Remove once PSTL integration is finished
Expand Down
2 changes: 2 additions & 0 deletions libcxx/utils/generate_iwyu_mapping.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ def generate_map(include):
public = []
if i == "__assert":
continue
elif i == "__assertion_handler.in":
continue
elif i == "__availability":
continue
elif i == "__bit_reference":
Expand Down
3 changes: 3 additions & 0 deletions libcxx/utils/libcxx/header_information.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@ def is_modulemap_header(header):
if header == "__config_site":
return False

if header == "__assertion_handler":
return False

# exclude libc++abi files
if header in ["cxxabi.h", "__cxxabi_config.h"]:
return False
Expand Down
22 changes: 22 additions & 0 deletions libcxx/vendor/llvm/default_assertion_handler.in
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// -*- C++ -*-
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to prefix this filename with __?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to, if it gets installed into $PREFIX/usr/include/c++/v1.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I mean this specific file. During installation, it gets renamed to __assertion_handler, so the generated include directory will contain the prefixed version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I would leave it without __, since that makes it more obvious that the two files are named differently.

//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#include <__config>
#include <__verbose_abort>

#if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG

#define _LIBCPP_ASSERTION_HANDLER(error_message, ...) ((void)error_message, _LIBCPP_VERBOSE_ABORT(__VA_ARGS__))

#else

// TODO(hardening): in production, trap rather than abort.
#define _LIBCPP_ASSERTION_HANDLER(error_message, ...) ((void)error_message, _LIBCPP_VERBOSE_ABORT(__VA_ARGS__))

#endif // #if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG