[RISCV] Implement getExprForFDESymbol to ensure RISCV_32_PCREL is used for the FDE location

[asb]

Summary

Follow binutils in using RISCV_32_PCREL for the FDE initial location. As explained in the relevant binutils commit , the ADD/SUB pair of relocations is problematic in the presence of linker relaxation.

This patch has the same end goal as D64715 but includes test changes and avoids adding a new global VariantKind to MCExpr.h (preferring RISCVMCExpr VKs like the rest of the RISC-V backend).

[luismarques]

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

41

Typo in "location".

45

LLVM has dropped support for DWARF64, so this should be correct more or less indefinitely, but this code is implicitly assuming that Encoding includes dwarf::DW_EH_PE_sdata4. Maybe assert that's the case, to trip if that ever changes?

llvm/test/MC/RISCV/fde-reloc.s

12

The issue doesn't seem to be specific to this patch, but when you insert instructions before .cfi_startproc the relocation doesn't seem to be updated correctly, unlike with binutils. Might be worth looking where EmitPersonality gets its Symbol from.

[luismarques]

Overall LGTM. Can be merged after fixing at least the typo.

[asb]

llvm/test/MC/RISCV/fde-reloc.s

12

I had a look and actually couldn't reproduce this issue when using GNU readelf -r on both llvm-mc and GNU as output.

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

 //===-- RISCVELFObjectWriter.cpp - RISCV ELF Writer -----------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "MCTargetDesc/RISCVFixupKinds.h"
+#include "MCTargetDesc/RISCVMCExpr.h"
 #include "MCTargetDesc/RISCVMCTargetDesc.h"
 #include "llvm/MC/MCELFObjectWriter.h"
 #include "llvm/MC/MCFixup.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/Support/ErrorHandling.h"
 
 using namespace llvm;
 
