This is an archive of the discontinued LLVM Phabricator instance.

[IR] Reformulate LLVM's EH funclet IR
ClosedPublic

Authored by majnemer on Dec 1 2015, 11:25 PM.

Details

Summary

While we have successfully implemented a funclet-oriented EH scheme on
top of LLVM IR, our scheme has some notable deficiencies:

  • catchendpad and cleanupendpad are necessary in the current design but they are difficult to explain to others, even to seasoned LLVM experts.
  • catchendpad and cleanupendpad are optimization barriers. They cannot be split and force all potentially throwing call-sites to be invokes. This has a noticable effect on the quality of our code generation.
  • catchpad, while similar in some aspects to invoke, is fairly awkward. It is unsplittable, starts a funclet, and has control flow to other funclets.
  • The nesting relationship between funclets is currently a property of control flow edges. Because of this, we are forced to carefully analyze the flow graph to see if there might potentially exist illegal nesting among funclets. While we have logic to clone funclets when they are illegally nested, it would be nicer if we had a representation which forbade them upfront.

Let's clean this up a bit by doing the following:

  • Instead, make catchpad more like cleanuppad and landingpad: no control flow, just a bunch of simple operands; catchpad would be splittable.
  • Introduce catchswitch, a control flow instruction designed to model the constraints of funclet oriented EH.
  • Make funclet scoping explicit by having funclet instructions consume the token produced by the funclet which contains them.
  • Remove catchendpad and cleanupendpad. Their presence can be inferred implicitly using coloring information.

