[RISCV] Don't incorrectly force relocation for %pcrel_lo

[lewis-revill]

Summary

This patch updatess the logic introduced by D54029, which attempted to check if a %pcrel_lo relocation should be forced based on whether the previous %pcrel_hi fixup would have a relocation. The check forced the relocation if the fragment containing the symbol associated with the %pcrel_hi and the fragment containing the symbol referenced by the %pcrel_hi were different.

However there are some occassions where the symbol referenced by %pcrel_hi is in a different fragment to the %pcrel_hi itself but the assembler determines it can be resolved. The %pcrel_lo would be incorrectly emitted in this case, resulting in an invalid object file that would not link (dangerous relocation: %pcrel_lo missing matching %pcrel_hi).

This patch explicitly stores %pcrel_hi fixups that were previously resolved and uses this to check whether a relocation should be forced for a %pcrel_lo fixup. A %pcrel_lo relocation should never be forced if the corresponding %pcrel_hi fixup was resolved without a relocation.

[lewis-revill]

Remove irrelevant dependencies.

[rogfer01]

Hi @lewis-revill, thanks for the patch.

In https://reviews.llvm.org/rL349764#305030 I noted a similar issue but I forgot to follow it up later. Your change fixes it, so feel free to use it as a regression test if you want.

Kind regards,

[jrtc27]

Could we not have a situation where the pcrel_hi20 is seen *after* the lo12, in which case the lo12 would be forced but the hi20 could still be evaluated? E.g. I think the following would be problematic:

foo:
    j 1f
0:  addi a0, a0, %pcrel_lo(1f)
    ret
1: auipc a0, %pcrel_hi(bar)
    j 0b

.align 2
bar:
    # ...

Obviously extremely pathological code, but nonetheless valid.

[lewis-revill]

In D57240#1385261, @jrtc27 wrote:

Could we not have a situation where the pcrel_hi20 is seen *after* the lo12, in which case the lo12 would be forced but the hi20 could still be evaluated? E.g. I think the following would be problematic:

foo:
    j 1f
0:  addi a0, a0, %pcrel_lo(1f)
    ret
1: auipc a0, %pcrel_hi(bar)
    j 0b

.align 2
bar:
    # ...

Obviously extremely pathological code, but nonetheless valid.

Good call. It's a pathological case but maybe there are other cases as well. It's not a nice situation. Somehow we have to ensure there can never be a pcrel_lo12 relocation without a corresponding pcrel_hi20 relocation. It might help to know whether the pcrel_hi20 has been seen at all, regardless of whether it was resolved.

So I guess the intended behaviour might be:

pcrel_hi20 seen and not resolved (IE relocation emitted) -> force relocation
pcrel_hi20 seen and resolved -> don't force relocation
pcrel_hi20 not seen -> don't force relocation? If evaluatePcRelLo thinks it can be evaluated does it matter if the pcrel_hi20 can or not?

I'll have another think and take a look at your testcase.

[jrtc27]

In D57240#1385323, @lewis-revill wrote:

In D57240#1385261, @jrtc27 wrote:

Could we not have a situation where the pcrel_hi20 is seen *after* the lo12, in which case the lo12 would be forced but the hi20 could still be evaluated? E.g. I think the following would be problematic:

foo:
    j 1f
0:  addi a0, a0, %pcrel_lo(1f)
    ret
1: auipc a0, %pcrel_hi(bar)
    j 0b

.align 2
bar:
    # ...

Obviously extremely pathological code, but nonetheless valid.

Good call. It's a pathological case but maybe there are other cases as well. It's not a nice situation. Somehow we have to ensure there can never be a pcrel_lo12 relocation without a corresponding pcrel_hi20 relocation. It might help to know whether the pcrel_hi20 has been seen at all, regardless of whether it was resolved.

So I guess the intended behaviour might be:

pcrel_hi20 seen and not resolved (IE relocation emitted) -> force relocation
pcrel_hi20 seen and resolved -> don't force relocation
pcrel_hi20 not seen -> don't force relocation? If evaluatePcRelLo thinks it can be evaluated does it matter if the pcrel_hi20 can or not?

I'll have another think and take a look at your testcase.

My suspicion is that it would be easier to teach MCAssembler::evaluateFixup how to deal with an extra level of indirection, since the problem stems from it believing the relocation can be evaluated and not knowing about the *real* symbol/relocation we care about.

[lewis-revill]

In D57240#1385541, @jrtc27 wrote:

My suspicion is that it would be easier to teach MCAssembler::evaluateFixup how to deal with an extra level of indirection, since the problem stems from it believing the relocation can be evaluated and not knowing about the *real* symbol/relocation we care about.

I've been thinking about this and I'm wondering whether it makes sense for shouldForceRelocation to be able to somehow indicate 'I don't know yet'. The assembler should then keep those results instead of recording the relocation, and allow the target to make a final decision later. Or alternatively allow the target to continue to remove things from this 'don't know' list and just force relocations for all the remaining ones at the end. Either way there needs to be some separation of the pass for evaluating fixups and recording the relocations / applying the fixups.

[lewis-revill]

Abandoned in favour of D61907

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h

 //===-- RISCVAsmBackend.h - RISCV Assembler Backend -----------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_RISCV_MCTARGETDESC_RISCVASMBACKEND_H
 #define LLVM_LIB_TARGET_RISCV_MCTARGETDESC_RISCVASMBACKEND_H
 
 #include "MCTargetDesc/RISCVFixupKinds.h"
 #include "MCTargetDesc/RISCVMCTargetDesc.h"
+#include "llvm/ADT/SmallSet.h"
 #include "llvm/MC/MCAsmBackend.h"
 #include "llvm/MC/MCFixupKindInfo.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 
 namespace llvm {
 class MCAssembler;
 class MCObjectTargetWriter;
 class raw_ostream;
 
 class RISCVAsmBackend : public MCAsmBackend {
   const MCSubtargetInfo &STI;
+  SmallSet<const MCFixup *, 8> ResolvedPCRelHiFixups;
   uint8_t OSABI;
   bool Is64Bit;
   bool ForceRelocs = false;
 
 public:
   RISCVAsmBackend(const MCSubtargetInfo &STI, uint8_t OSABI, bool Is64Bit)
       : MCAsmBackend(support::little), STI(STI), OSABI(OSABI),
         Is64Bit(Is64Bit) {}

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

 bool RISCVAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
                                             const MCFixup &Fixup,
                                             const MCValue &Target) {
   bool ShouldForce = false;
 
   switch ((unsigned)Fixup.getKind()) {
   default:
     break;
+  case RISCV::fixup_riscv_pcrel_hi20:
+    ResolvedPCRelHiFixups.insert(&Fixup);
+    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.
+    // For pcrel_lo12, force a relocation if the corresponding pcrel_hi20 hasn't
+    // already been resolved without a relocation.
     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_pcrel_hi20:
-      ShouldForce = T->getValue()->findAssociatedFragment() !=
-                    Fixup.getValue()->findAssociatedFragment();
+      if (ResolvedPCRelHiFixups.count(T) == 0)
+        ShouldForce = true;
       break;
     }
     break;
   }
 
   return ShouldForce || STI.getFeatureBits()[RISCV::FeatureRelax] ||
          ForceRelocs;
 }

test/MC/RISCV/fixups.s

 # CHECK-INSTR: lw a0, 1656(t1)
 addi a1, t1, %lo(val)
 # 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)
 
+# Create a new fragment boundary here.
+.align 0
 1:
 auipc t1, %pcrel_hi(.LBB0)
 # 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-FIXUP: fixup A - offset: 0, value: %pcrel_lo(.Ltmp0), kind: fixup_riscv_pcrel_lo12_i
 # CHECK-INSTR: addi t1, t1, -16
 sw t1, %pcrel_lo(1b)(t1)