[lld-macho] -all_load and -ObjC should not affect LC_LINKER_OPTION flags

[int3]

Summary

In particular, they should not cause archives to be eagerly loaded. This
matches ld64's behavior.

Fixes PR52246.

[int3]

comments

[thakis]

Nice!

lld/MachO/Driver.cpp

271–272

nit: I'd s/!= ForceLoad::No/s == ForceLoad::Default/ since then the reader doesn't have to read line 257 to realize that ::Yes never comes here and hence is uninteresting

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

66

nit: I'd find "have no effect on libraries loaded via LC_LINKER_OPTION flags" clearer

68

nit: it's a bit weird to call a static library .dylib -- why not create a real dylib with lld? (up to you though)

[int3]

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

68

whoops, this was a brain fart

68

should have just been .a

lld/MachO/Config.h

 struct SymbolPriorityEntry {
   // The priority given to a matching symbol, regardless of which object file
   // it originated from.
   size_t anyObjectFile = 0;
   // The priority given to a matching symbol from a particular object file.
   llvm::DenseMap<llvm::StringRef, size_t> objectFiles;
 };
 
+// Whether to force-load an archive.
+enum class ForceLoad {
+  Default, // Apply -all_load or -ObjC behaviors if those flags are enabled
+  Yes,     // Always load the archive, regardless of other flags
+  No,      // Never load the archive, regardless of other flags
+};
+
 extern Configuration *config;
 
 } // namespace macho
 } // namespace lld
 
 #endif

lld/MachO/Driver.cpp

case OPT_max_relative_cache_size_lto:
       break;
     }
   }
   return CHECK(parseCachePruningPolicy(ltoPolicy), "invalid LTO cache policy");
 }
 
 static DenseMap<StringRef, ArchiveFile *> loadedArchives;
 
-static InputFile *addFile(StringRef path, bool forceLoadArchive,
+static InputFile *addFile(StringRef path, ForceLoad forceLoadArchive,
                           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());
case file_magic::archive: {
 
     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 (config->allLoad || forceLoadArchive) {
+    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 ? "-force_load" : "-all_load";
+          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 (config->forceLoadObjC) {
+    } 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();
if (newFile && !isa<DylibFile>(newFile)) {
     if (config->printEachFile && magic != file_magic::archive)
       message(toString(newFile));
     inputFiles.insert(newFile);
   }
   return newFile;
 }
 
 static void addLibrary(StringRef name, bool isNeeded, bool isWeak,
-                       bool isReexport, bool isExplicit, bool forceLoad) {
+                       bool isReexport, bool isExplicit,
+                       ForceLoad forceLoadArchive) {
   if (Optional<StringRef> path = findLibrary(name)) {
     if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
-            addFile(*path, forceLoad, isExplicit))) {
+            addFile(*path, forceLoadArchive, 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 void addFramework(StringRef name, bool isNeeded, bool isWeak,
-                         bool isReexport, bool isExplicit) {
+                         bool isReexport, bool isExplicit,
+                         ForceLoad forceLoadArchive) {
   if (Optional<StringRef> path = findFramework(name)) {
     if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
-            addFile(*path, /*forceLoadArchive=*/false, isExplicit))) {
+            addFile(*path, forceLoadArchive, isExplicit))) {
       if (isNeeded)
         dylibFile->forceNeeded = true;
       if (isWeak)
         dylibFile->forceWeakImport = true;
       if (isReexport) {
         config->hasReexports = true;
         dylibFile->reexport = true;
       }
if (missingCount)
     fatal(Twine(args.getArgString(missingIndex)) + ": missing argument");
   for (const Arg *arg : args.filtered(OPT_UNKNOWN))
     error("unknown argument: " + arg->getAsString(args));
 
   for (const Arg *arg : args) {
     switch (arg->getOption().getID()) {
     case OPT_l: {
       StringRef name = arg->getValue();
-      bool forceLoad =
-          config->forceLoadSwift ? name.startswith("swift") : false;
+      ForceLoad forceLoadArchive =
+          config->forceLoadSwift && name.startswith("swift") ? ForceLoad::Yes
+                                                             : ForceLoad::No;
       addLibrary(name, /*isNeeded=*/false, /*isWeak=*/false,
-                 /*isReexport=*/false, /*isExplicit=*/false, forceLoad);
+                 /*isReexport=*/false, /*isExplicit=*/false, forceLoadArchive);
       break;
     }
     case OPT_framework:
       addFramework(arg->getValue(), /*isNeeded=*/false, /*isWeak=*/false,
-                   /*isReexport=*/false, /*isExplicit=*/false);
+                   /*isReexport=*/false, /*isExplicit=*/false, ForceLoad::No);
       break;
     default:
       error(arg->getSpelling() + " is not allowed in LC_LINKER_OPTION");
     }
   }
 }
 
 static void addFileList(StringRef path) {
   Optional<MemoryBufferRef> buffer = readFile(path);
   if (!buffer)
     return;
   MemoryBufferRef mbref = *buffer;
   for (StringRef path : args::getLines(mbref))
-    addFile(rerootPath(path), /*forceLoadArchive=*/false);
+    addFile(rerootPath(path), ForceLoad::Default);
 }
 
 // An order file has one entry per line, in the following format:
 //
 //   <cpu>:<object file>:<symbol name>
 //
 // <cpu> and <object file> are optional. If not specified, then that entry
 // matches any symbol of that name. Parsing this format is not quite
static void createFiles(const InputArgList &args) {
   // Other options should be handled via filtered() and/or getLastArg().
   for (const Arg *arg : args) {
     const Option &opt = arg->getOption();
     warnIfDeprecatedOption(opt);
     warnIfUnimplementedOption(opt);
 
     switch (opt.getID()) {
     case OPT_INPUT:
-      addFile(rerootPath(arg->getValue()), /*forceLoadArchive=*/false);
+      addFile(rerootPath(arg->getValue()), ForceLoad::Default);
       break;
     case OPT_needed_library:
       if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
-              addFile(rerootPath(arg->getValue()), false)))
+              addFile(rerootPath(arg->getValue()), ForceLoad::Default)))
         dylibFile->forceNeeded = true;
       break;
     case OPT_reexport_library:
