Emit the handler and clause locations immediately after the standard
xdata.
Clauses are emitted in the same order and format used to communiate them
to the CLR Execution Engine.
Add a lit test to verify correct table generation on a small but
interesting example function.
Details
Diff Detail
Event Timeline
lib/CodeGen/AsmPrinter/WinException.cpp | ||
---|---|---|
924 | typo - "regino" | |
941 | What do you mean by "the xdata proper" and is this the same thing you are referring to as "the standard xdata" in the review description? | |
961 | I've found that we've been kind of sloppy in the terminology we're using in comments to describe EH states. When you say "the state of the handler" do you mean the EH state of the code inside the handler or the EH state which is handled by that handler at the most recent invoke? | |
971 | So this is the EH state of code inside the handler? The low state if the handler has multiple states? | |
983 | Can you add some sort of debug check to account for the possibility that a poorly constructed ClrEHUnwindMap skips past AncestorState (i.e. state 2 unwinds to state 0 but AncestorState was 1)? | |
1037 | typo - "a labels" | |
1058 | Again, it seems like there is a possibility here of jumping over PendingState. | |
lib/CodeGen/AsmPrinter/WinException.h | ||
72 | Is there a reason only one of these parameters (for the two functions above) is const? It seems like they all could be. |
Thanks for the review! I believe I've incorporated everything.
lib/CodeGen/AsmPrinter/WinException.cpp | ||
---|---|---|
941 | Yes. I've expanded the comment to be more verbose. | |
961 | Part of the complication here is that nothing in the CLR docs/code has this notion of "EH states". I was referring to these things as states because it's the logical counterpart of what gets computed in ComputeStateNumbering for the other personalities. Really the "states" as they're used here are identifiers for the handlers and/or for the funclets (handlers and funclets have a 1:1 correspondence for CLR EH), and specifically they're the index of the corresponding entry in the unwind map. I've updated the comments throughout this function to be a bit more careful/verbose about that, let me know if you think further editing would be helpful. | |
971 | Hopefully this is clear given the above. "Low state" doesn't really mean anything to me. I'm talking about MBB membership here, so "the handler" means the funclet that this MBB is a (top-level) member of. | |
983 | Sure. I've added a check above that parent states are strictly less than child states, which will guarantee this loop terminates, and added an explicit assertion that PendingState never reaches NullState to be more easily diagnosable than the bounds assertion in the vector access. | |
1058 | This one I didn't change, since the PendingState we're walking to here was set to AncestorState in the call to RewindPendingTo a few lines above, which in turn was computed by getAncestor(..., NewState). So if we jumped over PendingState here it would be a bug in getAncestor, not bad input. I think the implicit check in the vector indexer is sufficient. | |
lib/CodeGen/AsmPrinter/WinException.h | ||
72 | They can and should be. Fixed, thanks. |
Some comments. I'm mainly interested in sharing the logic around EH_LABEL before committing this.
lib/CodeGen/AsmPrinter/WinException.cpp | ||
---|---|---|
906–907 | Do you need the IMGREL relocations to compute a label difference? You should just need to create an MCSymbolRefExpr like this code from emitCSpecificHandlerTable: const MCExpr *LabelDiff = MCBinaryExpr::createSub(MCSymbolRefExpr::create(TableEnd, Ctx), MCSymbolRefExpr::create(TableBegin, Ctx), Ctx); I think the assembler will do the same thing either way, but the .s file will look cleaner. | |
962–965 | Hah, I think you actually solved the problem that I avoided solving for __C_specific_handler. :) I might want to share the code at some point so we can get smaller SEH tables. | |
1028 | Can you use or adapt invoke_ranges to work here? The EH_LABEL tracking state machine is pretty subtle, I'd rather write it just once. | |
1150–1153 | I guess using getLabelPlusOne requires keeping the IMGREL stuff inside the label difference. | |
test/CodeGen/WinEH/wineh-coreclr.ll | ||
130 | Interesting. I think these cross-funclet label differences will prohibit us from placing CLR funclets into .text$x, unless we tweak them until they are a perfect match for some relocation. |
lib/CodeGen/AsmPrinter/WinException.cpp | ||
---|---|---|
906–907 | Updated, and yes that looks much nicer, thanks for the suggestion. | |
1028 | I'm not eager to switch to the invoke_ranges API as-is, since it only gets me within-MBB merging (so I still want merging up in this layer), and I find its SawPotentiallyThrowing state awkward to reason about. I'd be happy to move to an iterator that
Would that be useful for generating the SEH tables? I wouldn't mind taking a stab at refactoring both onto a shared iterator like that. | |
test/CodeGen/WinEH/wineh-coreclr.ll | ||
131 | We don't handle splitting out cold code yet in LLILC generally, so I'll cross that bridge when we get there :). |
lib/CodeGen/AsmPrinter/WinException.cpp | ||
---|---|---|
1150–1153 | I can (and did) reassociate so the add is on the result of the subtract. |
lib/CodeGen/AsmPrinter/WinException.cpp | ||
---|---|---|
1028 | If you could take a stab at refactoring it, that'd be awesome. The SEH and C++ table emission would be simpler with the interface you described. I don't think I did a particularly good job writing that adaptor. I think making it cross-MBB will actually fix a bug around SawPotentiallyThrowing that I added when doing that refactor... Oops. | |
test/CodeGen/WinEH/wineh-coreclr.ll | ||
131 | I mentioned this because I want to do this in WinException::endFunclet() as soon as possible. It'll be easy to turn off for the CLR personality for now. :) |
Simplify code to track current funclet state (and stop trying to get a MBB* from MF->end() after the last iteration)
Is there a reason only one of these parameters (for the two functions above) is const? It seems like they all could be.