Page MenuHomePhabricator

[libc++abi] Simplify __gxx_personality_v0
ClosedPublic

Authored by MaskRay on Dec 13 2020, 3:57 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG85f86e8a3cf9: [libc++abi] Simplify __gxx_personality_v0
Summary

In three cases we call scan_eh_tab to parse LSDA:

  • actions & _UA_SEARCH_PHASE
  • actions & _UA_CLEANUP_PHASE && actions & _UA_HANDLER_FRAME && !native_exception
  • actions & _UA_CLEANUP_PHASE && !(actions & _UA_HANDLER_FRAME)

Check `actions & _UA_CLEANUP_PHASE && actions & _UA_HANDLER_FRAME &&
native_exception` first, then we can move three scan_eh_tab onto one place.

Another simplification is that we can check whether the result of scan_eh_tab
is _UA_CONTINUE_UNWIND or _UA_FATAL_PHASE1_ERROR first. Then many of the
original checks will be dead and can thus be deleted.

Diff Detail

Event Timeline

MaskRay requested review of this revision.Dec 13 2020, 3:57 PM
MaskRay created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2020, 3:57 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
MaskRay edited the summary of this revision. (Show Details)Dec 13 2020, 3:57 PM

So, just to clarify because I'm really not familiar with this code -- this is supposed to be a NFC change, right? My understanding so far is that this doesn't change functionality, but I'd like to confirm. Also, do you know someone else that knows this code and can give a good review?

libcxxabi/src/cxa_personality.cpp
969

I don't think it hurts readability to leave it as __cxa_exception* exception_header, despite the cast.

MaskRay updated this revision to Diff 315198.Jan 7 2021, 12:09 PM
MaskRay edited the summary of this revision. (Show Details)

auto* -> __cxa_exception*

MaskRay added a subscriber: mstorsjo.EditedJan 7 2021, 12:15 PM

So, just to clarify because I'm really not familiar with this code -- this is supposed to be a NFC change, right? My understanding so far is that this doesn't change functionality, but I'd like to confirm. Also, do you know someone else that knows this code and can give a good review?

Yes. Both this and D93190 are NFC. check-cxxabi (libcxxabi/test/*catch* libcxxabi/test/*except*) is actually quite comprehensive.
The two main functions are mostly intact Howard after wrote them in 2012 (@mstorsjo has at least added the SEH part so may be familiar).
If it helps, I've written down the simplified version of scan_eh_tab and __gxx_personality_v0 (what they should be like) in https://maskray.me/blog/2020-12-12-c++-exception-handling-abi

Both libsupc++ and libcxxrt are simpler than our version. With the two refactoring our version should be less verbose.

FWIW, while I've touched this code on the outside, I'm not really familiar with it. But I can at least try out the patch in my setups (dwarf + SEH) and let you know whether it seems to work as intended there.

FWIW, while I've touched this code on the outside, I'm not really familiar with it. But I can at least try out the patch in my setups (dwarf + SEH) and let you know whether it seems to work as intended there.

Looks like it works fine in with my exception handling test set for mingw.

ldionne accepted this revision.Jan 7 2021, 1:58 PM

In that case, this LGTM. I think we do have a problem of code ownership in this area, but it's a separate topic!

This revision is now accepted and ready to land.Jan 7 2021, 1:58 PM
This revision was automatically updated to reflect the committed changes.