[MachO LLD] Respect -all_load with --start-lib --end-lib style archives

[NuriAmari]

The -all_load flag is intended to force the linker to load all lazy members, but doesn't do so if the archive is specified with --start-lib, --end-lib flags. This patch makes it so that -all_load also applies in this case.

[llvmbot]

@llvm/pr-subscribers-lld-macho

@llvm/pr-subscribers-lld

Author: Nuri Amari (NuriAmari)

Changes

The -all_load flag is intended to force the linker to load all lazy members, but doesn't do so if the archive is specified with --start-lib, --end-lib flags. This patch makes it so that -all_load also applies in this case.

---

Full diff: https://github.com/llvm/llvm-project/pull/93993.diff

2 Files Affected:

• (modified) lld/MachO/Driver.cpp (+3-2)
• (modified) lld/test/MachO/start-lib.s (+5)

diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp
index 4ee6a907b2f46..9a05e5532603a 100644
--- a/lld/MachO/Driver.cpp
+++ b/lld/MachO/Driver.cpp
@@ -1169,7 +1169,8 @@ static void createFiles(const InputArgList &args) {
 
     switch (opt.getID()) {
     case OPT_INPUT:
-      addFile(rerootPath(arg->getValue()), LoadType::CommandLine, isLazy);
+      addFile(rerootPath(arg->getValue()), LoadType::CommandLine,
+              isLazy && !config->allLoad);
       break;
     case OPT_needed_library:
       if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
@@ -1189,7 +1190,7 @@ static void createFiles(const InputArgList &args) {
         dylibFile->forceWeakImport = true;
       break;
     case OPT_filelist:
-      addFileList(arg->getValue(), isLazy);
+      addFileList(arg->getValue(), isLazy && !config->allLoad);
       break;
     case OPT_force_load:
       addFile(rerootPath(arg->getValue()), LoadType::CommandLineForce);
diff --git a/lld/test/MachO/start-lib.s b/lld/test/MachO/start-lib.s
index 2da940c929490..09df292e13fa8 100644
--- a/lld/test/MachO/start-lib.s
+++ b/lld/test/MachO/start-lib.s
@@ -86,6 +86,11 @@
 # RUN: not %lld --end-lib 2>&1 | FileCheck %s --check-prefix=STRAY
 # STRAY: error: stray --end-lib
 
+# RUN: %lld -dylib --start-lib %t/1.bc %t/2.o --end-lib -all_load -o %t/out
+# RUN: llvm-readobj -s %t/out | FileCheck --check-prefix=ALL-LOAD %s
+# ALL-LOAD-DAG: _foo
+# ALL-LOAD-DAG: _bar
+
 #--- main.s
 .globl _main
 _main:

[MaskRay]

Mach-O -all_load is like a global variant of ELF --whole-archive --no-whole-archive.
For ELF, we have rejected the proposal to treat --whole-archive --start-lib a.o --end-lib --no-whole-archive as .o
because there isn't a clear signal for --whole-archive to override --start-lib and we stick with the simplest implementation.

For Mach-O, it might make sense for the non-positional -all_load to override --start-lib.
But I expect more reasoning behind this change.

[kyulee-com]

lgtm. I believe this is required to support distributed thinlto (d-thinlto) by specifying each archive member explicitly using --start-lib/--end-lib, instead of passing the archive file directly.
Btw, I'm curious how we handle the -force_load case (case OPT_force_load) for d-thinlto.

[NuriAmari]

For Mach-O, it might make sense for the non-positional -all_load to override --start-lib.
But I expect more reasoning behind this change.

I think the flag being global makes a difference.

Since it is global, a user can supply the flag to a build system and expect it to work without inspecting the rest of the linker invocation. The same is not true of --whole-archive. The user already needs to see the entire invocation to position the —whole-archive / —no-whole-archive flags correctly, at which point they can just remove —start-lib / —end-lib as required. —We use —start-lib / —end-lib for distributed thin-lto. Users should be able to safely switch between thin-lto and distributed thin-lto without having to audit the custom flags they’ve provided (within reason).

To accomplish this without this change, the build system would need to inspect each linker invocation and remove —start-lib / —end-lib if -all_load is passed. That seems needlessly complex. It seems to me for a global flag, the behavior of a real archive, and —start-lib —end-lib should be the same.

Btw, I'm curious how we handle the -force_load case (case OPT_force_load) for d-thinlto.

That is an attribute of the library being consumed (link_whole). Libraries with this attribute are passed to the linker without --start-lib / --end-lib.

[MaskRay]

For Mach-O, it might make sense for the non-positional -all_load to override --start-lib.
But I expect more reasoning behind this change.

I think the flag being global makes a difference.

Since it is global, a user can supply the flag to a build system and expect it to work without inspecting the rest of the linker invocation. The same is not true of --whole-archive. The user already needs to see the entire invocation to position the —whole-archive / —no-whole-archive flags correctly, at which point they can just remove —start-lib / —end-lib as required. —We use —start-lib / —end-lib for distributed thin-lto. Users should be able to safely switch between thin-lto and distributed thin-lto without having to audit the custom flags they’ve provided (within reason).

To accomplish this without this change, the build system would need to inspect each linker invocation and remove —start-lib / —end-lib if -all_load is passed. That seems needlessly complex. It seems to me for a global flag, the behavior of a real archive, and —start-lib —end-lib should be the same.

Btw, I'm curious how we handle the -force_load case (case OPT_force_load) for d-thinlto.

That is an attribute of the library being consumed (link_whole). Libraries with this attribute are passed to the linker without --start-lib / --end-lib.

Yes, I agree with the reasoning. It's worth capturing some of the thinking into the commit message (first comment).