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

[mstorsjo]

Summary

When an object file contains an export directive, we normally do some
amount of deferred processing of them at the end of the linking
process. The -includeoptional option was handled after this, and
any object files (defining new exports) weren't handled.

Move the handling of the -includeoptional into the same late loop
which does the fixups for e.g. export directives.

Ideally, this would also be done for object files that are pulled
in by the wrap options, and for mingw autoimports, but those changes
require more modifications, to make them safe for potentially
being executed multiple times.

[mati865]

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?

[rnk]

lld/COFF/Driver.cpp

2194

Personally, I think long lambdas can make it hard to figure out the structure of the code, so I'd like to try and avoid them if possible, and if necessary, make it a standalone function or method.

2234–2235

Can this not be moved into the body of the loop?

2244

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.

[mstorsjo]

In 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.

lld/COFF/Driver.cpp

2234–2235

Hmm, good question. I guess it could be handled that way - let me try that...

2244

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...

[mstorsjo]

In D132361#3740753, @mstorsjo wrote:

In 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.

At least for the size of link jobs I have to test with here, with a total runtime of 1241 ms, the main loop iteration doesn't add up to even 1 ms.

[mstorsjo]

lld/COFF/Driver.cpp

2234–2235

After refamiliarizing myself with these parts of the COFF linker - yes, this should definitely just be moved into the current loop.

2244

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.

[mstorsjo]

Move handling of the -includeoptional options into the same loop as export.

[mati865]

LGTM

lld/COFF/Driver.cpp

void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
   if (config->mingw) {
     ctx.symtab.addAbsolute(mangle("__CTOR_LIST__"), 0);
     ctx.symtab.addAbsolute(mangle("__DTOR_LIST__"), 0);
   }
 
   // This code may add new undefined symbols to the link, which may enqueue more
   // symbol resolution tasks, so we need to continue executing tasks until we
   // converge.
   do {
     // Windows specific -- if entry point is not found,
     // search for its mangled names.
     if (config->entry)
       mangleMaybe(config->entry);
 
     // Windows specific -- Make sure we resolve all dllexported symbols.
     for (Export &e : config->exports) {
       if (!e.forwardTo.empty())
do {
     // compile those archive members by adding them to the link beforehand.
     if (!ctx.bitcodeFileInstances.empty())
       for (auto *s : lto::LTO::getRuntimeLibcallSymbols())
         ctx.symtab.addLibcall(s);
 
     // Windows specific -- if __load_config_used can be resolved, resolve it.
     if (ctx.symtab.findUnderscore("_load_config_used"))
       addUndefined(mangle("_load_config_used"));
-  } while (run());
 
     if (args.hasArg(OPT_include_optional)) {
       // Handle /includeoptional
       for (auto *arg : args.filtered(OPT_include_optional))
         if (isa_and_nonnull<LazyArchive>(ctx.symtab.find(arg->getValue())))
           addUndefined(arg->getValue());
-    while (run());
     }
+  } while (run());
 
   // Create wrapped symbols for -wrap option.
   std::vector<WrappedSymbol> wrapped = addWrappedSymbols(ctx, args);
   // Load more object files that might be needed for wrapped symbols.
   if (!wrapped.empty())
     while (run());
 
   if (config->autoImport || config->stdcallFixup) {
     // MinGW specific.
     // Load any further object files that might be needed for doing automatic
     // imports, and do stdcall fixups.

lld/test/COFF/includeoptional-export.s

+// REQUIRES: x86
+// RUN: split-file %s %t.dir
+
+// RUN: llvm-mc -filetype=obj -triple=x86_64-win32-gnu %t.dir/main.s -o %t.main.o
+// RUN: llvm-mc -filetype=obj -triple=x86_64-win32-gnu %t.dir/lib1.s -o %t.lib1.o
+// RUN: llvm-mc -filetype=obj -triple=x86_64-win32-gnu %t.dir/lib2.s -o %t.lib2.o
+
+// RUN: rm -f %t.lib.a
+// RUN: llvm-ar cru %t.lib.a %t.lib1.o %t.lib2.o
+// RUN: lld-link -dll -out:%t-1.dll -entry:entry %t.main.o %t.lib.a
+// RUN: lld-link -dll -out:%t-2.dll -entry:entry %t.main.o %t.lib.a -includeoptional:libfunc
+
+// RUN: llvm-readobj --coff-exports %t-1.dll | FileCheck --implicit-check-not=Name: %s --check-prefix=CHECK-DEFAULT
+// RUN: llvm-readobj --coff-exports %t-2.dll | FileCheck --implicit-check-not=Name: %s --check-prefix=CHECK-INCLUDEOPTIONAL
+
+// CHECK-DEFAULT: Name:
+// CHECK-DEFAULT: Name: myfunc
+
+// CHECK-INCLUDEOPTIONAL: Name:
+// CHECK-INCLUDEOPTIONAL: Name: libfunc
+// CHECK-INCLUDEOPTIONAL: Name: myfunc
+// CHECK-INCLUDEOPTIONAL: Name: otherlibfunc
+
+#--- main.s
+.global entry
+entry:
+  ret
+
+.global myfunc
+myfunc:
+  ret
+
+.section .drectve
+.ascii "-export:myfunc "
+
+#--- lib1.s
+.global libfunc
+libfunc:
+  call otherlibfunc
+  ret
+
+.section .drectve
+.ascii "-export:libfunc "
+
+#--- lib2.s
+.global otherlibfunc
+otherlibfunc:
+  ret
+
+.section .drectve
+.ascii "-export:otherlibfunc "