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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

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.



================
Comment at: lld/ELF/Relocations.cpp:1601
+  };
+  for (Symbol *sym : symtab->symbols())
+    fn(*sym);
----------------
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();
```


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


================
Comment at: lld/ELF/Symbols.h:284
+  uint8_t needsPlt : 1;
+  uint8_t needsNonGotPlt : 1;
+
----------------
perhaps needsCanonicalPlt?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114783