This is an archive of the discontinued LLVM Phabricator instance.

Differential D14021

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

Reviewers
rnk
Commits
rG99fba3c14164: Add cfi instr for CFA calculation when movpc is expanded to call and pop
rL252176: Add cfi instr for CFA calculation when movpc is expanded to call and pop
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

Event Timeline

violetav updated this revision to Diff 38237.Oct 23 2015, 9:03 AM
violetav retitled this revision from to Added cfi instructions for correct CFA calculation in case when movpc instruction expands to call and pop.
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 a subscriber: DavidKreitzer.Oct 23 2015, 1:20 PM
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
rnk edited edge metadata.

lgtm

This revision is now accepted and ready to land.Oct 27 2015, 1:27 PM
petarj edited edge metadata.Oct 29 2015, 11:57 PM
petarj added a subscriber: petarj.
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?

Closed by commit rL252176: Add cfi instr for CFA calculation when movpc is expanded to call and pop (authored by petarj). · Explain WhyNov 5 2015, 9:22 AM
This revision was automatically updated to reflect the committed changes.
DavidKreitzer mentioned this in D18046: [X86] Providing correct unwind info in function epilogue.May 2 2016, 3:41 PM

Revision Contents

PathSize
llvm/
trunk/
lib/
Target/
X86/
15 lines
test/
CodeGen/
X86/
42 lines

Diff 39374

llvm/trunk/lib/Target/X86/X86MCInstLower.cpp

Show First 20 Lines • Show All 1,134 Lines • ▼ Show 20 Lines case X86::MOVPC32r: {
// Emit the call. // Emit the call.
MCSymbol *PICBase = MF->getPICBaseSymbol(); MCSymbol *PICBase = MF->getPICBaseSymbol();
// FIXME: We would like an efficient form for this, so we don't have to do a // FIXME: We would like an efficient form for this, so we don't have to do a
// lot of extra uniquing. // lot of extra uniquing.
EmitAndCountInstruction(MCInstBuilder(X86::CALLpcrel32) EmitAndCountInstruction(MCInstBuilder(X86::CALLpcrel32)
.addExpr(MCSymbolRefExpr::create(PICBase, OutContext))); .addExpr(MCSymbolRefExpr::create(PICBase, OutContext)));
const X86FrameLowering* FrameLowering =
MF->getSubtarget<X86Subtarget>().getFrameLowering();
bool hasFP = FrameLowering->hasFP(*MF);
bool NeedsDwarfCFI = MMI->usePreciseUnwindInfo();
int stackGrowth = -RI->getSlotSize();
if (NeedsDwarfCFI && !hasFP) {
OutStreamer->EmitCFIAdjustCfaOffset(-stackGrowth);
}
// Emit the label. // Emit the label.
OutStreamer->EmitLabel(PICBase); OutStreamer->EmitLabel(PICBase);
// popl $reg // popl $reg
EmitAndCountInstruction(MCInstBuilder(X86::POP32r) EmitAndCountInstruction(MCInstBuilder(X86::POP32r)
.addReg(MI->getOperand(0).getReg())); .addReg(MI->getOperand(0).getReg()));
if (NeedsDwarfCFI && !hasFP) {
OutStreamer->EmitCFIAdjustCfaOffset(stackGrowth);
}
return; return;
} }
case X86::ADD32ri: { case X86::ADD32ri: {
// Lower the MO_GOT_ABSOLUTE_ADDRESS form of ADD32ri. // Lower the MO_GOT_ABSOLUTE_ADDRESS form of ADD32ri.
if (MI->getOperand(2).getTargetFlags() != X86II::MO_GOT_ABSOLUTE_ADDRESS) if (MI->getOperand(2).getTargetFlags() != X86II::MO_GOT_ABSOLUTE_ADDRESS)
break; break;
▲ Show 20 Lines • Show All 240 Lines • Show Last 20 Lines

llvm/trunk/test/CodeGen/X86/movpc32-check.ll

; RUN: llc < %s -mtriple=i686-pc-linux -relocation-model=pic | FileCheck %s
target datalayout = "e-m:e-p:32:32-f64:32:64-f80:32-n8:16:32-S128"
target triple = "i686-pc-linux"
; Function Attrs: nounwind
define void @test() #0 {
entry:
call void bitcast (void (...)* @bar to void ()*)(), !dbg !11
ret void, !dbg !12
}
declare void @bar(...) #1
attributes #0 = { nounwind "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="i686" "unsafe-fp-math"="false" "use-soft-float"="false" }
attributes #1 = { "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="i686" "unsafe-fp-math"="false" "use-soft-float"="false" }
!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!7, !8, !9}
!llvm.ident = !{!10}
!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 3.8.0 (http://llvm.org/git/clang.git 3490ab8630d5643f71f1f04e46984f05b27b8d67) (http://llvm.org/git/llvm.git d2643e2ff955ed234944fe3c6b4ffc1250085843)", isOptimized: false, runtimeVersion: 0, emissionKind: 1, enums: !2, subprograms: !3)
!1 = !DIFile(filename: "test.c", directory: "movpc-test")
!2 = !{}
!3 = !{!4}
!4 = distinct !DISubprogram(name: "test", scope: !1, file: !1, line: 2, type: !5, isLocal: false, isDefinition: true, scopeLine: 3, isOptimized: false, function: void ()* @test, variables: !2)
!5 = !DISubroutineType(types: !6)
!6 = !{null}
!7 = !{i32 2, !"Dwarf Version", i32 4}
!8 = !{i32 2, !"Debug Info Version", i32 3}
!9 = !{i32 1, !"PIC Level", i32 2}
!10 = !{!"clang version 3.8.0 (http://llvm.org/git/clang.git 3490ab8630d5643f71f1f04e46984f05b27b8d67) (http://llvm.org/git/llvm.git d2643e2ff955ed234944fe3c6b4ffc1250085843)"}
!11 = !DILocation(line: 4, column: 3, scope: !4)
!12 = !DILocation(line: 5, column: 1, scope: !4)
; CHECK: calll .L0$pb
; CHECK-NEXT: .Ltmp3:
; CHECK-NEXT: .cfi_adjust_cfa_offset 4
; CHECK-NEXT: .L0$pb:
; CHECK-NEXT: popl
; CHECK-NEXT: .Ltmp4:
; CHECK-NEXT: .cfi_adjust_cfa_offset -4