[NFCI][ELF] Merge AgainstSymbol and AgainstSymbolWithTargetVA #150798
Conversation
Created using spr 1.3.5 [skip ci]
Created using spr 1.3.5
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-elf Author: Jessica Clarke (jrtc27) ChangesThe former is just a special case of the latter, ignoring the expr and This just leaves MipsMultiGotPage as a special case; the only difference Full diff: https://github.com/llvm/llvm-project/pull/150798.diff 3 Files Affected:
diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index ed53228359f86..7579def5865bb 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -885,10 +885,12 @@ static void addPltEntry(Ctx &ctx, PltSection &plt, GotPltSection &gotPlt,
RelocationBaseSection &rel, RelType type, Symbol &sym) {
plt.addEntry(sym);
gotPlt.addEntry(sym);
- rel.addReloc({type, &gotPlt, sym.getGotPltOffset(ctx),
- sym.isPreemptible ? DynamicReloc::AgainstSymbol
- : DynamicReloc::AddendOnly,
- sym, 0, R_ABS});
+ if (sym.isPreemptible)
+ rel.addReloc({type, &gotPlt, sym.getGotPltOffset(ctx),
+ DynamicReloc::AgainstSymbol, sym, 0, R_ADDEND});
+ else
+ rel.addReloc({type, &gotPlt, sym.getGotPltOffset(ctx),
+ DynamicReloc::AddendOnly, sym, 0, R_ABS});
}
void elf::addGotEntry(Ctx &ctx, Symbol &sym) {
@@ -899,7 +901,7 @@ void elf::addGotEntry(Ctx &ctx, Symbol &sym) {
if (sym.isPreemptible) {
ctx.mainPart->relaDyn->addReloc({ctx.target->gotRel, ctx.in.got.get(), off,
DynamicReloc::AgainstSymbol, sym, 0,
- R_ABS});
+ R_ADDEND});
return;
}
@@ -921,7 +923,7 @@ static void addGotAuthEntry(Ctx &ctx, Symbol &sym) {
if (sym.isPreemptible) {
ctx.mainPart->relaDyn->addReloc({R_AARCH64_AUTH_GLOB_DAT, ctx.in.got.get(),
off, DynamicReloc::AgainstSymbol, sym, 0,
- R_ABS});
+ R_ADDEND});
return;
}
diff --git a/lld/ELF/SyntheticSections.cpp b/lld/ELF/SyntheticSections.cpp
index 690e9bf2d7d8b..6bb3aa2e5e745 100644
--- a/lld/ELF/SyntheticSections.cpp
+++ b/lld/ELF/SyntheticSections.cpp
@@ -1065,9 +1065,9 @@ void MipsGotSection::build() {
// for the TP-relative offset as we don't know how much other data will
// be allocated before us in the static TLS block.
if (s->isPreemptible || ctx.arg.shared)
- ctx.mainPart->relaDyn->addReloc(
- {ctx.target->tlsGotRel, this, offset,
- DynamicReloc::AgainstSymbolWithTargetVA, *s, 0, R_ABS});
+ ctx.mainPart->relaDyn->addReloc({ctx.target->tlsGotRel, this, offset,
+ DynamicReloc::AgainstSymbol, *s, 0,
+ R_ABS});
}
for (std::pair<Symbol *, size_t> &p : got.dynTlsSymbols) {
Symbol *s = p.first;
@@ -1648,14 +1648,11 @@ uint64_t DynamicReloc::getOffset() const {
int64_t DynamicReloc::computeAddend(Ctx &ctx) const {
switch (kind) {
case AddendOnly:
- case AgainstSymbolWithTargetVA: {
+ case AgainstSymbol: {
uint64_t ca = inputSec->getRelocTargetVA(
ctx, Relocation{expr, type, 0, addend, sym}, getOffset());
return ctx.arg.is64 ? ca : SignExtend64<32>(ca);
}
- case AgainstSymbol:
- assert(sym != nullptr);
- return addend;
case MipsMultiGotPage:
assert(sym == nullptr);
return getMipsPageAddr(outputSec->addr) + addend;
@@ -1698,7 +1695,7 @@ void RelocationBaseSection::addAddendOnlyRelocIfNonPreemptible(
// No need to write an addend to the section for preemptible symbols.
if (sym.isPreemptible)
addReloc({dynType, &isec, offsetInSec, DynamicReloc::AgainstSymbol, sym, 0,
- R_ABS});
+ R_ADDEND});
else
addReloc(DynamicReloc::AddendOnly, dynType, isec, offsetInSec, sym, 0,
R_ABS, addendRelType);
diff --git a/lld/ELF/SyntheticSections.h b/lld/ELF/SyntheticSections.h
index b72436372fbc1..5a2437c7961df 100644
--- a/lld/ELF/SyntheticSections.h
+++ b/lld/ELF/SyntheticSections.h
@@ -424,13 +424,9 @@ class DynamicReloc {
/// Useful for various relative and TLS relocations (e.g. R_X86_64_TPOFF64).
AddendOnly,
/// The resulting dynamic relocation references symbol #sym from the dynamic
- /// symbol table and uses #addend as the value of computeAddend(ctx).
+ /// symbol table and uses InputSection::getRelocTargetVA() for the final
+ /// addend.
AgainstSymbol,
- /// The resulting dynamic relocation references symbol #sym from the dynamic
- /// symbol table and uses InputSection::getRelocTargetVA() + #addend for the
- /// final addend. It can be used for relocations that write the symbol VA as
- // the addend (e.g. R_MIPS_TLS_TPREL64) but still reference the symbol.
- AgainstSymbolWithTargetVA,
/// This is used by the MIPS multi-GOT implementation. It relocates
/// addresses of 64kb pages that lie inside the output section.
MipsMultiGotPage,
@@ -458,9 +454,7 @@ class DynamicReloc {
uint64_t getOffset() const;
uint32_t getSymIndex(SymbolTableBaseSection *symTab) const;
- bool needsDynSymIndex() const {
- return kind == AgainstSymbol || kind == AgainstSymbolWithTargetVA;
- }
+ bool needsDynSymIndex() const { return kind == AgainstSymbol; }
/// Computes the addend of the dynamic relocation. Note that this is not the
/// same as the #addend member variable as it may also include the symbol
|
Created using spr 1.3.5 [skip ci]
Yeah I think Alex was a bit confused (and also there were some bugs that he faithfully replicated and since fixed, or that I'm fixing in the special case of #150729). I've been delving into how all this works to clean up some horrors in CHERI LLD (and many more in Morello LLD, where there is actually a legitimate use case for AgainstSymbol(WithTargetVA) with something other than R_ADDEND, due to ABI weirdness), and as part of that discovered the confusing and overly-complex nature of all this. With the exception of the MIPS GOT page (and Computed), the end state of this stack (see #150796, #150799, #150797) is everything consistently gets funnelled through getRelocTargetVA. I might also tackle that at some point, though I've already spent too much time on MIPS the past few days, despite not even caring about it any more downstream in CHERI LLVM... |
Eh, I decided to do it in #150810 because then I could do #150813, which is the true sensible (IMO) end state. |
|
Thanks for cleaning up https://reviews.llvm.org/D100490. It looks like I originally started that with a single boolean parameter for "sym/no sym" but it was quite confusing that way. I think all the cleanups that have happened since now makes this a feasible approach. |
Created using spr 1.3.5 [skip ci]
jrtc27
changed the base branch from
users/jrtc27/spr/main.nfcielf-merge-againstsymbol-and-againstsymbolwithtargetva-1
to
main
jrtc27
deleted the
users/jrtc27/spr/nfcielf-merge-againstsymbol-and-againstsymbolwithtargetva-1
branch
The former is just a special case of the latter, ignoring the expr and always just using the addend. If we use R_ADDEND as expr (which previously had no effect, and so was misleadingly R_ABS not R_ADDEND in all but one use) then we don't need to maintain this as a separate case. Aside from the internal Computed Kind, this just leaves MipsMultiGotPage as a special case; the only difference between the other two Kind values is what needsDynSymIndex returns. Reviewers: MaskRay Reviewed By: MaskRay Pull Request: llvm/llvm-project#150798
The former is just a special case of the latter, ignoring the expr and
always just using the addend. If we use R_ADDEND as expr (which
previously had no effect, and so was misleadingly R_ABS not R_ADDEND in
all but one use) then we don't need to maintain this as a separate case.
Aside from the internal Computed Kind, this just leaves MipsMultiGotPage
as a special case; the only difference between the other two Kind values
is what needsDynSymIndex returns.