This is an archive of the discontinued LLVM Phabricator instance.

Correct dwarf unwind information in function epilogue
ClosedPublic

Authored by djokov on Feb 2 2018, 8:23 AM.

Details

Summary

This is updated rL317100: Correct dwarf unwind information in function epilogue. The patch was reverted in r317726, because duplicating blocks with CFI instructions was an issue for compact unwind info on Darwin. That problem is solved with rL323883: Allow duplication of tails with CFI instructions.

This patch aims to provide correct dwarf unwind information in function epilogue for X86.
It consists of two parts. The first part inserts CFI instructions that set appropriate cfa offset and cfa register in emitEpilogue() in X86FrameLowering. This part is X86 specific.

The second part is platform independent and ensures that:

  • CFI instructions do not affect code generation (they are not counted as instructions when tail duplicating or tail merging)
  • Unwind information remains correct when a function is modified by different passes. This is done in a late pass by analyzing information about cfa offset and cfa register in BBs and inserting additional CFI directives where necessary.

Added CFIInstrInserter pass:

  • analyzes each basic block to determine cfa offset and register are valid at its entry and exit
  • verifies that outgoing cfa offset and register of predecessor blocks match incoming values of their successors
  • inserts additional CFI directives at basic block beginning to correct the rule for calculating CFA

Having CFI instructions in function epilogue can cause incorrect CFA calculation rule for some basic blocks. This can happen if, due to basic block reordering, or the existence of multiple epilogue blocks, some of the blocks have wrong cfa offset and register values set by the epilogue block above them.
CFIInstrInserter is currently run only on X86, but can be used by any target that implements support for adding CFI instructions in epilogue.

Diff Detail

Repository
rL LLVM

Event Timeline

