[lld-macho] Avoid force-loading the same archive twice

[int3]

Summary

We need to dedup archive loads (similar to what we do for dylib
loads).

I noticed this issue after building some Swift stuff that used
-force_load_swift_libs, as it caused some Swift archives to be loaded
many times.

[int3]

tweak test

[MaskRay]

(So ld64 -force_load behaves similar to link.exe -wholearchive: where input filenames are (unfortunately) deduplicated. I like the ELF --whole-archive no-magic version...)

[thakis]

lld/test/MachO/force-load.s

23

Hm, should we have an explicit test for this? ld64 does seem to do realpath(), and we kind of want to do that at some point too. So it seems a bit strange to have a test preventing this.

(My local ld64 test:

% ln -s libfoo.a libbar.a
% clang mainfoo.o libfoo.a -force_load libfoo.a -force_load libfoo.a -lfoo -L . libbar.a ../llvm-project/libfoo.a

)

[thakis]

(I agree with MaskRay that the ELF behavior is nicer, but you solve the problems you have, not the ones you wish you had.)

[int3]

lld/test/MachO/force-load.s

23

What are the contents of libfoo.a and libbar.a here?

This force-load.s test passes for me when I swap out LLD for ld64. I also tried using an explicit symlink instead of a CWD-relative path, and that passes for me too (i.e. raises a duplicate symbol error.)

[MaskRay]

You may need a test using -force_load for different archives.

[MaskRay]

Also, have you checked how symlinks/hardlinks behave? lld-link uses sys::fs::getUniqueID.

[int3]

Yeah as mentioned in my inline comment, my experiments show that ld64 treats symlinks as distinct inputs. But @thakis seems to think its otherwise, so we'll see...

If symlinks are distinct, then hardlinks should be distinct too, and that seems to hold up. I'll update the diff to show my test cases, which pass both with LLD and ld64.

[int3]

test symlinks and hardlinks

[MaskRay]

lld/test/MachO/force-load.s

28

This needs to unsupport testing on Windows .

[int3]

lld/test/MachO/force-load.s

28

as the XXX comment above says, I'm not sure this is worth committing, especially since it doesn't allow us to run the test on Windows. I just uploaded it to show you and @thakis that ld64 behaves similarly. I think the relative path check on line 21 sufficiently demonstrates that realpath() isn't being called, and naturally so symlinks and hardlinks wouldn't get resolved either.

I don't feel strongly about this, so lmk if you would prefer these test cases to be committed.

[thakis]

lld/test/MachO/force-load.s

23

libfoo.a contained void foo() {} in compiled, and libbar.a is a symlink to libffo.a. (and mainfoo.o is void foo(); int main() { foo(); }) This links fine for me with ld64.

28

We treat them as distinct inputs, but ld64 apparently doesn't (didn't test hardlinks, just symlinks). And we want to realpath() eventually, so as I said I wouldn't add these tests.

[int3]

lld/test/MachO/force-load.s

23

(I meant to ask about mainfoo.o instead of libbar.a... thanks for noticing :p)

I think your example links fine because the -force_loads aren't being applied to two different paths. Having an archive duplicated multiple times on the command line isn't an error if it's not being force-loaded.

E.g. these fail with ld64:

clang mainfoo.o libfoo.a -force_load libfoo.a -force_load ../tmp/libfoo.a -isysroot $(xcrun -show-sdk-path)
clang mainfoo.o libfoo.a -force_load libfoo.a -force_load libbar.a -isysroot $(xcrun -show-sdk-path)

[int3]

remove use of ln; add test for force-loading multiple non-conflicting archives, and a test for non-force-loading multiple conflicting archives

[thakis]

lld/test/MachO/force-load.s

23

Ah derp, of course. Sorry 😳

[int3]

lld/test/MachO/force-load.s

23

No worries :)

[thakis]

Looks like this breaks tests on some windows bots: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8844046562894240432/+/u/package_clang/stdout?format=raw

