Page MenuHomePhabricator

[libc++abi] Simplify scan_eh_tab
ClosedPublic

Authored by MaskRay on Dec 13 2020, 8:50 PM.

Details

Reviewers
rprichard
compnerd
Group Reviewers
Restricted Project
Commits
rGcfe9ccbddd98: [libc++abi] Simplify scan_eh_tab
Summary

All _URC_HANDLER_FOUND return values need to set landingPad
and its value does not matter for _URC_CONTINUE_UNWIND. So we
can always set landingPad to unify code.

For an exception specification (ttypeIndex < 0), we can check _UA_FORCE_UNWIND first.

The so-called type 3 search (actions & _UA_CLEANUP_PHASE && !(actions & _UA_HANDLER_FRAME))
is actually conceptually wrong. For a catch handler or an unmatched dynamic
exception specification, _UA_HANDLER_FOUND should be returned immediately. It
still appeared to work because the ttypeIndex==0 case would return
_UA_HANDLER_FOUND at a later time.

This patch fixes the conceptual error and simplifies the code by handling type 3
the same way as type 2 (which is also what libsupc++ does).
The only difference between phase 1 and phase 2 is what to do with a cleanup
(actionEntry==0, or a ttypeIndex==0 is found in the action record chain):
phase 1 returns _URC_CONTINUE_UNWIND while phase 2 returns _URC_HANDLER_FOUND.

Diff Detail

Event Timeline

MaskRay requested review of this revision.Dec 13 2020, 8:50 PM
MaskRay created this revision.
Herald added a project: Restricted Project. ยท View Herald TranscriptDec 13 2020, 8:50 PM
Herald added 1 blocking reviewer(s): Restricted Project. ยท View Herald Transcript
Herald added a subscriber: libcxx-commits. ยท View Herald Transcript
MaskRay edited the summary of this revision. (Show Details)Dec 13 2020, 9:01 PM
MaskRay updated this revision to Diff 311489.Dec 13 2020, 10:43 PM

Fix throw nullptr (which requires to adjust the pointer.

MaskRay edited the summary of this revision. (Show Details)Dec 13 2020, 10:46 PM

@mstorsjo Perhaps you can test this as well:)

(I have tested it on x86-64 and an ARM qemu-user environ)

@mstorsjo Perhaps you can test this as well:)

(I have tested it on x86-64 and an ARM qemu-user environ)

๐Ÿ˜ณ

@mstorsjo Perhaps you can test this as well:)

Seems good on mingw on i686 (dwarf), x86_64 (SEH), armv7 (dwarf), aarch64 (SEH).

@mstorsjo Perhaps you can test this as well:)

Seems good on mingw on i686 (dwarf), x86_64 (SEH), armv7 (dwarf), aarch64 (SEH).

Thanks!

May I get a LGTM from an owner? :)

ldionne added inline comments.
libcxxabi/src/cxa_personality.cpp
776โ€“778

I don't understand why we're allowed to remove this. Can you please explain? The same question applies to other removals of call_terminate below.

MaskRay marked an inline comment as done.Jan 11 2021, 4:27 PM
MaskRay added inline comments.
libcxxabi/src/cxa_personality.cpp
738

For regular unwinding: if the search phase returns _URC_HANDLER_FOUND (catch (...) always matches), the cleanup phase will get _UA_HANDLER_FRAME and will match as well. The check here is dead.

For _UA_FORCE_UNWIND, there is no search phase. The code path is skipped.

776โ€“778

For regular unwinding: if the search phase returns _URC_HANDLER_FOUND (can_catch returns true), the cleanup phase will match as well. These are some additional sanity checks but they are not useful and just clutter up the code. Neither libsupc++ nor libcxxrt has the additional checks.

For _UA_FORCE_UNWIND, there is no search phase. The code path is skipped.

808

For regular unwinding: if the search phase returns _URC_HANDLER_FOUND (the exception specifier cannot match the propagated exception), the cleanup phase will have exception_spec_can_catch returning true as well. The check here is dead.

For _UA_FORCE_UNWIND, there is no search phase. The code path is skipped.

MaskRay marked an inline comment as done.Jan 11 2021, 4:27 PM
smeenai added subscribers: compnerd, smeenai.

@compnerd is already part of the libc++abi group, but I'm also adding him explicitly in case he has the time to look, since he has a lot of relevant experience.

@compnerd is already part of the libc++abi group, but I'm also adding him explicitly in case he has the time to look, since he has a lot of relevant experience.

Ping @compnerd :)

I like the clean up here, but, I think there is some value in retaining the defensive sanity checks. You are correct that in most cases, this should never be hit in a proper unwind, but because this may be run in a last chance scenario, having the defensive checks is good for catching things going wrong.

libcxxabi/src/cxa_personality.cpp
738

Correct, this was a sanity check path. I think that rewriting it as an explicit check and terminate (which is annotated as being cold even) would be nice to retain. At the very least, we should assert that.

MaskRay updated this revision to Diff 318076.Jan 20 2021, 5:53 PM

Add assert

MaskRay marked an inline comment as done.Jan 20 2021, 5:54 PM
MaskRay updated this revision to Diff 318080.Jan 20 2021, 6:00 PM

fix an assert and comment

MaskRay updated this revision to Diff 318081.Jan 20 2021, 6:02 PM

forgot to include a paren change in the previous diff

compnerd accepted this revision.Jan 21 2021, 8:55 AM

Looks reasonable enough to me. Please ensure that @ldionne is okay with it as well.

This revision is now accepted and ready to land.Jan 21 2021, 8:55 AM

This really isn't in my area, so I'm deferring to @compnerd . Please feel free to go ahead. :-)

MaskRay updated this revision to Diff 318328.Jan 21 2021, 3:16 PM
MaskRay edited the summary of this revision. (Show Details)

Check _UA_FORCE_UNWIND first for exception specification

This revision was landed with ongoing or failed builds.Jan 21 2021, 3:19 PM
This revision was automatically updated to reflect the committed changes.