This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Generalize the customizeable assertion handler
ClosedPublic

Authored by ldionne on Jul 25 2022, 11:47 AM.

Details

Reviewers
philnik
Group Reviewers
Restricted Project
Commits
rG7de5aca84c54: [libc++] Generalize the customizeable assertion handler
Summary

Instead of taking a fixed set of arguments, use variadics so that
we can pass arbitrary arguments to the handler. This is the first
step towards using the handler to handle other non-assertion-related
failures, like std::unreachable and an exception being thrown in
-fno-exceptions mode, which would improve user experience by including
additional information in crashes (right now, we call abort() without
additional information).

Diff Detail

Event Timeline

ldionne created this revision.Jul 25 2022, 11:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2022, 11:47 AM
Herald added a subscriber: nemanjai. · View Herald Transcript
ldionne requested review of this revision.Jul 25 2022, 11:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2022, 11:47 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I'd like to get this change (and the followups) into LLVM 15, since those will impact the ABI, so it's much simpler to do before we've actually released the current (pre-this-patch) ABI. I may have to cherry-pick this on release/15.x.

philnik accepted this revision as: philnik.Jul 25 2022, 11:57 AM
philnik added a subscriber: philnik.

LGTM % nits.

libcxx/src/assert.cpp
28–61

Why are you __uglyfying the names inside the .cpp?

38–39

Duplicate comment?

ldionne updated this revision to Diff 447447.Jul 25 2022, 1:14 PM
ldionne marked 2 inline comments as done.

Address CI failures and comments.

ldionne added inline comments.Jul 25 2022, 1:15 PM
libcxx/src/assert.cpp
28–61

I'm used to it, I guess.

ldionne accepted this revision as: Restricted Project.Jul 26 2022, 4:42 AM

Shipping, the CI issue is something else which I am fixing separately.

This revision is now accepted and ready to land.Jul 26 2022, 4:42 AM
This revision was landed with ongoing or failed builds.Jul 26 2022, 4:43 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jul 26 2022, 9:48 AM
thakis added inline comments.
libcxx/src/assert.cpp
53

This requires API level 21. We still build for older Api levels in some configs. Can this use something that works in older Api levels?

(Also, this looks a bit unrelated to making the handler vararg?)

thakis added inline comments.Jul 27 2022, 5:51 AM
libcxx/src/assert.cpp
53

Ping?

ldionne marked 2 inline comments as done.Jul 28 2022, 7:27 AM
ldionne added inline comments.
libcxx/src/assert.cpp
53

Thanks for the heads up, see D130708.