[lld-macho] Fix loading same libraries from both LC_LINKER_OPTION and command line

[PRESIDENT810]

Summary

This fixes https://github.com/llvm/llvm-project/issues/56059 and https://github.com/llvm/llvm-project/issues/56440. This is inspired by tapthaker's patch (https://reviews.llvm.org/D127941), and has reused his test cases. This patch adds an bool "isCommandLineLoad" to indicate where archives are from. If lld tries to load the same library loaded previously by LC_LINKER_OPTION from CLI, it will use this isCommandLineLoad to determine if it should be affected by -all_load & -ObjC flags. This also prevents -force_load from affecting archives loaded previously from CLI without such flag, whereas tapthaker's patch will fail such test case (introduced by https://reviews.llvm.org/D128025).

[int3]

Nice fixes, thank you!

While looking back at my own diff, I noticed that one of the tests wasn't quite testing anything new... fixed in rG61ace8f78b1c52fd63fc8c2d800f08d7ddd87b0d. You might want to pull in that fix and check that your changes pass (though I'm guessing they do).

lld/MachO/Driver.cpp

278

I think this should work since LoadScope is just an enum and not an enum class?

1023–1026

hmm why hoist this out of addFile? feels like it'd make sense to have all the archive load logic in one place

lld/MachO/InputFiles.h

270–272 ↗

(On Diff #443912)

our enums are usually capitalized

[PRESIDENT810]

I just realized that -force_load should not work only if the same library is already explicitly passed from command line, but not when it is previously passed from LC_LINKER_OPTION. I revised how I determine if -force_load should work by saving libraries shown in command line in a DenseSet.

[int3]

Hm on seeing the latest iteration + thinking more about the problem, I'm wondering if we could simplify things a bit. This is my understanding of the requirements, correct me if I'm wrong:

1. -ObjC and -force_load only have an effect on command line arguments. LC_LINKER_OPTION libraries are only ever lazy-loaded
2. -force_load and -all_load take precedence over -ObjC, but only if the library hasn't already been loaded on the command line. Notably, if something has been loaded with -ObjC, we will never need to force-load it again later (so we should never need to compare LoadScope::AllLoad against LoadScope::ObjCLoad.)
3. Since lazy-loading an archive twice (or lazy-loading after an eager load) is redundant, we don't need to do anything if we see an LC_LINKER_OPTION of some libfoo.a if we have already loaded libfoo.a via a command-line argument
4. But if we have loaded libfoo.a via LC_LINKER_OPTION and then later see it on the command line, we need to load it if either -ObjC or force_load / all_load is used
5. If none of those flags are used, we could harmlessly lazy-load it a second time, but that would be unnecessary perf overhead

Prior to this fix, we only had the ForceLoad enum. ForceLoad::Yes and ForceLoad::Default only ever get passed to addFile if something is being loaded from the CLI. LC_LINKER_OPTION loads are always ForceLoad::No. So addFile already has enough information to determine whether something is a command-line load vs a LC load (although it's not super obvious from the name ForceLoad::No that this applies only to LC loads).

So here's my suggestion... we could rename ForceLoad to something more descriptive. enum LoadType { CommandLineLoad, CommandLineForceLoad, LCLinkerLoad } maybe. Also change loadedArchives to store a value of struct { ArchiveFile *file, bool isCommandLineLoad }. Now if we try to load an archive and find it already cached in loadedArchives, we only need to load it again if isCommandLineLoad is false and the current load is not another lazy load. In that case we can fall back to the existing logic; there isn't a need to calculate and store a LoadScope as isCommandLineLoad should be enough.

Pros of this design:

• No extra commandLineInputs cache
• Storing bool isCommandLineLoad rather than LoadScope makes it clear (from looking at the data structures alone) that our cache update policy depends only on whether something is being loaded from LC_LINKER_OPTION vs the CLI

Thoughts?

lld/MachO/Driver.cpp

278

nit: I don't know if nesting the enum members under two scopes is really necessary... why not leave it as a raw enum?

[PRESIDENT810]

I removed ForceLoad enum and add LoadFrom & LoadScope. LoadFrom indicates currently where is this archive from and LoadScope means what symbols should we load from this archive (only lazy load, load ObjC symbols, load all symbols or just skip loading this time). Now addFile accepts a LoadFrom enum so it is able to know if this archive is from CLI or LC_LINKER_OPTION. Based on this LoadFrom enum and the LoadFrom enum in its cache (if any), we can decide what symbols we need to load this time. I think this could be clearer than our previous ForceLoad? Also, I added a table in Config.h's comment to explain my implementation when encountered with these flags, please check it for me to make sure that I understand this right!

I didn't make loadedArchives to store extra information, instead I make LoadFrom an attribute in ArchiveFile class, so when we access loadedArchives and obtain the cached files, we are able to know if this archive is from CLI or not. That should do the same. Tell me if you think it's better to store LoadFrom in loadedArchives and I will change it, thanks!

[int3]

Thanks for the revision, but I still think this is more complicated than it needs to be. I think maybe my initial comment was not clear enough... in particular

there isn't a need to calculate and store a LoadScope as isCommandLineLoad should be enough.

i.e. we don't need the LoadScope enum at all. I've now suggested the code change to show how it would work...

Tell me if you think it's better to store LoadFrom in loadedArchives and I will change it, thanks!

I think it's better because we will never access it outside of Driver.cpp. So no need for it to be in the header. Also, from my previous comment:

Storing bool isCommandLineLoad rather than LoadScope makes it clear (from looking at the data structures alone) that our cache update policy depends only on whether something is being loaded from LC_LINKER_OPTION vs the CLI

i.e. could store just a boolean instead of the LoadFrom enum. This one is not such a big deal, if you prefer storing the enum that is fine too

lld/MachO/Driver.cpp

277–295

297

311

[PRESIDENT810]

Updated according to int3's guide. Yeah this way is much more simplified indeed...

[int3]

lgtm, thanks!

lld/MachO/Driver.cpp

for (const Arg *arg :
     case OPT_max_relative_cache_size_lto:
       add(Twine("cache_size=") + arg->getValue() + "%");
       break;
     }
   }
   return CHECK(parseCachePruningPolicy(ltoPolicy), "invalid LTO cache policy");
 }
 
