[MC] Relaxable Fragments Can be Linker Relaxable

[lenary]

When relaxing a relaxable fragment, the new fixup may end up being linker relaxable (when the one before relaxation was not).

This change ensures that:

• a new linker-relaxable fixup propagates that info into the fragment and the section.
• linker-relaxable relaxable fragments are handled in attemptToFoldSymbolOffsetDifference.

Fixes: #150071

[llvmbot]

@llvm/pr-subscribers-mc

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

Author: Sam Elliott (lenary)

Changes

When relaxing a relaxable fragment, the new fixup may end up being linker relaxable (when the one before relaxation was not).

This change ensures that:

• a new linker-relaxable fixup propagates that info into the fragment and the section.
• linker-relaxable relaxable fragments are handled in attemptToFoldSymbolOffsetDifference.

Fixes: #150071

---

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

5 Files Affected:

• (modified) llvm/lib/MC/MCAssembler.cpp (+8)
• (modified) llvm/lib/MC/MCExpr.cpp (+4-1)
• (modified) llvm/lib/MC/MCFragment.cpp (+2)
• (modified) llvm/lib/MC/MCSection.cpp (+2)
• (added) llvm/test/MC/RISCV/linker-relaxation-pr150071.s (+18)

diff --git a/llvm/lib/MC/MCAssembler.cpp b/llvm/lib/MC/MCAssembler.cpp
index 2b56e2a3dbf2a..4d487e6e92dfb 100644
--- a/llvm/lib/MC/MCAssembler.cpp
+++ b/llvm/lib/MC/MCAssembler.cpp
@@ -777,6 +777,14 @@ bool MCAssembler::relaxInstruction(MCFragment &F) {
   getEmitter().encodeInstruction(Relaxed, Data, Fixups, *F.getSubtargetInfo());
   F.setVarContents(Data);
   F.setVarFixups(Fixups);
+
+  for (const auto &Fixup : Fixups) {
+    if (!Fixup.isLinkerRelaxable())
+      continue;
+    F.setLinkerRelaxable();
+    F.getParent()->setLinkerRelaxable();
+  }
+
   return true;
 }
 
diff --git a/llvm/lib/MC/MCExpr.cpp b/llvm/lib/MC/MCExpr.cpp
index dbb2fd16eb2e5..c3ccdb3e31464 100644
--- a/llvm/lib/MC/MCExpr.cpp
+++ b/llvm/lib/MC/MCExpr.cpp
@@ -269,7 +269,7 @@ bool MCExpr::evaluateAsAbsolute(int64_t &Res, const MCAssembler *Asm,
   return IsRelocatable && Value.isAbsolute() && Value.getSpecifier() == 0;
 }
 
