State numbers are calculated by performing a walk from the innermost
funclet to the outermost funclet. Rudimentary support for the new EH
constructs has been added to the assembly printer, just enough to test
the new machinery.
Details
Diff Detail
Event Timeline
LGTM, though I'm less familiar with this part of the code. A few nits/questions inline, but all just minor take-it-or-leave-it suggestions.
lib/CodeGen/AsmPrinter/WinException.cpp | ||
---|---|---|
185–188 | Nit: I don't understand this change; the function will fail if you pass it a non-null non-GlobalValue Value, so to me it seems less error prone to type the parameter as GlobalValue and force callers to recognize the need to downcast. | |
lib/CodeGen/WinEHPrepare.cpp | ||
2581 | Nit: const_cast is unfortunate. Would it be horrible to push constness down or non-constness up? (I'm not familiar with the callers/callees imposing the restrictions here) | |
2949–2958 | Nit: this was a confusing read because it's not getting the unwind pred of BB, it's getting the unwind pred of a successor of BB. I think it would be easier to read if the parameter name were PredBB and/or a comment explained. | |
3344 | I don't understand this change, is there some style guideline against periods ending comments? The other comments in the file tend to have them, and this one is even a full sentence. |
Much excitement. :)
lib/CodeGen/AsmPrinter/WinException.cpp | ||
---|---|---|
185–188 | Right, the intent is for the assertion to fail at this point once we start passing such values through. Previously, we outlined funclets in IR, and the IR function was the GlobalValue that we passed through. With the new representation, we are passing a BasicBlock, which inherits from Value. We can't actually reach this code today because SDAGBuilder can't select our new pad instructions yet. Eventually we will get here, the assertion will fail, and we'll fix it up and give it the correct BasicBlock type. | |
lib/CodeGen/WinEHPrepare.cpp | ||
2581 | Pushing it down is the right thing in this case. The unwind map lives through codegen, and there shouldn't be non-const IR pointers hanging around during codegen. | |
2949–2958 | I guess it really does the following: Given BB which ends in an unwind edge, return the EHPad that this BB belongs to. If the unwind edge came from an invoke, return null. Call it getEHPadFromPredecessor? | |
3284 | Also left over from hurried pair programming. :) | |
lib/Target/X86/X86WinEHState.cpp | ||
400–405 | Probably worth commenting that we only ever create our own WinEHFuncInfo for testing purposes. MMI is non-null iff we are running as part of a CodeGen pass manager. Given that it's for testing only... we should heap allocate it with something like this: // If MMI is null, create our own WinEHFuncInfo. This only happens in opt tests. std::unique_ptr<WinEHFuncInfo> FuncInfoPtr; if (!MMI) FuncInfoPtr.reset(new WinEHFuncInfo()); WinEHFuncInfo &FuncInfo = *(MMI ? &MMI->getWinEHFuncInfo(&F) : FuncInfoPtr.get()); | |
496–501 | Both of these functions only consume WinEHFuncInfo. We should hoist this logic up to runOnFunction and only pass down WinEHFuncInfo. |
Could you please explain why you
initializeWinEHStatePassPass(PR);
in LLVMInitializeX86Target() regardless of whether the target is win32 or not?
Sorry if the question is silly. I actually have NFC about what's going on in this commit.
I am getting some nasty memory corruption problems which might or might not be caused by this unconditional initialization.
We have no idea at initialization time if the user wants to target win32 or
the host, so we couldn't conditionalize it if we wanted to.
We initialize the pass so that we can run it from opt and test it in
isolation. It's consistent what NVPTX does in
llvm/lib/Target/NVPTX/NVPTXTargetMachine.cpp. We just copied the technique.
This guy should be const. I'm reasonably confident that the const cascading will stop there, since all we use it for is to get an MCSymbol.