RISCV: adjust relocation emission

[compnerd]

Summary

Simplify and make the pair-wise relocation more precise. If either of
the symbol references are textual, the relocation must be delayed. If
the difference is across sections, delay it as well which partially
matches the behaviour of gas. We unfortunately do not handle the case
where the difference references a symbol that is not yet defined. In
such a case, we simply fail to resolve the difference, which should
hopefully not be too onerous (particularly since no other target
supports cross-section references and it is not clear if this was
intentional on the part of RISCV).

[asb]

Is this patch ready for review? Is there any more background on the need for this change?

[compnerd]

@asb no, this patch wasn't ready, I didn't want to loose the work on it though. This is dependent on D133456 though. I think that I have a variant of this which is nearly ready.

[compnerd]

Stylistic changes suggested by @MaskRay offline.

[MaskRay]

llvm/test/MC/RISCV/riscv64-64b-pcrel.s

1

You can guard negative tests with macros so that the positive tests can use llvm-mc return code of 0.

llvm-mc supports --defsym. I have added some tests using --defsym ERR=1.

[jrtc27]

It would've been nice if you asked for review once this was ready... as far as we're concerned it was still parked and not ready for review

[MaskRay]

The description says

We unfortunately do not handle the case where the difference references a symbol that is not yet defined. In such a case, we simply fail to resolve the difference, which should hopefully not be too onerous ...

Android riscv64 nterp: Fix "oat" code size calculation. is an instance to work around suppressed R_RISCV_ADD32/R_RISCV_SUB32. It reduces to:

# Both end and begin are not defined yet. We decide ADD/SUB relocations upfront and don't know they will be needed.
.4byte end-begin

begin:
  call foo
end:

I have elaborated this in my notes (https://maskray.me/blog/2021-03-14-the-dark-side-of-riscv-linker-relaxation "Label differences related to text section symbols") and will think about improvements in my free time.

D145474: Ideally we should delay the decision of PC-relative ...

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

static std::pair<unsigned, unsigned> getRelocPairForSize(unsigned Size) {
     case 8:
       return std::make_pair(RISCV::fixup_riscv_add_64,
                             RISCV::fixup_riscv_sub_64);
     }
   }
 
   static bool requiresFixups(MCContext &C, const MCExpr *Value,
                              const MCExpr *&LHS, const MCExpr *&RHS) {
-    auto IsMetadataOrEHFrameSection = [](const MCSection &S) -> bool {
-      // Additionally check .apple_names/.apple_types. They are fixed-size and
-      // do not need fixups. llvm-dwarfdump --apple-names does not process
-      // R_RISCV_{ADD,SUB}32 in them.
-      return S.getKind().isMetadata() || S.getName() == ".eh_frame" ||
-             S.getName() == ".apple_names" || S.getName() == ".apple_types";
-    };
-
     const auto *MBE = dyn_cast<MCBinaryExpr>(Value);
     if (MBE == nullptr)
       return false;
 
     MCValue E;
     if (!Value->evaluateAsRelocatable(E, nullptr, nullptr))
       return false;
     if (E.getSymA() == nullptr || E.getSymB() == nullptr)
       return false;
 
     const auto &A = E.getSymA()->getSymbol();
     const auto &B = E.getSymB()->getSymbol();
 
     LHS =
         MCBinaryExpr::create(MCBinaryExpr::Add, MCSymbolRefExpr::create(&A, C),
                              MCConstantExpr::create(E.getConstant(), C), C);
     RHS = E.getSymB();
 
-    // TODO: when available, R_RISCV_n_PCREL should be preferred.
-
-    // Avoid pairwise relocations for symbolic difference in debug and .eh_frame
-    if (A.isInSection())
-      return !IsMetadataOrEHFrameSection(A.getSection());
-    if (B.isInSection())
-      return !IsMetadataOrEHFrameSection(B.getSection());
-    // as well as for absolute symbols.
-    return !A.getName().empty() || !B.getName().empty();
+    // If either symbol is in a text section, we need to delay the relocation
+    // evaluation as relaxation may alter the size of the symbol.
+    //
+    // Unfortunately, we cannot identify if the symbol was built with relaxation
+    // as we do not track the state per symbol or section.  However, BFD will
+    // always emit the relocation and so we follow suit which avoids the need to
+    // track that information.
+    if (A.isInSection() && A.getSection().getKind().isText())
+      return true;
+    if (B.isInSection() && B.getSection().getKind().isText())
+      return true;
+
+    // Support cross-section symbolic differences ...
+    return A.isInSection() && B.isInSection() &&
+           A.getSection().getName() != B.getSection().getName();
   }
 
   void reset() override {
     static_cast<RISCVTargetStreamer *>(getTargetStreamer())->reset();
     MCELFStreamer::reset();
   }
 
 public:

llvm/test/MC/RISCV/riscv64-64b-pcrel.s

 # RUN: llvm-mc -triple riscv64-unknown-linux-gnu -filetype obj -o - %s \
 # RUN:   | llvm-readobj -r - | FileCheck %s
+# RUN: not llvm-mc -triple riscv64-unknown-linux-gnu -filetype obj --defsym ERR=1 -o /dev/null %s 2>&1 \
+# RUN:   | FileCheck %s --check-prefix CHECK-ERROR
 
 # CHECK: Relocations [
 # CHECK:  .relasx {
 # CHECK-NEXT:    0x0 R_RISCV_ADD64 y 0x0
 # CHECK-NEXT:    0x0 R_RISCV_SUB64 x 0x0
 # CHECK:  }
 # CHECK:  .relasy {
 # CHECK-NEXT:    0x0 R_RISCV_ADD64 x 0x0
 # CHECK-NEXT:    0x0 R_RISCV_SUB64 y 0x0
 # CHECK:  }
-# CHECK:  .relasz {
-# CHECK-NEXT:    0x0 R_RISCV_ADD64 z 0x0
-# CHECK-NEXT:    0x0 R_RISCV_SUB64 a 0x0
-# CHECK:  }
 # CHECK:  .relasa {
 # CHECK-NEXT:    0x0 R_RISCV_ADD64 a 0x0
 # CHECK-NEXT:    0x0 R_RISCV_SUB64 z 0x0
 # CHECK:  }
 # CHECK-NEXT: ]
 
   .section  sx,"aw",@progbits
 x:
   .quad y-x
 
   .section  sy,"aw",@progbits
 y:
   .quad x-y
 
   .section  sz
 z:
+.ifdef ERR
   .quad z-a
 
+# CHECK-ERROR: Cannot represent a difference across sections
+# CHECK-ERROR:  .quad z-a
+# CHECK-ERROR:        ^
+.else
+  .quad 0
+.endif
+
   .section  sa
 a:
   .quad a-z
 
 ## .apple_names/.apple_types are fixed-size and do not need fixups.
 ## llvm-dwarfdump --apple-names does not process R_RISCV_{ADD,SUB}32 in them.
 ## See llvm/test/DebugInfo/Generic/accel-table-hash-collisions.ll
   .section  .apple_types
         .word 0
         .word .Ltypes0-.apple_types
 .Ltypes0: