This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Rename __libcpp_assertion_handler to __libcpp_verbose_abort
ClosedPublic

Authored by ldionne on Jul 26 2022, 5:18 AM.

Details

Summary

With the goal of reusing that handler to do other things besides
handling assertions (such as terminating when an exception is thrown
under -fno-exceptions), the name __libcpp_assertion_handler doesn't
really make sense anymore.

Furthermore, I didn't want to use the name __libcpp_abort_handler,
since that would give the impression that the handler is called
whenever std::abort() is called, which is not the case at all.

Diff Detail

Event Timeline

ldionne created this revision.Jul 26 2022, 5:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2022, 5:18 AM
ldionne requested review of this revision.Jul 26 2022, 5:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2022, 5:18 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne updated this revision to Diff 447721.Jul 26 2022, 8:31 AM
ldionne edited the summary of this revision. (Show Details)

Don't mark the function as noreturn since it makes it impossible to test.

ldionne updated this revision to Diff 447742.Jul 26 2022, 9:52 AM

Try to fix modular build.

ldionne updated this revision to Diff 447822.Jul 26 2022, 2:01 PM

Fix backdeployment test

Some additional information on this patch like I just explained in Discord:

I want to use a verbose abort handler in more places, like when we std::abort due to an exception in -fno-exceptions mode (today the experience sucks cause it only calls std::abort() with no message, so good luck finding where an exception was thrown). In the future, we can also most likely call it from std::unreachable(). It would also allow de-duplicating the same functionality we have in libc++abi.

I have to admit that given this generality, I'm not 100% sure anymore it is worth making it possible for users to override the function, although I guess it doesn't add too much complexity. I'd welcome input on this question and also the naming of the function, documentation changes, etc.

In general LGTM!

I like the idea of using one function for all these ways to terminate the application.

However I'm not thrilled by the name, especially the verbose part. How do you feel about __libcpp_terminate_handler? (I know it's somewhat close to std::terminate.) Another suggestion __libcpp_fatal_error_handler.
However I don't want to make this a bikeshed, so when no better name is found I won't object against the proposed name.

libcxx/docs/UsingLibcxx.rst
213
Mordante accepted this revision as: Mordante.Jul 26 2022, 2:44 PM
philnik accepted this revision.Jul 26 2022, 2:48 PM
philnik added a subscriber: philnik.

I'm not really a fan of the name, but I don't have a better idea. So this LGTM % nit.

libcxx/docs/UsingLibcxx.rst
225–226
This revision is now accepted and ready to land.Jul 26 2022, 2:48 PM
ldionne marked 2 inline comments as done.Jul 29 2022, 6:20 AM

In general LGTM!

I like the idea of using one function for all these ways to terminate the application.

However I'm not thrilled by the name, especially the verbose part. How do you feel about __libcpp_terminate_handler?

There is already std::terminate_handler which does something really different. That's one of the reasons I want to keep handler out of the name, because I don't want folks to get the impression that this is called when std::abort is called, which is not the case. Instead, it's the other way around -- this function will call std::abort after adding some additional information.

In fact, this makes me realize that I should probably drop any mention of a "handler" in the documentation as well.

ldionne updated this revision to Diff 448606.Jul 29 2022, 6:20 AM

Address comments, drop mentions of "handler".

ldionne updated this revision to Diff 448607.Jul 29 2022, 6:22 AM

Drop more mentions of handler.

Will ship once CI is green.

Mordante accepted this revision.Jul 29 2022, 9:54 AM

In general LGTM!

I like the idea of using one function for all these ways to terminate the application.

However I'm not thrilled by the name, especially the verbose part. How do you feel about __libcpp_terminate_handler?

There is already std::terminate_handler which does something really different. That's one of the reasons I want to keep handler out of the name, because I don't want folks to get the impression that this is called when std::abort is called, which is not the case. Instead, it's the other way around -- this function will call std::abort after adding some additional information.

sounds good to me, and as said I don't want to start bikeshedding. So LGTM!