[PATCH] D132361: [LLD] [COFF] Fix export directives in object files from -includeoptional

Mon Aug 22 14:03:25 PDT 2022

mstorsjo added a comment.

In D132361#3740605 <https://reviews.llvm.org/D132361#3740605>, @mati865 wrote:

> Unfortunately I lack a lot of context here so cannot tell if there are alternative solutions.
> How expensive (in terms of performance) is calling `updateLinkLoop` that many times?

I think it might cost a little bit - but I think, in rough terms, that we already run it once or more for the bulk of the linking, and this adds mostly one or two more rounds in the end. But I guess it'd be good to get proper numbers for it.

================
Comment at: lld/COFF/Driver.cpp:2241
   if (args.hasArg(OPT_include_optional)) {
     // Handle /includeoptional
     for (auto *arg : args.filtered(OPT_include_optional))
----------------
rnk wrote:
> Can this not be moved into the body of the loop?
Hmm, good question. I guess it could be handled that way - let me try that...

================
Comment at: lld/COFF/Driver.cpp:2251
   // Create wrapped symbols for -wrap option.
   std::vector<WrappedSymbol> wrapped = addWrappedSymbols(ctx, args);
   // Load more object files that might be needed for wrapped symbols.
----------------
rnk wrote:
> I'm not sure if `addWrappedSymbols` can be called multiple times, but if it can, or if it can be made safe such that it is only done once, I'd suggest moving it into the loop as well.
I'll see if that's possible.

FWIW we have yet another `run()` invocation further below, in the `if (config->autoImport || config->stdcallFixup) {`, but that one doesn't even loop currently - not sure if we'd want to fuse that into the main loop too...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132361