[RISCV] Set DebugLoc of epilogue

[wangpc-pp]

There is a problem I found when discussing at
https://discourse.llvm.org/t/llvm-generates-wrong-dwarf-line-table-to-gdb/75454/10:
if we set a breakpoint to the end of a function, the debugger will
stop at the place after restoring registers, not before it, which
is a wrong behavior I think.

There are two problems here actually:

1. Instructions for restoring registers don't have debug line info
   as we just set it to DebugLoc().
2. We can't recognize the right epilogue beginning epilogue_begin.
   This is a feature introduced by https://reviews.llvm.org/D133376.
   In short, the epilogue beginning will be the first instruction
   with flag FrameDestroy. The problem for RISCV target is that
   we only set FrameDestroy flag for stack pointer recovering
   instructions(IIUC).

This PR fixes the first problem and the fix is to use the DebugLoc
getting from the insert point I like other in-tree targets.

As for second problem, I don't have a fix now and I think it may
be hard to fix, so I will leave it there.

A test is added to show the change, please see the topmost commit.

[llvmbot]

@llvm/pr-subscribers-backend-risc-v

Author: Wang Pengcheng (wangpc-pp)

Changes

There is a problem I found when discussing at
https://discourse.llvm.org/t/llvm-generates-wrong-dwarf-line-table-to-gdb/75454/10:
if we set a breakpoint to the end of a function, the debugger will
stop at the place after restoring registers, not before it, which
is a wrong behavior I think.

There are two problems here actually:

1. Instructions for restoring registers don't have debug line info
   as we just set it to DebugLoc().
2. We can't recognize the right epilogue beginning epilogue_begin.
   This is a feature introduced by https://reviews.llvm.org/D133376.
   In short, the epilogue beginning will be the first instruction
   with flag FrameDestroy. The problem for RISCV target is that
   we only set FrameDestroy flag for stack pointer recovering
   instructions(IIUC).

This PR fixes the first problem and the fix is copied from MIPS target.

As for second problem, I don't have a fix now and I think it may
be hard to fix, so I will leave it there.

I don't know how to test this, and there is no test coverage in tree,
any suggestions on how to test this are welcome.

---

Full diff: https://github.com/llvm/llvm-project/pull/74702.diff

1 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVInstrInfo.cpp (+6-2)

diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
index 1dcff7eb563e2..6169e6b8e1fa4 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
@@ -681,6 +681,10 @@ void RISCVInstrInfo::loadRegFromStackSlot(MachineBasicBlock &MBB,
                                           const TargetRegisterClass *RC,
                                           const TargetRegisterInfo *TRI,
                                           Register VReg) const {
+  DebugLoc DL;
+  if (I != MBB.end())
+    DL = I->getDebugLoc();
+
   MachineFunction *MF = MBB.getParent();
   MachineFrameInfo &MFI = MF->getFrameInfo();
 
@@ -741,7 +745,7 @@ void RISCVInstrInfo::loadRegFromStackSlot(MachineBasicBlock &MBB,
         MemoryLocation::UnknownSize, MFI.getObjectAlign(FI));
 
     MFI.setStackID(FI, TargetStackID::ScalableVector);
-    BuildMI(MBB, I, DebugLoc(), get(Opcode), DstReg)
+    BuildMI(MBB, I, DL, get(Opcode), DstReg)
         .addFrameIndex(FI)
         .addMemOperand(MMO);
   } else {
@@ -749,7 +753,7 @@ void RISCVInstrInfo::loadRegFromStackSlot(MachineBasicBlock &MBB,
         MachinePointerInfo::getFixedStack(*MF, FI), MachineMemOperand::MOLoad,
         MFI.getObjectSize(FI), MFI.getObjectAlign(FI));
 
-    BuildMI(MBB, I, DebugLoc(), get(Opcode), DstReg)
+    BuildMI(MBB, I, DL, get(Opcode), DstReg)
         .addFrameIndex(FI)
         .addImm(0)
         .addMemOperand(MMO);

[topperc]

What do X86 or Aarch64 do? I'm not sure if I trust copying something from MIPS. MIPS doesn't seem to be getting many contributions these days.

[wangpc-pp]

What do X86 or Aarch64 do? I'm not sure if I trust copying something from MIPS. MIPS doesn't seem to be getting many contributions these days.

Sorry for unclear description, most in-tree targets will use the DebugLoc getting from the insert point I. But the two targets (AArch64/X86) you mentioned don't do it in loadRegFromStackSlot... I don't know if they have the issue.

[asb]

I believe these were intentionally removed in 700042cd8890 which in turn references 3e08170 by @MatzeB.

You might also be interested in https://reviews.llvm.org/D143248 which sadly got completely stuck as nobody seems to be able to review it.

[wangpc-pp]

I believe these were intentionally removed in 700042cd8890 which in turn references 3e08170 by @MatzeB.

Oh thanks! I forgot that I glanced over the patch before...
But is this the right way for current situation? The debugger behaves unproperly.

You might also be interested in https://reviews.llvm.org/D143248 which sadly got completely stuck as nobody seems to be able to review it.

I will take a look! Thanks!

[asb]

But is this the right way for current situation? The debugger behaves unproperly.

My standard assumption would be that following what X86/AArch64/Arm chose to do is correct and that remaining issues are likely caused by something else. I could be wrong of course, but I think you'd need to try to convince us as reviewers of why a different choice vs those targets is desirable here.

I really don't think there's been all that much critical analysis of the debuggability of LLVM generated RISC-V executables (though issues have been raised and fixed over the years), so I'm sure there are problems to be resolved like the one you raised in this PR.

[LiqinWeng]

pls see:

1. https://reviews.llvm.org/D128806
2. https://reviews.llvm.org/D129173
   I remove it before