[lld/mac] Add reexports after reexporter to inputFiles

[thakis]

Summary

When a library "host"'s reexports change their installName with $ld$os10.11$install_name$host,
we used to write a load command for "host" but write the version numbers of the reexport
instead of "host". This fixes that.

I first thought that the rule is to take the version numbers from the library that originally had
that install name (implemented in D103819), but that's not what ld64 seems to be doing: It
takes the version number from the first dylib with that install name it loads, and it loads
the reexporting library before the reexports. We already did most of that, we just added
reexports before the reexporter. After this change, we add the reexporter before the reexports.

Addresses https://bugs.llvm.org/show_bug.cgi?id=49800#c11 part 1.

(ld64 seems to add reexports after processing _all_ files on the command line, while we add
them right after the reexporter. For the common case of reexport + $ld$ symbol changing back
to the exporter name, this doesn't make a difference, but you can construct a case where it
does. I expect this to not make a difference in practice though.)

[int3]

We already did most of that, we just added reexports before the reexporter.

Hmm I thought we avoided adding re-exports to the inputFiles SetVector unless isImplicitlyLinked() returned true for them in loadReexport(). How does this change work with that? Perhaps after D103430 we should always insert re-exports into inputFiles and set the explicitlyLinked flag accordingly?

As an aside, I would love for us to clean up the inputFile insertion / printEachFile logic, which AFAICT should always happen together for ObjFiles and DylibFiles. Right now some of them happen inside the ctors and some outside... I think we should be consistent about this. Can be done in a separate diff though (and I'm happy to do it myself).

Aside 2: isImplicitlyLinked is a rather confusing name (though ld64 uses "implicit" in the same sense), given that this comment https://github.com/llvm/llvm-project/blob/main/lld/MachO/Writer.cpp#L688 uses implicit/explicit to mean the reverse. We should probably rename/rephrase either one...

[thakis]

In D103821#2803575, @int3 wrote:

We already did most of that, we just added reexports before the reexporter.

Hmm I thought we avoided adding re-exports to the inputFiles SetVector unless isImplicitlyLinked() returned true for them in loadReexport(). How does this change work with that?

I think it happens to work in practice since libraries in the SDK reexport other libraries also in the SDK and they happen to share the same prefix.

Perhaps after D103430 we should always insert re-exports into inputFiles and set the explicitlyLinked flag accordingly?

Maybe!

As an aside, I would love for us to clean up the inputFile insertion / printEachFile logic, which AFAICT should always happen together for ObjFiles and DylibFiles. Right now some of them happen inside the ctors and some outside... I think we should be consistent about this. Can be done in a separate diff though (and I'm happy to do it myself).

The difference is that DylibFiles can trigger additional loads but ObjFiles can't (…or only via LC_LINK_COMMAND and it doesn't matter how those are ordered relative to the ObjFile, while DylibFile order matters). Making ObjFiles use the DylibFile approach sounds fine to me at first sight, but that's why DylibFiles currently are different.

Aside 2: isImplicitlyLinked is a rather confusing name (though ld64 uses "implicit" in the same sense), given that this comment https://github.com/llvm/llvm-project/blob/main/lld/MachO/Writer.cpp#L688 uses implicit/explicit to mean the reverse. We should probably rename/rephrase either one...

[thakis]

Hm I think I misunderstood the isImplicitlyLinked question. Let me take another look at that…

[thakis]

remove inputFiles.insert() in loadReexport()

[thakis]

Ok, I removed the inputFiles.insert(reexport) in loadReexport() which after adding in the ctor is no longer needed. (It's also not harmful thanks to the ordinal deduper and dead dylib stripper in Writer).

I think we still need exportingFile (and isImplicitlyLinked for it): We want to resolve against the reexport, but when targeting an older version then either:

1. the reexport is unused and now auto-stripped
2. it assumes the reexporter's install name via an $ld$ magic symbol

and it's then removed due to the code in Writer that merges ordinals.

Does that make sense?

[int3]

Makes sense (I think!) -- if I understand correctly, for the implicit linking stuff, by setting the exportingFile to point to an implicitly-linked dylib, we ensure that DylibFile::isReferenced() returns true since the symbols are now registered as belonging to that dylib, thus making the dead dylib stripping logic in Writer.cpp work.

[thakis]

Thanks!

In D103821#2803804, @int3 wrote:

Makes sense (I think!) -- if I understand correctly, for the implicit linking stuff, by setting the exportingFile to point to an implicitly-linked dylib, we ensure that DylibFile::isReferenced() returns true since the symbols are now registered as belonging to that dylib, thus making the dead dylib stripping logic in Writer.cpp work.

Right, that's my understanding too :) I think this is already covered by lld/test/MachO/implicit-dylib.s if I read that test right.

[thakis]

lld/MachO/InputFiles.cpp

874

One subtlety (mostly independent of this patch) here is that isImplicitlyLinked() sees the installName before it might have been modified by $ld$ symbols. But since those usually don't change the directory that should be fine in practice.

[stella.stamenova]

In the last week or so I've seen one of these tests fail occasionally with ACCESS_VIOLATION in our testing. Unfortunately, I can't get a local repro. Here's the (best) log I can get from our online testing:

$ ":" "RUN: at line 3"
$ "rm" "-rf" "D:\a\_work\1\b\llvm\Debug\tools\lld\test\MachO\Output\special-symbol-ld-install-name.s.tmp"
$ "split-file" "--no-leading-lines" "D:\a\_work\1\s\lld\test\MachO\special-symbol-ld-install-name.s" "D:\a\_work\1\b\llvm\Debug\tools\lld\test\MachO\Output\special-symbol-ld-install-name.s.tmp"
$ ":" "RUN: at line 5"
$ "d:\a\_work\1\b\llvm\debug\bin\llvm-mc.exe" "-filetype=obj" "-triple=x86_64-apple-darwin" "D:\a\_work\1\b\llvm\Debug\tools\lld\test\MachO\Output\special-symbol-ld-install-name.s.tmp/foo.s" "-o" "D:\a\_work\1\b\llvm\Debug\tools\lld\test\MachO\Output\special-symbol-ld-install-name.s.tmp/foo.o"
$ ":" "RUN: at line 10"
$ "ld64.lld" "-arch" "x86_64" "-platform_version" "macos" "10.15" "11.0" "-syslibroot" "D:/a/_work/1/s/lld/test\MachO\Inputs\MacOSX.sdk" "-fatal_warnings" "-o" "D:\a\_work\1\b\llvm\Debug\tools\lld\test\MachO\Output\special-symbol-ld-install-name.s.tmp/libfoo1.dylib" "D:\a\_work\1\b\llvm\Debug\tools\lld\test\MachO\Output\special-symbol-ld-install-name.s.tmp/libLDInstallName.tbd" "D:\a\_work\1\b\llvm\Debug\tools\lld\test\MachO\Output\special-symbol-ld-install-name.s.tmp/foo.o" "-dylib" "-platform_version" "macos" "11.0.0" "11.0.0"
$ ":" "RUN: at line 11"
$ "llvm-otool" "-L" "D:\a\_work\1\b\llvm\Debug\tools\lld\test\MachO\Output\special-symbol-ld-install-name.s.tmp/libfoo1.dylib"
$ "d:\a\_work\1\b\llvm\debug\bin\filecheck.exe" "--check-prefix=CASE1" "D:\a\_work\1\s\lld\test\MachO\special-symbol-ld-install-name.s"
$ ":" "RUN: at line 17"
$ "ld64.lld" "-arch" "x86_64" "-platform_version" "macos" "10.15" "11.0" "-syslibroot" "D:/a/_work/1/s/lld/test\MachO\Inputs\MacOSX.sdk" "-fatal_warnings" "-o" "D:\a\_work\1\b\llvm\Debug\tools\lld\test\MachO\Output\special-symbol-ld-install-name.s.tmp/libfoo2.dylib" "D:\a\_work\1\b\llvm\Debug\tools\lld\test\MachO\Output\special-symbol-ld-install-name.s.tmp/libLDInstallName.tbd" "D:\a\_work\1\b\llvm\Debug\tools\lld\test\MachO\Output\special-symbol-ld-install-name.s.tmp/foo.o" "-dylib" "-platform_version" "macos" "12.0.0" "12.0.0"
$ ":" "RUN: at line 18"
$ "llvm-otool" "-L" "D:\a\_work\1\b\llvm\Debug\tools\lld\test\MachO\Output\special-symbol-ld-install-name.s.tmp/libfoo2.dylib"
$ "d:\a\_work\1\b\llvm\debug\bin\filecheck.exe" "--check-prefix=CASE2" "D:\a\_work\1\s\lld\test\MachO\special-symbol-ld-install-name.s"
$ ":" "RUN: at line 23"
$ "ld64.lld" "-arch" "x86_64" "-platform_version" "macos" "10.15" "11.0" "-syslibroot" "D:/a/_work/1/s/lld/test\MachO\Inputs\MacOSX.sdk" "-o" "D:\a\_work\1\b\llvm\Debug\tools\lld\test\MachO\Output\special-symbol-ld-install-name.s.tmp/libfoo3.dylib" "D:\a\_work\1\b\llvm\Debug\tools\lld\test\MachO\Output\special-symbol-ld-install-name.s.tmp/libLDInstallNameInvalid.tbd" "D:\a\_work\1\b\llvm\Debug\tools\lld\test\MachO\Output\special-symbol-ld-install-name.s.tmp/foo.o" "-dylib" "-platform_version" "macos" "11.0.0" "11.0.0"
note: command had no output on stdout or stderr
error: command failed with exit status: 3221225477
$ "d:\a\_work\1\b\llvm\debug\bin\filecheck.exe" "--check-prefix=INVALID-VERSION" "D:\a\_work\1\s\lld\test\MachO\special-symbol-ld-install-name.s"

lld/MachO/Driver.cpp

case file_magic::macho_bundle:
     if (!isBundleLoader)
       error(path + ": unhandled file type");
     if (DylibFile *dylibFile = loadDylib(mbref, nullptr, isBundleLoader))
       newFile = dylibFile;
     break;
   default:
     error(path + ": unhandled file type");
   }
-  if (newFile) {
+  if (newFile && !isa<DylibFile>(newFile)) {
     // printArchiveMemberLoad() prints both .a and .o names, so no need to
     // print the .a name here.
-    if (config->printEachFile && magic != file_magic::archive &&
-        !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) {

lld/MachO/InputFiles.cpp

if (const load_command *cmd = findCommand(hdr, LC_ID_DYLIB)) {
     // macho_executable and macho_bundle don't have LC_ID_DYLIB,
     // so it's OK.
     error("dylib " + toString(this) + " missing LC_ID_DYLIB load command");
     return;
   }
 
   if (config->printEachFile)
     message(toString(this));
+  inputFiles.insert(this);
 
   deadStrippable = hdr->flags & MH_DEAD_STRIPPABLE_DYLIB;
 
   if (!checkCompatibility(this))
     return;
 
   for (auto *cmd : findCommands<rpath_command>(hdr, LC_RPATH)) {
     StringRef rpath{reinterpret_cast<const char *>(cmd) + cmd->path};
     rpaths.push_back(rpath);
   }
 
   // Initialize symbols.
   exportingFile = isImplicitlyLinked(installName) ? this : this->umbrella;
   if (const load_command *cmd = findCommand(hdr, LC_DYLD_INFO_ONLY)) {
     auto *c = reinterpret_cast<const dyld_info_command *>(cmd);
     parseTrie(buf + c->export_off, c->export_size,
               [&](const Twine &name, uint64_t flags) {
                 StringRef savedName = saver.save(name);
                 if (handleLDSymbol(savedName))
                   return;
                 bool isWeakDef = flags & EXPORT_SYMBOL_FLAGS_WEAK_DEFINITION;
DylibFile::DylibFile(const InterfaceFile &interface, DylibFile *umbrella,
   this->umbrella = umbrella;
 
   installName = saver.save(interface.getInstallName());
   compatibilityVersion = interface.getCompatibilityVersion().rawValue();
   currentVersion = interface.getCurrentVersion().rawValue();
 
   if (config->printEachFile)
     message(toString(this));
+  inputFiles.insert(this);
 
   if (!is_contained(skipPlatformChecks, installName) &&
       !is_contained(interface.targets(), config->platformInfo.target)) {
     error(toString(this) + " is incompatible with " +
           std::string(config->platformInfo.target));
     return;
   }
 

lld/test/MachO/implicit-dylibs.s

 # RUN: llvm-otool -l %t/test | FileCheck %s \
 # RUN:   --check-prefix=LOAD -DDIR=%t --implicit-check-not LC_LOAD_DYLIB
 
 # LOAD:          cmd LC_LOAD_DYLIB
 # LOAD-NEXT: cmdsize
 # LOAD-NEXT:    name /usr/lib/libSystem.dylib
 # LOAD:          cmd LC_LOAD_DYLIB
 # LOAD-NEXT: cmdsize
+# LOAD-NEXT:    name [[DIR]]/libreexporter.dylib
+# LOAD:          cmd LC_LOAD_DYLIB
+# LOAD-NEXT: cmdsize
 # LOAD-NEXT:    name /System/Library/Frameworks/Foo.framework/Versions/A/Foo
 # LOAD:          cmd LC_LOAD_DYLIB
 # LOAD-NEXT: cmdsize
 # LOAD-NEXT:    name /usr/lib/libc++abi.dylib
 # LOAD:          cmd LC_LOAD_DYLIB
 # LOAD-NEXT: cmdsize
 # LOAD-NEXT:    name /usr/lib/libtoplevel.dylib
-# LOAD:          cmd LC_LOAD_DYLIB
-# LOAD-NEXT: cmdsize
-# LOAD-NEXT:    name [[DIR]]/libreexporter.dylib
 
 # RUN: %lld -no_implicit_dylibs -syslibroot %t -o %t/no-implicit -lSystem -L%t -lreexporter %t/test.o
 # RUN: llvm-objdump --bind --no-show-raw-insn -d %t/no-implicit | FileCheck %s --check-prefix=NO-IMPLICIT
 # NO-IMPLICIT:     Bind table:
 # NO-IMPLICIT-DAG: __DATA __data {{.*}} pointer 0 libreexporter _foo
 # NO-IMPLICIT-DAG: __DATA __data {{.*}} pointer 0 libreexporter _toplevel
 # NO-IMPLICIT-DAG: __DATA __data {{.*}} pointer 0 libreexporter _sublevel
 # NO-IMPLICIT-DAG: __DATA __data {{.*}} pointer 0 libreexporter _framework_foo

lld/test/MachO/special-symbol-ld-install-name.s

 # REQUIRES: x86
 
 # RUN: rm -rf %t; split-file --no-leading-lines %s %t
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/foo.s -o %t/foo.o
 
 ## Case 1: special symbol $ld$install_name affects the install name
 ## since the specified version 11.0.0 matches the target version 11.0.0
 
 # RUN: %lld -o %t/libfoo1.dylib %t/libLDInstallName.tbd %t/foo.o -dylib -platform_version macos 11.0.0 11.0.0
-# RUN: llvm-objdump --macho --dylibs-used %t/libfoo1.dylib | FileCheck --check-prefix=CASE1 %s
+# RUN: llvm-otool -L %t/libfoo1.dylib | FileCheck --check-prefix=CASE1 %s
 # CASE1: /New (compatibility version 1.1.1, current version 5.0.0)
 
 ## Case 2: special symbol $ld$install_name does not affect the install name
 ## since the specified version 11.0.0 does not match the target version 12.0.0
 
 # RUN: %lld -o %t/libfoo2.dylib %t/libLDInstallName.tbd %t/foo.o -dylib -platform_version macos 12.0.0 12.0.0
-# RUN: llvm-objdump --macho --dylibs-used %t/libfoo2.dylib | FileCheck --check-prefix=CASE2 %s
+# RUN: llvm-otool -L %t/libfoo2.dylib | FileCheck --check-prefix=CASE2 %s
 # CASE2: /Old (compatibility version 1.1.1, current version 5.0.0)
 
 ## Check that we emit a warning for an invalid os version.
 
 # RUN: %no_fatal_warnings_lld -o %t/libfoo3.dylib %t/libLDInstallNameInvalid.tbd %t/foo.o -dylib \
 # RUN:  -platform_version macos 11.0.0 11.0.0 2>&1 | FileCheck --check-prefix=INVALID-VERSION %s
 
 # INVALID-VERSION: failed to parse os version, symbol '$ld$install_name$os11.a$/New' ignored
 
+## Case 3: If there's another library that has '/New' as its original
+## install_name, we should take current-version and compatibility-version from
+## the first library we load with that install name. For a reexport,
+## the first loaded library is the library doing the reexport, not the
+## reexported library.
+# RUN: %lld -o %t/libfoo3.dylib %t/libLDInstallNameNew.tbd %t/foo.o -dylib -platform_version macos 11.0.0 11.0.0
+# RUN: llvm-otool -L %t/libfoo3.dylib | FileCheck --check-prefix=CASE3 %s
+# RUN: %lld -o %t/libfoo3.dylib %t/libLDInstallName.tbd %t/libLDInstallNameNew.tbd %t/foo.o -dylib -platform_version macos 11.0.0 11.0.0
+# RUN: llvm-otool -L %t/libfoo3.dylib | FileCheck --check-prefix=CASE1 %s
+# RUN: %lld -o %t/libfoo3.dylib %t/libLDInstallNameNew.tbd %t/libLDInstallName.tbd %t/foo.o -dylib -platform_version macos 11.0.0 11.0.0
+# RUN: llvm-otool -L %t/libfoo3.dylib | FileCheck --check-prefix=CASE3 %s
+# CASE3: /New (compatibility version 4.5.6, current version 9.0.0)
+
 #--- foo.s
 .long _xxx@GOTPCREL
 
 #--- libLDInstallName.tbd
 --- !tapi-tbd-v3
 archs:           [ x86_64 ]
 uuids:           [ 'x86_64: 19311012-01AB-342E-812B-73A74271A715' ]
 platform:        macosx

 platform:        macosx
 install-name:    '/Old'
 current-version: 5
 compatibility-version: 1.1.1
 exports:
   - archs:           [ x86_64 ]
     symbols:         [ '$ld$install_name$os11.a$/New', _xxx ]
 ...
+
+#--- libLDInstallNameNew.tbd
+--- !tapi-tbd
+tbd-version: 4
+targets: [ x86_64-macos ]
+uuids:
+  - target: x86_64-macos
+    value:  2E994C7F-3F03-3A07-879C-55690D22BEDA
+install-name: '/New'
+current-version: 9
+compatibility-version: 4.5.6
+reexported-libraries:
+  - targets:         [ x86_64-macos ]
+    libraries:       [ '@loader_path/libLDInstallName' ]
+...

lld/test/MachO/tapi-link.s

 # CHECK-DAG: __DATA_CONST __got 0x[[ADDR]] pointer 0 libSystem ___nan
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 CoreFoundation _OBJC_CLASS_$_NSObject
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 CoreFoundation _OBJC_METACLASS_$_NSObject
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 CoreFoundation _OBJC_IVAR_$_NSConstantArray._count
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 CoreFoundation _OBJC_EHTYPE_$_NSException
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 libc++abi      ___gxx_personality_v0
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 libNested3     _deeply_nested
 
-# RUN: llvm-objdump --macho --all-headers %t/test | \
-# RUN:     FileCheck --check-prefix=LOAD %s
+# RUN: llvm-otool -l %t/test | FileCheck --check-prefix=LOAD %s
 
-# RUN: llvm-objdump --macho --all-headers %t/with-reexport | \
-# RUN:     FileCheck --check-prefixes=LOAD,LOAD-REEXPORT %s
-
-# LOAD:          cmd LC_LOAD_DYLIB
-# LOAD-NEXT:               cmdsize
-# LOAD-NEXT:                  name /usr/lib/libSystem.dylib
-# LOAD-NEXT:            time stamp
-# LOAD-NEXT:       current version 1.1.1
-# LOAD-NEXT: compatibility version
+# RUN: llvm-otool -l %t/with-reexport | \
+# RUN:     FileCheck --check-prefixes=LOAD-REEXPORT,LOAD %s
 
 # LOAD-REEXPORT:          cmd LC_LOAD_DYLIB
 # LOAD-REEXPORT-NEXT:               cmdsize
 # LOAD-REEXPORT-NEXT:                  name /usr/lib/libReexportSystem.dylib
 # LOAD-REEXPORT-NEXT:            time stamp
 # LOAD-REEXPORT-NEXT:       current version 1.0.0
 # LOAD-REEXPORT-NEXT: compatibility version
 
+# LOAD:          cmd LC_LOAD_DYLIB
+# LOAD-NEXT:               cmdsize
+# LOAD-NEXT:                  name /usr/lib/libSystem.dylib
+# LOAD-NEXT:            time stamp
+# LOAD-NEXT:       current version 1.1.1
+# LOAD-NEXT: compatibility version
+
 #--- test.s
 .section __TEXT,__text
 .global _main
 
 _main:
 ## This symbol is defined in an inner TAPI document within libSystem.tbd.
   movq ___nan@GOTPCREL(%rip), %rax
   ret