[PATCH] D101569: [LLD] [COFF] Fix automatic export of symbols from LTO objects

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: lld/COFF/Driver.cpp:1212
+    auto *defReg = dyn_cast<DefinedRegular>(s);
+    if (!defReg || defReg->data) {
+      if (Chunk *c = def->getChunk())
----------------
mstorsjo wrote:
> It turns out we can't actually call `def->getChunk()` on a `DefinedRegular` from a bitcode object file, because that symbol has got the `SectionChunk **data;` member set to `nullptr`, and `getChunk()` dereferences `*data`, so we have to do this dance so we won't crash on calling `def->getChunk()`.
Exciting. I took a look at LTO support a while back, and I wanted to rearrange how some of this stuff worked, but I never got around to it. I think I wanted to make some kind of DefinedBitcode symbol or something like that so that the fields would be less overloaded and more uniform.


================
Comment at: lld/COFF/Driver.cpp:1221
+          if (objSym.getName() == e.name) {
+            if (!objSym.isExecutable())
+              e.data = true;
----------------
mstorsjo wrote:
> Here we end up iterating over all symbols in the input bitcode file, to find the one we're looking at, to figure out whether it's data or a function. Not pretty...
Yeah, it would be nice to figure out how to avoid this, since it's O(n^2)-ish.

One way would be to store this bit directly on the DefinedRegular symbol object.

Another way would be to start making fake, synthetic SectionChunk objects for symbols from bitcode files. We could have the standard text, data, rdata, bss sections, for example. This would make the previous conditional unnecessary. That seems better.

The least invasive way would be to separately iterate over the bitcode input files and look their symbols up in the symbol table and use that to create exports. However, symbol resolution is already expensive, and this is basically a second symbol resolution phase.

I kind of lean towards the second option. It leads to less null checks in the long run.


================
Comment at: lld/COFF/Driver.cpp:2130
+    // are chosen to be exported.
+    maybeExportMinGWSymbols(args);
+  }
----------------
mstorsjo wrote:
> Here we need to decide what symbols to export before we run `addCombinedLTOObjects()`.
Yes, that makes sense: we can't really do effective LTO without knowing what the entry points are going to be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101569