[lld-macho] Fix an issue where Objective-C symbols where not force-loaded due to LC_LINKER_OPTION

[tt]

Summary

Resolves issue https://github.com/llvm/llvm-project/issues/56059

[int3]

Can you make the commit message more detailed? It's 1) nice not to make people go to GH issues to understand the changes 2) the description in the issue isn't super clear as-is (I understood the diff better after looking at the test case)

While refreshing my memory of how -force_load worked, I realized we lacked a test case covering what happens if a library is loaded as a regular (Default) load and then subsequently force-loaded. It looks like in that case ld64 doesn't force-load it (so our current implementation is behaving correctly in that regard). I'll put up a diff with those tests in a bit.

lld/MachO/Driver.cpp

250–251

how about having a single map of StringRef -> {ForceLoad, ArchiveFile *}, and then perform a load / update the map only if the existing entry has ForceLoad::No but the new load is ForceLoad::Yes or Default?

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

81

explicitly

95

explicitly

106

can we add a test case for the -force_load + LC_LINKER_OPTION combination?

[int3]

Superseded by D129556: [lld-macho] Fix loading same libraries from both LC_LINKER_OPTION and command line

[thakis]

this can be closed, something else landed elsewhere

[smeenai]

Placing back in author's queue to abandon

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;
+static DenseMap<StringRef, ArchiveFile *> lazyLoadedArchives;
+static DenseMap<StringRef, ArchiveFile *> forceLoadedArchives;
 
 static InputFile *addFile(StringRef path, ForceLoad forceLoadArchive,
                           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())
+
+    if (forceLoadArchive == ForceLoad::No) {
+      auto entry = lazyLoadedArchives.find(path);
+      if (entry != lazyLoadedArchives.end()) {
         return entry->second;
+      }
+    } else {
+      auto forceLoadedEntry = forceLoadedArchives.find(path);
+      if (forceLoadedEntry != forceLoadedArchives.end()) {
+        return forceLoadedEntry->second;
+      }
+    }
 
     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");
 
     auto *file = make<ArchiveFile>(std::move(archive));
     if ((forceLoadArchive == ForceLoad::Default && 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)));
       }
+
+      forceLoadedArchives[path] = file;
+
     } else if (forceLoadArchive == ForceLoad::Default &&
                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)));
       }
+
+      forceLoadedArchives[path] = file;
     }
 
     file->addLazySymbols();
-    newFile = loadedArchives[path] = file;
+    newFile = lazyLoadedArchives[path] = 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:
bool macho::link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
   ctx->e.initialize(stdoutOS, stderrOS, exitEarly, disableOutput);
   ctx->e.cleanupCallback = []() {
     resolvedFrameworks.clear();
     resolvedLibraries.clear();
     cachedReads.clear();
     concatOutputSections.clear();
     inputFiles.clear();
     inputSections.clear();
-    loadedArchives.clear();
+    forceLoadedArchives.clear();
+    lazyLoadedArchives.clear();
     loadedObjectFrameworks.clear();
     syntheticSections.clear();
     thunkMap.clear();
 
     firstTLVDataSection = nullptr;
     tar = nullptr;
     memset(&in, 0, sizeof(in));
 

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 explicily passed as well
+; RUN: %lld -all_load %t/load-framework-foo.o %t/load-library-foo.o \
+; RUN:   %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 -ObjC has effect when frameworks are loaded via LC_LINKER_OPTION flags
+;; and explicily passed as well
+; RUN: %lld -ObjC %t/load-framework-foo.o %t/load-library-foo.o \
+; RUN:   %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