-      if (auto *dylibFile = dyn_cast_or_null<DylibFile>(addFile(
-              rerootPath(arg->getValue()), /*forceLoadArchive=*/false))) {
+      if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
+              addFile(rerootPath(arg->getValue()), ForceLoad::Default))) {
         config->hasReexports = true;
         dylibFile->reexport = true;
       }
       break;
     case OPT_weak_library:
       if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
-              addFile(rerootPath(arg->getValue()), /*forceLoadArchive=*/false)))
+              addFile(rerootPath(arg->getValue()), ForceLoad::Default)))
         dylibFile->forceWeakImport = true;
       break;
     case OPT_filelist:
       addFileList(arg->getValue());
       break;
     case OPT_force_load:
-      addFile(rerootPath(arg->getValue()), /*forceLoadArchive=*/true);
+      addFile(rerootPath(arg->getValue()), ForceLoad::Yes);
       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=*/false);
+                 /*isExplicit=*/true, ForceLoad::Default);
       break;
     case OPT_framework:
     case OPT_needed_framework:
     case OPT_reexport_framework:
     case OPT_weak_framework:
       addFramework(arg->getValue(), opt.getID() == OPT_needed_framework,
                    opt.getID() == OPT_weak_framework,
-                   opt.getID() == OPT_reexport_framework, /*isExplicit=*/true);
+                   opt.getID() == OPT_reexport_framework, /*isExplicit=*/true,
+                   ForceLoad::Default);
       break;
     default:
       break;
     }
   }
 }
 
 static void gatherInputSections() {
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->outputType = getOutputType(args);
   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(), /*forceLoadArchive=*/false, /*isExplicit=*/false,
+    addFile(arg->getValue(), ForceLoad::Default, /*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();
   }
   config->ltoObjPath = args.getLastArgValue(OPT_object_path_lto);

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

 ;; D111706). Prior to the fix, this would trigger a heap-use-after-free when run
 ;; under ASAN.
 ; RUN: llc %t/foo.ll -o %t/foo.o -filetype=obj
 ; RUN: mkdir -p %t/Foo.framework
 ;; In a proper framework, this is technically supposed to be a symlink to the
 ;; actual archive at Foo.framework/Versions/Current, but we skip that here so
 ;; that this test can run on Windows.
 ; RUN: llvm-ar rcs %t/Foo.framework/Foo %t/foo.o
-; RUN: llc %t/objfile1.ll -o %t/objfile1.o -filetype=obj
+; RUN: llc %t/load-framework-foo.ll -o %t/load-framework-foo.o -filetype=obj
 ; RUN: llc %t/main.ll -o %t/main.o -filetype=obj
-; RUN: %lld %t/objfile1.o %t/main.o -o /dev/null -F%t
+; RUN: %lld %t/load-framework-foo.o %t/main.o -o %t/main -F%t
+; RUN: llvm-objdump --macho --syms %t/main | FileCheck %s --check-prefix=SYMS
+
+;; Make sure -all_load and -ObjC have no effect on libraries loaded via
+;; LC_LINKER_OPTION flags.
+; RUN: llc %t/load-library-foo.ll -o %t/load-library-foo.o -filetype=obj
+; RUN: llvm-ar rcs %t/libfoo.a %t/foo.o
+; RUN: %lld -all_load -ObjC %t/load-framework-foo.o %t/load-library-foo.o \
+; RUN:   %t/main.o -o %t/main -F%t -L%t
+; 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:
 
 ;--- framework.ll
 target triple = "x86_64-apple-macosx10.15.0"
 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 !0 = !{!"-framework", !"CoreFoundation"}
 !llvm.linker.options = !{!0}
 

 
 !0 = !{!"-why_load"}
 !llvm.linker.options = !{!0}
 
 define void @main() {
   ret void
 }
 
-;--- objfile1.ll
+;--- load-framework-foo.ll
 target triple = "x86_64-apple-macosx10.15.0"
 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 !0 = !{!"-framework", !"Foo"}
 !llvm.linker.options = !{!0}
 
+;--- load-library-foo.ll
+target triple = "x86_64-apple-macosx10.15.0"
+target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+
+!0 = !{!"-lfoo"}
+!llvm.linker.options = !{!0}
+
 ;--- main.ll
 target triple = "x86_64-apple-macosx10.15.0"
 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 define void @main() {
   ret void
 }
 
 !0 = !{!"-framework", !"Foo"}
 !llvm.linker.options = !{!0}
 
 ;--- foo.ll
 target triple = "x86_64-apple-macosx10.15.0"
 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
-define void @foo() {
-  ret void
-}
+%struct._class_t = type {}
+@"OBJC_CLASS_$_TestClass" = global %struct._class_t {}, section "__DATA, __objc_data", align 8