Page MenuHomePhabricator

[libc++] Add a lightweight overridable assertion handler
ClosedPublic

Authored by ldionne on Mar 11 2022, 11:00 AM.

Details

Reviewers
Mordante
Group Reviewers
Restricted Project
Commits
rGb0fd9497af6d: [libc++] Add a lightweight overridable assertion handler
Summary

This patch adds a lightweight assertion handler mechanism that can be
overriden at link-time in a fashion similar to operator new.

This is a third take on https://llvm.org/D121123 (which allowed customizing
the assertion handler at compile-time), and https://llvm.org/D119969
(which allowed customizing the assertion handler at runtime only).

This approach is, I think, the best of all three explored approaches.
Indeed, replacing the assertion handler in user code is ergonomic,
yet we retain the ability to provide a custom assertion handler when
deploying to older platforms that don't have a default handler in
the dylib.

As-is, this patch provides a pretty good amount of backwards compatibility
with the previous debug mode:

  • Code that used to set _LIBCPP_DEBUG=0 in order to get basic assertions in their code will still get basic assertions out of the box, but those assertions will be using the new assertion handler support.
  • Code that was previously compiled with references to __libcpp_debug_function and friends will work out-of-the-box, no changes required. This is because we provide the same symbols in the dylib as we used to.
  • Code that used to set a custom __libcpp_debug_function will stop compiling, because we don't provide that declaration anymore. Users will have to migrate to the new way of setting a custom assertion handler, which is extremely easy. I suspect that pool of users is very limited, so breaking them at compile-time is probably acceptable.

The main downside of this approach is that code being compiled with
assertions enabled but deploying to an older platform where the assertion
handler didn't exist yet will fail to compile. However users can easily
fix the problem by providing a custom assertion handler and defining
the _LIBCPP_AVAILABILITY_CUSTOM_ASSERTION_HANDLER_PROVIDED macro to
let the library know about the custom handler. In a way, this is
actually a feature because it avoids a load-time error that one would
otherwise get when trying to run the code on the older target.

Diff Detail

Event Timeline

ldionne created this revision.Mar 11 2022, 11:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2022, 11:00 AM
ldionne requested review of this revision.Mar 11 2022, 11:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2022, 11:00 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Note that I think it's probably possible to fix the problem of folks back-deploying to older platforms without setting a custom assertion handler by falling back on the old __libcpp_debug_function in those cases. This won't work on Apple, however existing users can't have been relying on it either, because we have never shipped __libcpp_debug_function.

In general I'm happy with this approach. I've some remarks. I like to see the CI green before making a final review.

libcxx/docs/UsingLibcxx.rst
185

Should we mention std::unexpected? This is used prior to C++17.

libcxx/include/__assert
45

I assume you didn't add [[noreturn]] to be compatible with C++98.

