[PATCH] D150004: [RISCV] .debug_line: emit relocations for assembly input files

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added a comment.

@reames My original attempt is this:

  diff --git a/llvm/include/llvm/MC/MCAsmBackend.h b/llvm/include/llvm/MC/MCAsmBackend.h
  index 354f9d8e993f..f200d5ef3aa8 100644
  --- a/llvm/include/llvm/MC/MCAsmBackend.h
  +++ b/llvm/include/llvm/MC/MCAsmBackend.h
  @@ -44,2 +44,5 @@ protected: // Can only create subclasses.
  
  +  // If true, force a MCFixup to emit a relocation.
  +  bool ForceRelocs = false;
  +
   public:
  @@ -51,2 +54,4 @@ public:
  
  +  bool getForceRelocs() { return ForceRelocs; }
  +
     /// Return true if this target might automatically pad instructions and thus
  diff --git a/llvm/lib/MC/MCObjectStreamer.cpp b/llvm/lib/MC/MCObjectStreamer.cpp
  index d609194992a5..fa65fb2857b9 100644
  --- a/llvm/lib/MC/MCObjectStreamer.cpp
  +++ b/llvm/lib/MC/MCObjectStreamer.cpp
  @@ -544,3 +544,5 @@ void MCObjectStreamer::emitDwarfAdvanceLineAddr(int64_t LineDelta,
     int64_t Res;
  -  if (AddrDelta->evaluateAsAbsolute(Res, getAssemblerPtr())) {
  +  MCAssembler *As = getAssemblerPtr();
  +  bool ForceRelocs = As && As->getBackend().getForceRelocs();
  +  if (!ForceRelocs && AddrDelta->evaluateAsAbsolute(Res, As)) {
       MCDwarfLineAddr::Emit(this, Assembler->getDWARFLinetableParams(), LineDelta,
  diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
  index b5670b6214c2..96bd6694aa6e 100644
  --- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
  +++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
  @@ -27,3 +27,2 @@ class RISCVAsmBackend : public MCAsmBackend {
     bool Is64Bit;
  -  bool ForceRelocs = false;
     const MCTargetOptions &TargetOptions;

I.e. toggling the fast path to be aware of RISC-V linker relaxation. However, I switched to the current version as it simplifies code and ensures an involved RISC-V special case (`getBackend().relaxDwarfLineAddr(DF, Layout, WasRelaxed)` in `MCAssembler::relaxDwarfLineAddr`) occurs in just one place. This simplification helps reasoning about the code.

As a penalty, we pay the `clang -c a.s` overhead (~2%) but the more common `clang -g -c a.c` is unaffected (possibly slightly faster since we avoid likely-fail `AddrDelta->evaluateAsAbsolute(Res, getAssemblerPtr())`).

---

We have a similar issue in `.debug_frame/.eh_frame`. I will send another patch to fix it.



================
Comment at: llvm/lib/MC/MCObjectStreamer.cpp:545
-  int64_t Res;
-  if (AddrDelta->evaluateAsAbsolute(Res, getAssemblerPtr())) {
-    MCDwarfLineAddr::Emit(this, Assembler->getDWARFLinetableParams(), LineDelta,
----------------
reames wrote:
> The code structure here makes it seem like evaluateAsAbsolute should be returning false in this case.  Removing the fastpath here certainly hides this bug, but do we have others lurking due to incorrect return results from evaluateAsAbsolute?
> 
> Not my area, so totally possible I'm off base here.  
See my main comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150004/new/

https://reviews.llvm.org/D150004