[RISCV] Emit relocation for uleb128

[kito-cheng]

Summary

Support for the uleb128 related relocation[1], ULEB128 format has used
by DWARF and exception handling table, which used ULEB128 to record data
and the distance of two symbols, however the linker relaxation might
change the symbol address, so we need to insert relocations for those
data to make sure they are still correct after linker relaxation.

[1] https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/361

[kito-cheng]

Changes:

• Add testcase.

[craig.topper]

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

222

Looks like most other places in RISCV use MCFixupKind(RISCV::fixup_riscv_set_uleb128) instead of the static_cast.

[jrtc27]

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

210

encodeULEB128 takes a uint64_t

214

Why not just use encodeULEB128's PadTo and encode 0?

[MaskRay]

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

204

delete the blank line. The prevailing LLVM style does not prefer inserting a blank line after a declaration.

214

-1ll is not tested. This needs a test with llvm-readelf -x $name or llvm-objdump -s dumping the section content.

llvm/test/MC/RISCV/fixups-expr.s

30 ↗

(On Diff #493308)

Suggest: create a new file for .uleb128. Add a test where .uleb128 precedes the label definition (it currently crashes).

Add a test with labels defined relative to two different sections.

I don't have a very strong opinion about whether non-local symbols should be allowed. For psABI, it may be safer to start with more restriction.

[craig.topper]

llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV.def

57

59 has been claimed by R_RISCV_PLT32 now.

[kito-cheng]

Changes:

• Update relocation number to 60 and 61.
• Move testcase to fixups-expr-uleb128.s
• Reserved enough bytes when the symbol can't evaluated at assembly time.

[kito-cheng]

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

210

evaluateAsAbsolute take int64_t &...added a check to make sure that won't overflow.

[kito-cheng]

Changes:

• Rebase
• Add new option to control the relocation emition for ULEB128: -riscv-enable-uleb128
• Default disable to prevent potential incompatible issue with old binutils, will default turn on in future.

[kito-cheng]

NOTE: psABI spec has merged and binutils support also merged into trunk.

[kito-cheng]

ping :)

[MaskRay]

