[RISCV] Properly evaluate VK_RISCV_PCREL_LO

[Bigcheese]

Summary

VK_RISCV_PCREL_LO has to be handled specially. The MCExpr inside is
actually the location of an auipc instruction with a VK_RISCV_PCREL_HI fixup
pointing to the real target.

[Bigcheese]

ping

[asb]

Sorry for the delay, I've been on paternity leave. Will review this and D43158 ASAP.

[asb]

And I have to apologise again that it has taken longer than expected to get back to you on this. I hope my slow feedback this time won't put you off future contributions - going forward, things should be much faster.

I fully agree that the pcrel_lo handling was incorrect and the code you've written seems sensible. Are you convinced that every case in the code that can reasonably exercised by tests is tested? We've now get just two instances of checking this pcrel_lo+pcrel_hi behaviour, which feels a little light.

Nit: The contents of test/MC/RISCV/pcrel-hilo.s should really go in relocations.s or a new relocations-pcrel-hilo.s.

lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp

15

Nit: should appear below RISCVMCExpr.h ("main module header" goes first https://llvm.org/docs/CodingStandards.html#include-style).

[PkmX]

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

42

This should also check for lo12_s.

42

Cast to unsigned to silence the warning about mismatching enum type.

lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp

60

Cast to unsigned to silence the warning about mismatching enum type.

test/MC/RISCV/fixups.s

31

Need a test case for S-type.

test/MC/RISCV/pcrel-hilo.s

14

Need a test case for S-type.

[PkmX]

Ping? This is the last issue blocking lld's RISC-V tests to be rewritten to use llvm-mc and llvm-objdump right now.

This patch also does not apply to trunk now, so it needs to be rebased and modified to handle S-type cases.

[PkmX]

Ping?

I have a patch available that handles PCREL_LO12_S and is rebased on trunk, if it is okay I will create a separate patch for reviewing.

[Bigcheese]

In D43157#1283535, @PkmX wrote:

Ping?

I have a patch available that handles PCREL_LO12_S and is rebased on trunk, if it is okay I will create a separate patch for reviewing.

Sure, that's fine. I won't have any time to look at this until after next week, but feel free to add me as a reviewer.

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 "MCTargetDesc/RISCVFixupKinds.h"
+#include "MCTargetDesc/RISCVMCExpr.h"
 #include "MCTargetDesc/RISCVMCTargetDesc.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/MC/MCAsmBackend.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"
class RISCVAsmBackend : public MCAsmBackend {
   uint8_t OSABI;
   bool Is64Bit;
 
 public:
   RISCVAsmBackend(const MCSubtargetInfo &STI, uint8_t OSABI, bool Is64Bit)
       : MCAsmBackend(), STI(STI), OSABI(OSABI), Is64Bit(Is64Bit) {}
   ~RISCVAsmBackend() override {}
 
+  bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
+                             const MCValue &Target) override {
+    if (Fixup.getKind() == RISCV::fixup_riscv_pcrel_lo12_i) {
+      const MCExpr *T = cast<RISCVMCExpr>(Fixup.getValue())->getPCRelHiExpr();
+      if (T->findAssociatedFragment() != Fixup.getValue()->findAssociatedFragment())
+        return true;
+    }
+    return false;
+  }
+
   void applyFixup(const MCAssembler &Asm, const MCFixup &Fixup,
                   const MCValue &Target, MutableArrayRef<char> Data,
                   uint64_t Value, bool IsResolved) const override;
 
   std::unique_ptr<MCObjectWriter>
   createObjectWriter(raw_pwrite_stream &OS) const override;
 
   bool fixupNeedsRelaxation(const MCFixup &Fixup, uint64_t Value,

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 "RISCVFixupKinds.h"
 #include "RISCVMCExpr.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/Object/ELF.h"
 #include "llvm/Support/ErrorHandling.h"
void RISCVMCExpr::printImpl(raw_ostream &OS, const MCAsmInfo *MAI) const {
   bool HasVariant = getKind() != VK_RISCV_None;
   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 (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;
   return getSubExpr()->evaluateAsRelocatable(Res, Layout, Fixup);
 }
 
 void RISCVMCExpr::visitUsedExpr(MCStreamer &Streamer) const {
   Streamer.visitUsedExpr(*getSubExpr());
 }
 
 RISCVMCExpr::VariantKind RISCVMCExpr::getVariantKindForName(StringRef name) {

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
+
 jal zero, .LBB0
 # CHECK-FIXUP: fixup A - offset: 0, value: .LBB0, kind: fixup_riscv_jal
-# CHECK-INSTR: jal zero, -16
+# CHECK-INSTR: jal zero, -24
 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, -32
 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:
 
 .fill 329876
 addi zero, zero, 0
 .LBB2:
 
 .set val, 0x12345678
 
 # CHECK-REL-NOT: R_RISCV

test/MC/RISCV/pcrel-hilo.s

+# RUN: llvm-mc -triple riscv32 -riscv-no-aliases < %s -show-encoding \
+# RUN:     | FileCheck -check-prefix=CHECK-FIXUP %s
+# RUN: llvm-mc -filetype=obj -triple riscv32 < %s \
+# RUN:     | llvm-objdump -riscv-no-aliases -d - \
+# RUN:     | FileCheck -check-prefix=CHECK-INSTR %s
+# RUN: llvm-mc -filetype=obj -triple=riscv32 %s \
+# RUN:     | llvm-readobj -r | FileCheck %s -check-prefix=CHECK-REL
+
+  .global foo
+1:
+auipc t1, %pcrel_hi(foo)
+# CHECK-FIXUP: fixup A - offset: 0, value: %pcrel_hi(foo), 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, 0
+
+# CHECK-REL: 0x0 R_RISCV_PCREL_HI20 foo 0x0
+# CHECK-REL: 0x4 R_RISCV_PCREL_LO12_I .text 0x0