[PATCH] D110450: [LLD] Remove global state in lld/COFF

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a comment.

Thanks for doing this @akhuang! Looks good generally, a few comments:



================
Comment at: lld/COFF/COFFLinkerContext.h:61
+  // plus one. This variable keeps it.
+  uint16_t numOutputSections;
+
----------------
I think this field isn't needed anymore, since after your patch, `DefinedAbsolute` and `applySecIdx()` have direct access to `ctx.outputSections.size()`, which this field duplicates. The only thing that is needed is probably an `assert()` (see other comment in Chunks.cpp)


================
Comment at: lld/COFF/CallGraphSort.cpp:76
 // weights.
-CallGraphSort::CallGraphSort(const COFFLinkerContext &ctx) {
-  MapVector<SectionPair, uint64_t> &profile = config->callGraphProfile;
+CallGraphSort::CallGraphSort(const COFFLinkerContext &c) : ctx(c) {
+  const MapVector<SectionPair, uint64_t> &profile = ctx.config.callGraphProfile;
----------------
Could be `ctx` see other comment.


================
Comment at: lld/COFF/Chunks.cpp:97
 
-static void applySecIdx(uint8_t *off, OutputSection *os) {
+static void applySecIdx(uint8_t *off, OutputSection *os,
+                        uint16_t numOutputSections) {
----------------
I would pass a `unsigned numOutputSections` here and assert within the function that the value fits on 16-bits.


================
Comment at: lld/COFF/DLL.cpp:107
+private:
+  COFFLinkerContext &ctx;
 };
----------------
akhuang wrote:
> aganea wrote:
> > I'm seeing that a lot of the `*Chunk` classes have this extra context now. Have you tried to measure peak RAM usage when linking large executables? Also in scenarios when `/Gy` is used, for example MinGW seems to use that.
> > 
> > My point being, if we can pass the context by argument that would be better maybe instead of storing it into these tiny objects. But I realize that isn't always practical, thus the global access proposed in D108850 - `COFFLinkerContext` would simply inherit  from `CommonLinkingContext` and then could be accessed from anywhere by a call to `context()`.
> Yeah, it feels non-ideal to store the reference in a bunch of Chunks, I'll try to see whether it's feasible to pass it by argument.
> 
> Memory usage doesn't seem to increase too much, though -- peak memory usage for linking chrome (measured with https://github.com/sgraham/tim/blob/master/tim.exe) goes up by 4mb (3972mb -> 3976mb). I think chrome builds with `/Gy`. 
Sounds good.


================
Comment at: lld/COFF/InputFiles.cpp:1066
 void BitcodeFile::parse() {
+  FakeSection *ltoTextSection = make<FakeSection>(IMAGE_SCN_MEM_EXECUTE);
+  FakeSection *ltoDataSection =
----------------
Shouldn't we keep this in `ctx`? Otherwise we'll be creating over and over these fake sections for each bitcode file?


================
Comment at: lld/COFF/PDB.cpp:1702
   ExitOnError exitOnErr;
-  uint32_t secrelReloc = getSecrelReloc();
+  uint32_t secrelReloc = getSecrelReloc(c->file->ctx.config.machine);
 
----------------
Could also be `const uint32_t secrelReloc...`


================
Comment at: lld/COFF/SymbolTable.h:51
 public:
-  SymbolTable(COFFLinkerContext &ctx) : ctx(ctx) {}
+  SymbolTable(COFFLinkerContext &c) : ctx(c) {}
 
----------------
Leaving `ctx` as a constructor parameter is also a perfectly valid thing to do! Please see https://stackoverflow.com/questions/6185020/initializing-member-variables-using-the-same-name-for-constructor-arguments-as-f/6185043#6185043


================
Comment at: lld/COFF/Symbols.cpp:82
 
-uint16_t DefinedAbsolute::numOutputSections;
+uint64_t DefinedAbsolute::getRVA() { return va - ctx.config.imageBase; }
 
----------------
Just a tiny detail, it is a bit confusing that the diff show a totally unrelated addition here, ie.before we had `numOutputSection` and now we have `::getRVA()`. Just my preference, but I usually insert new additions in places where nothing was changed, as compared with the previous revision.


================
Comment at: lld/COFF/Symbols.h:25
 
-std::string toString(coff::Symbol &b);
+// std::string toString(coff::Symbol &b);
 
----------------
Please remove commented out code.


================
Comment at: lld/COFF/Writer.cpp:679
+void Writer::sortBySectionOrder(std::vector<Chunk *> &chunks) {
+  auto getPriority = [this](const Chunk *c) {
     if (auto *sec = dyn_cast<SectionChunk>(c))
----------------
Please capture only `&ctx`


================
Comment at: lld/COFF/Writer.cpp:1875
   // against absolute symbols. See applySecIdx in Chunks.cpp..
-  DefinedAbsolute::numOutputSections = ctx.outputSections.size();
+  ctx.numOutputSections = ctx.outputSections.size();
 
----------------
Please remove, see the other comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110450