Page MenuHomePhabricator

[ELF] Split scanRelocations into scanRelocations/postScanRelocations
ClosedPublic

Authored by MaskRay on Nov 30 2021, 2:01 AM.

Details

Summary

The idea is to make scanRelocations just some actions are needed (GOT/PLT/etc)
and postpone the real work to postScanRelocations. It gives some flexibility:

  • Make it feasible to support .plt.got (PR32938): we need to know whether GLOB_DAT and JUMP_SLOT are both needed.
  • Make non-preemptible IFUNC handling slightly cleaner: avoid setting/clearing sym.gotInIgot
  • -z nocopyrel: report all copy relocation places for one symbol
  • Make parallel relocation scanning possible (if we can avoid all stateful operations and make Symbol attributes atomic), but parallelism may not be the appealing choice
  • Make GOT deduplication feasible

Since this patch moves a large chunk of code out of ELFT templates. My x86-64
executable is actually a few hundred bytes smaller.

For ppc32-ifunc-nonpreemptible-pic.s: I remove absolute relocation references to non-preemptible ifunc
because absolute relocation references are incorrect in -fpie mode.

Diff Detail

Event Timeline

MaskRay created this revision.Nov 30 2021, 2:01 AM
MaskRay requested review of this revision.Nov 30 2021, 2:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2021, 2:01 AM
MaskRay edited the summary of this revision. (Show Details)Nov 30 2021, 2:10 AM

I'm personally in favour of postponing some of the calculations from scan relocations as I think it can give us more flexibility. One possible concern is performance, although I'd expect that to be relatively small. I've left some subjective comments, I understand that this is WIP so no need to follow them.

lld/ELF/Relocations.cpp
1615

Maybe a bit easier to read by making fn specific to non-preemptible ifuncs:

for (Symbol *sym : symtab->symbols()) {
    bool canonicalised = handleNonPreemptibleIfunc();
    if (canonicalised)
      continue;
    if (sym.needsGot)
      addGotEntry(sym);
    if (sym.needsPlt && !sym.isInPlt())
      addPltEntry(in.plt, in.gotPlt, in.relaPlt, target->pltRel, sym);
}

for (InputFile *file : objectFiles)
    for (Symbol *sym : cast<ELFFileBase>(file)->getLocalSymbols())
      handleNonPreemptibleIfunc();
lld/ELF/Symbols.h
283

A potential danger of using flags for a very local use case is that they get used later on, or by mistake by earlier code. Will be worth a comment describing what an appropriate use of the flags is. For example:

// Temporary flags used to communicate which symbol entries need PLT and GOT entries during postScanRelocations();

In theory we could use a local container to store the flags, but this would come at a performance cost.

285

perhaps needsCanonicalPlt?

MaskRay updated this revision to Diff 390837.Nov 30 2021, 3:31 PM
MaskRay retitled this revision from [WIP][ELF] Split scanRelocations into scanRelocations/postScanRelocations to [ELF] Split scanRelocations into scanRelocations/postScanRelocations.
MaskRay edited the summary of this revision. (Show Details)

Ready for review

Moved copy relocations / canonical PLT entries code.

local symbols need GOT, too, because assembly can use GOT-generating relocations against local symbols.

MaskRay marked an inline comment as done.Nov 30 2021, 3:34 PM
MaskRay added inline comments.
lld/ELF/Symbols.h
285

needsCopy indicates whether copy relocations/canonical PLT entries are needed for non-ifunc symbols.

hasDirectReloc indicates whether canonical PLT entry is needed for ifunc. hasDirectReloc needs to be true even for isStaticLinkTimeConstant relocations.

Thanks for the update. Will try and take a look later this week.

peter.smith accepted this revision.Dec 3 2021, 3:51 AM

LGTM, I think this is a good simplification. The test changes all look related to GOT ordering, although I've concentrated my checking on AArch64. Maybe worth widening the reviewer pool to see if there are any alternative opinions.

lld/ELF/Relocations.cpp
1619

Worth an assert(sym.isFunc() && !sym.isGnuIFunc()) here?

lld/ELF/Symbols.h
287

My initial thought was that needsCopy isn't specific enough, although I've struggled to come up with anything better. I think needsCopyRelOrCanonicalPLT is the best I could come up with. Similarly for hasDirectReloc the best alternative I could think of was hasNonGotGeneratingReloc.

Best stick to the names here unless someone can think of a better one. Perhaps worth some comments:

// Needs a copy relocation if Object or a Canonical PLT if a non-ifunc function symbol.
uint8_t needsCopy : 1;
// Target of a non-GOT-generating relocation.
uint8_t hasDirectReloc : 1;
lld/test/ELF/aarch64-cortex-a53-843419-recognize.s
611 ↗(On Diff #390837)

I was curious why the .got entries were different. As part of the investigation I noticed a couple of things:
The first .globl dat has a typo, it should be .globl dat1 this causes GOT generating relocations to a local symbol which maybe causing some of the differences. The second is that the ADRPs that are notionally paired with ldr xr, [xs, :got_lo12: symbol] should be adrp xr, :got: symbol This doesn't matter for the test as we're not executing it, but it may be worth correcting at some point.

This revision is now accepted and ready to land.Dec 3 2021, 3:51 AM
MaskRay updated this revision to Diff 391742.Dec 3 2021, 2:43 PM
MaskRay marked 4 inline comments as done.

Add an assert

MaskRay added inline comments.
lld/ELF/Relocations.cpp
1619

Thanks. Applied. isFunc does not inclue isGnuIFunc() so I omitted isGnuIFunc().

lld/ELF/Symbols.h
283

In theory we could use a local container to store the flags, but this would come at a performance cost.

Some flags are non-local symbol only and using an indirection and another container may be better for the SymbolUnion structure size, which can save memory usage. For the immediate usage of this patch, both needsGot and hasDirectReloc can be used by local symbols, which may make the idea non-appealing.

287

I struggled a bit with needsCopyRelOrCanonicalPLT and needsCopy, too, and picked needsCopy because the former seems too long.

For hasDirectReloc, I thought about needsNonGotNonPlt (using the "needs" prefix). It is also too long but don't mind if others think it is better.

Looking at it. Sorry for the delay.

ikudrin added inline comments.Dec 8 2021, 1:54 AM
lld/ELF/Relocations.cpp
1106–1107

As we know that a PLT entry is needed, we can set the flag and simplify postScanRelocations() a bit.

1613

The symbol has not been added in PLT yet, right?

1619–1621

If the suggested change for processRelocAux() is applied.

1640–1641

Maybe extract a shortened version of fn, so that local symbols do not waste time checking flags that are always false for them?

lld/test/ELF/aarch64-ifunc-bti.s
44–45

The comment should be moved before <ifunc2>:

lld/test/ELF/arm-branch-undef-weak-plt-thunk.s
30

Do we really need such precise checks in the test?

lld/test/ELF/ppc32-canonical-plt.s
36–37
44–45
lld/test/ELF/ppc32-reloc-got.s
13–14

This is not checked (and not accurate), thus, can be removed.

19
22–23
22–30
lld/test/ELF/riscv-reloc-got.s
38–40

Sorry I missed this review - looks great! Did you try getting performance numbers using the tar packages that Rafael gathered up?

I have been working on a change to deduplicate redundant GOT entries (e.g. from GOT references to aliased symbols) a postscan pass should make that sort of optimisation more viable.

MaskRay updated this revision to Diff 393773.Dec 12 2021, 12:42 PM
MaskRay marked 13 inline comments as done.

address comments

lld/ELF/Relocations.cpp
1640–1641

A local symbol does not need to check needsCopy, but the majority of checks (non-preemptible ifunc/GOT/PLT) are possible.

lld/test/ELF/aarch64-cortex-a53-843419-recognize.s
611 ↗(On Diff #390837)

Fixed the typo in a separate commit. The differences disappeared.

lld/test/ELF/arm-branch-undef-weak-plt-thunk.s
30

No, this just keeps the state as is. It would be nice if movt/movw can symbolize the target then we can avoid the immediates.

MaskRay updated this revision to Diff 393776.Dec 12 2021, 1:34 PM
MaskRay edited the summary of this revision. (Show Details)

Move // needsCopy is cleared for sym and its aliases so that in later comment to this patch

ikudrin accepted this revision.Dec 13 2021, 4:55 AM

LGTM

MaskRay updated this revision to Diff 393949.Dec 13 2021, 9:56 AM
MaskRay edited the summary of this revision. (Show Details)

update description
update comment for needsCopy

This revision was landed with ongoing or failed builds.Dec 13 2021, 9:57 AM
This revision was automatically updated to reflect the committed changes.