-
Notifications
You must be signed in to change notification settings - Fork 11.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[libc++][hardening] Rework how the assertion handler can be overridden. #77883
[libc++][hardening] Rework how the assertion handler can be overridden. #77883
Conversation
Previously there were two ways to override the verbose abort function which gets called when a hardening assertion is triggered: - compile-time: define the `_LIBCPP_VERBOSE_ABORT` macro; - link-time: provide a definition of `__libcpp_verbose_abort` function. This patch adds a new configure-time approach: a vendor can provide a path to a custom header file whose contents will get injected into the build by CMake. The header must provide a definition of the `_LIBCPP_ASSERTION_HANDLER` macro which is what will get called should a hardening assertion fail. As of this patch, overriding `_LIBCPP_VERBOSE_ABORT` will still work, but the previous mechanisms will be effectively removed in a follow-up patch, making the configure-time mechanism the sole way of overriding the default handler. Note that `_LIBCPP_ASSERTION_HANDLER` only gets invoked when a hardening assertion fails. It does not affect other cases where `_LIBCPP_VERBOSE_ABORT` is currently used (e.g. when an exception is thrown in the `-fno-exceptions` mode). The library provides a default version of the custom header file that will get used if it's not overridden by the vendor. That allows us to always test the override mechanism and reduces the difference in configuration between the pristine version of the library and a platform-specific version.
@llvm/pr-subscribers-libcxx Author: Konstantin Varlamov (var-const) ChangesPreviously there were two ways to override the verbose abort function
This patch adds a new configure-time approach: a vendor can provide Note that The library provides a default version of the custom header file that Full diff: https://github.com/llvm/llvm-project/pull/77883.diff 8 Files Affected:
diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
index 75cb63222da35c..b09836a6ab69c8 100644
--- a/libcxx/CMakeLists.txt
+++ b/libcxx/CMakeLists.txt
@@ -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.")
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
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index 0fe3ab44d2466e..efbce0fee847d7 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -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)
+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}")
@@ -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}"
diff --git a/libcxx/include/__assert b/libcxx/include/__assert
index d4af7e6c7192ab..3c49c96f65a3e4 100644
--- a/libcxx/include/__assert
+++ b/libcxx/include/__assert
@@ -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
@@ -20,7 +20,7 @@
#define _LIBCPP_ASSERT(expression, message) \
(__builtin_expect(static_cast<bool>(expression), 1) \
? (void)0 \
- : _LIBCPP_VERBOSE_ABORT( \
+ : _LIBCPP_ASSERTION_HANDLER( \
"%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
diff --git a/libcxx/include/__assertion_handler.in b/libcxx/include/__assertion_handler.in
new file mode 100644
index 00000000000000..59342a3b0be891
--- /dev/null
+++ b/libcxx/include/__assertion_handler.in
@@ -0,0 +1,13 @@
+// -*- C++ -*-
+#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
diff --git a/libcxx/test/libcxx/lint/lint_headers.sh.py b/libcxx/test/libcxx/lint/lint_headers.sh.py
index ab237c968da7e1..aee10e13b9560c 100644
--- a/libcxx/test/libcxx/lint/lint_headers.sh.py
+++ b/libcxx/test/libcxx/lint/lint_headers.sh.py
@@ -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
diff --git a/libcxx/utils/generate_iwyu_mapping.py b/libcxx/utils/generate_iwyu_mapping.py
index 343538a6cae481..d5d1577e4a2ad6 100644
--- a/libcxx/utils/generate_iwyu_mapping.py
+++ b/libcxx/utils/generate_iwyu_mapping.py
@@ -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":
diff --git a/libcxx/utils/libcxx/header_information.py b/libcxx/utils/libcxx/header_information.py
index 54e18b5ea533dd..326931b0081c00 100644
--- a/libcxx/utils/libcxx/header_information.py
+++ b/libcxx/utils/libcxx/header_information.py
@@ -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
diff --git a/libcxx/vendor/llvm/default_assertion_handler.in b/libcxx/vendor/llvm/default_assertion_handler.in
new file mode 100644
index 00000000000000..2422b6dadbe644
--- /dev/null
+++ b/libcxx/vendor/llvm/default_assertion_handler.in
@@ -0,0 +1,14 @@
+// -*- C++ -*-
+#include <__config>
+#include <__verbose_abort>
+
+#if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG
+
+#define _LIBCPP_ASSERTION_HANDLER(...) _LIBCPP_VERBOSE_ABORT(__VA_ARGS__)
+
+#else
+
+// TODO(hardening): in production, trap rather than abort.
+#define _LIBCPP_ASSERTION_HANDLER(...) _LIBCPP_VERBOSE_ABORT(__VA_ARGS__)
+
+#endif // #if _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG
|
@nico Hi Nico, this patch still needs some work to update the documentation, but I wanted to give you a heads-up as soon as possible. This patch is not a breaking change (the new Please let us know if you have any concerns or feedback regarding this direction. Thanks! |
Also pinging @llvm/libcxx-vendors (please see the comment above). |
Thanks for the heads up! This seems very flexible, providing a @alanzhao1 can you take a look too? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of the patch, but I'm not convinced by the current configure approach.
libcxx/CMakeLists.txt
Outdated
"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.") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
libcxx/include/CMakeLists.txt
Outdated
@@ -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) |
There was a problem hiding this comment.
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.
libcxx/docs/UsingLibcxx.rst
Outdated
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
+1, thanks for the change and the heads up! One thing we (Chrome) will need to keep in mind is that we'll need to provide our own implementation of |
✅ With the latest revision this PR passed the C/C++ code formatter. |
Thanks for the feedback! One thing I'd like to mention is that based on @mordante's comment, we're thinking to change/simplify things a little bit:
If using CMake, I think this change should be completely invisible. But since you mentioned you don't use CMake, I just wanted to run it by you to make sure it doesn't make things worse for your use case. Thanks! (I hope to update the patch by end of day to implement the new mechanism outlined above) |
- Reformat tests; - Remove the header from test scripts in cases where it's no longer relevant.
@@ -0,0 +1,23 @@ | |||
// -*- C++ -*- |
There was a problem hiding this comment.
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 __
?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
libcxx/include/CMakeLists.txt
Outdated
@@ -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) | |||
configure_file("${LIBCXX_ASSERTION_HANDLER_FILE}" "${LIBCXX_GENERATED_INCLUDE_DIR}/__assertion_handler" @ONLY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I first tried using file(COPY ...)
, but there seem to be several downsides to it:
- It doesn't support changing the filename (I think). The newer (and simpler)
file(COPY_FILE ...)
command does, but it's only available starting from CMake3.21
, which is above the minimum version we support (3.20
). I tried renaming after copying, but it looks pretty ugly:
file(COPY "${LIBCXX_ASSERTION_HANDLER_FILE}" DESTINATION "${LIBCXX_GENERATED_INCLUDE_DIR}")
cmake_path(GET LIBCXX_ASSERTION_HANDLER_FILE FILENAME _assertion_handler_short_name)
file(RENAME "${LIBCXX_GENERATED_INCLUDE_DIR}/${_assertion_handler_short_name}" "${LIBCXX_GENERATED_INCLUDE_DIR}/__assertion_handler")
- The docs state the following:
An important difference is that configure_file() creates a dependency on the source file, so CMake will be re-run if it changes. The file(COPY_FILE) sub-command does not create such a dependency.
(It is said about COPY_FILE
but seems to apply to COPY
as well)
This seems to be a downside. Testing locally, I can confirm that modifying the header triggers a rebuild of cxx-test-depends
if I use configure_file
but not if I use file(COPY ...)
.
IIUC, the COPYONLY
option prevents variables from being expanded, making the behavior identical. Let me know if I'm missing anything!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Tagging @mordante to make sure this comment doesn't get lost among various threads)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once these few comments are addressed!
@@ -0,0 +1,23 @@ | |||
// -*- C++ -*- |
There was a problem hiding this comment.
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
.
into the generic include directory).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM w/ comments, once CI is green. It seems like the Windows CI is super backed up right now, I think it would be fine to merge without waiting for that signal. But if you do, let's keep an eye on the CI to make sure this doesn't fail.
@@ -0,0 +1,23 @@ | |||
// -*- C++ -*- |
There was a problem hiding this comment.
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.
For the record, the CI was green at the time of merging except for the Buildkite jobs which were still pending (specifically the Windows and FreeBSD jobs). I'll monitor their status and make sure to quickly address failures, if any (hopefully not). |
Sorry, I didn't see the ping. Have you considered putting this in __config_site? We (and I suppose others) already have to generate that one. |
After reading this for a bit, putting the macro in __config_site probably doesn't work, due to circular includes. Can you say a bit more about which problem this solves? Also, since the file is only getting copied, why is it called I think on our end, for layering reasons, we'll have to provide a version of this file that calls a weak symbol that calls std::abort and then provide a strong override in our base library that calls our dump override (…since a small number of test binaries don't link in our base library). So we'll basically have to reinvent what's in libc++. Not the end of the world, but a bunch of busywork, and it's not super clear why this approach is better to me. (It also doesn't have to be clear to me, of course! I'm still curious why the change was made.) |
Because of llvm/llvm-project#77883, libc++ headers now #include an __assertion_handler header file that is either generated by CMake or provided by the vendor. Since we don't use CMake for libc++, this CL vendors <__assertion_handler> by copying the file default_assertion_handler.in [0], which is the file that CMake uses to generate the default version of <__assertion_handler>. This CL is needed to unblock the libc++ autoroller (since otherwise libc++ won't compile due to the missing header). [0]: https://github.com/llvm/llvm-project/blob/main/libcxx/vendor/llvm/default_assertion_handler.in Bug: 1517992 Change-Id: Ia551bdb706f7e68db914ddf46bf2128cdb1a6284 Fixed: 1517992 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5208502 Reviewed-by: Hans Wennborg <hans@chromium.org> Commit-Queue: Alan Zhao <ayzhao@google.com> Cr-Commit-Position: refs/heads/main@{#1248897}
Update: all the jobs finished successfully. |
A big part of the motivation for this change is to make sure that the codegen can come down to a single instruction (intended to be Also, we feel that this should primarily be controlled by vendors, not users in most cases, and thus should be a configuration-time mechanism. If we ship libc++ on a platform where we can't afford to call We're really sorry it creates additional churn for you, but I think the most important thing is to make sure there is no downgrade in functionality, i.e., you would still be able to use the overrides you're currently using. It's a little hard to balance, but basically we're trying to impose the minimal performance penalty by default while still allowing for full customizability for platforms and projects that need it.
I don't feel very strongly about it, but I felt that the name makes it more obvious that this header is used as an input to the build configuration and not included (or meant to be included) as is from its directory. Also, while we don't currently plan to use e.g. variable expansion, I think it's more of an implementation detail whether the header is copied as-is or somehow modified by build rules. |
…n. (llvm#77883) Previously there were two ways to override the verbose abort function which gets called when a hardening assertion is triggered: - compile-time: define the `_LIBCPP_VERBOSE_ABORT` macro; - link-time: provide a definition of `__libcpp_verbose_abort` function. This patch adds a new configure-time approach: the vendor can provide a path to a custom header file which will get copied into the build by CMake and included by the library. The header must provide a definition of the `_LIBCPP_ASSERTION_HANDLER` macro which is what will get called should a hardening assertion fail. As of this patch, overriding `_LIBCPP_VERBOSE_ABORT` will still work, but the previous mechanisms will be effectively removed in a follow-up patch, making the configure-time mechanism the sole way of overriding the default handler. Note that `_LIBCPP_ASSERTION_HANDLER` only gets invoked when a hardening assertion fails. It does not affect other cases where `_LIBCPP_VERBOSE_ABORT` is currently used (e.g. when an exception is thrown in the `-fno-exceptions` mode). The library provides a default version of the custom header file that will get used if it's not overridden by the vendor. That allows us to always test the override mechanism and reduces the difference in configuration between the pristine version of the library and a platform-specific version.
…bort (#78561) In the hardening modes that can be used in production (`fast` and `extensive`), make a failed assertion invoke a trap instruction rather than calling verbose abort. In the debug mode, still keep calling verbose abort to provide a better user experience and to allow us to keep our existing testing infrastructure for verifying assertion messages. Since the debug mode by definition enables all assertions, we can be sure that we still check all the assertion messages in the library when running the test suite in the debug mode. The main motivation to use trapping in production is to achieve better code generation and reduce the binary size penalty. This way, the assertion handler can compile to a single instruction, whereas the existing mechanism with verbose abort results in generating a function call that in general cannot be optimized away (made worse by the fact that it's a variadic function, imposing an additional penalty). See the [RFC](https://discourse.llvm.org/t/rfc-hardening-in-libc/73925) for more details. Note that this mechanism can now be completely [overridden at CMake configuration time](#77883). This patch also significantly refactors `check_assertion.h` and expands its test coverage. The main changes: - when overriding `verbose_abort`, don't do matching inside the function -- just print the error message to `stderr`. This removes the need to set a global matcher and allows to do matching in the parent process after the child finishes; - remove unused logic for matching source locations and for using wildcards; - make matchers simple functors; - introduce `DeathTestResult` that keeps data about the test run, primarily to make it easier to test. In addition to the refactoring, `check_assertion.h` can now recognize when a process exits due to a trap.
After #77883, `fast` mode uses TRAP, and HWASAN replaces TRAP with abort or error exit code. On a quick looks it should be possible to avoid doing that in HWASAN, but historically this is convention for all sanitizers. Changing this behavior may break existing users. Other sanitizers are not affected because they don't install TRAP handlers by default. But if they do, they also replace TRAP with abort/exit.
…bort (#78561) In the hardening modes that can be used in production (`fast` and `extensive`), make a failed assertion invoke a trap instruction rather than calling verbose abort. In the debug mode, still keep calling verbose abort to provide a better user experience and to allow us to keep our existing testing infrastructure for verifying assertion messages. Since the debug mode by definition enables all assertions, we can be sure that we still check all the assertion messages in the library when running the test suite in the debug mode. The main motivation to use trapping in production is to achieve better code generation and reduce the binary size penalty. This way, the assertion handler can compile to a single instruction, whereas the existing mechanism with verbose abort results in generating a function call that in general cannot be optimized away (made worse by the fact that it's a variadic function, imposing an additional penalty). See the [RFC](https://discourse.llvm.org/t/rfc-hardening-in-libc/73925) for more details. Note that this mechanism can now be completely [overridden at CMake configuration time](llvm/llvm-project#77883). This patch also significantly refactors `check_assertion.h` and expands its test coverage. The main changes: - when overriding `verbose_abort`, don't do matching inside the function -- just print the error message to `stderr`. This removes the need to set a global matcher and allows to do matching in the parent process after the child finishes; - remove unused logic for matching source locations and for using wildcards; - make matchers simple functors; - introduce `DeathTestResult` that keeps data about the test run, primarily to make it easier to test. In addition to the refactoring, `check_assertion.h` can now recognize when a process exits due to a trap. NOKEYCHECK=True GitOrigin-RevId: 58780b811c23df3d928d8452ee21c862dde754a2
llvm/llvm-project#77883 adds a way for vendors to override how we handle assertions. If a vendor doesn't want to override it, `CMakeLists.txt` will copy the provided default template assertion handler file (https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/vendor/llvm/default_assertion_handler.in) to libcxx's generated include dir, which defaults to `SYSROOT/include/c++/v1`: https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/CMakeLists.txt#L72-L73 https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/CMakeLists.txt#L439 https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/include/CMakeLists.txt#L1024 We don't use CMake, so this renames the provided `vendor/llvm/default_assertion_handler.in` to `__assert_handler` in our `include` directory, which will be copied to `SYSROOT/include/c++/v1`.
This updates libcxx and libcxxabi to LLVM 18.1.2. Due to llvm/llvm-project#65534 we need to update both at the same time. Things to note: - Disable hardening mode: LLVM 18.1.2 adds support for "Hardening Modes" to libcxx (https://libcxx.llvm.org/Hardening.html). There are four modes: none, fast, extensive and debug. Different hardening modes make different trade-offs between the amount of checking and runtime performance. We for now disable it (i.e., set it to 'none') so that we don't have any effects on the performance. We can consider enabling it when we ever get to enable the debug version of libcxx. - Disable C++20 time zone support: LLVM 18.1.2 adds C++20 time zone support (https://libcxx.llvm.org/DesignDocs/TimeZone.html) to libcxx, which requires access IANA Time Zone Database. Currently it seems it only supports Linux: https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/src/tz.cpp#L45-L49 So this excludes the two source files from build (which is done via `CMakeLists.txt` in the upstream LLVM) and sets `_LIBCPP_HAS_NO_TIME_ZONE_DATABASE` macro in `__config_site`. In future maybe we can consider implementing this in JS. - `__cxa_init_primary_exception` support: llvm/llvm-project#65534 introduces `__cxa_init_primary_exception` and uses this in libcxx. Like several other methods like the below in `cxa_exception.cpp`, https://github.com/emscripten-core/emscripten/blob/fbdd9249e939660cb0d20f00d6bc1897c2f3905e/system/lib/libcxxabi/src/cxa_exception.cpp#L264-L269 this new function takes a pointer to a destructor, and in Wasm destructor returns a `this` pointer, which requires `ifdef __USING_WASM_EXCEPTION__` on its signature. And that it is also used in libcxx means we need to guard some code in libcxx with `ifdef __USING_WASM_EXCEPTION__` for the signature difference, and we need to build libcxx with `-D__USING_WASM_EXCEPTIONS__` when Wasm EH is used. Also we add Emscripte EH's counterpart in `cxa_exception_emscripten.cpp` too. - This version fixes long-running misbehaviors of `operator new` llvm/llvm-project#69498 seems to fix some long-running misbhaviors of `operator new`, which in emscripten we tried to fix in #11079 and #20154. So while resolving the conflicts, I effectively reverted #11079 and #20154 in `libcxx/src/stdlib_new_delete.cpp` and `libcxx/src/new.cpp`. - Add default `__assertion_handler` to `libcxx/include`: llvm/llvm-project#77883 adds a way for vendors to override how we handle assertions. If a vendor doesn't want to override it, CMake will copy the provided [default template assertion handler file](https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/vendor/llvm/default_assertion_handler.in) to libcxx's generated include dir, which defaults to `SYSROOT/include/c++/v1`: https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/CMakeLists.txt#L72-L73 https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/CMakeLists.txt#L439 https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/include/CMakeLists.txt#L1024 We don't use CMake, so this renames the provided `vendor/llvm/default_assertion_handler.in` to `__assert_handler` in our `libcxx/include` directory, which will be copied to `SYSROOT/include/c++/v1`. - Disable `__libcpp_verbose_abort directly` in code: In #20707 we decided to disable `__libcpp_verbose_abort` not to incur the code size increase that brings (it brings in `vfprintf` and its family). We disabled it in adding `#define _LIBCPP_AVAILABILITY_HAS_NO_VERBOSE_ABORT` in `__config_site`. But that `_NO_` macros are gone in LLVM 18.1.2, and changing it to `#define _LIBCPP_AVAILABILITY_HAS_VERBOSE_ABORT 0` does not work because it is overridden by this line, unless we provide our own vendor annotations: https://github.com/llvm/llvm-project/blob/38f5596feda3276a8aa64fc14e074334017088ca/libcxx/include/__availability#L138 I asked about this in llvm/llvm-project#87012 and llvm/llvm-project#71002 (comment) (and more comments below) but didn't get an actionable answer yet. So this disables `__libcpp_verbose_abort` in the code directly for now, hopefully temporarily. Each fix is described in its own commit, except for the fixes required to resolve conflicts when merging our changes, which I wasn't able to commit separately. Fixes #21603.
This updates libcxx and libcxxabi to LLVM 18.1.2. Due to llvm/llvm-project#65534 we need to update both at the same time. Things to note: - Disable hardening mode: LLVM 18.1.2 adds support for "Hardening Modes" to libcxx (https://libcxx.llvm.org/Hardening.html). There are four modes: none, fast, extensive and debug. Different hardening modes make different trade-offs between the amount of checking and runtime performance. We for now disable it (i.e., set it to 'none') so that we don't have any effects on the performance. We can consider enabling it when we ever get to enable the debug version of libcxx. - Disable C++20 time zone support: LLVM 18.1.2 adds C++20 time zone support (https://libcxx.llvm.org/DesignDocs/TimeZone.html) to libcxx, which requires access IANA Time Zone Database. Currently it seems it only supports Linux: https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/src/tz.cpp#L45-L49 So this excludes the two source files from build (which is done via `CMakeLists.txt` in the upstream LLVM) and sets `_LIBCPP_HAS_NO_TIME_ZONE_DATABASE` macro in `__config_site`. In future maybe we can consider implementing this in JS. - `__cxa_init_primary_exception` support: llvm/llvm-project#65534 introduces `__cxa_init_primary_exception` and uses this in libcxx. Like several other methods like the below in `cxa_exception.cpp`, https://github.com/emscripten-core/emscripten/blob/fbdd9249e939660cb0d20f00d6bc1897c2f3905e/system/lib/libcxxabi/src/cxa_exception.cpp#L264-L269 this new function takes a pointer to a destructor, and in Wasm destructor returns a `this` pointer, which requires `ifdef __USING_WASM_EXCEPTION__` on its signature. And that it is also used in libcxx means we need to guard some code in libcxx with `ifdef __USING_WASM_EXCEPTION__` for the signature difference, and we need to build libcxx with `-D__USING_WASM_EXCEPTIONS__` when Wasm EH is used. Also we add Emscripte EH's counterpart in `cxa_exception_emscripten.cpp` too. - This version fixes long-running misbehaviors of `operator new` llvm/llvm-project#69498 seems to fix some long-running misbhaviors of `operator new`, which in emscripten we tried to fix in emscripten-core#11079 and emscripten-core#20154. So while resolving the conflicts, I effectively reverted emscripten-core#11079 and emscripten-core#20154 in `libcxx/src/stdlib_new_delete.cpp` and `libcxx/src/new.cpp`. - Add default `__assertion_handler` to `libcxx/include`: llvm/llvm-project#77883 adds a way for vendors to override how we handle assertions. If a vendor doesn't want to override it, CMake will copy the provided [default template assertion handler file](https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/vendor/llvm/default_assertion_handler.in) to libcxx's generated include dir, which defaults to `SYSROOT/include/c++/v1`: https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/CMakeLists.txt#L72-L73 https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/CMakeLists.txt#L439 https://github.com/llvm/llvm-project/blob/26a1d6601d727a96f4301d0d8647b5a42760ae0c/libcxx/include/CMakeLists.txt#L1024 We don't use CMake, so this renames the provided `vendor/llvm/default_assertion_handler.in` to `__assert_handler` in our `libcxx/include` directory, which will be copied to `SYSROOT/include/c++/v1`. - Disable `__libcpp_verbose_abort directly` in code: In emscripten-core#20707 we decided to disable `__libcpp_verbose_abort` not to incur the code size increase that brings (it brings in `vfprintf` and its family). We disabled it in adding `#define _LIBCPP_AVAILABILITY_HAS_NO_VERBOSE_ABORT` in `__config_site`. But that `_NO_` macros are gone in LLVM 18.1.2, and changing it to `#define _LIBCPP_AVAILABILITY_HAS_VERBOSE_ABORT 0` does not work because it is overridden by this line, unless we provide our own vendor annotations: https://github.com/llvm/llvm-project/blob/38f5596feda3276a8aa64fc14e074334017088ca/libcxx/include/__availability#L138 I asked about this in llvm/llvm-project#87012 and llvm/llvm-project#71002 (comment) (and more comments below) but didn't get an actionable answer yet. So this disables `__libcpp_verbose_abort` in the code directly for now, hopefully temporarily. Each fix is described in its own commit, except for the fixes required to resolve conflicts when merging our changes, which I wasn't able to commit separately. Fixes emscripten-core#21603.
Because of llvm/llvm-project#77883, libc++ headers now #include an __assertion_handler header file that is either generated by CMake or provided by the vendor. Since we don't use CMake for libc++, this CL vendors <__assertion_handler> by copying the file default_assertion_handler.in [0], which is the file that CMake uses to generate the default version of <__assertion_handler>. This CL is needed to unblock the libc++ autoroller (since otherwise libc++ won't compile due to the missing header). [0]: https://github.com/llvm/llvm-project/blob/main/libcxx/vendor/llvm/default_assertion_handler.in Bug: 1517992 Change-Id: Ia551bdb706f7e68db914ddf46bf2128cdb1a6284 Fixed: 1517992 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5208502 Reviewed-by: Hans Wennborg <hans@chromium.org> Commit-Queue: Alan Zhao <ayzhao@google.com> Cr-Commit-Position: refs/heads/main@{#1248897} NOKEYCHECK=True GitOrigin-RevId: 0f6a1d2429de239f87ba3f199c14631d754acd1e
Previously there were two ways to override the verbose abort function
which gets called when a hardening assertion is triggered:
_LIBCPP_VERBOSE_ABORT
macro;__libcpp_verbose_abort
function.This patch adds a new configure-time approach: the vendor can provide
a path to a custom header file which will get copied into the build by
CMake and included by the library. The header must provide a definition of the
_LIBCPP_ASSERTION_HANDLER
macro which is what will get called shoulda hardening assertion fail. As of this patch, overriding
_LIBCPP_VERBOSE_ABORT
will still work, but the previous mechanismswill be effectively removed in a follow-up patch, making the
configure-time mechanism the sole way of overriding the default handler.
Note that
_LIBCPP_ASSERTION_HANDLER
only gets invoked when a hardeningassertion fails. It does not affect other cases where
_LIBCPP_VERBOSE_ABORT
is currently used (e.g. when an exception isthrown in the
-fno-exceptions
mode).The library provides a default version of the custom header file that
will get used if it's not overridden by the vendor. That allows us to
always test the override mechanism and reduces the difference in
configuration between the pristine version of the library and
a platform-specific version.