This is an archive of the discontinued LLVM Phabricator instance.

[lld/mac] Add support for $ld$previous symbols with explicit symbol name
ClosedPublic

Authored by thakis on Jul 28 2022, 12:24 PM.

Details

Summary

A symbol $ld$previous$/Another$1.2.3$1$3.0$14.0$_xxx$ means
"pretend symbol _xxx is in dylib /Another with version 1.2.3
if the deployment target is between 3.0 and 14.0 and we're
targeting platform 1 (ie macOS)".

This means dylibs can now inject synthetic dylibs into the link, so
DylibFile needs to grow a 3rd constructor.

The only other interesting thing is that such an injected dylib
counts as a use of the original dylib. This patch gets this mostly
right (if _only_ $ld$previous symbols are used from a dylib,
we don't add a dep on the dylib itself, matching ld64), but one case
where we don't match ld64 yet is that ld64 even omits the original
dylib when linking it with -needed-l. Lld currently still adds a load
command for the original dylib in that case. (That's for a future
patch.)

Fixes #56074.

Diff Detail

Event Timeline

thakis created this revision.Jul 28 2022, 12:24 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 28 2022, 12:24 PM
thakis requested review of this revision.Jul 28 2022, 12:24 PM
thakis added inline comments.
lld/MachO/InputFiles.h
240

Moving this to an initializer is mostly unrelated, just didn't want to duplicate it in the 3rd ctor. I could land this change as a tiny standalone commit if you want.

lld/MachO/Writer.cpp
797–800

The

std::vector<DylibFile *> dylibFiles;
  for (InputFile *file : inputFiles) {
    if (auto *dylibFile = dyn_cast<DylibFile>(file))
      dylibFiles.push_back(dylibFile);
  }

bit and dedenting this for loop here could also land in its own commit, to make this diff smaller, if you want.

thakis updated this revision to Diff 448431.Jul 28 2022, 1:50 PM

address bot feedback

lld/MachO/InputFiles.h
240

…nevermind, the enum is only predeclared in this header. Undid this part.

int3 accepted this revision.Jul 28 2022, 2:35 PM
int3 added a subscriber: int3.

just some questions about the comments but lgtm

lld/MachO/InputFiles.cpp
2111

what do you mean by "works" here? what's the failure case that we have avoided?

lld/MachO/InputFiles.h
245–249

can we make this private so it's clear that we should be calling isExplicitlyLinked()? Looks like it's only being written to from loadDylib but we could make that a friend function if you find a setter too ugly

246

nit: would prefer if all the methods were located together (so at line 227 above) instead of interleaved with fields

lld/test/MachO/special-symbol-ld-previous.s
33–34

when a symbol is part of $previous$ if and only if that symbol is referenced.

kind of confused by this. Did you mean "only when the named symbol is referenced"?

This revision is now accepted and ready to land.Jul 28 2022, 2:35 PM
thakis updated this revision to Diff 448471.Jul 28 2022, 4:15 PM
thakis marked 2 inline comments as done.

address comments, soup up test slightly

lld/MachO/InputFiles.cpp
2111

The tbd file usually contains the $ld$previous symbol for an old version, and then the symbol itself later, for newer deployment targets, like so:

 symbols:         [
   '$ld$previous$/Another$$1$3.0$14.0$_zzz$',
   _zzz,
]

If they weren't sorted, then _zzz would be added with the current dylib first (and since it's added first, it would win over the $ld$previous symbol.

Suggestions on how to reword the comment?

lld/test/MachO/special-symbol-ld-previous.s
33–34

Yes, that's what I mean. Reworded it a bit.

Note to future self: To test things I had foo.tbd containing:

--- !tapi-tbd-v3
archs:           [ x86_64 ]
uuids:           [ 'x86_64: 19311019-01AB-342E-812B-73A74271A715' ]
platform:        macosx
install-name:    '/Old'
current-version: 6
compatibility-version: 1.1.1
exports:
  - archs:           [ x86_64 ]
    symbols:         [
      '$ld$previous$/Another$$1$3.0$14.0$_zzz$',
      '$ld$previous$/New$1.2.3$1$3.0$14.0$_yyy$',
      _xxx,
      _yyy,
      _zzz,
   ]
...

and test.s containing

.subsections_via_symbols

.globl _main
_main:
    call _xxx
    call _yyy
    call _zzz
    ret

and I'd uncomment the various call instructions, and run out/gn/bin/clang test.s -isysroot $(xcrun -show-sdk-path) --target=x86_64-apple-macos foo.tbd followed by otool -L a.out, and then add -fuse-ld=lld to the clang command, and replace foo.tbd with -Wl,-needed_library,foo.tbd, etc.

(I'll land the current version addressing the first round of feedback for now, given it's approved. I'll address potential follow-on feedback in a follow-on change.)

Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2022, 5:37 PM
int3 added inline comments.Jul 28 2022, 7:04 PM
lld/MachO/InputFiles.cpp
2111

How about

The tbd file usually contains the $ld$previous symbol for an old version, and then the symbol itself later, for newer deployment targets, like so:
<example>
Since the symbols are sorted, adding them to the symtab in the given order means the $ld$previous version of _zzz will prevail, as desired.

lld/test/MachO/special-symbol-ld-previous.s
33–34

I think it's confusing because it's a bit of a run-on sentence. How about

## Case 4: special symbol $ld$previous affects the install name / compatibility version if and only if:
##   * the specified version 11.0.0 is within the affected range [3.0, 14.0), and
##   * the symbol name after $previous$ points to a referenced symbol
thakis marked an inline comment as done.Jul 29 2022, 9:57 AM

Thanks, did both in 2681c9e0654b848e8983c1087840717f9ad486c8.

Should we try merging the original change to the 15.0 branch?

(This has since been merged to 15.0, see #56074.)