This is an archive of the discontinued LLVM Phabricator instance.

[WinEH] Verify unwind edges against EH pad tree
ClosedPublic

Authored by JosephTremoulet on Jan 7 2016, 11:56 AM.

Details

Summary

Funclet EH personalities require a tree-like nesting among funclets
(enforced by the ParentPad linkage in the IR), and also require that
unwind edges conform to certain rules with respect to the tree:

  • An unwind edge may exit 0 or more ancestor pads
  • An unwind edge must enter exactly one EH pad, which must be distinct from any exited pads
  • A cleanupret's edge must exit its cleanuppad

Describe these rules in the LangRef, and enforce them in the verifier.

Diff Detail

Event Timeline

JosephTremoulet retitled this revision from to [WinEH] Verify unwind edges against EH pad tree.
JosephTremoulet updated this object.
JosephTremoulet added a subscriber: llvm-commits.
majnemer accepted this revision.Jan 7 2016, 2:58 PM
majnemer edited edge metadata.

LGTM

lib/IR/Verifier.cpp
2953

Would this be the same as FromPad = CSI; ? I'm not advocating one form over the other, just curious for my own edification. If I understand correctly, it would also imply that a catchswitch cannot unwind to itself.

This revision is now accepted and ready to land.Jan 7 2016, 2:58 PM
andrew.w.kaylor added inline comments.Jan 7 2016, 4:12 PM
docs/ExceptionHandling.rst
801

I found this line confusing. The cleanupret and catchswitch parts of the parenthetical clause seem to be redundant, having already been covered by previous statements, so it took me a bit to figure out how they related here to the call statement. I also think it would be better to leave the explicit verbiage explaining that a call may unwind to the parent function -- "implicitly via a call (which unwinds all the way to the current function's caller)".

docs/LangRef.rst
5597

This would read better as "beginning with either a cleanuppad or catchswitch instruction".

5598

Should "must have..." here be "must be..."?

lib/IR/Verifier.cpp
2937

Won't this be the same as the 'I' argument?

JosephTremoulet edited edge metadata.

address review feedback

JosephTremoulet marked 5 inline comments as done.Jan 8 2016, 1:13 PM

Feedback addressed.

docs/ExceptionHandling.rst
801

Updated

docs/LangRef.rst
5597

Updated (here and catchswitch)

5598

Changed from "unwind edge must have a legal target" to "unwind destination must be a legal target" (here and catchswitch)

lib/IR/Verifier.cpp
2937

Yep, thanks; kept the ToPad name to help readability below, but assigned from &I and rewrote appearances of &I to ToPad below.

2962

Sounds like you're understanding correctly. I thought about it, and think it's preferable to use CSI and catch the self-switch case here, so I've updated to do so and included a test. Also realized looking at this that we're allowing catchswitches to target their child catchpads, so added a check for that in D16011.

andrew.w.kaylor accepted this revision.Jan 8 2016, 1:49 PM
andrew.w.kaylor edited edge metadata.

Looks good.

JosephTremoulet marked 5 inline comments as done.