[RISCV] Leave pcrel_hi/pcrel_lo fixup pairs unresolved

[lewis-revill]

Summary

This patch modifies shouldForceRelocation to always return true for pcrel_hi and pcrel_lo fixups. This avoids having to use various levels of indirection in order to correctly determine whether a given one of these fixups can be safely resolved, since there are various situations where determining whether a given fixup can be safely resolved relies on knowing whether its corresponding fixup could be safely resolved.

[asb]

test/MC/RISCV/linker-relaxation.s

125

Shouldn't be this be .Ltmp1 0x0 just like in the RELAX-RELOC case?

[asb]

test/MC/RISCV/linker-relaxation.s

125

Hi Lewis, did you have any thoughts on this?

[jrtc27]

@asb I prefer D60657 over this as it seems to cover everything, and this is a rather big hammer to be hitting the problem with (that GNU as manages to avoid needing). Have you had a look at that one?

[jrtc27]

test/MC/RISCV/linker-relaxation.s

125

Yes, it should be. It's because RISCVMCExpr::evaluateAsRelocatableImpl evaluates it fully to the indirected symbol since RAB.willForceRelocations is false, unlike the RELAX-RELOC case.

[lewis-revill]

test/MC/RISCV/linker-relaxation.s

125

So, if I understand correctly, evaluatePCRelLo will do it's work regardless of whether the relocation is actually evaluated, it just uses this 'best guess' function RAB.willForceRelocations and some of its own logic. Is this not a bug regardless of this patch? Are we relying on the logic in shouldForceRelocations and evaluatePCRelLo producing the exact same answer?

Are there uses of evaluatePCRelLo that aren't part of this assembler pipeline? Because if not then doesn't that imply that for this patch the entire evaluatePCRelLo functionality would be completely removed?

[jrtc27]

Lewis, I believe this should now be marked abandoned in light of the other fixes?

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

bool RISCVAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
   bool ShouldForce = false;
 
   switch ((unsigned)Fixup.getKind()) {
   default:
     break;
   case RISCV::fixup_riscv_got_hi20:
   case RISCV::fixup_riscv_tls_got_hi20:
   case RISCV::fixup_riscv_tls_gd_hi20:
-    return true;
+  case RISCV::fixup_riscv_pcrel_hi20:
+      ShouldForce = true;
+      break;
   case RISCV::fixup_riscv_pcrel_lo12_i:
   case RISCV::fixup_riscv_pcrel_lo12_s:
-    // For pcrel_lo12, force a relocation if the target of the corresponding
-    // pcrel_hi20 is not in the same fragment.
+    // pcrel_lo fixups should always cause relocations to be forced, but we can
+    // do some error checking here anyway to ensure the corresponding fixup
+    // exists and is valid.
     const MCFixup *T = cast<RISCVMCExpr>(Fixup.getValue())->getPCRelHiFixup();
     if (!T) {
       Asm.getContext().reportError(Fixup.getLoc(),
                                    "could not find corresponding %pcrel_hi");
       return false;
     }
 
     switch ((unsigned)T->getKind()) {
     default:
       llvm_unreachable("Unexpected fixup kind for pcrel_lo12");
       break;
     case RISCV::fixup_riscv_got_hi20:
     case RISCV::fixup_riscv_tls_got_hi20:
     case RISCV::fixup_riscv_tls_gd_hi20:
-      ShouldForce = true;
-      break;
     case RISCV::fixup_riscv_pcrel_hi20:
-      ShouldForce = T->getValue()->findAssociatedFragment() !=
-                    Fixup.getValue()->findAssociatedFragment();
+      ShouldForce = true;
       break;
     }
     break;
   }
 
   return ShouldForce || STI.getFeatureBits()[RISCV::FeatureRelax] ||
          ForceRelocs;
 }

test/MC/RISCV/fixups.s

 # CHECK-FIXUP: fixup A - offset: 0, value: %lo(val), kind: fixup_riscv_lo12_i
 # CHECK-INSTR: addi a1, t1, 1656
 sw a0, %lo(val)(t1)
 # CHECK-FIXUP: fixup A - offset: 0, value: %lo(val), kind: fixup_riscv_lo12_s
 # CHECK-INSTR: sw a0, 1656(t1)
 
 1:
 auipc t1, %pcrel_hi(.LBB0)