$ ":" "RUN: at line 19"
$ "ld64.lld" "-arch" "x86_64" "-platform_version" "macos" "10.15" "11.0" "-syslibroot" "C:/b/s/w/ir/cache/builder/src/third_party/llvm/lld/test\MachO\Inputs\MacOSX.sdk" "-fatal_warnings" "-lSystem" "C:\b\s\w\ir\cache\builder\src\third_party\llvm-bootstrap\tools\lld\test\MachO\Output\force-load.s.tmp/foo.o" "-force_load" "C:\b\s\w\ir\cache\builder\src\third_party\llvm-bootstrap\tools\lld\test\MachO\Output\force-load.s.tmp/foo.a" "-force_load" "C:\b\s\w\ir\cache\builder\src\third_party\llvm-bootstrap\tools\lld\test\MachO\Output\force-load.s.tmp/foo.a" "C:\b\s\w\ir\cache\builder\src\third_party\llvm-bootstrap\tools\lld\test\MachO\Output\force-load.s.tmp/test.o" "-o" "/dev/null"
# command stderr:
ld64.lld: error: duplicate symbol: _bar

>>> defined in C:\b\s\w\ir\cache\builder\src\third_party\llvm-bootstrap\tools\lld\test\MachO\Output\force-load.s.tmp/foo.a(archive-foo.o)

>>> defined in C:\b\s\w\ir\cache\builder\src\third_party\llvm-bootstrap\tools\lld\test\MachO\Output\force-load.s.tmp/foo.a(archive-foo.o)

[thakis]

In D104353#2828411, @thakis wrote:

Looks like this breaks tests on some windows bots: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8844046562894240432/+/u/package_clang/stdout?format=raw

$ ":" "RUN: at line 19"
$ "ld64.lld" "-arch" "x86_64" "-platform_version" "macos" "10.15" "11.0" "-syslibroot" "C:/b/s/w/ir/cache/builder/src/third_party/llvm/lld/test\MachO\Inputs\MacOSX.sdk" "-fatal_warnings" "-lSystem" "C:\b\s\w\ir\cache\builder\src\third_party\llvm-bootstrap\tools\lld\test\MachO\Output\force-load.s.tmp/foo.o" "-force_load" "C:\b\s\w\ir\cache\builder\src\third_party\llvm-bootstrap\tools\lld\test\MachO\Output\force-load.s.tmp/foo.a" "-force_load" "C:\b\s\w\ir\cache\builder\src\third_party\llvm-bootstrap\tools\lld\test\MachO\Output\force-load.s.tmp/foo.a" "C:\b\s\w\ir\cache\builder\src\third_party\llvm-bootstrap\tools\lld\test\MachO\Output\force-load.s.tmp/test.o" "-o" "/dev/null"
# command stderr:
ld64.lld: error: duplicate symbol: _bar

>>> defined in C:\b\s\w\ir\cache\builder\src\third_party\llvm-bootstrap\tools\lld\test\MachO\Output\force-load.s.tmp/foo.a(archive-foo.o)

>>> defined in C:\b\s\w\ir\cache\builder\src\third_party\llvm-bootstrap\tools\lld\test\MachO\Output\force-load.s.tmp/foo.a(archive-foo.o)

Hm, but that didn't happen in our nightly build last night. So I guess something else broke the test since then?

[thakis]

lld/test/MachO/force-load.s

26

Hm, there's no FileCheck line with --check-prefix=DUP…

[thakis]

lld/MachO/Driver.cpp

268

I don't know if that's what's happening, but if loadArchiveMember() loads a .o file that contains LC_LINKER_OPTIONS that has a -l flag that can recursively call addFile() again which can resize loadedArchives and invalidate the cachedFile ref.

[thakis]

lld/MachO/Driver.cpp

268

Reverted in c9b241efd68c for now to green up bots. This sounds like a plausible theory to me.

[thakis]

lld/MachO/Driver.cpp

268

Reverted in c9b241efd68c for now to green up bots. This sounds like a plausible theory to me.

