[lld-macho] Handle $ld$hide[$os] symbols.

[oontvoo]

Summary

This is needed so that LLD wouldn't bind a symbol to a framework where it's hidden (would result in "missing symbol" runtime crash).

Perf impact seemed minimal:
(linking chromium)

x base.txt
+ with_hide.txt

SYSTEM CPU time: 
    N           Min           Max        Median           Avg        Stddev
x   5          0.88           0.9          0.89         0.892  0.0083666003
+   5          0.87           0.9          0.88         0.882   0.010954451
No difference proven at 95.0% confidence

USER CPU time: 
    N           Min           Max        Median           Avg        Stddev
x   5          3.51          3.54          3.51         3.518   0.013038405
+   5          3.47           3.5           3.5          3.49   0.014142136
Difference at 95.0% confidence
	-0.028 +/- 0.019837
	-0.795907% +/- 0.56387%
	(Student's t, pooled s = 0.0136015)

WALL time: 
    N           Min           Max        Median           Avg        Stddev
x   5          3.91          3.94          3.94          3.93   0.014142136
+   5          3.86           3.9           3.9         3.888   0.017888544
Difference at 95.0% confidence
	-0.042 +/- 0.0235167
	-1.0687% +/- 0.598389%
	(Student's t, pooled s = 0.0161245)

PR/52708

[oontvoo]

add tests

[oontvoo]

rename

[keith]

lld/MachO/InputFiles.cpp

1163

nit: looks like this could go above the bool assignments

1411

is == there right comparison here? not entirely sure of the use cases but should a .1 difference still have this logic apply?

[oontvoo]

review comment + more exp in tests

[oontvoo]

lld/MachO/InputFiles.cpp

1411

Yes, == is by design. We want exact version match.

Apparently, apple would list literally all the versions it wants to hide in the tbd (rather than using a min version).
Eg., if you want to hide foo for 10, 11, and 12, then you would need $ld$hide$os10.0$foo, $ld$hide$os11.0$foo, $ld$hide$os12.0$foo

[int3]

lld/MachO/InputFiles.cpp

1266–1267

this would be a much cleaner implementation IMO. could we do a few quick benchmarks to see if it actually makes a perf difference? if not, simpler is better :)

1411

is this really == and not <=?

1411

oops, ignore this comment, forgot to delete it after I saw your reply to keith

[int3]

lld/test/MachO/special-symbol-ld-hidden.s

13

should we check whether $ld$hide has an impact on re-exported symbols too?

25–28

shouldn't this be .quad? I believe .long is for 32-bit values

[oontvoo]

Updated diff: process all the $ld symbols. First.

Benchmarking this on chromium vs the previous diff showed little difference

SYSTEM CPU time: 
x one_pass.txt
+ two_passes.txt
    N           Min           Max        Median           Avg        Stddev
x   5          1.05          1.08          1.05         1.058   0.013038405
+   5          1.06          1.09          1.08         1.074   0.013416408
No difference proven at 95.0% confidence

USER CPU time: 
x one_pass.txt
+ two_passes.txt
    N           Min           Max        Median           Avg        Stddev
x   5          3.57          3.65          3.58         3.596   0.032093613
+   5           3.6           3.7          3.61         3.634   0.044497191
No difference proven at 95.0% confidence

WALL time: 
x one_pass.txt
+ two_passes.txt
    N           Min           Max        Median           Avg        Stddev
x   5          4.14          4.26          4.15         4.176    0.04929503
+   5          4.17          4.31           4.2         4.226   0.061073726
No difference proven at 95.0% confidence

(though I suspect it *might* make a difference with a larger app like Youtube ... will try benchmarking it later today)

[oontvoo]

Interestingly, the two-pass approach seemed faster 🤔 .
In any case, I guess we're good. :)

SYSTEM CPU time: 
x one_pass.txt
+ two_passes.txt
    N           Min           Max        Median           Avg        Stddev
x   5          1.08          1.19           1.1         1.116   0.046151923
+   5          0.91          1.04          0.93          0.95   0.054313902
Difference at 95.0% confidence
	-0.166 +/- 0.0735032
	-14.8746% +/- 6.58631%
	(Student's t, pooled s = 0.0503984)

USER CPU time: 
x one_pass.txt
+ two_passes.txt
    N           Min           Max        Median           Avg        Stddev
x   5          3.28           3.4          3.36          3.35    0.04472136
+   5          3.13           3.3          3.18         3.192   0.068337398
Difference at 95.0% confidence
	-0.158 +/- 0.0842243
	-4.71642% +/- 2.51416%
	(Student's t, pooled s = 0.0577495)

WALL time: 
x one_pass.txt
+ two_passes.txt
    N           Min           Max        Median           Avg        Stddev
x   5          5.54          5.65          5.62         5.608   0.042071368
+   5          4.79          5.31          4.93         4.966    0.21173096
Difference at 95.0% confidence
	-0.642 +/- 0.222622
	-11.4479% +/- 3.96971%
	(Student's t, pooled s = 0.152643)

[oontvoo]

added more tests

[oontvoo]

lld/test/MachO/Inputs/libReexportSystem2.tbd

1 ↗

(On Diff #395177)

This needs to be a standalone tbd because llvm-lit doesn't seem to understand tbd-v3 any more and I need the re-exports: [filename] for my test.

lld/test/MachO/special-symbol-ld-hidden.s

13

Yes - good point. seemed to have no effect on reexported symbols. (I've checked with ld64)

25–28

it needed to be 32-bit because of the @GOTPCREL

[oontvoo]

rebase

[int3]

lld/MachO/InputFiles.cpp

1286

why not just push back a pointer to symbol? InterfaceSymbol seems redundant

lld/test/MachO/Inputs/libReexportSystem2.tbd

1 ↗

(On Diff #395177)

weird, what's the error?

but in any case tbdv4 does support reexports too, see e.g. Inputs/libStubLink.tbd

lld/test/MachO/special-symbol-ld-hidden.s

25–28

oh right. it's weird to use GOTPCREL like this though, since this isn't actually an instruction stream. I think .quad _xxx makes more sense if you just want to add a reference to a symbol

most of our other tests do that too

[int3]

Oh yeah could you add the benchmark timings to the commit message? Along with the machine you ran them on

[oontvoo]

updated diff:

• store Symbol* instead of inventing a new InterfaceSymbol struct
• use inline tbd

[oontvoo]

lld/test/MachO/special-symbol-ld-hidden.s

25–28

also can't use .quad because objdump will crash with

objdump: error: ': truncated or malformed object (for BIND_OPCODE_SET_SEGMENT_AND_OFFSET_ULEB bad offset, not in section for opcode at: 0xa)

:(

[int3]

lld/test/MachO/special-symbol-ld-hidden.s

25–28

weird. can you look at the other tests that use quad to figure out the difference?

[oontvoo]

put the refs explicitly in data section

lld/test/MachO/special-symbol-ld-hidden.s

25–28

Oh, found it! I forgot the .data

[int3]

Thanks!

[oontvoo]

thx!

lld/MachO/InputFiles.h

public:
   // implemented in the bundle. When used like this, it is very similar
   // to a Dylib, so we re-used the same class to represent it.
   bool isBundleLoader;
 
 private:
   bool handleLDSymbol(StringRef originalName);
   void handleLDPreviousSymbol(StringRef name, StringRef originalName);
   void handleLDInstallNameSymbol(StringRef name, StringRef originalName);
+  void handleLDHideSymbol(StringRef name, StringRef originalName);
   void checkAppExtensionSafety(bool dylibIsAppExtensionSafe) const;
+
+  llvm::DenseSet<llvm::CachedHashStringRef> hiddenSymbols;
 };
 
 // .a file
 class ArchiveFile final : public InputFile {
 public:
   explicit ArchiveFile(std::unique_ptr<llvm::object::Archive> &&file);
   void addLazySymbols();
   void fetch(const llvm::object::Archive::Symbol &);

lld/MachO/InputFiles.cpp

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);
+    struct TrieEntry {
+      StringRef name;
+      uint64_t flags;
+    };
+
+    std::vector<TrieEntry> entries;
+    // Find all the $ld$* symbols to process first.
     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;
-                bool isTlv = flags & EXPORT_SYMBOL_FLAGS_KIND_THREAD_LOCAL;
-                symbols.push_back(symtab->addDylib(savedName, exportingFile,
-                                                   isWeakDef, isTlv));
+                entries.push_back({savedName, flags});
               });
+
+    // Process the "normal" symbols.
+    for (TrieEntry &entry : entries) {
+      if (exportingFile->hiddenSymbols.contains(
+              CachedHashStringRef(entry.name)))
+        continue;
+
+      bool isWeakDef = entry.flags & EXPORT_SYMBOL_FLAGS_WEAK_DEFINITION;
+      bool isTlv = entry.flags & EXPORT_SYMBOL_FLAGS_KIND_THREAD_LOCAL;
+
+      symbols.push_back(
+          symtab->addDylib(entry.name, exportingFile, isWeakDef, isTlv));
+    }
+
   } else {
     error("LC_DYLD_INFO_ONLY not found in " + toString(this));
     return;
   }
 }
 
 void DylibFile::parseLoadCommands(MemoryBufferRef mb) {
   auto *hdr = reinterpret_cast<const mach_header *>(mb.getBufferStart());
error(toString(this) + " is incompatible with " +
           std::string(config->platformInfo.target));
     return;
   }
 
   checkAppExtensionSafety(interface.isApplicationExtensionSafe());
 
   exportingFile = isImplicitlyLinked(installName) ? this : umbrella;
   auto addSymbol = [&](const Twine &name) -> void {
-    symbols.push_back(symtab->addDylib(saver.save(name), exportingFile,
+    StringRef savedName = saver.save(name);
+    if (exportingFile->hiddenSymbols.contains(CachedHashStringRef(savedName)))
+      return;
+
+    symbols.push_back(symtab->addDylib(savedName, exportingFile,
                                        /*isWeakDef=*/false,
                                        /*isTlv=*/false));
   };
-  // TODO(compnerd) filter out symbols based on the target platform
-  // TODO: handle weak defs, thread locals
+
+  std::vector<const llvm::MachO::Symbol *> normalSymbols;
+  normalSymbols.reserve(interface.symbolsCount());
   for (const auto *symbol : interface.symbols()) {
     if (!symbol->getArchitectures().has(config->arch()))
       continue;
-
     if (handleLDSymbol(symbol->getName()))
       continue;
 
     switch (symbol->getKind()) {
+    case SymbolKind::GlobalSymbol:               // Fallthrough
+    case SymbolKind::ObjectiveCClass:            // Fallthrough
+    case SymbolKind::ObjectiveCClassEHType:      // Fallthrough
+    case SymbolKind::ObjectiveCInstanceVariable: // Fallthrough
+      normalSymbols.push_back(symbol);
+    }
+  }
+
+  // TODO(compnerd) filter out symbols based on the target platform
+  // TODO: handle weak defs, thread locals
+  for (const auto *symbol : normalSymbols) {
+    switch (symbol->getKind()) {
     case SymbolKind::GlobalSymbol:
       addSymbol(symbol->getName());
       break;
     case SymbolKind::ObjectiveCClass:
       // XXX ld64 only creates these symbols when -ObjC is passed in. We may
       // want to emulate that.
       addSymbol(objc::klass + symbol->getName());
       addSymbol(objc::metaclass + symbol->getName());
       break;
     case SymbolKind::ObjectiveCClassEHType:
       addSymbol(objc::ehtype + symbol->getName());
       break;
     case SymbolKind::ObjectiveCInstanceVariable:
bool DylibFile::handleLDSymbol(StringRef originalName) {
 
   StringRef action;
   StringRef name;
   std::tie(action, name) = originalName.drop_front(strlen("$ld$")).split('$');
   if (action == "previous")
     handleLDPreviousSymbol(name, originalName);
   else if (action == "install_name")
     handleLDInstallNameSymbol(name, originalName);
+  else if (action == "hide")
+    handleLDHideSymbol(name, originalName);
   return true;
 }
 
 void DylibFile::handleLDPreviousSymbol(StringRef name, StringRef originalName) {
   // originalName: $ld$ previous $ <installname> $ <compatversion> $
   // <platformstr> $ <startversion> $ <endversion> $ <symbol-name> $
   StringRef installName;
   StringRef compatVersion;
void DylibFile::handleLDInstallNameSymbol(StringRef name,
   std::tie(condition, installName) = name.split('$');
   VersionTuple version;
   if (!condition.consume_front("os") || version.tryParse(condition))
     warn("failed to parse os version, symbol '" + originalName + "' ignored");
   else if (version == config->platformInfo.minimum)
     this->installName = saver.save(installName);
 }
 
+void DylibFile::handleLDHideSymbol(StringRef name, StringRef originalName) {
+  StringRef symbolName;
+  bool shouldHide = true;
+  if (name.startswith("os")) {
+    // If it's hidden based on versions.
+    name = name.drop_front(2);
+    StringRef minVersion;
+    std::tie(minVersion, symbolName) = name.split('$');
+    VersionTuple versionTup;
+    if (versionTup.tryParse(minVersion)) {
+      warn("Failed to parse hidden version, symbol `" + originalName +
+           "` ignored.");
+      return;
+    }
+    shouldHide = versionTup == config->platformInfo.minimum;
+  } else {
+    symbolName = name;
+  }
+
+  if (shouldHide)
+    exportingFile->hiddenSymbols.insert(CachedHashStringRef(symbolName));
+}
+
 void DylibFile::checkAppExtensionSafety(bool dylibIsAppExtensionSafe) const {
   if (config->applicationExtension && !dylibIsAppExtensionSafe)
     warn("using '-application_extension' with unsafe dylib: " + toString(this));
 }
 
 ArchiveFile::ArchiveFile(std::unique_ptr<object::Archive> &&f)
     : InputFile(ArchiveKind, f->getMemoryBufferRef()), file(std::move(f)) {}
 

lld/test/MachO/special-symbol-ld-hidden.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/ref-all.s -o %t/ref-all.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/ref-reexported.s -o %t/ref-reexported.o
+
+## Check that the hidden symbols(foo11) can't be referenced from HideFoo.tbd when current version is 11.
+# RUN: not %lld -o /dev/null %t/libHideFoo.tbd %t/ref-all.o -dylib -platform_version macos 11.0.0 11.0.0 2>&1 | FileCheck %s --check-prefix=ERROR
+
+## Check that the hidden symbol(foo11) can be referenced when the current version is NOT 11.
+# RUN: %lld -o %t/ref-foo-12.dylib %t/libHideFoo.tbd %t/ref-all.o -dylib -platform_version macos 12.0.0 12.0.0
+# RUN: llvm-objdump --macho --bind %t/ref-foo-12.dylib | FileCheck %s --check-prefix=HAS-FOO
+
+## Check that when we link multiple tbd files, foo11 comes from the tbd where it is visible.
+# RUN: %lld -o %t/ref-all.dylib %t/libHideFoo.tbd %t/libHasFoo.tbd %t/ref-all.o -dylib -platform_version macos 11.0.0 11.0.0
+# RUN: llvm-objdump --macho --bind %t/ref-all.dylib | FileCheck %s --check-prefix=FOO
+
+## Check that '$hide$' has no effect on reexported symbols.
+# RUN: %lld -o %t/reexport.dylib %t/libReexportSystem2.tbd %t/ref-reexported.o -dylib -platform_version macos 11.0.0 11.0.0
+# RUN: llvm-objdump --macho --bind %t/reexport.dylib | FileCheck %s --check-prefix=REEXP
+
+# ERROR:  error: undefined symbol: _OBJC_CLASS_$_foo11
+
+# HAS-FOO: __DATA __data              {{.*}} pointer         0 /HideFoo         _OBJC_CLASS_$_foo11
+
+# FOO:      segment  section            address    type       addend dylib            symbol
+# FOO-DAG: __DATA   __data               {{.*}} pointer         0 /HideFoo         _OBJC_CLASS_$_bar
+# FOO-DAG: __DATA   __data               {{.*}} pointer         0 /HideFoo         _OBJC_CLASS_$_foo10
+# FOO-DAG: __DATA   __data               {{.*}} pointer         0 /HasFoo          _OBJC_CLASS_$_foo11
+# FOO-DAG: __DATA   __data               {{.*}} pointer         0 /HideFoo         _xxx
+
+# REEXP: __DATA   __data             {{.*}} pointer         0 libSystem        ___nan
+
+#--- ref-all.s
+.data        
+.quad _xxx
+.quad _OBJC_CLASS_$_foo11
+.quad _OBJC_CLASS_$_foo10
+.quad _OBJC_CLASS_$_bar
+
+#--- ref-reexported.s
+.data
+.quad   ___nan
+
+#--- libHideFoo.tbd
+--- !tapi-tbd
+tbd-version: 4
+targets: [ x86_64-macos ]
+uuids:
+  - target: x86_64-macos
+    value:  2E994C7F-3F03-3A07-879C-55690D22BEDA
+install-name: '/HideFoo'
+current-version: 9
+compatibility-version: 4.5.6
+exports:
+  - targets:         [ x86_64-macos ]
+    symbols: [ '$ld$hide$os11.0$_OBJC_CLASS_$_foo11', '$ld$hide$os10.0$_OBJC_CLASS_$_foo10',  _xxx ]
+    objc-classes: [foo10, foo11, bar]
+...
+
+#--- libHasFoo.tbd
+--- !tapi-tbd
+tbd-version: 4
+targets: [ x86_64-macos ]
+uuids:
+  - target: x86_64-macos
+    value:  2E994C7F-3F03-3A07-879C-55690D22BEDB
+install-name: '/HasFoo'
+current-version: 9
+compatibility-version: 4.5.6
+exports:
+  - targets: [ x86_64-macos ]
+    symbols: [  _xxx ]
+    objc-classes: [foo10, foo11, bar]
+...
+
+#--- libReexportSystem2.tbd
+--- !tapi-tbd
+tbd-version:     4
+targets: [ x86_64-macos ]
+uuids:
+  - target:  x86_64-macos
+    value:   00000000-0000-0000-0000-000000000002
+install-name:    '/libReexportSystem2'
+current-version: 9
+exports:
+  - targets: [ x86_64-macos ]
+    symbols: [  '$ld$hide$___nan' ]
+reexported-libraries:
+  - targets:   [ x86_64-macos ]
+    libraries: [ '/usr/lib/libSystem.dylib' ]
+...
+

llvm/include/llvm/TextAPI/InterfaceFile.h

using const_filtered_symbol_iterator =
                       std::function<bool(const Symbol *)>>;
   using const_filtered_symbol_range =
       iterator_range<const_filtered_symbol_iterator>;
 
   const_symbol_range symbols() const {
     return {Symbols.begin(), Symbols.end()};
   }
 
+  size_t symbolsCount() const { return Symbols.size(); }
+
   const_filtered_symbol_range exports() const {
     std::function<bool(const Symbol *)> fn = [](const Symbol *Symbol) {
       return !Symbol->isUndefined();
     };
     return make_filter_range(
         make_range<const_symbol_iterator>({Symbols.begin()}, {Symbols.end()}),
         fn);
   }