+# CHECK-REL: R_RISCV_PCREL_HI20 .LBB0 0x0
 # CHECK-FIXUP: fixup A - offset: 0, value: %pcrel_hi(.LBB0), kind: fixup_riscv_pcrel_hi20
 # CHECK-INSTR: auipc t1, 0
 addi t1, t1, %pcrel_lo(1b)
+# CHECK-REL: R_RISCV_PCREL_LO12_I .LBB0 0x4
 # CHECK-FIXUP: fixup A - offset: 0, value: %pcrel_lo(.Ltmp0), kind: fixup_riscv_pcrel_lo12_i
-# CHECK-INSTR: addi t1, t1, -16
+# CHECK-INSTR: addi t1, t1, 0
 sw t1, %pcrel_lo(1b)(t1)
+# CHECK-REL: R_RISCV_PCREL_LO12_S .LBB0 0x8
 # CHECK-FIXUP: fixup A - offset: 0, value: %pcrel_lo(.Ltmp0), kind: fixup_riscv_pcrel_lo12_s
-# CHECK-INSTR: sw t1, -16(t1)
+# CHECK-INSTR: sw t1, 0(t1)
 
 jal zero, .LBB0
 # CHECK-FIXUP: fixup A - offset: 0, value: .LBB0, kind: fixup_riscv_jal
 # CHECK-INSTR: jal zero, -28
 jal zero, .LBB2
 # CHECK-FIXUP: fixup A - offset: 0, value: .LBB2, kind: fixup_riscv_jal
 # CHECK-INSTR: jal zero, 330996
 beq a0, a1, .LBB0

test/MC/RISCV/linker-relaxation.s

 # NORELAX-RELOC-NOT: R_RISCV_RELAX
 # RELAX-RELOC: R_RISCV_LO12_S bar 0x0
 # RELAX-RELOC: R_RISCV_RELAX - 0x0
 # RELAX-FIXUP: fixup A - offset: 0, value: %lo(bar), kind: fixup_riscv_lo12_s
 # RELAX-FIXUP: fixup B - offset: 0, value: 0, kind: fixup_riscv_relax
 
 2:
 auipc t1, %pcrel_hi(bar)
-# NORELAX-RELOC-NOT: R_RISCV_PCREL_HI20
+# NORELAX-RELOC: R_RISCV_PCREL_HI20 bar 0x0
 # NORELAX-RELOC-NOT: R_RISCV_RELAX
 # RELAX-RELOC: R_RISCV_PCREL_HI20 bar 0x0
 # RELAX-RELOC: R_RISCV_RELAX - 0x0
 # RELAX-FIXUP: fixup A - offset: 0, value: %pcrel_hi(bar), kind: fixup_riscv_pcrel_hi20
 # RELAX-FIXUP: fixup B - offset: 0, value: 0, kind: fixup_riscv_relax
 
 addi t1, t1, %pcrel_lo(2b)
-# NORELAX-RELOC-NOT: R_RISCV_PCREL_LO12_I
+# NORELAX-RELOC: R_RISCV_PCREL_LO12_I bar 0x4
 # NORELAX-RELOC-NOT: R_RISCV_RELAX
 # RELAX-RELOC: R_RISCV_PCREL_LO12_I .Ltmp1 0x0
 # RELAX-RELOC: R_RISCV_RELAX - 0x0
 # RELAX-FIXUP: fixup A - offset: 0, value: %pcrel_lo(.Ltmp1), kind: fixup_riscv_pcrel_lo12_i
 # RELAX-FIXUP: fixup B - offset: 0, value: 0, kind: fixup_riscv_relax
 
 sb t1, %pcrel_lo(2b)(a2)
-# NORELAX-RELOC-NOT: R_RISCV_PCREL_LO12_S
+# NORELAX-RELOC: R_RISCV_PCREL_LO12_S bar 0x8
 # NORELAX-RELOC-NOT: R_RISCV_RELAX
 # RELAX-RELOC: R_RISCV_PCREL_LO12_S .Ltmp1 0x0
 # RELAX-RELOC: R_RISCV_RELAX - 0x0
 # RELAX-FIXUP: fixup A - offset: 0, value: %pcrel_lo(.Ltmp1), kind: fixup_riscv_pcrel_lo12_s
 # RELAX-FIXUP: fixup B - offset: 0, value: 0, kind: fixup_riscv_relax