: MCELFObjectTargetWriter(Is64Bit, OSABI, ELF::EM_RISCV,
                               /*HasRelocationAddend*/ true) {}
 
 RISCVELFObjectWriter::~RISCVELFObjectWriter() {}
 
 unsigned RISCVELFObjectWriter::getRelocType(MCContext &Ctx,
                                             const MCValue &Target,
                                             const MCFixup &Fixup,
                                             bool IsPCRel) const {
+  const MCExpr *Expr = Fixup.getValue();
   // Determine the type of the relocation
   unsigned Kind = Fixup.getKind();
   if (IsPCRel) {
     switch (Kind) {
     default:
       llvm_unreachable("invalid fixup kind!");
     case FK_Data_4:
     case FK_PCRel_4:
case RISCV::fixup_riscv_call_plt:
       return ELF::R_RISCV_CALL_PLT;
     }
   }
 
   switch (Kind) {
   default:
     llvm_unreachable("invalid fixup kind!");
   case FK_Data_4:
+    if (Expr->getKind() == MCExpr::Target &&
+        cast<RISCVMCExpr>(Expr)->getKind() == RISCVMCExpr::VK_RISCV_32_PCREL)
+      return ELF::R_RISCV_32_PCREL;
     return ELF::R_RISCV_32;
   case FK_Data_8:
     return ELF::R_RISCV_64;
   case FK_Data_Add_1:
     return ELF::R_RISCV_ADD8;
   case FK_Data_Add_2:
     return ELF::R_RISCV_ADD16;
   case FK_Data_Add_4:

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCAsmInfo.h

 namespace llvm {
 class Triple;
 
 class RISCVMCAsmInfo : public MCAsmInfoELF {
   void anchor() override;
 
 public:
   explicit RISCVMCAsmInfo(const Triple &TargetTriple);
+
+  const MCExpr *getExprForFDESymbol(const MCSymbol *Sym, unsigned Encoding,
+                                    MCStreamer &Streamer) const override;
 };
 
 } // namespace llvm
 
 #endif

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

 //===-- RISCVMCAsmInfo.cpp - RISCV Asm properties -------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 // This file contains the declarations of the RISCVMCAsmInfo properties.
 //
 //===----------------------------------------------------------------------===//
 
 #include "RISCVMCAsmInfo.h"
+#include "MCTargetDesc/RISCVMCExpr.h"
 #include "llvm/ADT/Triple.h"
+#include "llvm/BinaryFormat/Dwarf.h"
+#include "llvm/MC/MCStreamer.h"
 using namespace llvm;
 
 void RISCVMCAsmInfo::anchor() {}
 
 RISCVMCAsmInfo::RISCVMCAsmInfo(const Triple &TT) {
   CodePointerSize = CalleeSaveStackSlotSize = TT.isArch64Bit() ? 8 : 4;
   CommentString = "#";
   AlignmentIsInBytes = false;
   SupportsDebugInformation = true;
   ExceptionsType = ExceptionHandling::DwarfCFI;
   Data16bitsDirective = "\t.half\t";
   Data32bitsDirective = "\t.word\t";
 }
+
+const MCExpr *RISCVMCAsmInfo::getExprForFDESymbol(const MCSymbol *Sym,
+                                                  unsigned Encoding,
+                                                  MCStreamer &Streamer) const {
+  if (!(Encoding & dwarf::DW_EH_PE_pcrel))
+    return MCAsmInfo::getExprForFDESymbol(Sym, Encoding, Streamer);
+
+  MCContext &Ctx = Streamer.getContext();
+  const MCExpr *ME =
+      MCSymbolRefExpr::create(Sym, MCSymbolRefExpr::VK_None, Ctx);
+  return RISCVMCExpr::create(ME, RISCVMCExpr::VK_RISCV_32_PCREL, Ctx);
+}

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

unsigned RISCVMCCodeEmitter::getImmOpValue(const MCInst &MI, unsigned OpNo,
   RISCV::Fixups FixupKind = RISCV::fixup_riscv_invalid;
   bool RelaxCandidate = false;
   if (Kind == MCExpr::Target) {
     const RISCVMCExpr *RVExpr = cast<RISCVMCExpr>(Expr);
 
     switch (RVExpr->getKind()) {
     case RISCVMCExpr::VK_RISCV_None:
     case RISCVMCExpr::VK_RISCV_Invalid:
+    case RISCVMCExpr::VK_RISCV_32_PCREL:
       llvm_unreachable("Unhandled fixup kind!");
     case RISCVMCExpr::VK_RISCV_TPREL_ADD:
       // tprel_add is only used to indicate that a relocation should be emitted
       // for an add instruction used in TP-relative addressing. It should not be
       // expanded as if representing an actual instruction operand and so to
       // encounter it here is an error.
       llvm_unreachable(
           "VK_RISCV_TPREL_ADD should not represent an instruction operand");

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.h

enum VariantKind {
     VK_RISCV_GOT_HI,
     VK_RISCV_TPREL_LO,
     VK_RISCV_TPREL_HI,
     VK_RISCV_TPREL_ADD,
     VK_RISCV_TLS_GOT_HI,
     VK_RISCV_TLS_GD_HI,
     VK_RISCV_CALL,
     VK_RISCV_CALL_PLT,
+    VK_RISCV_32_PCREL,
     VK_RISCV_Invalid
   };
 
 private:
   const MCExpr *Expr;
   const VariantKind Kind;
 
   int64_t evaluateAsInt64(int64_t Value) const;

llvm/test/MC/RISCV/fde-reloc.s

 # RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+relax < %s \
 # RUN:     | llvm-readobj -r | FileCheck -check-prefix=RELAX-RELOC %s
 # RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=-relax < %s \
 # RUN:     | llvm-readobj -r | FileCheck -check-prefix=NORELAX-RELOC %s
 
 func:
   .cfi_startproc
   ret
   .cfi_endproc
 
-# TODO: Should produce R_RISCV_32_PCREL for the FDE pc relocation. Many of the
-# ADD32/SUB32 relocations also can be safely resolved even with linker
-# relaxation enabled. This test is written to capture current behaviour, in
-# preparation for follow-on patches to fix it.
-
 # RELAX-RELOC:   Section (4) .rela.eh_frame {
-# RELAX-RELOC-NEXT:   0x1C R_RISCV_ADD32 - 0x0
-# RELAX-RELOC-NEXT:   0x1C R_RISCV_SUB32 - 0x0
+# RELAX-RELOC-NEXT:   0x1C R_RISCV_32_PCREL - 0x0
 # RELAX-RELOC-NEXT:   0x20 R_RISCV_ADD32 - 0x0
 # RELAX-RELOC-NEXT:   0x20 R_RISCV_SUB32 - 0x0
 # RELAX-RELOC-NEXT: }
 
 # NORELAX-RELOC:        Section (4) .rela.eh_frame {
 # NORELAX-RELOC-NEXT:    0x1C R_RISCV_32_PCREL - 0x0
 # NORELAX-RELOC-NEXT:  }