This patch looks fine as to implement the functionality, but I am afraid that the used requiresFixup and evaluateAsAbsolute are not really reliable (https://maskray.me/blog/2021-03-14-the-dark-side-of-riscv-linker-relaxation "Label differences related to text section symbols"). Ideally we should not need`EnableULEB128Reloc`.

I plan to improve the reliability shortly.

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

32

[kito-cheng]

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

32

The intention of this option is considering the binutils compatibility, binuilts has support that on trunk which will be part of binutils 2.41, but it's still not release yet.

Anyway personally I don't have strong opinion on the default value of this option.

[kito-cheng]

Changes:

• Rebase to trunk.
• Apply @MaskRay's suggesion.

[MaskRay]

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

204

evaluateAsAbsolute has now been correct (D153097). I am now mainly concerned with our inaccurate requiresFixups and whether there will be users rely on a possibly buggy behavior...

llvm/test/MC/RISCV/fixups-expr-uleb128.s

61

Add -NEXT whenever applicable. Check the preceding line .rela.xxx { and the the pairing } to assert that there is no other relocations.

[kito-cheng]

Changes:

• Update testcase to make sure no other unexpected relocation.

[kito-cheng]

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

204

Did you have suggestion on this, I guess I am not really family enough around that :(

[MaskRay]

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

204

I took quite some time today to investigate it.... See D155357

I think we need a custom fixup to represent the fixup_riscv_set_uleb128 and fixup_riscv_sub_uleb128 pair.

In .handleAddSubRelocations, split the fixup into 2 relocations.

[MaskRay]

llvm/test/MC/RISCV/fixups-expr-uleb128.s

60

The interesting case is when we use a linker-relaxable instruction here.

+relax and -relax can check the behavior. See my llvm/test/MC/RISCV/fixups-expr.s change.

[kito-cheng]

Changes:

• Update testcase
• Based on D155357

[kito-cheng]

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

204

Thanks, I am based on D155357 now :)

[kito-cheng]

ping

[MaskRay]

I am out of town for a few days, but I'll try to read this soon.

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

128

Only used once. Inline the variable where it is used.

132

Delete blank line here.

llvm/test/MC/RISCV/fixups-expr-uleb128.s

19

For a cl::opt option that defaults to false, we usually don't test the case when we explicitly set it to false. This additional case doesn't pull its weight.

[kito-cheng]

Changes:

• Address @MaskRay's comments.

[MaskRay]

llvm/include/llvm/MC/MCFixup.h

28 ↗

(On Diff #548129)

We do not need this generic fixup.

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

164

These fixup_riscv_{set,add,sub}_* from D103539 are unneeded. I did not put the cleanup into D155357 to minimize the changes in D155357 . I have now removed the unneeded fixup kinds in c8ed138c34ddb22610f18468c1b7938f9e2abae5.

I think you can just use FirstLiteralRelocationKind + ELF::R_RISCV_SET_ULEB128 instead of adding new fixup kinds.

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

143

Prefer getContext().reportError for possible error paths. report_fatal_error should be avoided if possible.

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

108

Unneeded. See my other comment.

[MaskRay]

I think the right implementation will be quite different from this patch. We need to modify quite a few more places including shouldForceRelocation, MCAssembler::relaxLEB (new function, similar to relaxDwarf*), and MCObjectStreamer::emitULEB128Value.
I am working on an alternative (current draft: https://github.com/MaskRay/llvm-project/tree/mc-uleb128)

[kito-cheng]

@MaskRay Really appropriate your help! it seems like I didn't handle the dwarf stuffs right according your version, and ULEB128 relocation support might be a compatible issue between GNU and LLVM toolchain, so I wondering do you think it's better to split this patch into add relocation number only and add the lld support(D142880) first for LLVM 17 branch?

[MaskRay]

In D142879#4576026, @kito-cheng wrote:

@MaskRay Really appropriate your help! it seems like I didn't handle the dwarf stuffs right according your version, and ULEB128 relocation support might be a compatible issue between GNU and LLVM toolchain, so I wondering do you think it's better to split this patch into add relocation number only and add the lld support(D142880) first for LLVM 17 branch?

Thanks:) Having the ELF_RELOC(R_RISCV_SET_ULEB128, 60) ... 61 change in release/17.x will be useful.

Having the lld change will be good for our compatibility story if we can catch the release schedule. But it seems that the change is quite complex and I am unsure we have sufficient time...

[kito-cheng]

MaskRay has landed a better version :)

llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV.def

 ELF_RELOC(R_RISCV_RELAX,             51)
 ELF_RELOC(R_RISCV_SUB6,              52)
 ELF_RELOC(R_RISCV_SET6,              53)
 ELF_RELOC(R_RISCV_SET8,              54)
 ELF_RELOC(R_RISCV_SET16,             55)
 ELF_RELOC(R_RISCV_SET32,             56)
 ELF_RELOC(R_RISCV_32_PCREL,          57)
 ELF_RELOC(R_RISCV_IRELATIVE,         58)
 ELF_RELOC(R_RISCV_PLT32,             59)
+ELF_RELOC(R_RISCV_SET_ULEB128,       60)
+ELF_RELOC(R_RISCV_SUB_ULEB128,       61)

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

const static MCFixupKindInfo Infos[] = {
       {"fixup_riscv_add_32", 0, 32, 0},
       {"fixup_riscv_sub_32", 0, 32, 0},
 
       {"fixup_riscv_add_64", 0, 64, 0},
       {"fixup_riscv_sub_64", 0, 64, 0},
 
       {"fixup_riscv_set_6b", 2, 6, 0},
       {"fixup_riscv_sub_6b", 2, 6, 0},
+
+      {"fixup_riscv_set_uleb128", 0, 0, 0},
+      {"fixup_riscv_sub_uleb128", 0, 0, 0},
   };
   static_assert((std::size(Infos)) == RISCV::NumTargetFixupKinds,
                 "Not all fixup kinds added to Infos array");
 
   // Fixup kinds from .reloc directive are like R_RISCV_NONE. They
   // do not require any extra processing.
   if (Kind >= FirstLiteralRelocationKind)
     return MCAsmBackend::getFixupKindInfo(FK_NONE);
static uint64_t adjustFixupValue(const MCFixup &Fixup, uint64_t Value,
   case RISCV::fixup_riscv_set_16:
   case RISCV::fixup_riscv_add_16:
   case RISCV::fixup_riscv_sub_16:
   case RISCV::fixup_riscv_set_32:
   case RISCV::fixup_riscv_add_32:
   case RISCV::fixup_riscv_sub_32:
   case RISCV::fixup_riscv_add_64:
   case RISCV::fixup_riscv_sub_64:
+  case RISCV::fixup_riscv_set_uleb128:
+  case RISCV::fixup_riscv_sub_uleb128:
   case FK_Data_1:
   case FK_Data_2:
   case FK_Data_4:
   case FK_Data_8:
   case FK_Data_6b:
     return Value;
   case RISCV::fixup_riscv_set_6b:
     return Value & 0x03;

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

unsigned RISCVELFObjectWriter::getRelocType(MCContext &Ctx,
   case RISCV::fixup_riscv_set_16:
     return ELF::R_RISCV_SET16;
   case RISCV::fixup_riscv_add_16:
     return ELF::R_RISCV_ADD16;
   case RISCV::fixup_riscv_sub_16:
     return ELF::R_RISCV_SUB16;
   case RISCV::fixup_riscv_set_32:
     return ELF::R_RISCV_SET32;
   case RISCV::fixup_riscv_add_32:
     return ELF::R_RISCV_ADD32;
   case RISCV::fixup_riscv_sub_32:
     return ELF::R_RISCV_SUB32;
   case RISCV::fixup_riscv_add_64:
     return ELF::R_RISCV_ADD64;
   case RISCV::fixup_riscv_sub_64:
     return ELF::R_RISCV_SUB64;
+  case RISCV::fixup_riscv_set_uleb128:
+    return ELF::R_RISCV_SET_ULEB128;
+  case RISCV::fixup_riscv_sub_uleb128:
+    return ELF::R_RISCV_SUB_ULEB128;
   }
 }
 
 std::unique_ptr<MCObjectTargetWriter>
 llvm::createRISCVELFObjectWriter(uint8_t OSABI, bool Is64Bit) {
   return std::make_unique<RISCVELFObjectWriter>(OSABI, Is64Bit);
 }

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

 
 public:
   RISCVELFStreamer(MCContext &C, std::unique_ptr<MCAsmBackend> MAB,
                    std::unique_ptr<MCObjectWriter> MOW,
                    std::unique_ptr<MCCodeEmitter> MCE)
       : MCELFStreamer(C, std::move(MAB), std::move(MOW), std::move(MCE)) {}
 
   void emitValueImpl(const MCExpr *Value, unsigned Size, SMLoc Loc) override;
+  void emitULEB128Value(const MCExpr *Value) override;
 };
 
 namespace llvm {
 
 class RISCVTargetELFStreamer : public RISCVTargetStreamer {
 private:
   StringRef CurrentVendor;
 

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

 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCValue.h"
 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/RISCVAttributes.h"
 
 using namespace llvm;
 
 // This part is for ELF object output.
 RISCVTargetELFStreamer::RISCVTargetELFStreamer(MCStreamer &S,
                                                const MCSubtargetInfo &STI)
     : RISCVTargetStreamer(S), CurrentVendor("riscv"), STI(STI) {
   MCAssembler &MCA = getStreamer().getAssembler();
   const FeatureBitset &Features = STI.getFeatureBits();
   auto &MAB = static_cast<RISCVAsmBackend &>(MCA.getBackend());
   setTargetABI(RISCVABI::computeTargetABI(STI.getTargetTriple(), Features,
                                           MAB.getTargetOptions().getABIName()));
 }
void RISCVTargetELFStreamer::emitDirectiveVariantCC(MCSymbol &Symbol) {
   getStreamer().getAssembler().registerSymbol(Symbol);
   cast<MCSymbolELF>(Symbol).setOther(ELF::STO_RISCV_VARIANT_CC);
 }
 
 bool RISCVELFStreamer::requiresFixups(MCContext &C, const MCExpr *Value,
                                       const MCExpr *&LHS, const MCExpr *&RHS) {
   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();
 
   // Avoid ADD/SUB if Kind is not VK_None, e.g. A@plt - B + C.
   if (E.getSymA()->getKind() != MCSymbolRefExpr::VK_None)
     return false;
 
   // 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
void RISCVELFStreamer::emitValueImpl(const MCExpr *Value, unsigned Size,
   DF->getFixups().push_back(
       MCFixup::create(DF->getContents().size(), A, Add, Loc));
   DF->getFixups().push_back(
       MCFixup::create(DF->getContents().size(), B, Sub, Loc));
 
   DF->getContents().resize(DF->getContents().size() + Size, 0);
 }
 
+void RISCVELFStreamer::emitULEB128Value(const MCExpr *Value) {
+  const MCExpr *A, *B;
+  if (!requiresFixups(getContext(), Value, A, B))
+    return MCELFStreamer::emitULEB128Value(Value);
+
+  SMLoc Loc = Value->getLoc();
+
+  int64_t IntValue;
+  unsigned PadTo = 0;
+  if (!Value->evaluateAsAbsolute(IntValue, getAssemblerPtr())) {
+    const FeatureBitset &Features =
+        getContext().getSubtargetInfo()->getFeatureBits();
+    unsigned XLen = Features[RISCV::Feature64Bit] ? 64 : 32;
+
+    // Emit a placeholder to make sure we'll emit enough space could be filled
+    // by linker later.
+    IntValue = 0;
+    // Each byte in ULEB128 format can encoding at most 7 bits.
+    PadTo = divideCeil(XLen, 7);
+  }
+
+  if (IntValue < 0)
+    report_fatal_error("Can't encoding negative value in uleb128 format.");
+
+  MCDataFragment *DF = getOrCreateDataFragment();
+  flushPendingLabels(DF, DF->getContents().size());
+  MCDwarfLineEntry::make(this, getCurrentSectionOnly());
+
+  DF->getFixups().push_back(
+      MCFixup::create(DF->getContents().size(), A,
+                      MCFixupKind(RISCV::fixup_riscv_set_uleb128), Loc));
+  DF->getFixups().push_back(
+      MCFixup::create(DF->getContents().size(), B,
+                      MCFixupKind(RISCV::fixup_riscv_sub_uleb128), Loc));
+
+  SmallString<128> Tmp;
+  raw_svector_ostream OSE(Tmp);
+  encodeULEB128(IntValue, OSE, PadTo);
+  emitBytes(OSE.str());
+}
+
 namespace llvm {
 MCELFStreamer *createRISCVELFStreamer(MCContext &C,
                                       std::unique_ptr<MCAsmBackend> MAB,
                                       std::unique_ptr<MCObjectWriter> MOW,
                                       std::unique_ptr<MCCodeEmitter> MCE,
                                       bool RelaxAll) {
   RISCVELFStreamer *S =
       new RISCVELFStreamer(C, std::move(MAB), std::move(MOW), std::move(MCE));
   S->getAssembler().setRelaxAll(RelaxAll);
   return S;
 }
 } // namespace llvm

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

enum Fixups {
   // paired relocations.
   fixup_riscv_sub_64,
   // 6-bit fixup corresponding to R_RISCV_SET6 for local label assignment in
   // DWARF CFA.
   fixup_riscv_set_6b,
   // 6-bit fixup corresponding to R_RISCV_SUB6 for local label assignment in
   // DWARF CFA.
   fixup_riscv_sub_6b,
+  // fixup corresponding to R_RISCV_SET_ULEB128 for local label assignment.
+  fixup_riscv_set_uleb128,
+  // fixup corresponding to R_RISCV_SUB_ULEB128 for local label assignment.
+  fixup_riscv_sub_uleb128,
 
   // Used as a sentinel, must be the last
   fixup_riscv_invalid,
   NumTargetFixupKinds = fixup_riscv_invalid - FirstTargetFixupKind
 };
 
 static inline std::pair<MCFixupKind, MCFixupKind>
 getRelocPairForSize(unsigned Size) {

llvm/test/MC/RISCV/fixups-expr-uleb128.s

+# RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=+relax %s \
+# RUN:     | llvm-readobj -r - | FileCheck -check-prefix RELAX-32 %s
+# RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=-relax %s \
+# RUN:     | llvm-readobj -r - | FileCheck -check-prefix RELAX-32 %s
+
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+relax %s \
+# RUN:     | llvm-readobj -r - | FileCheck -check-prefix RELAX-64 %s
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=-relax %s \
+# RUN:     | llvm-readobj -r - | FileCheck -check-prefix RELAX-64 %s
+
+# Check that subtraction expressions are emitted as two relocations always.
+
+.text
+.globl G1
+.globl G2
+.L1:
+G1:
+addi a0, a0, 0
+.L2:
+G2:
+
+.data
+.uleb128 G3-G1
+.uleb128 .L2-.L1
+.uleb128 G2-G1
+
+# RELAX-32: 0x0 R_RISCV_SET_ULEB128 G3 0x0
+# RELAX-32: 0x0 R_RISCV_SUB_ULEB128 G1 0x0
+# RELAX-32: 0x5 R_RISCV_SET_ULEB128 .L2 0x0
+# RELAX-32: 0x5 R_RISCV_SUB_ULEB128 .L1 0x0
+# RELAX-32: 0x6 R_RISCV_SET_ULEB128 G2 0x0
+# RELAX-32: 0x6 R_RISCV_SUB_ULEB128 G1 0x0
+
+# RELAX-64: 0x0 R_RISCV_SET_ULEB128 G3 0x0
+# RELAX-64: 0x0 R_RISCV_SUB_ULEB128 G1 0x0
+# RELAX-64: 0xA R_RISCV_SET_ULEB128 .L2 0x0
+# RELAX-64: 0xA R_RISCV_SUB_ULEB128 .L1 0x0
+# RELAX-64: 0xB R_RISCV_SET_ULEB128 G2 0x0
+# RELAX-64: 0xB R_RISCV_SUB_ULEB128 G1 0x0
+
+.text
+.globl G3
+G3:
+nop