Skip to content

[RISCV][NFC] Refactor Vendor Reloc Declarations #138226

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lenary
Copy link
Member

@lenary lenary commented May 2, 2025

This change makes it much easier for external projects to opt-in to supporting relocations from specific vendors (or with specific tags), rather than having to support all of them at once.

This change makes it much easier for external projects to opt-in to
supporting relocations from specific vendors (or with specific tags),
rather than having to support all of them at once.
@llvmbot
Copy link
Member

llvmbot commented May 2, 2025

@llvm/pr-subscribers-llvm-binary-utilities

@llvm/pr-subscribers-backend-risc-v

Author: Sam Elliott (lenary)

Changes

This change makes it much easier for external projects to opt-in to supporting relocations from specific vendors (or with specific tags), rather than having to support all of them at once.


Full diff: https://github.com/llvm/llvm-project/pull/138226.diff

3 Files Affected:

  • (modified) llvm/include/llvm/BinaryFormat/ELF.h (+2-2)
  • (modified) llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV_nonstandard.def (+28-17)
  • (modified) llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp (+2-2)
diff --git a/llvm/include/llvm/BinaryFormat/ELF.h b/llvm/include/llvm/BinaryFormat/ELF.h
index 1f3cea4bd1ae6..77077dad01f60 100644
--- a/llvm/include/llvm/BinaryFormat/ELF.h
+++ b/llvm/include/llvm/BinaryFormat/ELF.h
@@ -700,9 +700,9 @@ enum : unsigned {
 // ELF Relocation types for RISC-V
 enum {
 #include "ELFRelocs/RISCV.def"
-#define ELF_RISCV_NONSTANDARD_RELOC(_vendor, name, value) name = value,
+#define ELF_RISCV_NONSTANDARD_RELOC_ALL(name, value) name = value,
 #include "ELFRelocs/RISCV_nonstandard.def"
-#undef ELF_RISCV_NONSTANDARD_RELOC
+#undef ELF_RISCV_NONSTANDARD_RELOC_ALL
 };
 
 enum {
diff --git a/llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV_nonstandard.def b/llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV_nonstandard.def
index 7ae3d3f205772..a9c635ae13592 100644
--- a/llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV_nonstandard.def
+++ b/llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV_nonstandard.def
@@ -6,23 +6,34 @@
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef ELF_RISCV_NONSTANDARD_RELOC
-#error "ELF_RISCV_NONSTANDARD_RELOC must be defined"
+// This file defines information about RISC-V's nonstandard relocation codes.
+// This can be used when parsing relocations, or when printing them, to provide
+// better information.
+//
+// Unlike the mappings provided in RISCV.def via `ELF_RELOC`, these are not
+// expected to be 1:1 mappings - multiple vendors may reuse relocation IDs.
+
+// For ease of use, `ELF_RISCV_NONSTANDARD_RELOC_ALL` invokes the macros for
+// each vendor.
+
+#ifdef ELF_RISCV_NONSTANDARD_RELOC_ALL
+#define ELF_RISCV_NONSTANDARD_RELOC_QUALCOMM(NAME, ID) ELF_RISCV_NONSTANDARD_RELOC_ALL(NAME, ID)
 #endif
 
-// ELF_RISCV_NONSTANDARD_RELOC(VENDOR, NAME, ID) defines information about
-// nonstandard relocation codes. This can be used when parsing relocations, or
-// when printing them, to provide better information.
-//
-// VENDOR should be the symbol name expected in the associated `R_RISCV_VENDOR`
-// relocation. NAME and ID work like `ELF_RELOC` but the mapping is not expected
-// to be 1:1.
-//
-// The mapping in RISCV.def is 1:1, and should be used when the only information
-// available is the relocation enum value.
+// For each Vendor identifier, VENDOR, as associated with an `R_RISCV_VENDOR`,
+// the `ELF_RISCV_NONSTANDARD_RELOC_<VENDOR>(NAME, ID)` macro defines the
+// relocations available for that vendor identifier.
 
-// Qualcomm Nonstandard Relocations
-ELF_RISCV_NONSTANDARD_RELOC(QUALCOMM, R_RISCV_QC_ABS20_U,    192)
-ELF_RISCV_NONSTANDARD_RELOC(QUALCOMM, R_RISCV_QC_E_BRANCH,   193)
-ELF_RISCV_NONSTANDARD_RELOC(QUALCOMM, R_RISCV_QC_E_32,       194)
-ELF_RISCV_NONSTANDARD_RELOC(QUALCOMM, R_RISCV_QC_E_JUMP_PLT, 195)
+// QUALCOMM Nonstandard Relocations
+#ifndef ELF_RISCV_NONSTANDARD_RELOC_QUALCOMM
+#define ELF_RISCV_NONSTANDARD_RELOC_QUALCOMM(_NAME, _ID)
+#endif
+
+ELF_RISCV_NONSTANDARD_RELOC_QUALCOMM(R_RISCV_QC_ABS20_U,    192)
+ELF_RISCV_NONSTANDARD_RELOC_QUALCOMM(R_RISCV_QC_E_BRANCH,   193)
+ELF_RISCV_NONSTANDARD_RELOC_QUALCOMM(R_RISCV_QC_E_32,       194)
+ELF_RISCV_NONSTANDARD_RELOC_QUALCOMM(R_RISCV_QC_E_JUMP_PLT, 195)
+
+#ifdef ELF_RISCV_NONSTANDARD_RELOC_ALL
+#undef ELF_RISCV_NONSTANDARD_RELOC_QUALCOMM
+#endif
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
index d8cfeb07e52b6..60c9eb4645842 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
@@ -48,9 +48,9 @@ std::optional<MCFixupKind> RISCVAsmBackend::getFixupKind(StringRef Name) const {
 #define ELF_RELOC(NAME, ID) .Case(#NAME, ID)
 #include "llvm/BinaryFormat/ELFRelocs/RISCV.def"
 #undef ELF_RELOC
-#define ELF_RISCV_NONSTANDARD_RELOC(_VENDOR, NAME, ID) .Case(#NAME, ID)
+#define ELF_RISCV_NONSTANDARD_RELOC_ALL(NAME, ID) .Case(#NAME, ID)
 #include "llvm/BinaryFormat/ELFRelocs/RISCV_nonstandard.def"
-#undef ELF_RISCV_NONSTANDARD_RELOC
+#undef ELF_RISCV_NONSTANDARD_RELOC_ALL
                .Case("BFD_RELOC_NONE", ELF::R_RISCV_NONE)
                .Case("BFD_RELOC_32", ELF::R_RISCV_32)
                .Case("BFD_RELOC_64", ELF::R_RISCV_64)

@lenary
Copy link
Member Author

lenary commented May 2, 2025

@tclin914 I will wait until your PR that hits the same files is landed, and then refactor your changes in this PR.

Copy link
Contributor

@svs-quic svs-quic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

A more general question - is there a reason you call it nonstandard_reloc and not vendor_reloc? Can there be other types of non_standard relocations apart from the vendor ones?

ELF_RISCV_NONSTANDARD_RELOC_QUALCOMM(R_RISCV_QC_E_JUMP_PLT, 195)

#ifdef ELF_RISCV_NONSTANDARD_RELOC_ALL
#undef ELF_RISCV_NONSTANDARD_RELOC_QUALCOMM
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if it was defined by line 29? Should we undef it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at what the relevant clang .def files do in similar circumstances and they unconditionally undef everything, so I just did that.

@lenary
Copy link
Member Author

lenary commented May 2, 2025

A more general question - is there a reason you call it nonstandard_reloc and not vendor_reloc? Can there be other types of non_standard relocations apart from the vendor ones?

The ABI is reasonably inconsistent about the use of both terms, but the table says "Reserved for nonstandard extensions", and the relevant section was "Nonstandard relocations" until i added "(a.k.a. Vendor-Specific Relocations)" recently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants