[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

1162

nit: looks like this could go above the bool assignments

1387

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

1387

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

1251–1253

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 :)

1387

is this really == and not <=?

1387

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

[int3]

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

12

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

24–27

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

12

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

24–27

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

[oontvoo]

rebase

[int3]

lld/MachO/InputFiles.cpp

1262

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

24–27

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

24–27

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

24–27

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

24–27

Oh, found it! I forgot the .data

[int3]

Thanks!

[oontvoo]

thx!

lld/MachO/InputFiles.h

public:
 
   // An executable can be used as a bundle loader that will load the output
   // file being linked, and that contains symbols referenced, but not
   // 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);
+  bool handleLDSymbol(StringRef originalName, InputFile *file);
   void handleLDPreviousSymbol(StringRef name, StringRef originalName);
   void handleLDInstallNameSymbol(StringRef name, StringRef originalName);
+  void handleLDHideSymbol(StringRef name, StringRef originalName,
+                          InputFile *file);
   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

DylibFile::DylibFile(MemoryBufferRef mb, DylibFile *umbrella,
 
   // 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))
+                if (handleLDSymbol(savedName, exportingFile))
                   return;
+                if (exportingFile->hiddenSymbols.contains(
+                        CachedHashStringRef(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));
               });
   } else {
     error("LC_DYLD_INFO_ONLY not found in " + toString(this));
     return;
   }
 }
 
 void DylibFile::parseLoadCommands(MemoryBufferRef mb) {
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
   for (const auto *symbol : interface.symbols()) {
     if (!symbol->getArchitectures().has(config->arch()))
       continue;
-
-    if (handleLDSymbol(symbol->getName()))
+    // FIXME: We could process all the $ld$hide symbols first
+    // then the "normal" symbols to avoid inserting then deleting from
+    // symtab.
+    if (handleLDSymbol(symbol->getName(), exportingFile))
       continue;
 
     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:
if (is_contained(skipPlatformChecks, intfRef.getInstallName()) ||
         is_contained(targets, config->platformInfo.target))
       loadReexport(intfRef.getInstallName(), exportingFile, topLevel);
   }
 }
 
 // $ld$ symbols modify the properties/behavior of the library (e.g. its install
 // name, compatibility version or hide/add symbols) for specific target
 // versions.
-bool DylibFile::handleLDSymbol(StringRef originalName) {
+bool DylibFile::handleLDSymbol(StringRef originalName, InputFile *file) {
   if (!originalName.startswith("$ld$"))
     return false;
 
   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, file);
   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,
+                                   InputFile *file) {
+  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)
+    return;
+
+  hiddenSymbols.insert(CachedHashStringRef(symbolName));
+  symtab->removeSymbol(symbolName, file);
+}
+
 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/MachO/SymbolTable.h

public:
 
   Symbol *addUndefined(StringRef name, InputFile *, bool isWeakRef);
 
   Symbol *addCommon(StringRef name, InputFile *, uint64_t size, uint32_t align,
                     bool isPrivateExtern);
 
   Symbol *addDylib(StringRef name, DylibFile *file, bool isWeakDef, bool isTlv);
   Symbol *addDynamicLookup(StringRef name);
+  void removeSymbol(StringRef name, const InputFile *file);
 
   Symbol *addLazy(StringRef name, ArchiveFile *file,
                   const llvm::object::Archive::Symbol &sym);
 
   Defined *addSynthetic(StringRef name, InputSection *, uint64_t value,
                         bool isPrivateExtern, bool includeInSymtab,
                         bool referencedDynamically);
 

lld/MachO/SymbolTable.cpp

 
 Symbol *SymbolTable::find(CachedHashStringRef cachedName) {
   auto it = symMap.find(cachedName);
   if (it == symMap.end())
     return nullptr;
   return symVector[it->second];
 }
 
+void SymbolTable::removeSymbol(StringRef name, const InputFile *file) {
+  CachedHashStringRef cached(name);
+  Symbol *sym = find(cached);
+
+  if (!sym || sym->getFile() == nullptr ||
+      !sym->getFile()->getName().equals(file->getName()))
+    return;
+
+  // This function should only be called while we're still parsing the file.
+  // If the symbol is already referenced then something is wrong.
+  assert(isa<DylibSymbol>(sym));
+  assert(!dyn_cast<DylibSymbol>(sym)->isReferenced());
+  symMap.erase(cached);
+  // XXX: We don't remove it from the vector to avoid having to adjust indices.
+}
+
 std::pair<Symbol *, bool> SymbolTable::insert(StringRef name,
                                               const InputFile *file) {
   auto p = symMap.insert({CachedHashStringRef(name), (int)symVector.size()});
 
   Symbol *sym;
   if (!p.second) {
     // Name already present in the symbol table.
     sym = symVector[p.first->second];

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
+
+## Check that the hidden symbol is not referenceble from the tbd.
+# 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 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
+
+# ERROR:  error: undefined symbol: _OBJC_CLASS_$_foo11
+
+# FOO: Bind table:
+# FOO-NEXT: segment  section            address    type       addend dylib            symbol
+# FOO-DAG: __DATA_CONST __got              0x{{.*}} pointer         0 /HideFoo         _OBJC_CLASS_$_bar
+# FOO-DAG: __DATA_CONST __got              0x{{.*}} pointer         0 /HideFoo         _OBJC_CLASS_$_foo10
+# FOO-DAG: __DATA_CONST __got              0x{{.*}} pointer         0 /HasFoo          _OBJC_CLASS_$_foo11
+# FOO-DAG: __DATA_CONST __got              0x{{.*}} pointer         0 /HideFoo         _xxx
+
+
+#--- ref-all.s
+.long _xxx@GOTPCREL
+.long _OBJC_CLASS_$_foo11@GOTPCREL
+.long _OBJC_CLASS_$_foo10@GOTPCREL
+.long _OBJC_CLASS_$_bar@GOTPCREL
+
+#--- 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]
+...