-static DenseMap<StringRef, ArchiveFile *> loadedArchives;
+struct ArchiveFileInfo {
+  ArchiveFile *file;
+  bool isCommandLineLoad;
+};
+
+static DenseMap<StringRef, ArchiveFileInfo> loadedArchives;
 
 static InputFile *addFile(StringRef path, ForceLoad forceLoadArchive,
-                          bool isLazy = false, bool isExplicit = true,
-                          bool isBundleLoader = false) {
+                          bool isCommandLineLoad, bool isLazy = false,
+                          bool isExplicit = true, bool isBundleLoader = false) {
   Optional<MemoryBufferRef> buffer = readFile(path);
   if (!buffer)
     return nullptr;
   MemoryBufferRef mbref = *buffer;
   InputFile *newFile = nullptr;
 
   file_magic magic = identify_magic(mbref.getBuffer());
   switch (magic) {
   case file_magic::archive: {
     // Avoid loading archives twice. If the archives are being force-loaded,
     // loading them twice would create duplicate symbol errors. In the
     // non-force-loading case, this is just a minor performance optimization.
     // We don't take a reference to cachedFile here because the
     // loadArchiveMember() call below may recursively call addFile() and
     // invalidate this reference.
     auto entry = loadedArchives.find(path);
-    if (entry != loadedArchives.end())
-      return entry->second;
 
+    ArchiveFile *file;
+    if (entry == loadedArchives.end()) {
+      // No cached archive, we need to create a new one
       std::unique_ptr<object::Archive> archive = CHECK(
           object::Archive::create(mbref), path + ": failed to parse archive");
 
       if (!archive->isEmpty() && !archive->hasSymbolTable())
         error(path + ": archive has no index; run ranlib to add one");
+      file = make<ArchiveFile>(std::move(archive));
+    } else {
+      file = entry->second.file;
+      // If file is previously loaded via command line, or is loaded via
+      // LC_LINKER_OPTION and being loaded via LC_LINKER_OPTION again,
+      // using the cached archive should be enough
+      if (entry->second.isCommandLineLoad ||
+          entry->second.isCommandLineLoad == isCommandLineLoad)
+        return file;
+    }
 
-    auto *file = make<ArchiveFile>(std::move(archive));
-    if ((forceLoadArchive == ForceLoad::Default && config->allLoad) ||
+    if ((isCommandLineLoad && config->allLoad) ||
         forceLoadArchive == ForceLoad::Yes) {
       if (Optional<MemoryBufferRef> buffer = readFile(path)) {
         Error e = Error::success();
         for (const object::Archive::Child &c : file->getArchive().children(e)) {
           StringRef reason =
               forceLoadArchive == ForceLoad::Yes ? "-force_load" : "-all_load";
           if (Error e = file->fetch(c, reason))
             error(toString(file) + ": " + reason +
                   " failed to load archive member: " + toString(std::move(e)));
         }
         if (e)
           error(toString(file) +
                 ": Archive::children failed: " + toString(std::move(e)));
       }
-    } else if (forceLoadArchive == ForceLoad::Default &&
-               config->forceLoadObjC) {
+    } else if (isCommandLineLoad && config->forceLoadObjC) {
       for (const object::Archive::Symbol &sym : file->getArchive().symbols())
         if (sym.getName().startswith(objc::klass))
           file->fetch(sym);
 
       // TODO: no need to look for ObjC sections for a given archive member if
       // we already found that it contains an ObjC symbol.
       if (Optional<MemoryBufferRef> buffer = readFile(path)) {
         Error e = Error::success();
         for (const object::Archive::Child &c : file->getArchive().children(e)) {
           Expected<MemoryBufferRef> mb = c.getMemoryBufferRef();
           if (!mb || !hasObjCSection(*mb))
             continue;
           if (Error e = file->fetch(c, "-ObjC"))
             error(toString(file) + ": -ObjC failed to load archive member: " +
                   toString(std::move(e)));
         }
         if (e)
           error(toString(file) +
                 ": Archive::children failed: " + toString(std::move(e)));
       }
     }
 
     file->addLazySymbols();
-    newFile = loadedArchives[path] = file;
+    loadedArchives[path] = ArchiveFileInfo{file, isCommandLineLoad};
+    newFile = file;
     break;
   }
   case file_magic::macho_object:
     newFile = make<ObjFile>(mbref, getModTime(path), "", isLazy);
     break;
   case file_magic::macho_dynamically_linked_shared_lib:
   case file_magic::macho_dynamically_linked_shared_lib_stub:
   case file_magic::tapi_file:
if (config->printEachFile && magic != file_magic::archive && !isLazy)
       message(toString(newFile));
     inputFiles.insert(newFile);
   }
   return newFile;
 }
 
 static void addLibrary(StringRef name, bool isNeeded, bool isWeak,
                        bool isReexport, bool isExplicit,
-                       ForceLoad forceLoadArchive) {
+                       ForceLoad forceLoadArchive,
+                       bool isCommandLineLoad = true) {
   if (Optional<StringRef> path = findLibrary(name)) {
     if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
-            addFile(*path, forceLoadArchive, /*isLazy=*/false, isExplicit))) {
+            addFile(*path, forceLoadArchive, isCommandLineLoad,
+                    /*isLazy=*/false, isExplicit))) {
       if (isNeeded)
         dylibFile->forceNeeded = true;
       if (isWeak)
         dylibFile->forceWeakImport = true;
       if (isReexport) {
         config->hasReexports = true;
         dylibFile->reexport = true;
       }
     }
     return;
   }
   error("library not found for -l" + name);
 }
 
 static DenseSet<StringRef> loadedObjectFrameworks;
 static void addFramework(StringRef name, bool isNeeded, bool isWeak,
                          bool isReexport, bool isExplicit,
-                         ForceLoad forceLoadArchive) {
+                         ForceLoad forceLoadArchive,
+                         bool isCommandLineLoad = true) {
   if (Optional<StringRef> path = findFramework(name)) {
     if (loadedObjectFrameworks.contains(*path))
       return;
 
-    InputFile *file =
-        addFile(*path, forceLoadArchive, /*isLazy=*/false, isExplicit);
+    InputFile *file = addFile(*path, forceLoadArchive, isCommandLineLoad,
+                              /*isLazy=*/false, isExplicit, false);
     if (auto *dylibFile = dyn_cast_or_null<DylibFile>(file)) {
       if (isNeeded)
         dylibFile->forceNeeded = true;
       if (isWeak)
         dylibFile->forceWeakImport = true;
       if (isReexport) {
         config->hasReexports = true;
         dylibFile->reexport = true;
void macho::parseLCLinkerOption(InputFile *f, unsigned argc, StringRef data) {
 
   unsigned i = 0;
   StringRef arg = argv[i];
   if (arg.consume_front("-l")) {
     ForceLoad forceLoadArchive =
         config->forceLoadSwift && arg.startswith("swift") ? ForceLoad::Yes
                                                           : ForceLoad::No;
     addLibrary(arg, /*isNeeded=*/false, /*isWeak=*/false,
-               /*isReexport=*/false, /*isExplicit=*/false, forceLoadArchive);
+               /*isReexport=*/false, /*isExplicit=*/false, forceLoadArchive,
+               /*isCommandLineLoad=*/false);
   } else if (arg == "-framework") {
     StringRef name = argv[++i];
     addFramework(name, /*isNeeded=*/false, /*isWeak=*/false,
-                 /*isReexport=*/false, /*isExplicit=*/false, ForceLoad::No);
+                 /*isReexport=*/false, /*isExplicit=*/false, ForceLoad::No,
+                 /*isCommandLineLoad=*/false);
   } else {
     error(arg + " is not allowed in LC_LINKER_OPTION");
   }
 }
 
 static void addFileList(StringRef path, bool isLazy) {
   Optional<MemoryBufferRef> buffer = readFile(path);
   if (!buffer)
     return;
   MemoryBufferRef mbref = *buffer;
   for (StringRef path : args::getLines(mbref))
-    addFile(rerootPath(path), ForceLoad::Default, isLazy);
+    addFile(rerootPath(path), ForceLoad::Default, true, isLazy);
 }
 
 // We expect sub-library names of the form "libfoo", which will match a dylib
 // with a path of .*/libfoo.{dylib, tbd}.
 // XXX ld64 seems to ignore the extension entirely when matching sub-libraries;
 // I'm not sure what the use case for that is.
 static bool markReexport(StringRef searchName, ArrayRef<StringRef> extensions) {
   for (InputFile *file : inputFiles) {
static void createFiles(const InputArgList &args) {
   bool isLazy = false;
   for (const Arg *arg : args) {
     const Option &opt = arg->getOption();
     warnIfDeprecatedOption(opt);
     warnIfUnimplementedOption(opt);
 
     switch (opt.getID()) {
     case OPT_INPUT:
-      addFile(rerootPath(arg->getValue()), ForceLoad::Default, isLazy);
+      addFile(rerootPath(arg->getValue()), ForceLoad::Default, true, isLazy);
       break;
     case OPT_needed_library:
       if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
-              addFile(rerootPath(arg->getValue()), ForceLoad::Default)))
+              addFile(rerootPath(arg->getValue()), ForceLoad::Default, true)))
         dylibFile->forceNeeded = true;
       break;
     case OPT_reexport_library:
       if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
-              addFile(rerootPath(arg->getValue()), ForceLoad::Default))) {
+              addFile(rerootPath(arg->getValue()), ForceLoad::Default, true))) {
         config->hasReexports = true;
         dylibFile->reexport = true;
       }
       break;
     case OPT_weak_library:
       if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
