This is an archive of the discontinued LLVM Phabricator instance.

[FunctionAttrs] Fix nounwind inference for landingpads
ClosedPublic

Authored by nikic on Apr 6 2023, 3:04 AM.

Details

Summary

Currently, FunctionAttrs treats landingpads as non-throwing, and will infer nounwind for functions with landingpads (and without something like resumes that continue unwinding). There are two problems with this:

  • Non-cleanup landingpads with catch/filter clauses do not necessarily catch all exceptions. Unless there are catch ptr null or filter [0 x ptr] zeroinitializer clauses, we should assume that we may unwind past this landingpad. This seems like an outright bug.
  • Cleanup landingpads are skipped during phase one unwinding, so we effectively need to support unwinding past them. Marking these nounwind is technically correct, but not compatible with how unwinding works in reality.

Fixes https://github.com/llvm/llvm-project/issues/61945.

Diff Detail

Event Timeline

nikic created this revision.Apr 6 2023, 3:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2023, 3:04 AM
nikic requested review of this revision.Apr 6 2023, 3:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2023, 3:04 AM

While we're here, can you also add handling for catchpad/cleanuppad? (A conservative implementation for them is fine for now.)

nikic added a comment.Apr 6 2023, 11:25 AM

While we're here, can you also add handling for catchpad/cleanuppad? (A conservative implementation for them is fine for now.)

I think those are effectively already handled via catchswitch here: https://github.com/llvm/llvm-project/blob/aee4399f58f4826180138fb294b6b0359c892709/llvm/lib/IR/Instruction.cpp#L741-L742

Doesn't that have the same problem as the issue you're trying to fix? Specifically, if there's a catchswitch pointing to a cleanuppad that ends in "unreachable", mayThrow() returns false for both the catchswitch and the cleanuppad.

nikic added a comment.Apr 6 2023, 11:44 AM

Doesn't that have the same problem as the issue you're trying to fix? Specifically, if there's a catchswitch pointing to a cleanuppad that ends in "unreachable", mayThrow() returns false for both the catchswitch and the cleanuppad.

Yeah, you're right. In that case the catchswich is not "unwind to caller" so it won't be considered mayThrow() and we do need separate handling for the cleanuppad.

nikic updated this revision to Diff 512450.Apr 11 2023, 7:34 AM

Also handle cleanuppad.

I think we should fold this logic into mayThrow, e.g., via flags picking a behavior. Other places are broken as well and should default to this semantics.

nikic updated this revision to Diff 512689.Apr 12 2023, 1:41 AM

Move logic to mayThrow() behind a flag. I tried doing this without the flag initially, but this does affect some optimizations.

I've opted to make the invoke throwing, rather than the landingpad itself. I think this more closely matches reality, in that it's not the landingpad that's going to throw, it's the invoke that will fail to catch the exception and continue unwinding.

Note that the logic for non-cleanup landingpads applies always, even without the flag. That part should be independent of phase one unwinding.

efriedma added inline comments.Apr 12 2023, 10:27 AM
llvm/lib/IR/Instruction.cpp
775

The "pad" in the destination on Windows can be either a cleanuppad or a catchswitch; you might need to handle both cases? (Either that, or change the way catchswitch is handled.)

nikic updated this revision to Diff 513090.Apr 13 2023, 1:06 AM

I'm handling the cleanuppad itself as throwing now, so we don't need to consider all the different places it can occur (invoke, catchswitch, cleanupret). As this is for the phase-one case only it doesn't really matter whether the cleanuppad itself unwinds or whatever is calling it.

efriedma accepted this revision.Apr 13 2023, 10:50 AM

Looks fine, unless @jdoerfert has some comment on the API.

This revision is now accepted and ready to land.Apr 13 2023, 10:50 AM
This revision was landed with ongoing or failed builds.Apr 14 2023, 2:46 AM
This revision was automatically updated to reflect the committed changes.