libcxx/test/libcxx/assertions/assertion_handler.abort.pass.cpp
11 ↗(On Diff #414705)

Are there already tests _LIBCPP_ASSERT does nothing when -D_LIBCPP_ENABLE_ASSERTIONS=0?

libcxx/utils/generate_feature_test_macro_components.py
944 ↗(On Diff #414705)

This looks suspicious. Most headers don't include __assert.
Is this required for all headers? When it is I prefer to see required part be moved to __config.
Alternatively we need to enforce all headers include version. I added this, but there are no validations executed in the CI.

ldionne updated this revision to Diff 415954.Mar 16 2022, 1:31 PM
ldionne marked 2 inline comments as done.

Address some comments and rebase onto main. Let's see what CI says.

libcxx/docs/UsingLibcxx.rst
185

Since I'm specifically talking about noexcept and not throw(), I think this is sufficient -- std::unexpected will never be called if an exception is thrown from a noexcept function. Please let me know if you disagree and would still like me to add precision, and I can do it.

libcxx/include/__assert
45

Actually, I omitted [[noreturn]] because I wanted to leave the door open for an assertion handler to return. Yes, I know the documentation says users shouldn't do it, but I was thinking that perhaps there are useful applications I'm not thinking about.

I'm not married to this -- do you think I should strengthen the signature and force everybody's handler to be __attribute__((noreturn))? I assume that works with all supported compilers, I can confirm if we agree we want to do this.

libcxx/test/libcxx/assertions/assertion_handler.abort.pass.cpp
11 ↗(On Diff #414705)

I don't think so. I'll add one, thanks.

ldionne added inline comments.Mar 16 2022, 1:36 PM
libcxx/utils/generate_feature_test_macro_components.py
944 ↗(On Diff #414705)

We need to settle on a way users can be *guaranteed* that std::__libcpp_assertion_handler will be declared. I'm currently using "include any libc++ header and it will be declared", but it could be something else, like "include <cassert> and you can be guaranteed it'll be declared", or whatever. It could also be something more libc++ specific like "include <__libcpp_assertion_handler> and it will be declared", but I like that less.

WDYT about including <__assert> in <cassert>, and saying that users only need to include <cassert> in order to define a custom assertion handler? The only thing I'd be slightly worried about is giving the false impression that this assertion handler will be used for assert(...), which is not the case. Actually, while we're at it, do we think that the assert macro should use the custom assertion handler? That's a really good question, I guess.

ldionne updated this revision to Diff 416252.Mar 17 2022, 10:59 AM

Should fix the CI

ldionne updated this revision to Diff 416314.Mar 17 2022, 2:08 PM

Rebase onto main to trigger CI (which should hopefully be fixed now).

Mordante added inline comments.Mar 19 2022, 7:13 AM
libcxx/include/__assert
45

I don't think the strengthening is required. It won't work in C++03 and when there are indeed user who want to return it really becomes UB. I can think of one use case: users using a sanitizer and only want to log asserts to see whether the sanitzers pick up an error.

libcxx/utils/generate_feature_test_macro_components.py
944 ↗(On Diff #414705)

The only issue is, that not every header is required to include version.
git grep -L --no-recursive 'include.*version' -- libcxx/include/ shows a lot of our c headers, barrier and initializer_list.
So it we use version we need to add that to these headers.
Since all new C++ proposals add a new feature-test-macro, I think there's not a big chance of regressions.

So I like the idea users only need to include any Standard header, but then we need to make sure it works for all of them.
I think any header is safer, when it requires <cassert> I'm not sure whether a tool like IWYU will remove the include by accident.

I think using this handler for assert sounds interesting. But I see a bit of an issue, we only ship cassert and not assert.h. I'm not sure whether that can cause subtile issues depending on whether cassert or assert.h is included. But when we go that route it should be done in a separate patch.

ldionne updated this revision to Diff 417676.Mar 23 2022, 10:03 AM
ldionne marked 3 inline comments as done.
ldionne edited the summary of this revision. (Show Details)

Make sure load-time failures can't happen when back-deploying, and add more tests.

I think this should be ready to go if CI is happy, and I'll address comments about being able to customize the default handler after including any header in separate patches, as outlined in the comments.

ldionne added inline comments.Mar 23 2022, 10:04 AM
libcxx/include/__assert
45

SGTM

libcxx/utils/generate_feature_test_macro_components.py
944 ↗(On Diff #414705)

I agree that any header should be sufficient. I propose this because the patch is starting to grow quite a bit: For now, I'll require that <__assert> be included before defining a custom handler. Then, I will relax this requirement in future patches:

  1. Refactor the generated tests that check for per-header properties -- each header should be tested in a different translation unit instead.
  2. Add a test that checks that all public headers provide a declaration for the assertion handler

Once we have that framework in place, we can also easily add a test to check that e.g. all public headers define e.g. _LIBCPP_VERSION (we have such tests today but they are written manually, see e.g. libcxx/test/libcxx/numerics/cfenv/version.pass.cpp).

Mordante accepted this revision as: Mordante.Mar 23 2022, 10:25 AM

LGTM when the CI passes.

libcxx/include/__availability
177

Nice solution using the availability macro!

libcxx/utils/generate_feature_test_macro_components.py
944 ↗(On Diff #414705)

SGTM.

ldionne accepted this revision as: Restricted Project.Mar 23 2022, 12:35 PM
This revision is now accepted and ready to land.Mar 23 2022, 12:35 PM
This revision was automatically updated to reflect the committed changes.
smeenai added a subscriber: smeenai.Aug 4 2022, 4:22 PM
smeenai added inline comments.
libcxx/include/__assert
32

Was the intent here to also catch cases where _LIBCPP_ENABLE_ASSERTIONS is undefined because _LIBCPP_ENABLE_ASSERTIONS_DEFAULT was undefined? If so, this check won't catch that, because the undefined _LIBCPP_ENABLE_ASSERTIONS macro will just evaluate to 0 here. _LIBCPP_ENABLE_ASSERTIONS_DEFAULT is defined in __config_site normally so you wouldn't run into this, but I'm not sure what the desired behavior was if that ended up being undefined (e.g. because you're using a different build system).

ldionne marked an inline comment as done.Aug 17 2022, 12:29 PM
ldionne added inline comments.
libcxx/include/__assert
32

Sorry for not noticing that earlier. I believe my intent was to avoid confusion with _LIBCPP_DEBUG, which allowed other values like 2 to be specified.

ayzhao added a subscriber: ayzhao.Aug 17 2022, 2:46 PM

We tried overriding the abort handler in Chromium, and this fails with a linker error when we target Windows, statically link libc++ along with our custom handler (in base/nodebug_assertion.cc), enable ThinLTO, and build using clang-cl:

lld-link: error: duplicate symbol: void __cdecl std::Cr::__libcpp_verbose_abort(char const *, ...)
>>> defined at ../../base/nodebug_assertion.cc
>>>            obj/base/nodebug_assertion/nodebug_assertion.obj
>>> defined at ../../buildtools/third_party/libc++/trunk/src/verbose_abort.cpp
>>>            obj/buildtools/third_party/libc++/libc++/verbose_abort.obj

I believe this is due to the fact that Windows binaries don't have the concept of weak symbols, so the linker is unable to override the libc++ version.

Since working with weak symbols can be flaky, would it be possible to implement this using function pointers? E.g. make __libcpp_verbose_abort(...) a static function pointer that we call _LIBCPP_ASSERT(...), and add a method __libcpp_override_abort(...) that users can call to set and override that function pointer.

ayzhao added a comment.Sep 2 2022, 4:06 PM

We tried overriding the abort handler in Chromium, and this fails with a linker error when we target Windows, statically link libc++ along with our custom handler (in base/nodebug_assertion.cc), enable ThinLTO, and build using clang-cl:

lld-link: error: duplicate symbol: void __cdecl std::Cr::__libcpp_verbose_abort(char const *, ...)
>>> defined at ../../base/nodebug_assertion.cc
>>>            obj/base/nodebug_assertion/nodebug_assertion.obj
>>> defined at ../../buildtools/third_party/libc++/trunk/src/verbose_abort.cpp
>>>            obj/buildtools/third_party/libc++/libc++/verbose_abort.obj

I believe this is due to the fact that Windows binaries don't have the concept of weak symbols, so the linker is unable to override the libc++ version.

Since working with weak symbols can be flaky, would it be possible to implement this using function pointers? E.g. make __libcpp_verbose_abort(...) a static function pointer that we call _LIBCPP_ASSERT(...), and add a method __libcpp_override_abort(...) that users can call to set and override that function pointer.

It turns out the linker failure we're seeing is caused by lld not supporting weak symbols in LLVM bitcode inputs when outputting COFF. D133165 should hopefully fix this.