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

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

lib/Target/X86/X86MCInstLower.cpp
1151–1153

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

1162–1164

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

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

1162

ditto?

DavidKreitzer added a subscriber: DavidKreitzer.Oct 23 2015, 1:20 PM
DavidKreitzer added inline comments.
lib/Target/X86/X86MCInstLower.cpp
1150

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

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
lib/
CodeGen/
AsmPrinter/
3 lines
Target/
X86/
15 lines
test/
CodeGen/
X86/
42 lines

Diff 38431

lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp

Show First 20 Lines • Show All 234 Lines • ▼ Show 20 Lines case MCCFIInstruction::OpSameValue:
OutStreamer->EmitCFISameValue(Inst.getRegister()); OutStreamer->EmitCFISameValue(Inst.getRegister());
break; break;
case MCCFIInstruction::OpGnuArgsSize: case MCCFIInstruction::OpGnuArgsSize:
OutStreamer->EmitCFIGnuArgsSize(Inst.getOffset()); OutStreamer->EmitCFIGnuArgsSize(Inst.getOffset());
break; break;
case MCCFIInstruction::OpEscape: case MCCFIInstruction::OpEscape:
OutStreamer->EmitCFIEscape(Inst.getValues()); OutStreamer->EmitCFIEscape(Inst.getValues());
break; break;
case MCCFIInstruction::OpAdjustCfaOffset:
rnkUnsubmitted
Not Done
ReplyInline Actions

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

rnk: Whoever lands first, make sure that this doesn't duplicate the entry that Michael added in…
OutStreamer->EmitCFIAdjustCfaOffset(Inst.getOffset());
break;
} }
} }
void AsmPrinter::emitDwarfDIE(const DIE &Die) const { void AsmPrinter::emitDwarfDIE(const DIE &Die) const {
// Emit the code (index) for the abbreviation. // Emit the code (index) for the abbreviation.
if (isVerbose()) if (isVerbose())
OutStreamer->AddComment("Abbrev [" + Twine(Die.getAbbrevNumber()) + "] 0x" + OutStreamer->AddComment("Abbrev [" + Twine(Die.getAbbrevNumber()) + "] 0x" +
Twine::utohexstr(Die.getOffset()) + ":0x" + Twine::utohexstr(Die.getOffset()) + ":0x" +
▲ Show 20 Lines • Show All 44 Lines • Show Last 20 Lines

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->hasDebugInfo();
int stackGrowth = -RI->getSlotSize();
if (NeedsDwarfCFI && !hasFP) {
DavidKreitzerUnsubmitted
Not Done
ReplyInline Actions

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.

DavidKreitzer: You really only need these CFA adjust directives when precise instruction-level stack unwinding…
mkuperUnsubmitted
Not Done
ReplyInline Actions

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().

mkuper: This is already checking only hasDebugInfo() and won't emit the adjustment if CFI is only used…
OutStreamer->EmitCFIAdjustCfaOffset(-stackGrowth);
rnkUnsubmitted
Not Done
ReplyInline Actions

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

rnk: Do you need to create this MCCFIInstruction that you ultimately don't use?
}
rnkUnsubmitted
Not Done
ReplyInline Actions

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

rnk: We're already inside AsmPrinter. You don't need to add entries to the FrameInstructions vector…
// 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);
rnkUnsubmitted
Not Done
ReplyInline Actions

ditto?

rnk: ditto?
}
return; return;
rnkUnsubmitted
Not Done
ReplyInline Actions

Ditto, you shouldn't need to call addFrameInst.

rnk: Ditto, you shouldn't need to call addFrameInst.
} }
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;
// Okay, we have something like: // Okay, we have something like:
▲ Show 20 Lines • Show All 239 Lines • Show Last 20 Lines

test/CodeGen/X86/movpc32-check.ll

  • This file was added.
; 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