-              addFile(rerootPath(arg->getValue()), ForceLoad::Default)))
+              addFile(rerootPath(arg->getValue()), ForceLoad::Default, true)))
         dylibFile->forceWeakImport = true;
       break;
     case OPT_filelist:
       addFileList(arg->getValue(), isLazy);
       break;
     case OPT_force_load:
-      addFile(rerootPath(arg->getValue()), ForceLoad::Yes);
+      addFile(rerootPath(arg->getValue()), ForceLoad::Yes, true);
       break;
     case OPT_l:
     case OPT_needed_l:
     case OPT_reexport_l:
     case OPT_weak_l:
       addLibrary(arg->getValue(), opt.getID() == OPT_needed_l,
                  opt.getID() == OPT_weak_l, opt.getID() == OPT_reexport_l,
                  /*isExplicit=*/true, ForceLoad::Default);
       break;
     case OPT_framework:
     case OPT_needed_framework:
     case OPT_reexport_framework:
config->printDylibSearch =
       args.hasArg(OPT_print_dylib_search) || getenv("RC_TRACE_DYLIB_SEARCHING");
   config->printEachFile = args.hasArg(OPT_t);
   config->printWhyLoad = args.hasArg(OPT_why_load);
   config->omitDebugInfo = args.hasArg(OPT_S);
   config->errorForArchMismatch = args.hasArg(OPT_arch_errors_fatal);
   if (const Arg *arg = args.getLastArg(OPT_bundle_loader)) {
     if (config->outputType != MH_BUNDLE)
       error("-bundle_loader can only be used with MachO bundle output");
-    addFile(arg->getValue(), ForceLoad::Default, /*isLazy=*/false,
+    addFile(arg->getValue(), ForceLoad::Default, true, /*isLazy=*/false,
             /*isExplicit=*/false,
             /*isBundleLoader=*/true);
   }
   if (const Arg *arg = args.getLastArg(OPT_umbrella)) {
     if (config->outputType != MH_DYLIB)
       warn("-umbrella used, but not creating dylib");
     config->umbrella = arg->getValue();
   }

lld/test/MachO/lc-linker-option.ll

 ; RUN: llvm-objdump --macho --syms %t/main | FileCheck %s --check-prefix=SYMS
 
 ;; Note that _OBJC_CLASS_$_TestClass is *not* included here.
 ; SYMS:       SYMBOL TABLE:
 ; SYMS-NEXT:  g     F __TEXT,__text _main
 ; SYMS-NEXT:  g     F __TEXT,__text __mh_execute_header
 ; SYMS-EMPTY:
 
+;; Make sure -all_load has effect when libraries are loaded via LC_LINKER_OPTION flags and explicitly passed as well
+; RUN: %lld -all_load %t/load-framework-foo.o %t/load-library-foo.o %t/main.o -o %t/main -F%t -L%t -lfoo
+; RUN: llvm-objdump --macho --syms %t/main | FileCheck %s --check-prefix=SYMS_ALL_LOAD
+
+;; Note that _OBJC_CLASS_$_TestClass is *included* here.
+; SYMS_ALL_LOAD:       SYMBOL TABLE:
+; SYMS_ALL_LOAD-NEXT:  g     F __TEXT,__text _main
+; SYMS_ALL_LOAD-NEXT:  g     O __DATA,__objc_data _OBJC_CLASS_$_TestClass
+; SYMS_ALL_LOAD-NEXT:  g     F __TEXT,__text __mh_execute_header
+; SYMS_ALL_LOAD-EMPTY:
+
+;; Make sure -force_load has effect when libraries are loaded via LC_LINKER_OPTION flags and explicitly passed as well
+; RUN: %lld %t/load-library-foo.o %t/main.o -o %t/main -F%t -L%t -force_load %t/libfoo.a
+; RUN: llvm-objdump --macho --syms %t/main | FileCheck %s --check-prefix=SYMS_FORCE_LOAD
+
+;; Note that _OBJC_CLASS_$_TestClass is *included* here.
+; SYMS_FORCE_LOAD:       SYMBOL TABLE:
+; SYMS_FORCE_LOAD-NEXT:  g     F __TEXT,__text _main
+; SYMS_FORCE_LOAD-NEXT:  g     O __DATA,__objc_data _OBJC_CLASS_$_TestClass
+; SYMS_FORCE_LOAD-NEXT:  g     F __TEXT,__text __mh_execute_header
+; SYMS_FORCE_LOAD-EMPTY:
+
+;; Make sure -ObjC has effect when frameworks are loaded via LC_LINKER_OPTION flags and explicitly passed as well
+; RUN: %lld -ObjC %t/load-framework-foo.o %t/load-library-foo.o %t/main.o -o %t/main -F%t -L%t -framework Foo
+; RUN: llvm-objdump --macho --syms %t/main | FileCheck %s --check-prefix=SYMS_OBJC_LOAD
+
+;; Note that _OBJC_CLASS_$_TestClass is *included* here.
+; SYMS_OBJC_LOAD:       SYMBOL TABLE:
+; SYMS_OBJC_LOAD-NEXT:  g     F __TEXT,__text _main
+; SYMS_OBJC_LOAD-NEXT:  g     O __DATA,__objc_data _OBJC_CLASS_$_TestClass
+; SYMS_OBJC_LOAD-NEXT:  g     F __TEXT,__text __mh_execute_header
+; SYMS_OBJC_LOAD-EMPTY:
+
 ;; Make sure that frameworks containing object files or bitcode instead of
 ;; dylibs or archives do not cause duplicate symbol errors
 ; RUN: mkdir -p %t/Foo.framework
 ; RUN: llc --filetype=obj %t/foo.ll -o %t/Foo.framework/Foo
 ; RUN: llc --filetype=obj %t/load-framework-twice.ll -o %t/main
 ;; Order of the object with the LC_LINKER_OPTION vs -framework arg is important.
 ; RUN: %lld %t/main -F %t -framework Foo -framework Foo -o /dev/null
 ; RUN: %lld -F %t -framework Foo -framework Foo %t/main -o /dev/null