djokov created this revision.Feb 2 2018, 8:23 AM
wmi added a subscriber: wmi.Feb 7 2018, 2:14 PM
aprantl added inline comments.Feb 13 2018, 8:58 AM
lib/CodeGen/BranchFolding.cpp
300 ↗(On Diff #132590)

This should probably be !isMetaInstruction()?

370 ↗(On Diff #132590)

this too?

wmi added a comment.Feb 15 2018, 5:17 PM

Thanks for working on the patch. We count on it to enable libunwind.

lib/CodeGen/CFIInstrInserter.cpp
80–82 ↗(On Diff #132590)

The size of the vector should be equal or larger than the number of BBs inside of MF, so not quite small . Consider use std::vector?

163–164 ↗(On Diff #132590)

equivalent to: for (MachineInstr &MI : *MBBInfo.MBB)

168–177 ↗(On Diff #132590)

This is an incomplete list of CFI operations. Theoretically it is possible that OpRememberState/OpRestoreState/... is inserted before CFIInstrInserter pass, then the calculation result here may be incorrect. For those unsupported operations, consider adding an error report and a TODO here?

263 ↗(On Diff #132590)

We compare predecessors and successors of each BB, then each pair of BB are compared twice. Is the iteration of predecessors only or successors only enough?

268–274 ↗(On Diff #132590)

The report here and below looks cumbersome. Maybe change CFIInstrInserter::report to:

void CFIInstrInserter::report(MachineBasicBlock &Pred, MachineBasicBlock &Succ) {
    errs() << "*** " << Inconsistent CFA Reg and Offset between pred and succ << " ***\n"
    errs() << "Pred outgoing CFA Reg:" << Pred.OutgoingCFARegister;
    errs() << "Pred outgoing CFA Offset:" << Pred.OutgoingCFAOffset;
    errs() << "Succ incoming CFA Reg:" << Succ.IncomingCFARegister;
    errs() << "Succ incoming CFA Offset:" << Pred.IncomingCFAOffset;
}

So all the error reports here and below can share the same function.

djokov changed the edit policy from "All Users" to "Custom Policy".Feb 21 2018, 7:32 AM
djokov changed the edit policy from "Custom Policy" to "All Users".Feb 21 2018, 7:34 AM
violetav added inline comments.Feb 21 2018, 7:46 AM
lib/CodeGen/BranchFolding.cpp
300 ↗(On Diff #132590)

It sounds reasonable that other pseudo instructions shouldn't affect the decisions concerning tail merging. However, currently there is a lack of support required for their merging. I am not familiar with how these instructions should be handled; should they be part of the common tail if they exist in some but not all blocks that share that tail, etc. As this patch is concerned with adding CFI instructions in epilogue, I thought I'd rather leave out the support for merging other pseudo instructions for some other patch.

djokov updated this revision to Diff 135259.Feb 21 2018, 7:52 AM

Addressed comments from review.

djokov marked 5 inline comments as done.Feb 21 2018, 7:54 AM

Any other comments?

Thanks for bringing this up again! Sorry for taking so long to take another look at it.

lib/CodeGen/CFIInstrInserter.cpp
53 ↗(On Diff #135259)

This can be:

if (unsigned ErrorNum = verify(MF))
182 ↗(On Diff #135259)

Why are the report_fatal_errors guarded by NDEBUG here?

204 ↗(On Diff #135259)

I suppose these also need a TODO. I would also write somewhere that the CSRs are not yet handled by this pass.

test/CodeGen/X86/avx512-regcall-Mask.ll
196 ↗(On Diff #135259)

I'm guessing this was not intended. I can see that X86FrameLowering::mergeSPUpdate doesn't support merging instructions with CFI.
I think you should add support for that to avoid regressions like this one.

test/CodeGen/X86/frame-lowering-debug-intrinsic-2.ll
20 ↗(On Diff #135259)

Same here.

test/CodeGen/X86/push-cfi-debug.ll
26 ↗(On Diff #135259)

Same here.

test/CodeGen/X86/push-cfi-obj.ll
15 ↗(On Diff #135259)

As expected, this can prove that .eh_frame is going to get bigger. If you feel like doing it, it would be nice to see what the % size increase is for something like the test-suite.

test/CodeGen/X86/push-cfi.ll
77 ↗(On Diff #135259)

Same here.

thegameg requested changes to this revision.Mar 1 2018, 9:25 AM
thegameg added inline comments.
lib/CodeGen/CFIInstrInserter.cpp
81 ↗(On Diff #135259)

No need for struct here.

89 ↗(On Diff #135259)

And here.

92 ↗(On Diff #135259)

And here.

106 ↗(On Diff #135259)

And here.

133 ↗(On Diff #135259)

And here.

153 ↗(On Diff #135259)

And here.

217 ↗(On Diff #135259)

And here.

219 ↗(On Diff #135259)

And here.

229 ↗(On Diff #135259)

And here.

237 ↗(On Diff #135259)

And here.

278 ↗(On Diff #135259)

And here.

279 ↗(On Diff #135259)

And here.

295 ↗(On Diff #135259)

And here.

297 ↗(On Diff #135259)

And here.

This revision now requires changes to proceed.Mar 1 2018, 9:25 AM
djokov updated this revision to Diff 139147.Mar 20 2018, 9:40 AM

Addressed comments. Added support for merging instructions in X86FrameLowering::mergeSPUpdate() when CFI are present.

djokov marked 20 inline comments as done.Mar 20 2018, 9:43 AM
violetav added inline comments.Mar 20 2018, 9:46 AM
lib/CodeGen/CFIInstrInserter.cpp
182 ↗(On Diff #135259)

I thought that possible wrong value of CFA should not be reported and abort compilation in release builds. Do you think that these should be reported as errors in release builds as well?

test/CodeGen/X86/stack-probe-red-zone.ll
4 ↗(On Diff #139147)

Because now CFI instructions don't get in the way of merging add/sub instructions, I changed this test a bit so that 'sub' and 'add' don't end up next to each other and get merged, with only 'ret' remaining. This way red zone can still get checked in the test.

thegameg added a subscriber: craig.topper.

Added @craig.topper for the X86FrameLowering::mergeSPUpdates changes.

lib/CodeGen/CFIInstrInserter.cpp
182 ↗(On Diff #135259)

I'm not sure, but from what I see an OpRemember/RestoreState CFI_INSTRUCTION can only be created through MIR. I think if at some point we add code that generates one of these, we would want this pass to be updated as well, right?

Do you think reporting this error in release builds is unreasonable?

lib/Target/X86/X86FrameLowering.cpp
408 ↗(On Diff #139147)

The famous question: should this be skipMetaInstructions(Backward|Forward) or something similar (like you did in countsAsInstruction)?

I would at least merge the two functions (Debug + CFI) since this assumes we won't have a mix like: DBG_VALUE, CFI_INSTRUCTION, DBG_VALUE, CFI_INSTRUCTION.

test/CodeGen/X86/O0-pipeline.ll
54 ↗(On Diff #139147)

After r327917, the MachineOutliner will add some CFIs. Can you add a test where we make sure this pass runs afterwards?

test/CodeGen/X86/throws-cfi-fp.ll
86 ↗(On Diff #139147)

Is all this metadata necessary?

test/CodeGen/X86/throws-cfi-no-fp.ll
85 ↗(On Diff #139147)

Same here, necessary?

dstenb added a subscriber: dstenb.Apr 18 2018, 7:38 AM
djokov updated this revision to Diff 143107.Apr 19 2018, 9:05 AM

Addressed comments.

violetav added inline comments.Apr 19 2018, 9:10 AM
lib/CodeGen/CFIInstrInserter.cpp
182 ↗(On Diff #135259)

I'm not sure, but from what I see an OpRemember/RestoreState CFI_INSTRUCTION can only be created through MIR. I think if at some point we add code that generates one of these, we would want this pass to be updated as well, right?

Yes.

Do you think reporting this error in release builds is unreasonable?

I don't think it's unreasonable, I just thought that if verifier doesn't report errors in release builds, neither should these. Maybe there could be an option, to emit warnings, erorrs, or to ignore inconsistent CFA values altogether. Different build types could have different default values set for this option.

lib/Target/X86/X86FrameLowering.cpp
408 ↗(On Diff #139147)

I changed the code in mergeSPUpdates(), so that it supports the case when ADD/SUB/LEA is succeded by one CFI instruction, and that there are no DBG_VALUE or other instructions between ADD/SUB/LEA and its corresponding CFI instruction. The case where there are multiple CFI instructions below the ADD/SUB/LEA, e.g.:
...
add
cfi_def_cfa_offset
cfi_offset
...
is not currently supported.

test/CodeGen/X86/O0-pipeline.ll
54 ↗(On Diff #139147)

I moved CFIInstrInserter pass from preEmitPass() (runs before MachineOutliner) to preEmitPass2() (runs after outliner).
I added a test that checks that CFIInstrInserter runs after passes that add CFI instructions.

Thanks for working on this again!

lib/CodeGen/CFIInstrInserter.cpp
182 ↗(On Diff #135259)

Makes sense, thanks!

lib/Target/X86/X86FrameLowering.cpp
408 ↗(On Diff #139147)

All right this could work for now. Can you document that somewhere? Can you put this explanation in a comment below with a TODO/FIXME?

test/CodeGen/X86/GlobalISel/add-scalar.ll
23 ↗(On Diff #139147)

I think this change made sense before. What happened?

test/CodeGen/X86/O0-pipeline.ll
54 ↗(On Diff #139147)

Ideally this would be part of the AsmPrinter since it's pretty much "fixing" the CFI based on the final layout but doesn't modify the code. I wonder what was meant to be added in preEmitPass2 instead of the retpoline pass, but I think this is pretty much OK to add it here. If not, just making sure that this runs after the outliner (or whatever pass deals with CFI last) should also be fine.

test/CodeGen/X86/avx512-regcall-Mask.ll
197 ↗(On Diff #143107)

Hmm, I still don't really understand this change.

violetav added inline comments.Apr 20 2018, 8:14 AM
test/CodeGen/X86/GlobalISel/add-scalar.ll
23 ↗(On Diff #139147)

The test was changed in the meantime: https://reviews.llvm.org/D45146#change-3jDxqMq6Nhg4.

test/CodeGen/X86/avx512-regcall-Mask.ll
197 ↗(On Diff #143107)

I'll try to explain.
Without CFI instructions in epilogue, merging would occur, but adjust_cfa_offset used to have wrong offset value set.

Then, when CFI in epilogue were added, they prevented merging, and the situation was the following:

...

callq test_argv64i1
addq $16, %rsp
.cfi_adjust_cfa_offset -16
addq $8, %rsp
.cfi_def_cfa_offset 40
popq %r12
.cfi_def_cfa_offset 32

...

Now, CFI instruction, in this case the '.cfi_def_cfa_offset 40', doesn't prevent merging, and correct offset (24) is set to cfi_adjust_cfa directive.
Merging and creating corresponding adjust_cfa_offset happens after emitEpilogue(), when cfi in epilogue are already inserted. Hope this helps clear things up a bit.

I have some tests failing on my side:

Failing Tests (12):
  LLVM :: CodeGen/X86/O3-pipeline.ll
  LLVM :: CodeGen/X86/avoid-sfb.ll
  LLVM :: CodeGen/X86/cmpxchg-i128-i1.ll
  LLVM :: CodeGen/X86/fmaxnum.ll
  LLVM :: CodeGen/X86/fminnum.ll
  LLVM :: CodeGen/X86/fp-arith.ll
  LLVM :: CodeGen/X86/mmx-arith.ll
  LLVM :: CodeGen/X86/pr32241.ll
  LLVM :: CodeGen/X86/pr32284.ll
  LLVM :: CodeGen/X86/schedule-x86_32.ll
  LLVM :: CodeGen/X86/vector-arith-sat.ll
test/CodeGen/X86/avx512-regcall-Mask.ll
197 ↗(On Diff #143107)

Oh yes! I assumed the test was correct before (rookie mistake), but your patch actually fixes this. Thanks for the explanation.

@thegameg Hm, that's strange. They pass on my side. What are the failures that you're having?

@thegameg Hm, that's strange. They pass on my side. What are the failures that you're having?

Sorry, I might have had an old version applied. They all pass except one:

FAIL: LLVM :: CodeGen/X86/vector-arith-sat.ll (2952 of 3239)

which was added yesterday.

djokov updated this revision to Diff 143570.Apr 23 2018, 8:32 AM

Added comment in mergeSPUpdates(). Rebased patch.

thegameg accepted this revision.Apr 23 2018, 9:36 AM

LGTM, thanks!

This revision is now accepted and ready to land.Apr 23 2018, 9:36 AM
This revision was automatically updated to reflect the committed changes.
craig.topper added inline comments.May 2 2018, 3:32 PM
llvm/trunk/lib/Target/X86/X86FrameLowering.cpp
426

Should this be handling LEA64r too? That's the LEA instruction for 64-bit mode. LEA64_32r is created by two address instruction pass when an ADD needs to be made 3 three address to avoid destroying its input registers. It would surprised if LEA64_32r was ever used with a stack pointer.

craig.topper added inline comments.May 2 2018, 4:50 PM
llvm/trunk/lib/CodeGen/CFIInstrInserter.cpp
147

Should we be using a worklist here instead of recursion?