[lld/mac] Implement support for private extern symbols

Private extern symbols are used for things scoped to the linkage unit.
They cause duplicate symbol errors (so they're in the symbol table,
unlike TU-scoped truly local symbols), but they don't make it into the
export trie. They are created e.g. by compiling with
-fvisibility=hidden.

If two weak symbols have differing privateness, the combined symbol is
non-private external. (Example: inline functions and some TUs that
include the header defining it were built with
-fvisibility-inlines-hidden and some weren't).

A weak private external symbol implicitly has its "weak" dropped and
behaves like a regular strong private external symbol: Weak is an export
trie concept, and private symbols are not in the export trie.

If a weak and a strong symbol have different privateness, the strong
symbol wins.

If two common symbols have differing privateness, the larger symbol
wins. If they have the same size, the privateness of the symbol seen
later during the link wins (!) -- this is a bit lame, but it matches
ld64 and this behavior takes 2 lines less to implement than the less
surprising "result is non-private external), so match ld64.
(Example: `int a` in two .c files, both built with -fcommon,
one built with -fvisibility=hidden and one without.)

This also makes `__dyld_private` a true TU-local symbol, matching ld64.
To make this work, make the `const char*` StringRefZ ctor to correctly
set `size` (without this, writing the string table crashed when calling
getName() on the __dyld_private symbol).

Mention in CommonSymbol's comment that common symbols are now disabled
by default in clang.

Mention in -keep_private_externs's HelpText that the flag only has an
effect with `-r` (which we don't implement yet -- so this patch here
doesn't regress any behavior around -r + -keep_private_externs)). ld64
doesn't explicitly document it, but the commit text of
http://reviews.llvm.org/rL216146 does, and ld64's
OutputFile::buildSymbolTable() checks `_options.outputKind() ==
Options::kObjectFile` before calling `_options.keepPrivateExterns()`
(the only reference to that function).

Fixes PR48536.

Differential Revision: https://reviews.llvm.org/D93609

Author: nico

lld/MachO/Driver.cpp

@@ -524,7 +524,7 @@ static void replaceCommonSymbols() {
 
     replaceSymbol<Defined>(sym, sym->getName(), isec, /*value=*/0,
                            /*isWeakDef=*/false,
-                           /*isExternal=*/true);
+                           /*isExternal=*/true, common->privateExtern);
   }
 }
 

lld/MachO/InputFiles.cpp

@@ -280,22 +280,33 @@ void ObjFile::parseRelocations(const section_64 &sec,
 static macho::Symbol *createDefined(const structs::nlist_64 &sym,
                                     StringRef name, InputSection *isec,
                                     uint32_t value) {
-  if (sym.n_type & N_EXT)
-    // Global defined symbol
-    return symtab->addDefined(name, isec, value, sym.n_desc & N_WEAK_DEF);
-  // Local defined symbol
+  // Symbol scope is determined by sym.n_type & (N_EXT | N_PEXT):
+  // N_EXT: Global symbols
+  // N_EXT | N_PEXT: Linkage unit (think: dylib) scoped
+  // N_PEXT: Does not occur in input files in practice,
+  //         a private extern must be external.
+  // 0: Translation-unit scoped. These are not in the symbol table.
+
+  if (sym.n_type & (N_EXT | N_PEXT)) {
+    assert((sym.n_type & N_EXT) && "invalid input");
+    return symtab->addDefined(name, isec, value, sym.n_desc & N_WEAK_DEF,
+                              sym.n_type & N_PEXT);
+  }
   return make<Defined>(name, isec, value, sym.n_desc & N_WEAK_DEF,
-                       /*isExternal=*/false);
+                       /*isExternal=*/false, /*isPrivateExtern=*/false);
 }
 
 // Absolute symbols are defined symbols that do not have an associated
 // InputSection. They cannot be weak.
 static macho::Symbol *createAbsolute(const structs::nlist_64 &sym,
                                      StringRef name) {
-  if (sym.n_type & N_EXT)
-    return symtab->addDefined(name, nullptr, sym.n_value, /*isWeakDef=*/false);
+  if (sym.n_type & (N_EXT | N_PEXT)) {
+    assert((sym.n_type & N_EXT) && "invalid input");
+    return symtab->addDefined(name, nullptr, sym.n_value, /*isWeakDef=*/false,
+                              sym.n_type & N_PEXT);
+  }
   return make<Defined>(name, nullptr, sym.n_value, /*isWeakDef=*/false,
-                       /*isExternal=*/false);
+                       /*isExternal=*/false, /*isPrivateExtern=*/false);
 }
 
 macho::Symbol *ObjFile::parseNonSectionSymbol(const structs::nlist_64 &sym,
@@ -306,7 +317,8 @@ macho::Symbol *ObjFile::parseNonSectionSymbol(const structs::nlist_64 &sym,
     return sym.n_value == 0
                ? symtab->addUndefined(name, sym.n_desc & N_WEAK_REF)
                : symtab->addCommon(name, this, sym.n_value,
-                                   1 << GET_COMM_ALIGN(sym.n_desc));
+                                   1 << GET_COMM_ALIGN(sym.n_desc),
+                                   sym.n_type & N_PEXT);
   case N_ABS:
     return createAbsolute(sym, name);
   case N_PBUD:

lld/MachO/Options.td

@@ -346,7 +346,7 @@ def bundle_loader : Separate<["-"], "bundle_loader">,
 def grp_object : OptionGroup<"object">, HelpText<"CREATING AN OBJECT FILE">;
 
 def keep_private_externs : Flag<["-"], "keep_private_externs">,
-     HelpText<"Do not convert private external symbols to static symbols">,
+     HelpText<"Do not convert private external symbols to static symbols (only valid with -r)">,
      Flags<[HelpHidden]>,
      Group<grp_object>;
 def d : Flag<["-"], "d">,

lld/MachO/SymbolTable.cpp

@@ -38,16 +38,22 @@ std::pair<Symbol *, bool> SymbolTable::insert(StringRef name) {
 }
 
 Symbol *SymbolTable::addDefined(StringRef name, InputSection *isec,
-                                uint32_t value, bool isWeakDef) {
+                                uint32_t value, bool isWeakDef,
+                                bool isPrivateExtern) {
   Symbol *s;
   bool wasInserted;
   bool overridesWeakDef = false;
   std::tie(s, wasInserted) = insert(name);
 
   if (!wasInserted) {
     if (auto *defined = dyn_cast<Defined>(s)) {
-      if (isWeakDef)
+      if (isWeakDef) {
+        // Both old and new symbol weak (e.g. inline function in two TUs):
+        // If one of them isn't private extern, the merged symbol isn't.
+        if (defined->isWeakDef())
+          defined->privateExtern &= isPrivateExtern;
         return s;
+      }
       if (!defined->isWeakDef())
         error("duplicate symbol: " + name);
     } else if (auto *dysym = dyn_cast<DylibSymbol>(s)) {
@@ -57,8 +63,9 @@ Symbol *SymbolTable::addDefined(StringRef name, InputSection *isec,
     // of a name conflict, we fall through to the replaceSymbol() call below.
   }
 
-  Defined *defined = replaceSymbol<Defined>(s, name, isec, value, isWeakDef,
-                                            /*isExternal=*/true);
+  Defined *defined =
+      replaceSymbol<Defined>(s, name, isec, value, isWeakDef,
+                             /*isExternal=*/true, isPrivateExtern);
   defined->overridesWeakDef = overridesWeakDef;
   return s;
 }
@@ -82,7 +89,7 @@ Symbol *SymbolTable::addUndefined(StringRef name, bool isWeakRef) {
 }
 
 Symbol *SymbolTable::addCommon(StringRef name, InputFile *file, uint64_t size,
-                               uint32_t align) {
+                               uint32_t align, bool isPrivateExtern) {
   Symbol *s;
   bool wasInserted;
   std::tie(s, wasInserted) = insert(name);
@@ -98,7 +105,7 @@ Symbol *SymbolTable::addCommon(StringRef name, InputFile *file, uint64_t size,
     // a name conflict, we fall through to the replaceSymbol() call below.
   }
 
-  replaceSymbol<CommonSymbol>(s, name, file, size, align);
+  replaceSymbol<CommonSymbol>(s, name, file, size, align, isPrivateExtern);
   return s;
 }
 

lld/MachO/SymbolTable.h

@@ -33,11 +33,12 @@ class Symbol;
 class SymbolTable {
 public:
   Symbol *addDefined(StringRef name, InputSection *isec, uint32_t value,
-                     bool isWeakDef);
+                     bool isWeakDef, bool isPrivateExtern);
 
   Symbol *addUndefined(StringRef name, bool isWeakRef);
 
-  Symbol *addCommon(StringRef name, InputFile *, uint64_t size, uint32_t align);
+  Symbol *addCommon(StringRef name, InputFile *, uint64_t size, uint32_t align,
+                    bool isPrivateExtern);
 
   Symbol *addDylib(StringRef name, DylibFile *file, bool isWeakDef, bool isTlv);
 

lld/MachO/Symbols.h

@@ -47,7 +47,11 @@ class Symbol {
 
   Kind kind() const { return static_cast<Kind>(symbolKind); }
 
-  StringRef getName() const { return {name.data, name.size}; }
+  StringRef getName() const {
+    if (nameSize == (uint32_t)-1)
+      nameSize = strlen(nameData);
+    return {nameData, nameSize};
+  }
 
   virtual uint64_t getVA() const { return 0; }
 
@@ -80,20 +84,26 @@ class Symbol {
   uint32_t symtabIndex = UINT32_MAX;
 
 protected:
-  Symbol(Kind k, StringRefZ name) : symbolKind(k), name(name) {}
+  Symbol(Kind k, StringRefZ name)
+      : symbolKind(k), nameData(name.data), nameSize(name.size) {}
 
   Kind symbolKind;
-  StringRefZ name;
+  const char *nameData;
+  mutable uint32_t nameSize;
 };
 
 class Defined : public Symbol {
 public:
   Defined(StringRefZ name, InputSection *isec, uint32_t value, bool isWeakDef,
-          bool isExternal)
+          bool isExternal, bool isPrivateExtern)
       : Symbol(DefinedKind, name), isec(isec), value(value),
-        overridesWeakDef(false), weakDef(isWeakDef), external(isExternal) {}
+        overridesWeakDef(false), privateExtern(isPrivateExtern),
+        weakDef(isWeakDef), external(isExternal) {}
 
   bool isWeakDef() const override { return weakDef; }
+  bool isExternalWeakDef() const {
+    return isWeakDef() && isExternal() && !privateExtern;
+  }
   bool isTlv() const override {
     return !isAbsolute() && isThreadLocalVariables(isec->flags);
   }
@@ -110,6 +120,7 @@ class Defined : public Symbol {
   uint32_t value;
 
   bool overridesWeakDef : 1;
+  bool privateExtern : 1;
 
 private:
   const bool weakDef : 1;
@@ -148,14 +159,17 @@ class Undefined : public Symbol {
 //
 // The compiler creates common symbols when it sees tentative definitions.
 // (You can suppress this behavior and let the compiler create a regular
-// defined symbol by passing -fno-common.) When linking the final binary, if
-// there are remaining common symbols after name resolution is complete, the
-// linker converts them to regular defined symbols in a __common section.
+// defined symbol by passing -fno-common. -fno-common is the default in clang
+// as of LLVM 11.0.) When linking the final binary, if there are remaining
+// common symbols after name resolution is complete, the linker converts them
+// to regular defined symbols in a __common section.
 class CommonSymbol : public Symbol {
 public:
-  CommonSymbol(StringRefZ name, InputFile *file, uint64_t size, uint32_t align)
+  CommonSymbol(StringRefZ name, InputFile *file, uint64_t size, uint32_t align,
+               bool isPrivateExtern)
       : Symbol(CommonKind, name), file(file), size(size),
-        align(align != 1 ? align : llvm::PowerOf2Ceil(size)) {
+        align(align != 1 ? align : llvm::PowerOf2Ceil(size)),
+        privateExtern(isPrivateExtern) {
     // TODO: cap maximum alignment
   }
 
@@ -164,6 +178,7 @@ class CommonSymbol : public Symbol {
   InputFile *const file;
   const uint64_t size;
   const uint32_t align;
+  const bool privateExtern;
 };
 
 class DylibSymbol : public Symbol {

lld/MachO/SyntheticSections.cpp

@@ -362,12 +362,10 @@ void WeakBindingSection::writeTo(uint8_t *buf) const {
 }
 
 bool macho::needsBinding(const Symbol *sym) {
-  if (isa<DylibSymbol>(sym)) {
+  if (isa<DylibSymbol>(sym))
     return true;
-  } else if (const auto *defined = dyn_cast<Defined>(sym)) {
-    if (defined->isWeakDef() && defined->isExternal())
-      return true;
-  }
+  if (const auto *defined = dyn_cast<Defined>(sym))
+    return defined->isExternalWeakDef();
   return false;
 }
 
@@ -380,7 +378,7 @@ void macho::addNonLazyBindingEntries(const Symbol *sym,
       in.weakBinding->addEntry(sym, section, offset, addend);
   } else if (auto *defined = dyn_cast<Defined>(sym)) {
     in.rebase->addEntry(section, offset);
-    if (defined->isWeakDef() && defined->isExternal())
+    if (defined->isExternalWeakDef())
       in.weakBinding->addEntry(sym, section, offset, addend);
   } else if (isa<DSOHandle>(sym)) {
     error("cannot bind to " + DSOHandle::name);
@@ -446,8 +444,10 @@ void StubHelperSection::setup() {
   in.got->addEntry(stubBinder);
 
   inputSections.push_back(in.imageLoaderCache);
-  symtab->addDefined("__dyld_private", in.imageLoaderCache, 0,
-                     /*isWeakDef=*/false);
+  dyldPrivate =
+      make<Defined>("__dyld_private", in.imageLoaderCache, 0,
+                    /*isWeakDef=*/false,
+                    /*isExternal=*/false, /*isPrivateExtern=*/false);
 }
 
 ImageLoaderCacheSection::ImageLoaderCacheSection() {
@@ -555,7 +555,7 @@ void macho::prepareBranchTarget(Symbol *sym) {
       }
     }
   } else if (auto *defined = dyn_cast<Defined>(sym)) {
-    if (defined->isWeakDef() && defined->isExternal()) {
+    if (defined->isExternalWeakDef()) {
       if (in.stubs->addEntry(sym)) {
         in.rebase->addEntry(in.lazyPointers, sym->stubsIndex * WordSize);
         in.weakBinding->addEntry(sym, in.lazyPointers,
@@ -570,9 +570,10 @@ ExportSection::ExportSection()
 
 void ExportSection::finalizeContents() {
   trieBuilder.setImageBase(in.header->addr);
-  // TODO: We should check symbol visibility.
   for (const Symbol *sym : symtab->getSymbols()) {
     if (const auto *defined = dyn_cast<Defined>(sym)) {
+      if (defined->privateExtern)
+        continue;
       trieBuilder.addSymbol(*defined);
       hasWeakSymbol = hasWeakSymbol || sym->isWeakDef();
     }
@@ -710,6 +711,13 @@ void SymtabSection::finalizeContents() {
     }
   }
 
+  // __dyld_private is a local symbol too. It's linker-created and doesn't
+  // exist in any object file.
+  if (Defined* dyldPrivate = in.stubHelper->dyldPrivate) {
+    uint32_t strx = stringTableSection.addString(dyldPrivate->getName());
+    localSymbols.push_back({dyldPrivate, strx});
+  }
+
   for (Symbol *sym : symtab->getSymbols()) {
     uint32_t strx = stringTableSection.addString(sym->getName());
     if (auto *defined = dyn_cast<Defined>(sym)) {
@@ -752,18 +760,31 @@ void SymtabSection::writeTo(uint8_t *buf) const {
     nList->n_strx = entry.strx;
     // TODO populate n_desc with more flags
     if (auto *defined = dyn_cast<Defined>(entry.sym)) {
+      uint8_t scope = 0;
+      if (defined->privateExtern) {
+        // Private external -- dylib scoped symbol.
+        // Promote to non-external at link time.
+        assert(defined->isExternal() && "invalid input file");
+        scope = MachO::N_PEXT;
+      } else if (defined->isExternal()) {
+        // Normal global symbol.
+        scope = MachO::N_EXT;
+      } else {
+        // TU-local symbol from localSymbols.
+        scope = 0;
+      }
+
       if (defined->isAbsolute()) {
-        nList->n_type = MachO::N_EXT | MachO::N_ABS;
+        nList->n_type = scope | MachO::N_ABS;
         nList->n_sect = MachO::NO_SECT;
         nList->n_value = defined->value;
       } else {
-        nList->n_type =
-            (defined->isExternal() ? MachO::N_EXT : 0) | MachO::N_SECT;
+        nList->n_type = scope | MachO::N_SECT;
         nList->n_sect = defined->isec->parent->index;
         // For the N_SECT symbol type, n_value is the address of the symbol
         nList->n_value = defined->getVA();
       }
-      nList->n_desc |= defined->isWeakDef() ? MachO::N_WEAK_DEF : 0;
+      nList->n_desc |= defined->isExternalWeakDef() ? MachO::N_WEAK_DEF : 0;
     } else if (auto *dysym = dyn_cast<DylibSymbol>(entry.sym)) {
       uint16_t n_desc = nList->n_desc;
       MachO::SET_LIBRARY_ORDINAL(n_desc, dysym->file->ordinal);

lld/MachO/SyntheticSections.h

@@ -327,6 +327,7 @@ class StubHelperSection : public SyntheticSection {
   void setup();
 
   DylibSymbol *stubBinder = nullptr;
+  Defined *dyldPrivate = nullptr;
 };
 
 // This section contains space for just a single word, and will be used by dyld

lld/test/MachO/dylink-lazy.s

@@ -27,7 +27,7 @@
 # RUN: llvm-objdump --macho --rebase %t/dylink-lazy-pie | FileCheck %s --check-prefix=PIE
 
 # CHECK-LABEL: SYMBOL TABLE:
-# CHECK:       {{0*}}[[#%x, IMGLOADER:]] {{.*}} __DATA,__data __dyld_private
+# CHECK:       {{0*}}[[#%x, IMGLOADER:]] l {{.*}} __DATA,__data __dyld_private
 
 # CHECK-LABEL: Disassembly of section __TEXT,__text:
 # CHECK:         callq 0x[[#%x, HELLO_STUB:]]