Not sure why it's only on that bot, but the test failed 3 times in succession:

https://ci.chromium.org/ui/p/chromium/builders/try/win_upload_clang/1608/overview
https://ci.chromium.org/ui/p/chromium/builders/try/win_upload_clang/1609/overview
https://ci.chromium.org/ui/p/chromium/builders/try/win_upload_clang/1610/overview

But after the revert this test is happy:

https://ci.chromium.org/ui/p/chromium/builders/try/win_upload_clang/1611/overview

So it looks like the revert helped.

[int3]

The real culprit is https://github.com/llvm/llvm-project/commit/1d31fb8d122b1117cf20a9edc09812db8472e930, which I (unfortunately) thought simple enough to be landed without review. And I only landed it today, which explains why the breakage only happened today.

The issue is that it made rerootPath return a temporary std::string instead of an interned string, which then gets passed to addFile as a StringRef, so it gets prematurely freed.

I'll back out the std::string change and re-land this force-load diff.

lld/test/MachO/force-load.s

26

whoops. I'll fix it as part of the re-land.

[thakis]

lld/MachO/Driver.cpp

268

I don't know if that's what's happening, but if loadArchiveMember() loads a .o file that contains LC_LINKER_OPTIONS that has a -l flag that can recursively call addFile() again which can resize loadedArchives and invalidate the cachedFile ref.

I see you removed the ref in the reland. For completeness, here's a test case to for this. It doesn't trigger a crash even with asan with the first iteration of this patch, but it does cause a recursive call of this function and things only work by luck. (But it's addressed in the reland.)

% cat g.cc
void g() {}
% clang -c g.cc
% libtool -static -o libfoo.a g.o
% cat autolink_libfoo.cc
asm(".linker_option \"-lfoo\"");
void g();
void f() { g(); }
% clang -c autolink_libfoo.cc
% libtool -static -o libbar.a autolink_libfoo.o
% cat main_f.cc
void g();
int main() { g(); }
% clang -c main_f.cc
% out/gn/bin/ld64.lld -dynamic -arch x86_64 main_f.o -force_load libbar.a -platform_version macos 10.15.0 10.15.0 -o a.out -L. -lSystem

lld/MachO/Driver.cpp

static std::vector<ArchiveMember> getArchiveMembers(MemoryBufferRef mb) {
   }
   if (err)
     fatal(mb.getBufferIdentifier() +
           ": Archive::children failed: " + toString(std::move(err)));
 
   return v;
 }
 
+static DenseMap<StringRef, ArchiveFile *> loadedArchives;
+
 static InputFile *addFile(StringRef path, bool 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());
   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.
+    ArchiveFile *&cachedFile = loadedArchives[path];
+    if (cachedFile)
+      return cachedFile;
+
     std::unique_ptr<object::Archive> file = CHECK(
         object::Archive::create(mbref), path + ": failed to parse archive");
 
     if (!file->isEmpty() && !file->hasSymbolTable())
       error(path + ": archive has no index; run ranlib to add one");
 
     if (config->allLoad || forceLoadArchive) {
       if (Optional<MemoryBufferRef> buffer = readFile(path)) {
         for (const ArchiveMember &member : getArchiveMembers(*buffer)) {
           if (Optional<InputFile *> file = loadArchiveMember(
                   member.mbref, member.modTime, path, /*objCOnly=*/false)) {
             inputFiles.insert(*file);
             printArchiveMemberLoad(
                 (forceLoadArchive ? "-force_load" : "-all_load"),
                 inputFiles.back());
           }
         }
       }
if (config->allLoad || forceLoadArchive) {
                   member.mbref, member.modTime, path, /*objCOnly=*/true)) {
             inputFiles.insert(*file);
             printArchiveMemberLoad("-ObjC", inputFiles.back());
           }
         }
       }
     }
 
