Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

Support $ld$ symbols #48769

Open Quuxplusone opened 3 years ago

Quuxplusone commented 3 years ago
Bugzilla Link PR49800
Status NEW
Importance P enhancement
Reported by Shoaib Meenai (smeenai@fb.com)
Reported on 2021-03-31 22:32:50 -0700
Last modified on 2021-11-04 21:15:40 -0700
Version unspecified
Hardware PC All
CC alexander.v.shaposhnikov@gmail.com, gkm@fb.com, jezreel@gmail.com, keithbsmiley@gmail.com, llvm-bugs@lists.llvm.org, nicolasweber@gmx.de, smeenai@fb.com
Fixed by commit(s)
Attachments tmp.diff (3793 bytes, text/plain)
tmp.diff (1247 bytes, text/plain)
unimpl.txt (42566 bytes, text/plain)
Blocks
Blocked by
See also
There's a bunch of symbols prefixed with $ld$ that have special meaning to the
linker. The ones I could find:

$ld$hide$
$ld$add$
$ld$weak$
$ld$install_name$
$ld$compatibility_version$
$ld$previous$

All of these except $ld$previous$ have been around for quite some time, and
have the effects you would expect from their names. Apple's open source release
of tapi has code to handle them, as does ld64's dylib parser. The tapi in LLVM
does not appear to have code to handle them so far, as far as I can see.

$ld$previous$ is new in the ld64 609 source code release (corresponding to
Xcode 12). It seems to be able to override the install name of the entire
library or the install name that particular symbols are associated with. The
former functionality is used in Xcode 12.5's SDK for the newly introduced
AVFAudio framework; its install name is overridden to AVFoundation if you're
targeting iOS 14.5 or newer. The following demonstrates this behavior:

$ cat use.s
    .long   _AVAudioBitRateStrategy_Constant@GOTPCREL

$ llvm-mc -filetype obj -triple x86_64-apple-ios14.0.0-simulator -o use.o use.s
$ ld -platform_version ios-simulator 14.0.0 14.5 -arch x86_64 -dylib \
    -o libuse.dylib use.o -framework AVFoundation \
    -syslibroot /Applications/Xcode_12.5_beta_3.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator.sdk
$ objdump --macho --dylibs-used libuse.dylib
libuse.dylib:
        libuse.dylib (compatibility version 0.0.0, current version 0.0.0)
        /System/Library/Frameworks/AVFoundation.framework/AVFoundation (compatibility version 1.0.0, current version 2.0.0)

$ llvm-mc -filetype obj -triple x86_64-apple-ios14.5.0-simulator -o use.o use.s
$ ld -platform_version ios-simulator 14.5.0 14.5 -arch x86_64 -dylib \
    -o libuse.dylib use.o -framework AVFoundation \
    -syslibroot /Applications/Xcode_12.5_beta_3.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator.sdk
$ objdump --macho --dylibs-used libuse.dylib
libuse.dylib:
        libuse.dylib (compatibility version 0.0.0, current version 0.0.0)
        /System/Library/Frameworks/AVFoundation.framework/AVFoundation (compatibility version 1.0.0, current version 2.0.0)
        /System/Library/Frameworks/AVFAudio.framework/AVFAudio (compatibility version 1.0.0, current version 1.0.0)

In the first case, since our minimum version is < 14.5, the install name
override kicks in. In the second case, our minimum version is >= 14.5, so we
see AVFAudio in the dylibs list. Since LLD doesn't implement this, both cases
will add AVFAudio to the dylibs list right now, and we'll produce a binary that
can't be run on older OS versions.

Interestingly enough, $ld$previous$ is implemented in a centralized location in
ld64 (in its generic dylib parser, which is the base parser class for Mach-O
dylibs and TBDs), whereas the other symbols are separately implemented in ld64
and tapi. There's also a FIXME in ld64 609's source to issue warnings for the
other $ld$ symbols once $ld$previous$ is submitted and documented, so perhaps
the eventual plan is for $ld$previous$ to subsume the functionality of the
others and be the only one?
Quuxplusone commented 3 years ago

https://bugs.llvm.org/show_bug.cgi?id=50304 is an example bug (likely) caused by LLD not respecting an $ld$install_name symbol.

Quuxplusone commented 3 years ago

I'll grab this if you don't mind.

Quuxplusone commented 3 years ago
I've almost implemented it already, have not sent for review yet.
Will do ~this week.
Quuxplusone commented 3 years ago

Attached tmp.diff (3793 bytes, text/plain): sketch

Quuxplusone commented 3 years ago

_Bug 50304 has been marked as a duplicate of this bug._

Quuxplusone commented 3 years ago

(Let me know if you get busy and could use help here after all.)

Quuxplusone commented 3 years ago

This is almost done (we have had a few discussions with Jez, the initial bits will be sent for review very soon), sorry about the delay and thanks for the draft of your diff.

Quuxplusone commented 3 years ago

_Bug 50411 has been marked as a duplicate of this bug._

Quuxplusone commented 3 years ago

Just want to re-iterate that I'm really looking forward to having this fixed and that I'm more than happy to help with this if you're busy with other things.

Quuxplusone commented 3 years ago

Nico, have you tried the latest version ? I'm wondering if there is anything missing now, e.g. if we have a use case for other special symbols or not.

Quuxplusone commented 3 years ago

Attached tmp.diff (1247 bytes, text/plain): hack to print unimplemented magic symbols

Quuxplusone commented 3 years ago

Attached unimpl.txt (42566 bytes, text/plain): Missing $ld$ symbol support

Quuxplusone commented 3 years ago

I'll look at problem (1) since it's my bug from aeae3e0ba9061

Quuxplusone commented 3 years ago
(Also, this might be a good idea?

@@ -1033,8 +1033,10 @@ void DylibFile::handleLDPreviousSymbol(StringRef name,
StringRef originalName) {
   std::tie(endVersion, name) = name.split('$');
   std::tie(symbolName, rest) = name.split('$');
   // TODO: ld64 contains some logic for non-empty symbolName as well.
-  if (!symbolName.empty())
+  if (!symbolName.empty()) {
+    warn("non-empty symbol name, symbol '" + originalName + "' ignored");
     return;
+  }
   unsigned platform;
   if (platformStr.getAsInteger(10, platform) ||
       platform != static_cast<unsigned>(config->platform()))

)
Quuxplusone commented 3 years ago
(In reply to Nico Weber from comment #13)
> I'll look at problem (1) since it's my bug from aeae3e0ba9061

https://reviews.llvm.org/D103821
Quuxplusone commented 3 years ago

Looks like https://reviews.llvm.org/D103505 implemented some of these