[PATCH] D114783: [ELF] Split scanRelocations into scanRelocations/postScanRelocations

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Relocations.cpp:1612
+      } else {
+        if (!sym.isInPlt())
+          addPltEntry(in.plt, in.gotPlt, in.relaPlt, target->pltRel, sym);
----------------
peter.smith wrote:
> Worth an `assert(sym.isFunc() && !sym.isGnuIFunc())` here?
Thanks. Applied. isFunc does not inclue isGnuIFunc() so I omitted isGnuIFunc().


================
Comment at: lld/ELF/Symbols.h:287
+  uint8_t needsPlt : 1;
+  uint8_t needsCopy : 1;
+  uint8_t hasDirectReloc : 1;
----------------
peter.smith wrote:
> 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;
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.


================
Comment at: lld/ELF/Symbols.h:282
 
+  uint8_t needsGot : 1;
+  uint8_t needsPlt : 1;
----------------
peter.smith wrote:
> 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.
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114783/new/

https://reviews.llvm.org/D114783