This is an archive of the discontinued LLVM Phabricator instance.

[lld/mac] Add reexports after reexporter to inputFiles
ClosedPublic

Authored by thakis on Jun 7 2021, 8:31 AM.

Details

Reviewers
int3
gkm
Group Reviewers
Restricted Project
Commits
rG17c43c404535: [lld/mac] Add reexports after reexporter to inputFiles
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.)

Diff Detail

Event Timeline

thakis created this revision.Jun 7 2021, 8:31 AM
Herald added a reviewer: gkm. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
thakis requested review of this revision.Jun 7 2021, 8:31 AM
thakis retitled this revision from [lld/mac] Take dylib version numbers from first loaded dylib with name, load reexports after reexporter to [lld/mac] Add reexports after reexporter to inputFiles.Jun 7 2021, 9:03 AM
int3 added a comment.Jun 7 2021, 12:27 PM

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 added a comment.EditedJun 7 2021, 12:33 PM

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...

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

thakis updated this revision to Diff 350394.Jun 7 2021, 12:52 PM

remove inputFiles.insert() in loadReexport()

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 accepted this revision.EditedJun 7 2021, 2:00 PM

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.

This revision is now accepted and ready to land.Jun 7 2021, 2:00 PM
thakis added a comment.Jun 7 2021, 2:03 PM

Thanks!

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.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2021, 2:04 PM
thakis added inline comments.Jun 7 2021, 2:28 PM
lld/MachO/InputFiles.cpp
872

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.

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"