Added cfi instructions for correct CFA calculation in case when movpc instruction expands to call and pop
ClosedPublic

Authored by violetav on Oct 23 2015, 9:03 AM.

Details

Summary

This fixes the issue of wrong CFA calculation in the following case:

0x08048400 <+0>:	push   %ebx
0x08048401 <+1>:	sub    $0x8,%esp
0x08048404 <+4>:	**call   0x8048409 <test+9>**
0x08048409 <+9>:	**pop    %eax**
0x0804840a <+10>:	add    $0x1bf7,%eax
0x08048410 <+16>:	mov    %eax,%ebx
0x08048412 <+18>:	call   0x80483f0 <bar>
0x08048417 <+23>:	add    $0x8,%esp
0x0804841a <+26>:	pop    %ebx
0x0804841b <+27>:	ret

The highlighted instructions are a product of movpc instruction. The call instruction changes the stack pointer, and pop instruction restores its value. However, the rule for computing CFA is not updated and is wrong on the pop instruction. So, e.g. backtrace in gdb doesn't work when on the pop instruction. This solution adds cfi instructions for both call and pop instructions.

  • cfi_adjust_cfa_offset** instruction is used with the appropriate offset for setting the rules to calculate CFA correctly.

Diff Detail

Repository
rL LLVM
violetav retitled this revision from to Added cfi instructions for correct CFA calculation in case when movpc instruction expands to call and pop.Oct 23 2015, 9:03 AM
violetav updated this object.
violetav added a reviewer: rnk.
violetav set the repository for this revision to rL LLVM.
violetav added a subscriber: llvm-commits.
rnk added inline comments.Oct 23 2015, 9:40 AM
lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
243 ↗(On Diff #38237)

Whoever lands first, make sure that this doesn't duplicate the entry that Michael added in D13767.

lib/Target/X86/X86MCInstLower.cpp
1151–1153 ↗(On Diff #38237)

We're already inside AsmPrinter. You don't need to add entries to the FrameInstructions vector anymore.

1165–1167 ↗(On Diff #38237)

Ditto, you shouldn't need to call addFrameInst.

violetav updated this revision to Diff 38241.Oct 23 2015, 10:39 AM

Removed adding cfi instructions to FrameInstructions vector.

rnk added inline comments.Oct 23 2015, 11:46 AM
lib/Target/X86/X86MCInstLower.cpp
1151 ↗(On Diff #38241)

Do you need to create this MCCFIInstruction that you ultimately don't use?

1163 ↗(On Diff #38241)

ditto?

DavidKreitzer added inline comments.
lib/Target/X86/X86MCInstLower.cpp
1150 ↗(On Diff #38241)

You really only need these CFA adjust directives when precise instruction-level stack unwinding support is needed. That's certainly the case when the CFI is being used to generate debug info. But it isn't the case when the CFI is being used to generate the unwind tables for synchronous EH. For synchronous EH, these extra CFA adjust directives just waste space in the .eh_frame section.

For D13767, Michael is planning to add a usePreciseUnwindInfo function. It would be nice sync with him and add "&& usePreciseUnwindInfo" to this condition. Also at line 1162.

mkuper added a subscriber: mkuper.Oct 24 2015, 7:38 AM
mkuper added inline comments.
lib/Target/X86/X86MCInstLower.cpp
1150 ↗(On Diff #38241)

This is already checking only hasDebugInfo() and won't emit the adjustment if CFI is only used for EH. If I land after this, I'll change the condition to usePreciseUnwindInfo().

violetav updated this revision to Diff 38431.EditedOct 26 2015, 10:17 AM

Removed unnecessary calls to createAdjustCfaOffset method.

rnk accepted this revision.Oct 27 2015, 1:27 PM

lgtm

This revision is now accepted and ready to land.Oct 27 2015, 1:27 PM
petarj added a subscriber: petarj.Oct 29 2015, 11:57 PM
violetav updated this revision to Diff 39193.Nov 4 2015, 5:41 AM

Made the following changes according to the D13767 that landed:

  1. Removed the changes made in the AsmPrinterDwarf.cpp (adding support for OpAdjustCfaOffset)
  2. Changed the condition from hasDebugInfo() to usePreciseUnwindInfo() in X86MCInstLower.cpp.
rnk added a comment.Nov 4 2015, 10:55 AM

Still seems fine. Do you need someone to commit this?

This revision was automatically updated to reflect the committed changes.