-    newFile = make<ArchiveFile>(std::move(file));
+    newFile = cachedFile = make<ArchiveFile>(std::move(file));
     break;
   }
   case file_magic::macho_object:
     newFile = make<ObjFile>(mbref, getModTime(path), "");
     break;
   case file_magic::macho_dynamically_linked_shared_lib:
   case file_magic::macho_dynamically_linked_shared_lib_stub:
   case file_magic::tapi_file:

lld/test/MachO/archive.s

 
 # RUN: %lld %t/test.a %t/main.o -o %t/all-load -all_load
 # RUN: llvm-nm %t/all-load | FileCheck %s --check-prefix ALL-LOAD
 # ALL-LOAD: T _bar
 # ALL-LOAD: T _boo
 # ALL-LOAD: T _main
 # ALL-LOAD: T _unused
 
+## Multiple archives defining the same symbols aren't an issue, due to lazy
+## loading
+# RUN: cp %t/test.a %t/test2.a
+# RUN: %lld %t/test.a %t/test2.a %t/main.o -o /dev/null
+
 #--- 2.s
 .globl _boo
 _boo:
   ret
 
 #--- 3.s
 .globl _bar
 _bar:

lld/test/MachO/force-load.s

 # REQUIRES: x86
 # RUN: rm -rf %t; split-file %s %t
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/archive-foo.s -o %t/archive-foo.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/archive-baz.s -o %t/archive-baz.o
 # RUN: llvm-ar rcs %t/foo.a %t/archive-foo.o
+# RUN: llvm-ar rcs %t/baz.a %t/archive-baz.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/foo.s -o %t/foo.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/test.s -o %t/test.o
 
-# RUN: %lld -force_load %t/foo.a %t/foo.o %t/test.o -o %t/test-force-load-first
+# RUN: %lld -lSystem -force_load %t/foo.a %t/foo.o %t/test.o -o %t/test-force-load-first
 # FORCE-LOAD-FIRST:  __TEXT,archive _foo
 # RUN: llvm-objdump --syms %t/test-force-load-first | FileCheck %s --check-prefix=FORCE-LOAD-FIRST
 
-# RUN: %lld %t/foo.o -force_load %t/foo.a %t/test.o -o %t/test-force-load-second
+# RUN: %lld %t/foo.o -lSystem -force_load %t/foo.a %t/test.o -o %t/test-force-load-second
 # RUN: llvm-objdump --syms %t/test-force-load-second | FileCheck %s --check-prefix=FORCE-LOAD-SECOND
 # FORCE-LOAD-SECOND: __TEXT,obj _foo
 
+## Force-loading the same path twice is fine
+# RUN: %lld -lSystem %t/foo.o -force_load %t/foo.a -force_load %t/foo.a %t/test.o -o /dev/null
+
+## Note that we do not call realpath() before dedup'ing the force-load
+## arguments, so this is an error.
+# RUN: cd %t; not %lld -lSystem %t/foo.o -force_load %t/foo.a -force_load foo.a \
+# RUN:   %t/test.o -o /dev/null 2>&1
+
+# DUP: error: duplicate symbol: _bar
+
+## Force-loading two different paths w/o conflicting symbols is fine
+# RUN: %lld -lSystem -force_load %t/foo.a -force_load %t/baz.a %t/test.o -o %t/test-two-force-loads
+# RUN: llvm-objdump --syms %t/test-two-force-loads | FileCheck %s --check-prefix=TWICE
+# TWICE-DAG: __TEXT,archive _foo
+# TWICE-DAG: __TEXT,archive _bar
+# TWICE-DAG: __TEXT,archive _baz
+
 #--- archive-foo.s
 .section __TEXT,archive
-.globl _foo
+.globl _foo, _bar
 .weak_definition _foo
 _foo:
+_bar:
+
+#--- archive-baz.s
+.section __TEXT,archive
+.globl _baz
+_baz:
 
 #--- foo.s
 .section __TEXT,obj
 .globl _foo
 .weak_definition _foo
 _foo:
 
 #--- test.s
 .globl _main
 _main:
   ret