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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: lld/COFF/Driver.cpp:2241
   if (args.hasArg(OPT_include_optional)) {
     // Handle /includeoptional
     for (auto *arg : args.filtered(OPT_include_optional))
----------------
mstorsjo wrote:
> 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...
After refamiliarizing myself with these parts of the COFF linker - yes, this should definitely just be moved into the current loop.


================
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.
----------------
mstorsjo wrote:
> 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...
Yes, ideally we'd move that and `ctx.symtab.loadMinGWSymbols();` into the loop as well. But making them safe to potentially be run multiple times is more effort, but is a riskier change, and less backportable, so I think I'd hold off of that until a separate, potential later change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132361