[RISCV] Properly evaluate fixup_riscv_pcrel_lo12

[PkmX]

Summary

This is a update to D43157 to correctly handle fixup_riscv_pcrel_lo12.

Notable changes:

• Rebased onto trunk
• Handle and test S-type
• Test case pcrel-hilo.s is merged into relocations.s

[PkmX]

Rebase onto trunk as the declaration of RISCVAsmBackend is split into its own header file.

@asb Any chance to look at this patch?

[jrtc27]

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

44

getPCRelHiExpr can return null if you do stupid things in your assembly (ie use %pcrel_lo(foo) where foo doesn't refer to an auipc x?, %pcrel_hi(sym) but is still at a constant offset), and in future most definitely will be when using %got_pcrel_hi.

[asb]

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

44

Other than this issue, this patch looks good to me. @PkmX: can you add test cases for these potential issues?

[PkmX]

Rebased onto trunk.

• Error out if %pcrel_lo(sym) does not point to a %pcrel_hi.
• Added a corresponding test case pcrel-lo12-invalid.s.

[PkmX]

Specify that pcrel-lo12-invalid.s will only produce an error with relaxation disabled, otherwise the assembler still emits a relocation which will fail at link time.

[asb]

This looks good to me, but there's one addition test I'd like to see. You explain that the intended behaviour is that there will be no compile-time error if linker relaxation is enabled. It would be good to have this reflected in a test. Maybe adding extra RUN lines to pcrel-lo12-invalid.s and adding a comment that explains that is expected with linker relaxation enabled.

[jrtc27]

In D54029#1329560, @asb wrote:

This looks good to me, but there's one addition test I'd like to see. You explain that the intended behaviour is that there will be no compile-time error if linker relaxation is enabled. It would be good to have this reflected in a test. Maybe adding extra RUN lines to pcrel-lo12-invalid.s and adding a comment that explains that is expected with linker relaxation enabled.

I personally would regard that as an implementation detail. There's no reason we couldn't give an error; we bail out of shouldForceRelocation early without bothering to check the relocation if relaxation is enabled, but there's nothing stopping us from putting the %pcrel_lo error check before that other than being slightly slower and increasing code complexity.

[asb]

In D54029#1329843, @jrtc27 wrote:

I personally would regard that as an implementation detail. There's no reason we couldn't give an error; we bail out of shouldForceRelocation early without bothering to check the relocation if relaxation is enabled, but there's nothing stopping us from putting the %pcrel_lo error check before that other than being slightly slower and increasing code complexity.

It's an implementation detail, but it's useful for the tests to capture the current behaviour so it becomes obvious if it changes unexpectedly. In terms of testing, it's also a case where there's an obvious possibility for the assembler to do something wrong (mis-assemble or crash). From that perspective, it's worth covering the case. Though I agree, the comment could be clear that this is the current behaviour rather than the only valid behaviour.

[PkmX]

@jrtc27 Actually I agree it should produce an error regardless of whether relaxation is enabled, since it is most likely an user error to write a %pcrel_lo that doesn't point to an auipc, and the earlier we can catch it the better.

[jrtc27]

In D54029#1329871, @PkmX wrote:

@jrtc27 Actually I agree it should produce an error regardless of whether relaxation is enabled, since it is most likely an user error to write a %pcrel_lo that doesn't point to an auipc, and the earlier we can catch it the better.

If you're editing this revision, could you please consider incorporating the getPCRelHiExpr -> getPCRelHiFixup changes (or equivalent) in D55279 which are necessary for supporting things other than %pcrel_hi? It seems slightly wrong to be landing a revision knowing that it will be refactored later.

[PkmX]

• %pcrel_lo that doesn't point to a valid auipc will always produce an error, even if relaxation is enabled. The test pcrel-lo12-invalid.s is modified accordingly.
• getPCRelHiExpr is renamed to getPCRelHiFixup and returns a MCFixup instead, to allow expansion for non %pcrel_hi modifiers in later patches.

[jrtc27]

Thanks, looks good to me now.

[apazos]

Hi James, I encountered another case not handled yet in this patch. It produces the error: could not find corresponding %pcrel_hi

.option push
.option norelax
la gp, __global_pointer$
.option pop
la sp, _sp

[lewis-revill]

In D54029#1404753, @apazos wrote:

Hi James, I encountered another case not handled yet in this patch. It produces the error: could not find corresponding %pcrel_hi

.option push
.option norelax
la gp, __global_pointer$
.option pop
la sp, _sp

This also occurs when you manually expand la.

Regardless I believe the problem is caused because EmitLabel doesn't have any checks for whether a new data fragment should be introduced, whereas EmitInstToData does. So when the .Lpcrel_hiX label is expanded/parsed it's parent fragment is the one before the directive changed the STI flags, but the auipc and its %pcrel_hi fixup is located in the fragment after.

When getPCRelHiFixup() searches for the %pcrel_hi it fails to find it since it uses the fragment & offset of the label to search, whereas the fixup is in another fragment.

This is just from some initial debugging, so I'm not sure whether this is the only issue and/or what the best solution would be.

[lewis-revill]

In D54029#1407041, @lewis-revill wrote:

In D54029#1404753, @apazos wrote:

Hi James, I encountered another case not handled yet in this patch. It produces the error: could not find corresponding %pcrel_hi

.option push
.option norelax
la gp, __global_pointer$
.option pop
la sp, _sp

This also occurs when you manually expand la.

Regardless I believe the problem is caused because EmitLabel doesn't have any checks for whether a new data fragment should be introduced, whereas EmitInstToData does. So when the .Lpcrel_hiX label is expanded/parsed it's parent fragment is the one before the directive changed the STI flags, but the auipc and its %pcrel_hi fixup is located in the fragment after.

When getPCRelHiFixup() searches for the %pcrel_hi it fails to find it since it uses the fragment & offset of the label to search, whereas the fixup is in another fragment.

This is just from some initial debugging, so I'm not sure whether this is the only issue and/or what the best solution would be.

Sure enough, threading through an MCSubtargetInfo to EmitLabel and checking for CanReuseDataFragment before inserting the label into the current section fixes the issue.

[apazos]

Thanks for the analysis and suggestion Lewis.

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h

public:
   void applyFixup(const MCAssembler &Asm, const MCFixup &Fixup,
                   const MCValue &Target, MutableArrayRef<char> Data,
                   uint64_t Value, bool IsResolved,
                   const MCSubtargetInfo *STI) const override;
 
   std::unique_ptr<MCObjectTargetWriter>
   createObjectTargetWriter() const override;
 
-  // If linker relaxation is enabled, or the relax option had previously been
-  // enabled, always emit relocations even if the fixup can be resolved. This is
-  // necessary for correctness as offsets may change during relaxation.
   bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
-                             const MCValue &Target) override {
-    return STI.getFeatureBits()[RISCV::FeatureRelax] || ForceRelocs;
-  }
+                             const MCValue &Target) override;
 
   bool fixupNeedsRelaxation(const MCFixup &Fixup, uint64_t Value,
                             const MCRelaxableFragment *DF,
                             const MCAsmLayout &Layout) const override {
     llvm_unreachable("Handled by fixupNeedsRelaxationAdvanced");
   }
 
   bool fixupNeedsRelaxationAdvanced(const MCFixup &Fixup, bool Resolved,

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

 //===-- RISCVAsmBackend.cpp - RISCV Assembler Backend ---------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "RISCVAsmBackend.h"
+#include "RISCVMCExpr.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/MC/MCAssembler.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCDirectives.h"
 #include "llvm/MC/MCELFObjectWriter.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 
+// If linker relaxation is enabled, or the relax option had previously been
+// enabled, always emit relocations even if the fixup can be resolved. This is
+// necessary for correctness as offsets may change during relaxation.
+bool RISCVAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
+                                            const MCFixup &Fixup,
+                                            const MCValue &Target) {
+  if (STI.getFeatureBits()[RISCV::FeatureRelax] || ForceRelocs)
+    return true;
+
+  switch ((unsigned)Fixup.getKind()) {
+  default:
+    return false;
+  case RISCV::fixup_riscv_pcrel_lo12_i:
+  case RISCV::fixup_riscv_pcrel_lo12_s:
+    // For pcrel_lo12, MCAssembler determines if the fixup can be resolved
+    // based on the difference between pcrel_hi20 and pcrel_lo12. However,
+    // it should actually be based on pcrel_lo12 and the target of pcrel_hi20.
+    //
+    // Force a relocation here if the target isn't in same section.
+    const MCExpr *T = cast<RISCVMCExpr>(Fixup.getValue())->getPCRelHiExpr();
+    if (!T) {
+      Asm.getContext().reportError(Fixup.getLoc(),
+                                   "could not find corresponding %pcrel_hi");
+      return false;
+    }
+
+    return T->findAssociatedFragment() !=
+           Fixup.getValue()->findAssociatedFragment();
+  }
+}
+
 bool RISCVAsmBackend::fixupNeedsRelaxationAdvanced(const MCFixup &Fixup,
                                                    bool Resolved,
                                                    uint64_t Value,
                                                    const MCRelaxableFragment *DF,
                                                    const MCAsmLayout &Layout,
                                                    const bool WasForced) const {
   // Return true if the symbol is actually unresolved.
   // Resolved could be always false when shouldForceRelocation return true.

lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.h

public:
   };
 
 private:
   const MCExpr *Expr;
   const VariantKind Kind;
 
   int64_t evaluateAsInt64(int64_t Value) const;
 
