[DebugInfo] Fix ARM/AArch64 prologue_end position

[kubamracek]

Summary

It seems that DWARF prologue_end flag, which should be placed right after the function prologue, is often placed at the function entry point for ARM and AArch64. I filed https://llvm.org/bugs/show_bug.cgi?id=24117 about it, where I thought this was a recent regression for -O1, but in fact I found an even easier test case which reproduces this bug with basically all versions of LLVM and at -O0:

int func(void);
void prologue_end_test() {
  func();
  func();
}

compiles to (with -g -O0 -armv7):

_prologue_end_test:
Lfunc_begin0:
    .file   1 "foo.c"
    .loc    1 2 0                   @ foo.c:2:0
    .cfi_startproc
@ BB#0:                                 @ %entry
    .loc    1 3 3 prologue_end      @ foo.c:3:3
    push    {r7, lr}
    mov r7, sp
    sub sp, #8
    bl  _func
    .loc    1 4 3                   @ foo.c:4:3
    str r0, [sp, #4]            @ 4-byte Spill
    bl  _func
    ...

Obviously, the prologue isn't empty, and the prologue_end line should actually be placed before the first bl _func call.

This bug is not present on x86/x86_64. The reason why this happens is that the ARM and AArch64 backends emit line number information for the prologue's MIs. When generating the prologue, the DebugLoc from the first MI is copied to all the instructions of the prologue (but the first MI's DebugLoc can be empty in which case things are fine). The X86 backend doesn't do that and it never assigns line information to the prologue. The DWARF debug info writer attaches the prologue_end flag to the first instruction that has a non-empty debug location.

This patch changes the ARM and AArch64 prologues not to have line information.

[aprantl]

There should be a comment explaining that the DebugLocs are dropped because of the LLVM convention that the prologue end is marked by the first instruction with a non-empty DebugLoc.

[kubamracek]

In D11268#206453, @aprantl wrote:

There should be a comment explaining that the DebugLocs are dropped because of the LLVM convention that the prologue end is marked by the first instruction with a non-empty DebugLoc.

In the test cases or in the backend changes? (Sorry, I'm not really familiar with these parts of LLVM)

[iid_iunknown]

ARM and AArch64 appear to be not the only backends with this problem.
Mips, SystemZ and Sparc backends similarly place prologue_end at the function entry point.

[kubamracek]

BTW, I think this is why ASan test cases, such as https://github.com/llvm-mirror/compiler-rt/blob/master/test/asan/TestCases/use-after-free.cc need to be currently XFAIL'd on ARM.

[kubamracek]

Extending the fix to all backends that attach debug locations to generated prologue. Added test cases for all these targets. Added a comment explaining why we want to use empty/invalid location (actually, I found that comment in the XCore backend, and just copied it).

[asl]

lib/Target/Mips/Mips16InstrInfo.cpp

448 ↗

(On Diff #30142)

Wow. Looks like MIPS backend certainly deserves a good run of clang-format! (unrelated to this patch :) )

[asl]

In general, LGTM. But we certainly need to ping the maintainers of the backends :)

[kubamracek]

In D11268#209702, @asl wrote:

In general, LGTM. But we certainly need to ping the maintainers of the backends :)

Could you please CC them on this patch?

[rengolin]

LGTM.

[echristo]

One inline comment. My general preference would be an explicit DebugLoc in all of these uses, but maybe it doesn't matter that much. The inline comment does :)

-eric

lib/Target/ARM/ARMFrameLowering.cpp

327–328

The DebugLoc for all uses of emitSPUpdate is now just DebugLoc so you can remove the parameter.

[kubamracek]

lib/Target/ARM/ARMFrameLowering.cpp

327–328

Hm, but emitSPUpdate is also used from emitEpilogue and eliminateCallFramePseudoInstr, where some debug location is still used (and it's not relevant to the bug I'm trying to fix, which is wrong prologue_end in DWARF).

[echristo]

Sure, if you could look at that and write it in a way that doesn't it'd be appreciated because it's mostly dead at this point, but please go ahead here.

-eric

[iid_iunknown]

Hi Kuba,

This review has been hold for some time.
There are merge conflicts with the current trunk now. I rebased your changes, solved the conflicts and updated the patch for your convenience. It is not possible to update someone else's patch so I created a new request: http://reviews.llvm.org/D14338.

Please note that the changes in MipsSEFrameLowering.cpp, MipsSEInstrInfo.cpp, Mips/prologue_end.ll and delay-slot.ll are no longer needed as they have been done in r246309.

The prologue_end.ll tests fail as the subprogram definitions are required to be distinct. They have been marked distinct to make the tests pass.

Would you take a look at the new patch and let me know if you were going to commit your changes, please? I could commit it for you if this would be more convenient.

Thank you.

[kubamracek]

Thanks for doing this! Please go ahead and commit (this patch was already LGTM'd so I think it's okay). Please note that when you commit, you might see a buildbot failure in ARM/AArch64 tests in compiler-rt/sanitizers/ASan/TSan -- this might actually be okay, IIRC there are a few XFAILs because of this bug. Let me know if you see such a test failure.

[iid_iunknown]

Thank you for your review and remarks!
D14338 has been committed. I will let you know if such problems with the tests arise.

lib/Target/AArch64/AArch64FrameLowering.cpp

void AArch64FrameLowering::emitPrologue(MachineFunction &MF,
   const Function *Fn = MF.getFunction();
   const AArch64RegisterInfo *RegInfo = static_cast<const AArch64RegisterInfo *>(
       MF.getSubtarget().getRegisterInfo());
   const TargetInstrInfo *TII = MF.getSubtarget().getInstrInfo();
   MachineModuleInfo &MMI = MF.getMMI();
   AArch64FunctionInfo *AFI = MF.getInfo<AArch64FunctionInfo>();
   bool needsFrameMoves = MMI.hasDebugInfo() || Fn->needsUnwindTableEntry();
   bool HasFP = hasFP(MF);
-  DebugLoc DL = MBB.findDebugLoc(MBBI);
+  DebugLoc DL;
 
   // All calls are tail calls in GHC calling conv, and functions have no
   // prologue/epilogue.
   if (MF.getFunction()->getCallingConv() == CallingConv::GHC)
     return;
 
   int NumBytes = (int)MFI->getStackSize();
   if (!AFI->hasStackFrame()) {
bool AArch64FrameLowering::spillCalleeSavedRegisters(
     const std::vector<CalleeSavedInfo> &CSI,
     const TargetRegisterInfo *TRI) const {
   MachineFunction &MF = *MBB.getParent();
   const TargetInstrInfo &TII = *MF.getSubtarget().getInstrInfo();
   unsigned Count = CSI.size();
   DebugLoc DL;
   assert((Count & 1) == 0 && "Odd number of callee-saved regs to spill!");
 
-  if (MI != MBB.end())
-    DL = MI->getDebugLoc();
-
   for (unsigned i = 0; i < Count; i += 2) {
     unsigned idx = Count - i - 2;
     unsigned Reg1 = CSI[idx].getReg();
     unsigned Reg2 = CSI[idx + 1].getReg();
     // GPRs and FPRs are saved in pairs of 64-bit regs. We expect the CSI
     // list to come in sorted by frame index so that we can issue the store
     // pair instructions directly. Assert if we see anything otherwise.
     //

lib/Target/ARM/ARMFrameLowering.cpp

void ARMFrameLowering::emitPrologue(MachineFunction &MF,
   const ARMBaseInstrInfo &TII = *STI.getInstrInfo();
   assert(!AFI->isThumb1OnlyFunction() &&
          "This emitPrologue does not support Thumb1!");
   bool isARM = !AFI->isThumbFunction();
   unsigned Align = STI.getFrameLowering()->getStackAlignment();
   unsigned ArgRegsSaveSize = AFI->getArgRegsSaveSize();
   unsigned NumBytes = MFI->getStackSize();
   const std::vector<CalleeSavedInfo> &CSI = MFI->getCalleeSavedInfo();
-  DebugLoc dl = MBBI != MBB.end() ? MBBI->getDebugLoc() : DebugLoc();
+  DebugLoc dl;
   unsigned FramePtr = RegInfo->getFrameRegister(MF);
 
   // Determine the sizes of each callee-save spill areas and record which frame
   // belongs to which callee-save spill areas.
   unsigned GPRCS1Size = 0, GPRCS2Size = 0, DPRCSSize = 0;
   int FramePtrSpillFI = 0;
   int D8SpillFI = 0;
 
   // All calls are tail calls in GHC calling conv, and functions have no
   // prologue/epilogue.
   if (MF.getFunction()->getCallingConv() == CallingConv::GHC)
     return;
 
   StackAdjustingInsts DefCFAOffsetCandidates;
   bool HasFP = hasFP(MF);
 
   // Allocate the vararg register save area.
   if (ArgRegsSaveSize) {
     emitSPUpdate(isARM, MBB, MBBI, dl, TII, -ArgRegsSaveSize,
                  MachineInstr::FrameSetup);
     DefCFAOffsetCandidates.addInst(std::prev(MBBI), ArgRegsSaveSize, true);
   }
 
   if (!AFI->hasStackFrame() &&
       (!STI.isTargetWindows() || !WindowsRequiresStackProbe(MF, NumBytes))) {
     if (NumBytes - ArgRegsSaveSize != 0) {
       emitSPUpdate(isARM, MBB, MBBI, dl, TII, -(NumBytes - ArgRegsSaveSize),
                    MachineInstr::FrameSetup);
void ARMFrameLowering::emitPushInst(MachineBasicBlock &MBB,
                                     bool NoGap,
                                     bool(*Func)(unsigned, bool),
                                     unsigned NumAlignedDPRCS2Regs,
                                     unsigned MIFlags) const {
   MachineFunction &MF = *MBB.getParent();
   const TargetInstrInfo &TII = *MF.getSubtarget().getInstrInfo();
 
   DebugLoc DL;
-  if (MI != MBB.end()) DL = MI->getDebugLoc();
 
   SmallVector<std::pair<unsigned,bool>, 4> Regs;
   unsigned i = CSI.size();
   while (i != 0) {
     unsigned LastReg = 0;
     for (; i != 0; --i) {
       unsigned Reg = CSI[i-1].getReg();
       if (!(Func)(Reg, STI.isTargetDarwin())) continue;

test/DebugInfo/AArch64/prologue_end.ll

+; RUN: llc -disable-fp-elim -O0 %s -mtriple aarch64-apple-darwin -o - | FileCheck %s
+
+; int func(void);
+; void prologue_end_test() {
+;   func();
+;   func();
+; }
+
+define void @prologue_end_test() nounwind uwtable {
+  ; CHECK: prologue_end_test:
+  ; CHECK: .cfi_startproc
+  ; CHECK: stp x29, x30
+  ; CHECK: mov x29, sp
+  ; CHECK: sub sp, sp
+  ; CHECK: .loc 1 3 3 prologue_end
+  ; CHECK: bl _func
+  ; CHECK: bl _func
+entry:
+  %call = call i32 @func(), !dbg !11
+  %call1 = call i32 @func(), !dbg !12
+  ret void, !dbg !13
+}
+
+declare i32 @func()
+
+!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.7.0 (trunk 242129)", isOptimized: false, runtimeVersion: 0, emissionKind: 1, enums: !2, subprograms: !3)
+!1 = !DIFile(filename: "foo.c", directory: "/tmp")
+!2 = !{}
+!3 = !{!4}
+!4 = !DISubprogram(name: "prologue_end_test", scope: !1, file: !1, line: 2, type: !5, isLocal: false, isDefinition: true, scopeLine: 2, isOptimized: false, function: void ()* @prologue_end_test, variables: !2)
+!5 = !DISubroutineType(types: !6)
+!6 = !{null}
+!7 = !{i32 2, !"Dwarf Version", i32 2}
+!8 = !{i32 2, !"Debug Info Version", i32 3}
+!9 = !{i32 1, !"PIC Level", i32 2}
+!10 = !{!"clang version 3.7.0 (trunk 242129)"}
+!11 = !DILocation(line: 3, column: 3, scope: !4)
+!12 = !DILocation(line: 4, column: 3, scope: !4)
+!13 = !DILocation(line: 5, column: 1, scope: !4)

test/DebugInfo/ARM/prologue_end.ll

+; RUN: llc -disable-fp-elim -O0 %s -mtriple armv7-apple-darwin -o - | FileCheck %s
+
+; int func(void);
+; void prologue_end_test() {
+;   func();
+;   func();
+; }
+
+define void @prologue_end_test() nounwind uwtable {
+  ; CHECK: prologue_end_test:
+  ; CHECK: .cfi_startproc
+  ; CHECK: push {r7, lr}
+  ; CHECK: mov r7, sp
+  ; CHECK: sub sp, sp, #8
+  ; CHECK: .loc 1 3 3 prologue_end
+  ; CHECK: bl _func
+  ; CHECK: bl _func
+entry:
+  %call = call i32 @func(), !dbg !13
+  %call1 = call i32 @func(), !dbg !14
+  ret void, !dbg !15
+}
+
+declare i32 @func()
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!7, !8, !9, !10, !11}
+!llvm.ident = !{!12}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 3.7.0 (trunk 242129)", isOptimized: false, runtimeVersion: 0, emissionKind: 1, enums: !2, subprograms: !3)
+!1 = !DIFile(filename: "foo.c", directory: "/tmp")
+!2 = !{}
+!3 = !{!4}
+!4 = !DISubprogram(name: "prologue_end_test", scope: !1, file: !1, line: 2, type: !5, isLocal: false, isDefinition: true, scopeLine: 2, isOptimized: false, function: void ()* @prologue_end_test, variables: !2)
+!5 = !DISubroutineType(types: !6)
+!6 = !{null}
+!7 = !{i32 2, !"Dwarf Version", i32 2}
+!8 = !{i32 2, !"Debug Info Version", i32 3}
+!9 = !{i32 1, !"wchar_size", i32 4}
+!10 = !{i32 1, !"min_enum_size", i32 4}
+!11 = !{i32 1, !"PIC Level", i32 2}
+!12 = !{!"clang version 3.7.0 (trunk 242129)"}
+!13 = !DILocation(line: 3, column: 3, scope: !4)
+!14 = !DILocation(line: 4, column: 3, scope: !4)
+!15 = !DILocation(line: 5, column: 1, scope: !4)