[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 ↗

(On Diff #215905)

Typo in "location".

45 ↗

(On Diff #215905)

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 ↗

(On Diff #215905)

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 ↗

(On Diff #215905)

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/trunk/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/trunk/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/trunk/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);
+
+  // The default symbol subtraction results in an ADD/SUB relocation pair.
+  // Processing this relocation pair is problematic when linker relaxation is
+  // enabled, so we follow binutils in using the R_RISCV_32_PCREL relocation
+  // for the FDE initial location.
+  MCContext &Ctx = Streamer.getContext();
+  const MCExpr *ME =
+      MCSymbolRefExpr::create(Sym, MCSymbolRefExpr::VK_None, Ctx);
+  assert(Encoding & dwarf::DW_EH_PE_sdata4 && "Unexpected encoding");
+  return RISCVMCExpr::create(ME, RISCVMCExpr::VK_RISCV_32_PCREL, Ctx);
+}

llvm/trunk/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/trunk/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/trunk/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:  }