+  bool evaluatePCRelLo(MCValue &Res, const MCAsmLayout *Layout,
+                       const MCFixup *Fixup) const;
+
   explicit RISCVMCExpr(const MCExpr *Expr, VariantKind Kind)
       : Expr(Expr), Kind(Kind) {}
 
 public:
   static const RISCVMCExpr *create(const MCExpr *Expr, VariantKind Kind,
                                    MCContext &Ctx);
 
   VariantKind getKind() const { return Kind; }
 
   const MCExpr *getSubExpr() const { return Expr; }
 
+  /// Get the MCExpr of the VK_RISCV_PCREL_HI Fixup that the
+  /// VK_RISCV_PCREL_LO points to.
+  ///
+  /// \returns nullptr if this isn't a VK_RISCV_PCREL_LO pointing to a
+  /// VK_RISCV_PCREL_HI.
+  const MCExpr *getPCRelHiExpr() const;
+
   void printImpl(raw_ostream &OS, const MCAsmInfo *MAI) const override;
   bool evaluateAsRelocatableImpl(MCValue &Res, const MCAsmLayout *Layout,
                                  const MCFixup *Fixup) const override;
   void visitUsedExpr(MCStreamer &Streamer) const override;
   MCFragment *findAssociatedFragment() const override {
     return getSubExpr()->findAssociatedFragment();
   }
 

lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp

 //===-- RISCVMCExpr.cpp - RISCV specific MC expression classes ------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file contains the implementation of the assembly expression modifiers
 // accepted by the RISCV architecture (e.g. ":lo12:", ":gottprel_g1:", ...).
 //
 //===----------------------------------------------------------------------===//
 
 #include "RISCV.h"
 #include "RISCVMCExpr.h"
+#include "RISCVFixupKinds.h"
 #include "llvm/MC/MCAssembler.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSymbolELF.h"
 #include "llvm/MC/MCValue.h"
 #include "llvm/Support/ErrorHandling.h"
 
 using namespace llvm;
bool HasVariant =
       ((getKind() != VK_RISCV_None) && (getKind() != VK_RISCV_CALL));
   if (HasVariant)
     OS << '%' << getVariantKindName(getKind()) << '(';
   Expr->print(OS, MAI);
   if (HasVariant)
     OS << ')';
 }
 
+const MCExpr *RISCVMCExpr::getPCRelHiExpr() const {
+  MCValue AUIPCLoc;
+  if (!getSubExpr()->evaluateAsRelocatable(AUIPCLoc, nullptr, nullptr))
+    return nullptr;
+
+  const MCSymbolRefExpr *AUIPCSRE = AUIPCLoc.getSymA();
+  if (!AUIPCSRE)
+    return nullptr;
+
+  const auto *DF =
+      dyn_cast_or_null<MCDataFragment>(AUIPCSRE->findAssociatedFragment());
+  if (!DF)
+    return nullptr;
+
+  const MCSymbol *AUIPCSymbol = &AUIPCSRE->getSymbol();
+  const MCExpr *TargetExpr = nullptr;
+  for (const MCFixup &F : DF->getFixups()) {
+    if ((unsigned)F.getKind() != RISCV::fixup_riscv_pcrel_hi20)
+      continue;
+    if (F.getOffset() != AUIPCSymbol->getOffset())
+      continue;
+    TargetExpr = F.getValue();
+    break;
+  }
+  return TargetExpr;
+}
+
+bool RISCVMCExpr::evaluatePCRelLo(MCValue &Res, const MCAsmLayout *Layout,
+                                  const MCFixup *Fixup) const {
+  // VK_RISCV_PCREL_LO has to be handled specially.  The MCExpr inside is
+  // actually the location of a auipc instruction with a VK_RISCV_PCREL_HI fixup
+  // pointing to the real target.  We need to generate an MCValue in the form of
+  // (<real target> + <offset from this fixup to the auipc fixup>).  The Fixup
+  // is pcrel relative to the VK_RISCV_PCREL_LO fixup, so we need to add the
+  // offset to the VK_RISCV_PCREL_HI Fixup from VK_RISCV_PCREL_LO to correct.
+  MCValue AUIPCLoc;
+  if (!getSubExpr()->evaluateAsValue(AUIPCLoc, *Layout))
+    return false;
+
+  const MCSymbolRefExpr *AUIPCSRE = AUIPCLoc.getSymA();
+  // Don't try to evaluate %pcrel_hi/%pcrel_lo pairs that cross fragment
+  // boundries.
+  if (!AUIPCSRE ||
+      findAssociatedFragment() != AUIPCSRE->findAssociatedFragment())
+    return false;
+
+  const MCSymbol *AUIPCSymbol = &AUIPCSRE->getSymbol();
+  if (!AUIPCSymbol)
+    return false;
+
+  const MCExpr *TargetExpr = getPCRelHiExpr();
+  if (!TargetExpr)
+    return false;
+
+  MCValue Target;
+  if (!TargetExpr->evaluateAsValue(Target, *Layout))
+    return false;
+
+  if (!Target.getSymA() || !Target.getSymA()->getSymbol().isInSection())
+    return false;
+
+  if (&Target.getSymA()->getSymbol().getSection() !=
+      findAssociatedFragment()->getParent())
+    return false;
+
+  uint64_t AUIPCOffset = AUIPCSymbol->getOffset();
+
+  Res = MCValue::get(Target.getSymA(), nullptr,
+                     Target.getConstant() + (Fixup->getOffset() - AUIPCOffset));
+  return true;
+}
+
 bool RISCVMCExpr::evaluateAsRelocatableImpl(MCValue &Res,
                                             const MCAsmLayout *Layout,
                                             const MCFixup *Fixup) const {
+  if (Kind == VK_RISCV_PCREL_LO && evaluatePCRelLo(Res, Layout, Fixup))
+    return true;
+
   if (!getSubExpr()->evaluateAsRelocatable(Res, Layout, Fixup))
     return false;
 
   // Some custom fixup types are not valid with symbol difference expressions
   if (Res.getSymA() && Res.getSymB()) {
     switch (getKind()) {
     default:
       return true;

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)
 
+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)
+# CHECK-FIXUP: fixup A - offset: 0, value: %pcrel_lo(.Ltmp0), kind: fixup_riscv_pcrel_lo12_s
+# CHECK-INSTR: sw t1, -16(t1)
+
 jal zero, .LBB0
 # CHECK-FIXUP: fixup A - offset: 0, value: .LBB0, kind: fixup_riscv_jal
-# CHECK-INSTR: jal zero, -16
+# 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
 # CHECK-FIXUP: fixup A - offset: 0, value: .LBB0, kind: fixup_riscv_branch
-# CHECK-INSTR: beq a0, a1, -24
+# CHECK-INSTR: beq a0, a1, -36
 blt a0, a1, .LBB1
 # CHECK-FIXUP: fixup A - offset: 0, value: .LBB1, kind: fixup_riscv_branch
 # CHECK-INSTR: blt a0, a1, 1108
 
 .fill 1104
 
 .LBB1:
 

test/MC/RISCV/pcrel-lo12-invalid.s

+# RUN: not llvm-mc -triple riscv32 -filetype obj < %s -o /dev/null 2>&1 | FileCheck %s
+
+1:
+  addi a0, a0, %pcrel_lo(1b) # CHECK: :[[@LINE]]:3: error: could not find corresponding %pcrel_hi

test/MC/RISCV/relocations.s

 # INSTR: sb t1, %lo(foo)(a2)
 # FIXUP: fixup A - offset: 0, value: %lo(foo), kind: fixup_riscv_lo12_s
 
 sb t1, %lo(foo+4)(a2)
 # RELOC: R_RISCV_LO12_S foo 0x4
 # INSTR: sb t1, %lo(foo+4)(a2)
 # FIXUP: fixup A - offset: 0, value: %lo(foo+4), kind: fixup_riscv_lo12_s
 
+.L0:
 auipc t1, %pcrel_hi(foo)
 # RELOC: R_RISCV_PCREL_HI20 foo 0x0
 # INSTR: auipc t1, %pcrel_hi(foo)
 # FIXUP: fixup A - offset: 0, value: %pcrel_hi(foo), kind: fixup_riscv_pcrel_hi20
 
 auipc t1, %pcrel_hi(foo+4)
 # RELOC: R_RISCV_PCREL_HI20 foo 0x4
 # INSTR: auipc t1, %pcrel_hi(foo+4)
 # FIXUP: fixup A - offset: 0, value: %pcrel_hi(foo+4), kind: fixup_riscv_pcrel_hi20
 
-addi t1, t1, %pcrel_lo(foo)
-# RELOC: R_RISCV_PCREL_LO12_I foo 0x0
-# INSTR: addi t1, t1, %pcrel_lo(foo)
-# FIXUP: fixup A - offset: 0, value: %pcrel_lo(foo), kind: fixup_riscv_pcrel_lo12_i
-
-addi t1, t1, %pcrel_lo(foo+4)
-# RELOC: R_RISCV_PCREL_LO12_I foo 0x4
-# INSTR: addi t1, t1, %pcrel_lo(foo+4)
-# FIXUP: fixup A - offset: 0, value: %pcrel_lo(foo+4), kind: fixup_riscv_pcrel_lo12_i
-
-sb t1, %pcrel_lo(foo)(a2)
-# RELOC: R_RISCV_PCREL_LO12_S foo 0x0
-# INSTR: sb t1, %pcrel_lo(foo)(a2)
-# FIXUP: fixup A - offset: 0, value: %pcrel_lo(foo), kind: fixup_riscv_pcrel_lo12_s
-
-sb t1, %pcrel_lo(foo+4)(a2)
-# RELOC: R_RISCV_PCREL_LO12_S foo 0x4
-# INSTR: sb t1, %pcrel_lo(foo+4)(a2)
-# FIXUP: fixup A - offset: 0, value: %pcrel_lo(foo+4), kind: fixup_riscv_pcrel_lo12_s
+addi t1, t1, %pcrel_lo(.L0)
+# RELOC: R_RISCV_PCREL_LO12_I .L0 0x0
+# INSTR: addi t1, t1, %pcrel_lo(.L0)
+# FIXUP: fixup A - offset: 0, value: %pcrel_lo(.L0), kind: fixup_riscv_pcrel_lo12_i
+
+sb t1, %pcrel_lo(.L0)(a2)
+# RELOC: R_RISCV_PCREL_LO12_S .L0 0x0
+# INSTR: sb t1, %pcrel_lo(.L0)(a2)
+# FIXUP: fixup A - offset: 0, value: %pcrel_lo(.L0), kind: fixup_riscv_pcrel_lo12_s
 
 jal zero, foo
 # RELOC: R_RISCV_JAL
 # INSTR: jal zero, foo
 # FIXUP: fixup A - offset: 0, value: foo, kind: fixup_riscv_jal
 
 bgeu a0, a1, foo
 # RELOC: R_RISCV_BRANCH
 # INSTR: bgeu a0, a1, foo
 # FIXUP: fixup A - offset: 0, value: foo, kind: fixup_riscv_branch