-/// Helper method for \see EvaluateSymbolAdd().
+/// Helper method for \see evaluateSymbolicAdd().
 static void attemptToFoldSymbolOffsetDifference(const MCAssembler *Asm,
                                                 bool InSet, const MCSymbol *&A,
                                                 const MCSymbol *&B,
@@ -361,6 +361,9 @@ static void attemptToFoldSymbolOffsetDifference(const MCAssembler *Asm,
         if (BBeforeRelax && AAfterRelax)
           return;
       }
+      if (F->getKind() == MCFragment::FT_Relaxable && Asm->hasFinalLayout() && F->isLinkerRelaxable())
+        // FIXME: More accurate calculation of AAfterRelax/BBeforeRelax?
+        return;
       if (&*F == FA) {
         // If FA and FB belong to the same subsection, the loop will find FA and
         // we can resolve the difference.
diff --git a/llvm/lib/MC/MCFragment.cpp b/llvm/lib/MC/MCFragment.cpp
index 3c395e5ccdb0b..68e7d61fe2f7b 100644
--- a/llvm/lib/MC/MCFragment.cpp
+++ b/llvm/lib/MC/MCFragment.cpp
@@ -68,6 +68,8 @@ LLVM_DUMP_METHOD void MCFragment::dump() const {
       OS << "\n  Fixup @" << F.getOffset() << " Value:";
       F.getValue()->print(OS, nullptr);
       OS << " Kind:" << F.getKind();
+      if (F.isLinkerRelaxable())
+        OS << " LinkerRelaxable";
     }
   };
 
diff --git a/llvm/lib/MC/MCSection.cpp b/llvm/lib/MC/MCSection.cpp
index 023f7f27de0aa..54e3ebcedb46f 100644
--- a/llvm/lib/MC/MCSection.cpp
+++ b/llvm/lib/MC/MCSection.cpp
@@ -41,6 +41,8 @@ LLVM_DUMP_METHOD void MCSection::dump(
   raw_ostream &OS = errs();
 
   OS << "MCSection Name:" << getName();
+  if (isLinkerRelaxable())
+    OS << " LinkerRelaxable";
   for (auto &F : *this) {
     OS << '\n';
     F.dump();
diff --git a/llvm/test/MC/RISCV/linker-relaxation-pr150071.s b/llvm/test/MC/RISCV/linker-relaxation-pr150071.s
new file mode 100644
index 0000000000000..7d5297fbaa937
--- /dev/null
+++ b/llvm/test/MC/RISCV/linker-relaxation-pr150071.s
@@ -0,0 +1,18 @@
+# RUN: llvm-mc --triple=riscv32 -mattr=+relax,+experimental-xqcilb \
+# RUN:    %s -filetype=obj -o - -riscv-add-build-attributes \
+# RUN:    | llvm-objdump -dr - \
+# RUN:    | FileCheck %s
+
+.global foo
+
+bar:
+  jal x1, foo
+# CHECK: qc.e.jal 0x0 <bar>
+# CHECK-NEXT: R_RISCV_VENDOR QUALCOMM
+# CHECK-NEXT: R_RISCV_CUSTOM195 foo
+# CHECK-NEXT: R_RISCV_RELAX *ABS*
+  bne a0, a1, bar
+# CHECK-NEXT: bne a0, a1, 0x6 <bar+0x6>
+# CHECK-NEXT: R_RISCV_BRANCH bar
+  ret
+# CHECK-NEXT: ret

[lenary]

This does not cope with the other direction correctly, but I'm not sure we can.

The other direction is when a linker relaxable fixup is replaced by a non-linker-relaxable fixup in relaxation, which might need the fragment to be marked as not linker relaxable. I'm not sure this will ever happen though.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[svs-quic]

LGTM but please wait for someone else to review before merging.

[MaskRay]

This does not look right. The LinkerRelaxable property should be set exclusively by MCObjectStreamer and remain unchanged. Modifying it during relaxation is inappropriate. Request changes to prevent accidental land. I'll think about this.

For example, MCObjectStreamer::emitDwarfAdvanceLineAddr absoluteSymbolDiff relies on LinkerRelaxable being correct at streaming time.

[lenary]

@MaskRay I wasn't going to land this until you had approved it, accidentally or not.

Modifying it during relaxation is inappropriate.

So we can never relaxInstruction from something not linker-relaxable to one that is? That is going to cause problems on RISC-V, where most of the RVC fixups are not linker relaxable, but more and more of the 32-bit instructions should be linker relaxable (if we had kept up with the psABI).

There's a lot more detail in the issue, hopefully it clarifies what this is trying to fix. This is causing real bugs for us with the LLVM 21 branch.

[MaskRay]

This appears incorrect. The LinkerRelaxable property should be set exclusively by MCObjectStreamer and remain unchanged. Modifying it during relaxation is inappropriate. Please revise to prevent unintended changes.

Thank you! I know you wouldn't😊 I've just gotten into the habit of clicking "Request Changes" in these situations...

In the past, changes to areas I care about, like lld and MC, were sometimes approved and landed before their implications were fully thought through, requiring me to revise them later.
I manage code reviews in my limited spare time (less than a year ago), and I occasionally miss notifications (sorry for any delays).

---

At streaming/fragment generation time, we want to support code like absoluteSymbolDiff to check LinkerRelaxable and determine whether a new fragment is needed.
If an instruction can become linker-relaxable after assembler relaxation, it must be marked LinkerRelaxable upfront.
While this conservative approach may result in more FT_Relaxable fragments (I'm considering renaming this to FT_Backend), it preserves a critical invariant.

... but more and more of the 32-bit instructions should be linker relaxable (if we had kept up with the psABI).

Assembler relaxing them from 32-bit to 48-bit and allowing linker relaxation?
We'll need to conservatively mark them as LinkerRelaxable during streaming.

Aside: It seems the base RISC-V ISA has significant issues. Sigh...

After assembler layout, it might be safe to clear the LinkerRelaxable bit.

[lenary]

Assembler relaxing them from 32-bit to 48-bit and allowing linker relaxation?

No. Today, in rv32izca, If you write jal x1, foo (assume foo is undefined), it will be compressed to c.jal foo before it is emitted to the streamer. The fixup on c.jal foo is not linker relaxable, but the instruction itself is relaxable, and we rely on assembler relaxation to turn c.jal foo back to jal x1, foo. Today, the fixup on jal x1, foo is not marked linker relaxable, but the psabi now has relaxations for the corresponding relocation, so we should be marking fixup_riscv_jal as a RelaxCandidate. This is why we haven't hit this case yet.

We do hit this case because in rv32izca_xqcilb, if you write qc.e.j foo, it gets compressed to c.j foo before it is emitted to the streamer, and we need to assembler-relax it back to qc.e.j foo (which the user wrote). The fixup on qc.e.j is marked as linker relaxable.

---

Here's why I don't fully understand your analysis of absoluteSymbolDiff - the c.j foo which is sent to the streamer will have mayNeedRelaxation return true, so this instruction will end up in the variable part of a fragment, as far as I can understand from your code. Differences that cross this variable part should cause absoluteSymbolDiff to return std::nullopt, and all the codepaths will use expressions rather than doing the eager-evaluation to a constant. Note, in particular, the point here is to mark something that the assembler already doesn't know how big it is (at streaming time) as "we will never know", rather than "we will know after layout".

I used llvm-mc -g (i.e., debug info generated by the assembler) and then llvm-readelf/llvm-dwarfdump to look at the output and we end up with a ADD16/SUB16 relocation pair for advancing the PC over the now-linker-relaxable instruction (as I would have expected), and another pair for advancing the PC over the instruction afterwards (which is not necessary but not wrong, IIUC). There is still a DW_LNS_advance_pc with an eagerly-evaluated absolute symbol difference for advancing over the ret.

---

I don't think we can conservatively mark the fixups as linker relaxable when streaming, and then clear the bit later, but I'm not 100% sure. I thought we didn't have an accurate MCSubtargetInfo, but it looks like we do have that for the fragment.