N.B. The state numbering code for the CLR has been updated but the
veracity of it's output cannot be spoken for. An expert should take a
look to make sure the results are reasonable.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
andrew.w.kaylor added inline comments.Dec 2 2015, 5:59 PM
include/llvm/IR/Instructions.h
3861 ↗(On Diff #41591)

Should you also assert(UnwindCase)?

3889 ↗(On Diff #41591)

The word 'handler' seems to be missing here and below.

4142 ↗(On Diff #41591)

Is OuterScope here really the outer scope or is it the CatchSwitch statement?

This feels like a glitch in the inheritance hierarchy. This argument has different significance for CleanupPadInst than it does here, right?

majnemer updated this revision to Diff 41703.Dec 2 2015, 5:59 PM
majnemer edited edge metadata.
  • Move colorEHFunclets to EHPersonalities.h
  • Fix some first draft review comments
docs/ExceptionHandling.rst
697 ↗(On Diff #41591)

This change seems reasonable, I'll give it a shot.

712 ↗(On Diff #41591)

I'll add an example which uses funclet parent tokens.

717–719 ↗(On Diff #41591)

Done.

723–726 ↗(On Diff #41591)

The token consumed by the catchpad prohibits simplifycfg from merging together two catchpads belonging to different catchswitches. While we could make this an IR invariant, we get it for free by encoding it in the instruction.

724 ↗(On Diff #41591)

Done.

docs/LangRef.rst
5328 ↗(On Diff #41591)

Done.

5334 ↗(On Diff #41591)

Done.

5334 ↗(On Diff #41591)

Done.

5342 ↗(On Diff #41591)

I believe that the CatchSwitchInst in-memory representation and the textual representation should have a 1-1 correspondence with the in-memory representation. However, I like the idea of catchswitch within none ....

5345–5346 ↗(On Diff #41591)

Done.

5348 ↗(On Diff #41591)

Done.

5355 ↗(On Diff #41591)

Done.

5389 ↗(On Diff #41591)

Done.

5402 ↗(On Diff #41591)

Done.

5518–5528 ↗(On Diff #41591)

Done.

8627–8632 ↗(On Diff #41591)

Done.

include/llvm/Bitcode/LLVMBitCodes.h
425 ↗(On Diff #41591)

That would gratuitously break bitcode compatibility.

include/llvm/IR/InstrTypes.h
1140 ↗(On Diff #41591)

Done.

1151 ↗(On Diff #41591)

Done.

include/llvm/IR/Instructions.h
3848 ↗(On Diff #41591)

Done.

3947 ↗(On Diff #41591)

Done.

4086 ↗(On Diff #41591)

It makes it easier to keep the code in sync with the public constructors which need the same computation to allocate the memory for the use list.

4114 ↗(On Diff #41591)

Done.

4131 ↗(On Diff #41591)

Same reason as catchpad.

4191 ↗(On Diff #41591)

The operator constraints were a victim working through various iterations of EH cleanups. They didn't end up catching any bugs that the verifier couldn't catch. Even so, the getters are all strongly-typed.

lib/Analysis/CFG.cpp
241 ↗(On Diff #41591)

Done in rL254562

lib/CodeGen/WinEHPrepare.cpp
86 ↗(On Diff #41591)

Nothing changed per-se, I believe we were just getting lucky. The problematic case is iterating over FuncletBlocks in cloneCommonBlocks. We clone each block and insert it after the original. Using a map doesn't ensure any particular order, resulting in the possibility of different output if we cloned a block multiple times for a funclet.

321 ↗(On Diff #41591)

Done.

416–417 ↗(On Diff #41591)

Please feel free :)

lib/IR/Instructions.cpp
994–995 ↗(On Diff #41591)

Done.

1002 ↗(On Diff #41591)

I'm not sure I understand what's wrong with inOutermostScope.

lib/IR/Verifier.cpp
2907 ↗(On Diff #41591)

Done.

2910 ↗(On Diff #41591)

Done.

2916 ↗(On Diff #41591)

Done.

2919 ↗(On Diff #41591)

Done, added in visitEHPadPredecessors.

lib/Transforms/Utils/InlineFunction.cpp
183–203 ↗(On Diff #41591)

Done.

261–264 ↗(On Diff #41591)

Done.

338 ↗(On Diff #41591)

Done.

361–362 ↗(On Diff #41591)

Done.

1110–1115 ↗(On Diff #41591)

Done.

lib/Transforms/Utils/SimplifyCFG.cpp
3282–3283 ↗(On Diff #41591)

Added a comment stating this.

3503–3506 ↗(On Diff #41591)

Added a comment stating this.

test/CodeGen/WinEH/wineh-cloning.ll
551–553 ↗(On Diff #41591)

Done.

test/CodeGen/X86/win32-seh-cleanupendpad.ll
1 ↗(On Diff #41591)

Done.

majnemer added inline comments.Dec 2 2015, 6:03 PM
include/llvm/IR/Instructions.h
3861 ↗(On Diff #41703)

Done.

3889 ↗(On Diff #41703)

Done.

4163 ↗(On Diff #41703)

The way I saw it, the outer scope of a catchpad is it's catchswitch.

majnemer updated this revision to Diff 41715.Dec 2 2015, 9:42 PM
  • Address Andy's latest comments
  • Make the IR a little easier to read
majnemer added inline comments.Dec 2 2015, 11:27 PM
lib/CodeGen/WinEHPrepare.cpp
1858–1860 ↗(On Diff #41715)

Thinking this over, it is not enough for the catchpad to be deleted even if it is immediately followed by unreachable. For the C++ personality, the catchpad denotes program code executed by the personality routine (copy constructors for catch parameters). While grotesque, it is conceivable that the copy-constructor never returns and continues to call other functions, etc.

AFAICT, it is OK if the catch-parameter's type will not result in copy-constructors getting executed.

JosephTremoulet added inline comments.Dec 3 2015, 9:22 AM
docs/ExceptionHandling.rst
757–760 ↗(On Diff #41715)

Bother. I rather liked the idea that "handler", "funclet", and "pad that produces a token" were all interchangeable (modulo a little trickery around SEH catches, but my mental model for that is that EH prep moves the code out of the pad/handler/funclet and then the empty funclet doesn't have any code generated for it). Having catchswitch produce a token means that the "pad tree" has extra nodes as compared to the "funclet tree". But yeah, maybe that's the right way to go.

docs/LangRef.rst
5342 ↗(On Diff #41715)

I can see the merit to keeping that 1-1 correspondence, but honestly then I'd rather achieve that by omitting the None token from both representations for top-level pads, so that we don't have to think about it in the common case that we're looking at or building a top-level pad, and so that someone trying to grok the IR concepts doesn't need to puzzle over it before understanding the nested cases, at the cost of a bit of complexity in the constructors that we'll mostly never look at again and also the cost of having top-level <-> nested rewrites be a replacement rather than a mutation. I don't feel strongly enough to insist, though.

8618–8631 ↗(On Diff #41715)

1 - I don't think the " Catchpad should say something similar, and cleanupret and catchret should either refer to this or describe themselves that it's UB to exit the pad that's not the current top of the handler stack" part is done.

2 - We also need to explain that all of these exits must have the same target. Are you still intending to tag invokes in funclets with bundles? If so, this agreement can be a verifier rule. If not, it's a verifier rule that the cleanuprets have to match, will have to be UB if an invoke doesn't match the cleanupret (and I don't know what we do if there is no cleanupret but there are invokes that go to different places), and could be either UB or a verifier rule that nested catchswitches/cleanuprets/terminatepads have to match the cleanuprets.

include/llvm/Analysis/CFG.h
92 ↗(On Diff #41715)

This is the only change in this file now; do you want to revert it?

include/llvm/Bitcode/LLVMBitCodes.h
425 ↗(On Diff #41715)

Not disagreeing, but trying to understand:

  • What's the issue with the gratuitous change?
  • Does this have to do with the official compatibility promises around releases, would you be doing this differently depending if 3.7 did/didn't include FUNC_CODE_OPERAND_BUNDLE?
  • If we need to avoid breaking bitcode compatibility in this way, do we also need bitcode autoupdate logic that rewrites bitcode using endpads to the new IR?
  • Is the idea that 54 will be unused forever, or is it fair game for the next opcode that gets introduced? Does the answer to that change after 3.8 is released?
include/llvm/IR/Instructions.h
4189 ↗(On Diff #41715)

Sure, the verifier check can in fact catch more bugs and should have been there even with the creation methods strongly-typed -- the benefit of strongly typing the creation methods would be in catching errors statically rather than dynamically and in making the correct pattern a tiny bit more discoverable. That said, I agree it's fine this way.

4220 ↗(On Diff #41715)

Since the catchswitch is the OuterScope of the catchpad that contains this, I think the naming here is a bit confusing -- what would you think of changing this method to getTargetScope or getDestinationScope or something along those lines?

lib/Analysis/EHPersonalities.cpp
52–57 ↗(On Diff #41715)

Should this comment or the method name be changed to reflect the fact that a catchswitch gets its own color but isn't a funclet?

lib/CodeGen/WinEHPrepare.cpp
86 ↗(On Diff #41715)

It wasn't luck. We had non-deterministic output early on, but determinized it in rL246245 by passing around the deterministically-ordered EntryBlocks vector and using it rather than map iteration to drive the outer loop in the cloning code (so that if we cloned a block multiple times we'd do so with a deterministic order of target funclets) and changing the insertion points for the new blocks to be immediately after their originals. Undoing that here seems gratuitous, and IIUC the DenseMap will require a contiguous allocation sized like the number of BasicBlocks, which, while it's not a show-stopper and I'm sure this isn't the only place where we do such a thing, does give me pause, especially when an alternative is available that just needs a contiguous allocation sized like the number of funclets.

1858–1860 ↗(On Diff #41715)

1 - I think you're replying to another review comment here? This one is about dealing with invokes etc. that target something other than their enclosing funclet's unwind destination, but you seem to be discussing the comment about SimplifyCFGOpt::SimplifyUnreachable

2 - regarding SimplifyUnreachable:
A - Yeesh. Are those really not elidable copies? I'd still like to eventually do this, at least for the CoreCLR personality.
B - I realized after making the original comment that it really should be two discrete steps -- a catchpad whose first instruction is unreachable can (at least for CoreCLR) be removed, and, separately, a catchswitch with no catchpads can be clipped out

lib/IR/Instructions.cpp
1002 ↗(On Diff #41715)

There are subtly different types of nesting at play:
1 - Whether a thing is nested inside a funclet
2 - Whether a thing is nested inside a handler
3 - Whether a thing is nested inside an EH pad
4 - Whether a thing is nested inside a try region

ISTM that you're using the term Scope to mean #3 in getOuterScope(), but you're using the same term Scope to mean the union of #3 and #4 in inOutermostScope(). So,
1 - Maybe this is just a naming issue, and getOuterScope() should be getOuterPad() or getParentPad() or getEnclosingPad()?
2 - I'd argue that we've strenuously avoided referencing try blocks as regions/scopes/containers in the IR model, so doing so here seems incongruous.
3 - Even assuming for the sake of argument that we want IR to talk about try scopes, it seems odd that the only way in which it does so is to ask if a handler is for a toplevel try in the root function:
3a - Why wouldn't we want to know whether a handler is for a toplevel try in its enclosing funclet?
3b - Why wouldn't we want a way to get from a handler to the handler for the next-outer try? Actually, we have that, but for cleanups we call it getCleanupRetUnwindDest(), for catchswitch/terminatepad we call it getUnwindDest(), and for catchpads we call it getCatchswitch()->getUnwindDest(). Since all of those use the "unwind destination" terminology, switching to "scope" here is, IMO, confusing.

IOW, I think calling this concept "outermost scope" conflates terminology, and that code needing this predicate would be more readable if callers just explicitly checked the conjunction (OuterScope/EnclosingPad == None && UnwindsToCaller/UnwindDest == nullptr).

lib/IR/Verifier.cpp
2850 ↗(On Diff #41715)

s/it's/its/

lib/Transforms/Utils/InlineFunction.cpp
1109 ↗(On Diff #41715)

This can be assert(NumColors == 1) now, can't it? We should have colored all reachable code, do we ever try to inline an unreachable callsite?

1433 ↗(On Diff #41715)

s/it's/its/

majnemer updated this revision to Diff 41780.Dec 3 2015, 10:54 AM
  • Make the catchswitch syntax more like invoke
docs/ExceptionHandling.rst
761 ↗(On Diff #41780)

This is out of sync with your latest update, right?

docs/LangRef.rst
5368 ↗(On Diff #41780)

The syntax is stale here.

5430 ↗(On Diff #41780)

Stale syntax

include/llvm/IR/Instructions.h
4164 ↗(On Diff #41715)

I suppose that makes some sense. It would be nice to have a comment somewhere in the class definition explaining that.

lib/Analysis/CFG.cpp
21 ↗(On Diff #41715)

Can these includes go away now?

rnk added inline comments.Dec 3 2015, 1:55 PM
lib/CodeGen/AsmPrinter/WinException.cpp
415–419 ↗(On Diff #41780)

The way this is supposed to work is that we're not supposed to emit separate start/end labels for an invoke that happens to be in BaseState. This would occur in this kind of situation:

void f() {
  HasDtor o;
  try {
    g(1);
  } catch(...) {
    g(2); // Also in BaseState
  }
}

In this case, we don't need separate start/end labels for the g(2) call, we can just use the funclet start entry in BaseState. We can treat it just like a potentially throwing call.

443–447 ↗(On Diff #41780)

Yeah, we should be able to do that.

469–473 ↗(On Diff #41780)

I don't think I understand the question.

I think the idea is, when we get to the end of the funclet, if there was some previous state change out of the base state, we should emit label entries to transition back to the base state for the epilogue.

lib/CodeGen/WinEHPrepare.cpp
180–184 ↗(On Diff #41780)

I would expect the CLR to have the same kinds of constraints as C++ here.

The base state is the state of the funclet prologues and epilogues, and it is the state that we previously assigned to the endpads. If there is a plain callsite at the beginning of a funclet before any new exceptional scopes are created, it should be in the base state. If that callsite throws, the runtime should take the appropriate "next action", which would follow the catchswitch unwind edge or the cleanupret unwind edge.

Now that we don't have endpads, we need to identify callsites that unwind out of the funclet. We do this by comparing the unwind destination of the funclet with the unwind destination of each call site, and if they agree, they must be in the base state.

If we don't do this, we would end up in a situation where a call inside a catch funclet gets a state outside of the CatchLow/CatchHigh interval. I'm pretty sure this would confuse CxxFrameHandler3.

andrew.w.kaylor added inline comments.Dec 3 2015, 3:27 PM
docs/LangRef.rst
8579 ↗(On Diff #41780)

This syntax is out-of-date.

lib/Analysis/EHPersonalities.cpp
12 ↗(On Diff #41780)

Is this still needed?

54 ↗(On Diff #41780)

Are there circumstances where the colors don't include the root funclet?

lib/Analysis/ScalarEvolutionExpander.cpp
102 ↗(On Diff #41780)

This will crash if the terminatepad unwinds to caller.

lib/AsmParser/LLParser.cpp
2371 ↗(On Diff #41780)

This comment should have been deleted.

5189 ↗(On Diff #41780)

Just out of curiosity, is there a reason we can't allow "to label %<unwind>"? It's an easy error to make when hand-editing IR.

5206 ↗(On Diff #41780)

/// := catchpad within <catchswitch> [<args>*]

majnemer updated this revision to Diff 41832.Dec 3 2015, 4:53 PM
  • Move colorEHFunclets to EHPersonalities.h
  • Fix some first draft review comments
  • Address Andy's latest comments
  • Make the IR a little easier to read
  • Make the catchswitch syntax more like invoke
  • Address Andy's latest comments.
  • Address Joseph's review comments
  • Remove unneeded #includes
rnk added inline comments.Dec 3 2015, 5:16 PM
lib/Analysis/EHPersonalities.cpp
54 ↗(On Diff #41832)

I think the comment means that the list of colors may include the entry block, which isn't strictly speaking a funclet.

lib/AsmParser/LLParser.cpp
5189–5190 ↗(On Diff #41832)

I like staying consistent with invoke, which does "unwind label %foo".

lib/Analysis/EHPersonalities.cpp
54 ↗(On Diff #41832)

Right, I'm pretty sure I wrote the comment, and that's what I meant -- that the colors *for any given block* may or may not include the root.

lib/CodeGen/AsmPrinter/WinException.cpp
415–419 ↗(On Diff #41780)

Ok, I think I get it now. The comment here isn't quite right, though -- should probably be something like

Indicate a change of state to the base state.  The caller won't expect start/end
EH labels for this transition, which is handy since we won't have them available
in the case that BaseState == NullState and this is a call rather than an invoke.
469–473 ↗(On Diff #41780)

Makes sense now, thanks.

799–804 ↗(On Diff #41832)

Looks like clang-format mangled this comment's line wrapping

lib/CodeGen/WinEHPrepare.cpp
180–184 ↗(On Diff #41780)

Thanks for the explanation. CoreCLR wants what it calls "duplicate clauses" reported for funclets that correspond to handlers which were themselves in try blocks, so we actually do want to report a state range with the outer state number for a call at the top-level of such a funclet, so for CoreCLR BaseState is always NullState to force that reporting, and I hadn't been thinking of them as distinct concepts.

lib/Analysis/EHPersonalities.cpp
54 ↗(On Diff #41832)

Oh, I see. For some reason I read it as saying that the complete set of funclets for the function could "possibly" include the entry block, and it confused me because that will always be a color. I see now that it is actually saying that the set of colors for any given block may (or may not) include the entry block. That makes a lot more sense.

As a minor nit, the commas in this sentence shouldn't be there. I'm going to blame my reading comprehension failure on that.

At the risk of sounding pedantic, it is a little ambiguous as to whether 'F' refers to the set of funclets that are colors for B or a single funclet in that set. From context it has to be a single funclet, but grammatically I think it's the set. I suggest this:

"For any block B the 'colors' of B are the set of funclets (possibly including a root 'funclet' representing the main function) such that each funclet F in that set will need to directly contain B or a copy of B...."

lib/AsmParser/LLParser.cpp
5189–5190 ↗(On Diff #41832)

I'm happy with "unwind label %foo" as the preferred and documented syntax. I was only suggesting that the parser tolerate the presence of a 'to' token here. I suppose that's too imprecise for the philosophy of the IR. I can live with that. The number of people who would ever care is likely to be less than five and may only be one.

rnk added inline comments.Dec 4 2015, 11:08 AM
lib/CodeGen/WinEHPrepare.cpp
180–184 ↗(On Diff #41832)

It sounds like it's OK if C++ and the CLR number these slightly differently:

void f() {
  try {
    g(0); // state 0
    try {
      g(1); // state 1
    } catch( ...) {
      // C++ needs to be in TryHigh+1 to CatchHigh interval
      g(2); // CLR state 0, C++ state 2
    }
  } catch(...) {
  }
}

You actually want to assign the outer state of 0 to the g(2) call site, and the table emitter will do the work of emitting duplicate clauses on the handler.

In that case, I think you can avoid filling in FuncletBaseStateMap, always use BaseState = NullState, and things will work.

JosephTremoulet added inline comments.Dec 6 2015, 5:10 PM
lib/CodeGen/WinEHPrepare.cpp
86 ↗(On Diff #41832)

IIUC the DenseMap will require a contiguous allocation sized like the number of BasicBlocks

Um, sorry, clearly I didn't understand correctly. For one, I meant the MapVector, not the DenseMap, and for two, its vector would be sized like the number of funclets, not the number of blocks. So that's no different footprint-wise than the explicit EntryBlocks vector, and to boot is easier to read and maintain. So, disregard this comment, sorry for the noise.

(and, separately, IIUC now, the DenseMap does indeed fit all its entries in a contiguous allocation, but I'm less concerned with that since the API surface is the same and we can always switch it back to std::map if it makes something blow up [or have DenseMap degrade gracefully or etc])

andrew.w.kaylor added inline comments.Dec 7 2015, 1:11 PM
lib/CodeGen/WinEHPrepare.cpp
169 ↗(On Diff #41832)

Am I right in thinking that you could assert(FuncletPad || FuncletEntryBB == &Fn->getEntryBlock()) here?

215 ↗(On Diff #41832)

This code could really use some comments. I had to build it and step through some examples in the debugger to understand how it works. It would be nice to have a single large comment block somewhere explaining the whole process of state numbering for a C++ function in addition to a few remarks sprinkled throughout this function.

219 ↗(On Diff #41832)

s/no/not

243 ↗(On Diff #41832)

This assigns the same base state to all catchpads within a catchswitch. That seems sketchy to me. Consider the following example:

void foo() {
  try {
    f(1);
  } catch (int) {
    f(2);
    try {
      f(3);
    } catch (...) { }
  } catch (...) {
    f(4);
    try {
      f(5);
    } catch (...) {}
  }
}

Because both outer-level catches get the same base state (1), this state will be assigned to both f(2) and f(4). I think it's possible that __CxxFrameHandler3 will correctly deal with the same state appearing in two different funclets, but I'm not confident of that.

261 ↗(On Diff #41832)

With the new implementation, I don't think it is possible to get here twice for the same cleanup pad. So this could be made an assertion, right?

lib/IR/Verifier.cpp
3011 ↗(On Diff #41832)

Shouldn't this be verifying the unwind destination?

3022 ↗(On Diff #41832)

This should also exclude CatchPadInst.

3048 ↗(On Diff #41832)

This should also exclude CatchPadInst.

andrew.w.kaylor added inline comments.Dec 7 2015, 5:08 PM
lib/Transforms/Utils/InlineFunction.cpp
371 ↗(On Diff #41832)

This needs an RAUW for the catchswitch case.

andrew.w.kaylor added inline comments.Dec 8 2015, 4:18 PM
lib/Transforms/Utils/SimplifyCFG.cpp
3276 ↗(On Diff #41832)

Can a cleanupret ever unwind to a catchpad? If not, this should be CleanupPadInst.

3284 ↗(On Diff #41832)

Can you explain to me the scenario where we can't remove the empty cleanup pad?

I would have expected that for any legal IR there would be something we could do to remove the cleanup pad.

test/CodeGen/WinEH/wineh-cloning.ll
276 ↗(On Diff #41832)

Here, on the other hand, I don't think we need to generalize, since this block won't be cloned.

281 ↗(On Diff #41832)

Is there any reason to keep this test case? There doesn't seem to be anything interesting happening here.

326 ↗(On Diff #41832)

This test case also seems to be redundant now.

383 ↗(On Diff #41832)

It bothers me that this is still apparently legal. I would think that an exception thrown from within a top level EH pad should only be able to unwind to caller. In any event, the test is now verifying that we did not resolve the problem that the old implementation of this test was looking for.

What would the state numbering code do with this if we left it in the state that is checked here?

479 ↗(On Diff #41832)

This comment is incorrect now, since there is no invoke in %merge anymore. I think this test case is also obsolete.

lib/Transforms/Utils/InlineFunction.cpp
1443–1451 ↗(On Diff #41832)

I think you can remove CallSiteEHPad && from each of these three tests, as it's redundant with the guarding check up on line 1434.

lib/Transforms/Utils/InlineFunction.cpp
1099 ↗(On Diff #41832)

This could be

if (CallerPersonality) {
  assert(CalledPersonality);

since the code just above sets CalledPersonality if it is null but CallerPersonality is not.

lib/Transforms/Utils/InlineFunction.cpp
1099 ↗(On Diff #41832)

...no, I just totally misread that. Sorry for the noise.

majnemer added inline comments.Dec 11 2015, 12:47 AM
docs/ExceptionHandling.rst
617–618 ↗(On Diff #41832)

Done.

632–633 ↗(On Diff #41832)

cleanupret and catchret are similar in that they both exit their constituent funclet. I'll keep it as cleanupret unless there is an overwhelming desire to move in that direction.

761 ↗(On Diff #41832)

Done.

docs/LangRef.rst
5342 ↗(On Diff #41715)

I considered this approach while designing the new instructions (i.e. making getOuterScope return nullptr if it has no lexical parent). I rejected it for a few reasons:

  • It makes it all too easy to get the nesting wrong if no token is necessary. Things will seem to work until they don't. Making the token always explicit is a way of making IR creators consider the nesting case.
  • It provides us a sentinel value which we can use when wanting to talk about scopes. nullptr is often used to indicate failure and I wanted to make sure this confusion would never occur.

Regardless, we can do this change down the road at any time.

8618–8631 ↗(On Diff #41715)

#1 is done.

With regard to #2, we will tag call-sites in funclets with bundles but this does not allow us to enforce this as a verifier rule. Tail-merging can still make invokes with conflicting/illegal unwind destinations show up in other funclets. The tagging will allow us to get rid of them in WinEHPrepare.

5368 ↗(On Diff #41832)

Done.

5430 ↗(On Diff #41832)

Done.

8579 ↗(On Diff #41832)

Done.

include/llvm/Analysis/CFG.h
92 ↗(On Diff #41832)

Done.

include/llvm/Bitcode/LLVMBitCodes.h
425 ↗(On Diff #41832)

Bitcode is supposed to be stable within reason; we are breaking compatibility because it is not sensible to add heroic logic to the auto-upgrader to make these instructions: nobody should have MSVC++ EH bitcodes around.

It is fair game for 54 to be reused because, for all intents and purposes, it has never been used except by a tiny number of people.

Operand bundles are currently under development by other folks, it would be in poor taste to renumber them under their feet.

include/llvm/IR/Instructions.h
4164 ↗(On Diff #41832)

Done.

4220 ↗(On Diff #41832)

getTargetScope and getDestinationScope make you feel like they imply the existence of CFG edges which aren't really there. I'll leave this alone unless a compelling name shows up.

lib/Analysis/CFG.cpp
21 ↗(On Diff #41832)

Done.

lib/Analysis/EHPersonalities.cpp
12 ↗(On Diff #41832)

Yes, for llvm::successors.

52–57 ↗(On Diff #41832)

Done.

54 ↗(On Diff #41832)

Done.

lib/Analysis/ScalarEvolutionExpander.cpp
102–103 ↗(On Diff #41832)

This code cannot fire if the terminatepad unwinds to caller. It is only possible to get here if we have a PHI on the terminatepad (or one of it's predecessors) and the PHI's use is dominated by the terminatepad. Such a use shouldn't exist if the terminatepad unwinds to caller.

lib/AsmParser/LLParser.cpp
2371–2372 ↗(On Diff #41832)

Done.

lib/CodeGen/AsmPrinter/WinException.cpp
799–804 ↗(On Diff #41832)

Done.

lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
228–234 ↗(On Diff #41832)

The PHIs in the IR are not the same as the ones we need in the machine CFG. This is because invokes have a single successor in the IR but multiple successors in the machine CFG. To disable the demotion, we would need to fix the PHIs as we build the machine CFG.

lib/CodeGen/WinEHPrepare.cpp
169 ↗(On Diff #41832)

Done.

219 ↗(On Diff #41832)

Done.

261 ↗(On Diff #41832)

It still is because two cleanuprets from the same cleanuppad can unwind to the same funclet-pad. We will then do getEHPadFromPredecessor and visit the same cleanup twice.

lib/IR/Instructions.cpp
936 ↗(On Diff #41832)

Done.

1002 ↗(On Diff #41832)

Done.

lib/IR/Verifier.cpp
401–402 ↗(On Diff #41832)

This is UB, langref is updated.

2850 ↗(On Diff #41832)

Done.

3011 ↗(On Diff #41832)

Done.

3022 ↗(On Diff #41832)

visitEHPadPredecessors should do this for us when we visit CatchPadInst.

3048 ↗(On Diff #41832)

visitEHPadPredecessors should do this for us when we visit CatchPadInst.

lib/Transforms/Utils/InlineFunction.cpp
371 ↗(On Diff #41832)

Done.

1433 ↗(On Diff #41832)

Done.

1443–1451 ↗(On Diff #41832)

Done.

lib/Transforms/Utils/SimplifyCFG.cpp
3276 ↗(On Diff #41832)

Done.

test/CodeGen/WinEH/wineh-cloning.ll
276 ↗(On Diff #41832)

Done.

281 ↗(On Diff #41832)

Done.

326 ↗(On Diff #41832)

Done.

383 ↗(On Diff #41832)

There is no problem here because funclet ancestry is explicit. It should not be necessary for us to do any cloning. It is up to the front-end to emit IR that the personality routine is capable of. However, this example is impossible to construct from the front-end.

479 ↗(On Diff #41832)

Done.

majnemer updated this revision to Diff 42509.Dec 11 2015, 12:50 AM
  • Move colorEHFunclets to EHPersonalities.h
  • Fix some first draft review comments
  • Address Andy's latest comments
  • Make the IR a little easier to read
  • Make the catchswitch syntax more like invoke
  • Address Andy's latest comments.
  • Address Joseph's review comments
  • Remove unneeded #includes
  • Address Joseph's comments
  • Don't unswitch cleanuppads, we don't know how to split invoke edges to them.
  • Don't sink into a catchswitch, it won't work.
  • Make CatchSwitchInst::handlers() return BasicBlock * intead of const Use &
  • Address review comments
rnk added inline comments.Dec 11 2015, 9:39 AM
lib/CodeGen/WinEHPrepare.cpp
223–231 ↗(On Diff #42509)

It really could. Comments fell by the wayside because we were impatiently trying to pair program something that would work.

I'm probably the person who should write them, though, so I'd rather commit this and then try to clean up the algorithm in tree.

There's other stuff to do as well. It would be good to share the EH traversal between the CLR, C++, and SEH, so this is probably going to get rewritten.

250 ↗(On Diff #42509)

This assigns the same base state to all catchpads within a catchswitch. That seems sketchy to me.

That's not actually different from how things work with catchendpad today, and I think it's consistent with what MSVC does. Here's how I read the states produced by MSVC on your example:

void f(int);
void foo() {
  try {
    f(1); // state 0
  } catch (int) {
    f(2); // state 1
    try {
      f(3); // state 2
    } catch (...) {
      // state 3
    }
  } catch (...) {
    f(4); // state 1
    try {
      f(5); // state 4
    } catch (...) {
      // state 5
    }
  }
}
// unwind map
// 0 -> -1
// 1 -> -1
// 2 -> 1
// 3 -> 1
// 4 -> 1
// 5 -> 1

Clang today computes the same table as MSVC on this example, and I'm pretty sure this patch will keep our behavior here the same.

rnk added inline comments.Dec 11 2015, 10:10 AM
lib/Transforms/Utils/SimplifyCFG.cpp
3284 ↗(On Diff #42509)

I made this change and I think I completely misunderstood what the code was trying to do. I thought it was trying to merge consecutive cleanups together, not just remove empty cleanups. We can probably revert this.

LGTM aside from:

  • two naming issues (which, since they're naming issues, are bikeshedding, and could always be changed post-commit anyway):
    • The term "outer scope" used for the "within" argument of pads seems discordant with the rest of our terminology. I'd rather refer to them as "pads" since that's what we call them everywhere else, and just need an adjective. Since the textual IR uses "within", having a good counterpart for that would work, something like getOuterPad or getEnclosingPad or getContainingPad. Since in a few places we'll need to talk about the transitive closure of it, calling it getParentPad would also work for me, so that "ancestor" will be available for the transitive case.
    • I still think it's confusing that a catchret's "outer" scope is the parent of the catchswitch. I tried a few more suggestions inline, of course you'd want to s/Scope/Pad/ in them if the previous bullet sways you.
  • one inline nit
  • the thing Andy pointed out in SimplifyCFG
include/llvm/IR/Instructions.h
4220 ↗(On Diff #41832)

I'd argue the edge is there -- you're getting the "scope" that contains the target of the CFG edge. I think getReturnedToScope could make sense, but is awkward. getScopeAtDestination? getOuterScopeOfCatchswitch? getNotExitedAncestorScope? getExitScope? getInvokeScope? getPreExceptionScope? getPreviousScope? getPriorScope?

3907 ↗(On Diff #42509)

nit: seems like the two handler_helpers should be private.

rnk edited edge metadata.Dec 11 2015, 11:11 AM

I like getParentPad best from your list of names for getOuterScope.

majnemer updated this revision to Diff 42556.Dec 11 2015, 12:42 PM
majnemer edited edge metadata.
  • Address review comments
include/llvm/IR/InstrTypes.h
1122 ↗(On Diff #42556)

(here and other constructors and Create methods) parameter should probably be renamed ParentPad as well.

lib/CodeGen/WinEHPrepare.cpp
207 ↗(On Diff #42556)

This parameter name too

lib/IR/Verifier.cpp
2994 ↗(On Diff #42556)

nit: Also these locals (this, visitCatchSwitchInst, and visitTerminatePadInst)

andrew.w.kaylor accepted this revision.Dec 11 2015, 3:53 PM
andrew.w.kaylor edited edge metadata.

Except for the irreducible loop issue, this looks good to me. I would be OK with putting off addressing that problem until some later time, as it seems like a fairly unlikely corner case, but I do think we should probably think about it.

lib/CodeGen/WinEHPrepare.cpp
223–232 ↗(On Diff #42556)

Sounds like a plan.

250 ↗(On Diff #42556)

OK, if MSVC does this also then I feel a lot better about the long term viability of doing it this way.

test/CodeGen/WinEH/wineh-cloning.ll
288–289 ↗(On Diff #42556)

The original concern was that some future optimization would innocently transform otherwise reasonable code into this form. I think the hypothetically dangerous input looked something like this:

left:
  cleanuppad within none []
  call void @h(i32 1)
  invoke void @f()
    to label %unreachable unwind label %not_right
not_right:
  cleanuppad within none []
  call void @h(i32 2)
  call void @f()
  unreachable
right:
  cleanuppad within none []
  call void @h(i32 2)
  invoke void @f()
    to label %unreachable unwind label %not_left
not_left:
  cleanuppad within none []
  call void @h(i32 1)
  call void @f()
  unreachable

So if we can get that out of the clang front end, then a reasonable optimization could transform it into this test case, and I don't see how the code generator could do anything reasonable with the form shown in this test case.

This revision is now accepted and ready to land.Dec 11 2015, 3:53 PM
majnemer added inline comments.Dec 11 2015, 6:24 PM
include/llvm/IR/InstrTypes.h
1122 ↗(On Diff #42556)

Done.

lib/CodeGen/WinEHPrepare.cpp
207 ↗(On Diff #42556)

Done.

lib/IR/Verifier.cpp
2994 ↗(On Diff #42556)

Done.

test/CodeGen/WinEH/wineh-cloning.ll
288–289 ↗(On Diff #42556)

Such a hypothetical optimization would be defeated once we add support for bundle operands in the front-end. The bundle operands will defeat tail-merging/CSE because the call-sites will look like they have different operands.

test/CodeGen/WinEH/wineh-cloning.ll
288–289 ↗(On Diff #42556)

I think this is another example of a case where we'll need the bundles tagging calls/invokes in EH pads. With that, the notion of which pad an invoke "is in" will be well-defined, and we'll be able to add a rule that it's UB to take an invoke's unwind edge if its unwind dest's parent pad isn't the parent of the pad that the invoke is in. WinEHPrep will then be able to prune the UB unwind edges, so if presented with the IR in this test (which, as David points out, couldn't be the result of optimizing your example anymore, but I think would still be legal/verifiable IR) it would see that the two invokes to @f() unwind to illegal destinations, and rewrite those as calls.

majnemer updated this revision to Diff 42619.Dec 11 2015, 7:06 PM
majnemer edited edge metadata.
  • Move colorEHFunclets to EHPersonalities.h
  • Fix some first draft review comments
  • Address Andy's latest comments
  • Make the IR a little easier to read
  • Make the catchswitch syntax more like invoke
  • Address Andy's latest comments.
  • Address Joseph's review comments
  • Remove unneeded #includes
  • Address Joseph's comments
  • Don't unswitch cleanuppads, we don't know how to split invoke edges to them.
  • Don't sink into a catchswitch, it won't work.
  • Make CatchSwitchInst::handlers() return BasicBlock * intead of const Use &
  • Address review comments
  • Address review comments
  • Address review comments.
JosephTremoulet accepted this revision.Dec 11 2015, 7:22 PM
JosephTremoulet edited edge metadata.

LGTM, no caveats.

This revision was automatically updated to reflect the committed changes.
llvm/trunk/test/CodeGen/X86/win32-seh-catchpad.ll