[lld/mac] Implement support for private extern symbols

[thakis]

Summary

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.

[thakis]

• update tests (even found a bug in the common symbol handling)
• tweak behavior of two commons with same size to match ld64
• add many tests. i verified that ld64 has the same behavior on these tests

still need to look at weak private external XXXs.

[int3]

looks pretty good!

lld/MachO/Driver.cpp

528

nit: seems unnecessary to have the argument name in a comment when the argument itself is descriptive, but I guess it might look more uniform this way. up to you

lld/MachO/Symbols.h

28

This defeats the purpose of using StringRefZ over StringRef though -- it's meant to be a performance optimization (see the comment on the identically-named class in lld/ELF... I probably should have copied it over and/or factored the class out)

Would a constexpr constructor work here?

123

how about combining privateExtern and external into one enum, so that the invalid state of external = false, privateExtern = true is not representable?

lld/test/MachO/private-extern.s

118

how do you feel about hyphenating the check prefix?

lld/test/MachO/symtab.s

39

I think including the string offsets in the expected test output was a mistake (probably mine), let's remove it to reduce future churn

[thakis]

address XXXs about weak private externs and add test for them

I'll reply to comments in a bit. This is now done (except for review comments).

[thakis]

lld/MachO/SyntheticSections.cpp

381

This is the only change from defined->isWeakDef() && defined->isExternal() to isExternalWeakDef() where I couldn't come up with a test case that shows the difference.

[thakis]

address comments

[thakis]

Comments addressed, thanks!

Re binding vs private externals: See also "references to internal symbol never need binding" comments in ld64.

lld/MachO/Driver.cpp

528

Yup, done. That's a left-over from an earlier iteration.

lld/MachO/Symbols.h

28

This ctor is exclusively used for the __dyld_private symbol, so performance probably isn't all that important :) Do you expect this to be used more often? Maybe it's not intentional that it's used so little?

But sure, undid this and instead imported the technique for dealing with this from ELF to Symbol::getName().

123

The current setup has the advantage that the external bit can stay const, which is why I when with two independent bits. It's also closer to how things look in the input and output files. ld64 does this dance where it converts all file bits to its own concepts at input and back at output and I find that more confusing then helpful.

I don't feel terribly strongly about that though. If you want I can change it to an enum.

lld/test/MachO/private-extern.s

118

No strong opinion. The advantage of no hyphens is that -NOT, -DAG etc look more obviously special, the downside is that it's a bit harder to read. Added hyphens.

[int3]

nice work!

lld/MachO/SymbolTable.cpp

15

leftover include?

lld/MachO/Symbols.h

28

oops, not having that ctor used elsewhere was an oversight I guess

51

is the cast necessary?

123

ah fair enough. it's fine as-is

lld/test/MachO/private-extern.s

105

ultra nit

lld/test/MachO/weak-private-extern.s

15

20–21

other reviewers have expressed a preference against using obj2yaml in tests unless absolutely necessary... though I don't remember the exact reasons (cc @smeenai). In this case I think we could use llvm-readobj --syms -- it would make the expected value more readable too

25

I was going to suggest that it might be worth testing TLV weak private externs too, then I realized that we lacked a test for regular TLV weakdefs 🤭

we could address both in a separate diff I guess

[thakis]

Thanks! Submitting.

lld/MachO/Symbols.h

51

../../lld/MachO/Symbols.h:51:18: warning: comparison of integers of different signs: 'uint32_t' (aka 'unsigned int') and 'int' [-Wsign-compare]

lld/test/MachO/weak-private-extern.s

20–21

Oh yeah, that's nicer, thanks.

25

Will send you tests for TLV weaks tomorrow :)

[thakis]

(Also makes linking Chromium Framework faster >20%, from ~5.1s to ~3.8s or so. Didn't profile why -- maybe the export trie construction algorithm needs some speeding up if that should be the cause. I guess I should profile before/after.)

[thakis]

Actual bench.py / ministat before/after data:

    N           Min           Max        Median           Avg        Stddev
x   5     4.9493809      5.168751     5.0355039     5.0416348   0.082035677
+   5     3.7375929     3.9309309     3.7923329     3.8123539   0.077711957
Difference at 95.0% confidence
	-1.22928 +/- 0.116534
	-24.3826% +/- 2.31143%
	(Student's t, pooled s = 0.0799031)

Here's a profile of the "before" run: https://imgur.com/a/fzG2yqw In the "after" profile, TrieBuilder isn't in the profile at all (…not surprisingly, since it's ~20 exports vs 797022 before). So for links that have many exports (possibly Chromiums component build; haven't tried that yet, but it's many smaller dylibs there instead of a single huge one), TrieBuilder might need work at some point in the future. For now, this change here sidesteps that :)

lld/MachO/Driver.cpp

for (macho::Symbol *sym : symtab->getSymbols()) {
     // but it's not really worth supporting the linking of 64-bit programs on
     // 32-bit archs.
     isec->data = {nullptr, static_cast<size_t>(common->size)};
     isec->flags = S_ZEROFILL;
     inputSections.push_back(isec);
 
     replaceSymbol<Defined>(sym, sym->getName(), isec, /*value=*/0,
                            /*isWeakDef=*/false,
-                           /*isExternal=*/true);
+                           /*isExternal=*/true, common->privateExtern);
   }
 }
 
 static inline char toLowerDash(char x) {
   if (x >= 'A' && x <= 'Z')
     return x - 'A' + 'a';
   else if (x == ' ')
     return '-';
   return x;

lld/MachO/InputFiles.cpp

for (size_t i = 0; i < relInfos.size(); i++) {
     InputSection *subsec = findContainingSubsection(subsecMap, &r.offset);
     subsec->relocs.push_back(r);
   }
 }
 
 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,
                                               StringRef name) {
   uint8_t type = sym.n_type & N_TYPE;
   switch (type) {
   case N_UNDF:
     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:
   case N_INDR:
     error("TODO: support symbols of type " + std::to_string(type));
     return nullptr;
   case N_SECT:
     llvm_unreachable(

lld/MachO/Options.td

def bundle_loader : Separate<["-"], "bundle_loader">,
      MetaVarName<"<executable>">,
      HelpText<"Resolve undefined symbols from <executable>">,
      Flags<[HelpHidden]>,
      Group<grp_bundle>;
 
 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">,
      HelpText<"Force tentative into real definitions for common symbols">,
      Flags<[HelpHidden]>,
      Group<grp_object>;
 
 def grp_resolve : OptionGroup<"resolve">, HelpText<"SYMBOL RESOLUTION">;

lld/MachO/SymbolTable.h

  * Note that the SymbolTable handles name collisions by calling
  * replaceSymbol(), which does an in-place update of the Symbol via `placement
  * new`. Therefore, there is no need to update any relocations that hold
  * pointers the "old" Symbol -- they will automatically point to the new one.
  */
 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);
 
   Symbol *addLazy(StringRef name, ArchiveFile *file,
                   const llvm::object::Archive::Symbol &sym);
 
   Symbol *addDSOHandle(const MachHeaderSection *);
 

lld/MachO/SymbolTable.cpp

 //===- SymbolTable.cpp ----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "SymbolTable.h"
 #include "Config.h"
 #include "InputFiles.h"
 #include "Symbols.h"
 #include "lld/Common/ErrorHandler.h"
 #include "lld/Common/Memory.h"
+#include <assert.h>
 
 using namespace llvm;
 using namespace lld;
 using namespace lld::macho;
 
 Symbol *SymbolTable::find(StringRef name) {
   auto it = symMap.find(llvm::CachedHashStringRef(name));
   if (it == symMap.end())
std::pair<Symbol *, bool> SymbolTable::insert(StringRef name) {
 
   // Name is a new symbol.
   Symbol *sym = reinterpret_cast<Symbol *>(make<SymbolUnion>());
   symVector.push_back(sym);
   return {sym, true};
 }
 
 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)) {
       overridesWeakDef = !isWeakDef && dysym->isWeakDef();
     }
     // Defined symbols take priority over other types of symbols, so in case
     // 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;
 }
 
 Symbol *SymbolTable::addUndefined(StringRef name, bool isWeakRef) {
   Symbol *s;
   bool wasInserted;
   std::tie(s, wasInserted) = insert(name);
 
   auto refState = isWeakRef ? RefState::Weak : RefState::Strong;
 
   if (wasInserted)
     replaceSymbol<Undefined>(s, name, refState);
   else if (auto *lazy = dyn_cast<LazySymbol>(s))
     lazy->fetchArchiveMember();
   else if (auto *dynsym = dyn_cast<DylibSymbol>(s))
     dynsym->refState = std::max(dynsym->refState, refState);
   else if (auto *undefined = dyn_cast<Undefined>(s))
     undefined->refState = std::max(undefined->refState, refState);
   return s;
 }
 
 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);
 
   if (!wasInserted) {
     if (auto *common = dyn_cast<CommonSymbol>(s)) {
       if (size < common->size)
         return s;
     } else if (isa<Defined>(s)) {
       return s;
     }
     // Common symbols take priority over all non-Defined symbols, so in case of
     // 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;
 }
 
 Symbol *SymbolTable::addDylib(StringRef name, DylibFile *file, bool isWeakDef,
                               bool isTlv) {
   Symbol *s;
   bool wasInserted;
   std::tie(s, wasInserted) = insert(name);

lld/MachO/Symbols.h

 namespace macho {
 
 class InputSection;
 class MachHeaderSection;
 class DylibFile;
 class ArchiveFile;
 
 struct StringRefZ {
   StringRefZ(const char *s) : data(s), size(-1) {}
   StringRefZ(StringRef s) : data(s.data()), size(s.size()) {}
 
   const char *data;
   const uint32_t size;
 };
 
 class Symbol {
 public:
   enum Kind {
     DefinedKind,
     UndefinedKind,
     CommonKind,
     DylibKind,
     LazyKind,
     DSOHandleKind,
   };
 
   virtual ~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; }
 
   virtual uint64_t getFileOffset() const {
     llvm_unreachable("attempt to get an offset from a non-defined symbol");
   }
 
   virtual bool isWeakDef() const { llvm_unreachable("cannot be weak def"); }
public:
   // both these sections at once.
   uint32_t gotIndex = UINT32_MAX;
 
   uint32_t stubsIndex = UINT32_MAX;
 
   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);
   }
 
   bool isExternal() const { return external; }
   bool isAbsolute() const { return isec == nullptr; }
 
   uint64_t getVA() const override;
   uint64_t getFileOffset() const override;
 
   static bool classof(const Symbol *s) { return s->kind() == DefinedKind; }
 
   InputSection *isec;
   uint32_t value;
 
   bool overridesWeakDef : 1;
+  bool privateExtern : 1;
 
 private:
   const bool weakDef : 1;
   const bool external : 1;
 };
 
 // This enum does double-duty: as a symbol property, it indicates whether & how
 // a dylib symbol is referenced. As a DylibFile property, it indicates the kind

 //
 // Using tentative definitions is usually considered a bad practice; you should
 // write only declarations (such as "extern int foo;") to header files.
 // Nevertheless, the linker and the compiler have to do something to support
 // bad code by allowing duplicate definitions for this particular case.
 //
 // 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
   }
 
   static bool classof(const Symbol *s) { return s->kind() == CommonKind; }
 
   InputFile *const file;
   const uint64_t size;
   const uint32_t align;
+  const bool privateExtern;
 };
 
 class DylibSymbol : public Symbol {
 public:
   DylibSymbol(DylibFile *file, StringRefZ name, bool isWeakDef,
               RefState refState, bool isTlv)
       : Symbol(DylibKind, name), file(file), refState(refState),
         weakDef(isWeakDef), tlv(isTlv) {}

lld/MachO/SyntheticSections.h

public:
   StubHelperSection();
   uint64_t getSize() const override;
   bool isNeeded() const override;
   void writeTo(uint8_t *buf) const override;
 
   void setup();
 
   DylibSymbol *stubBinder = nullptr;
+  Defined *dyldPrivate = nullptr;
 };
 
 // This section contains space for just a single word, and will be used by dyld
 // to cache an address to the image loader it uses. Note that unlike the other
 // synthetic sections, which are OutputSections, the ImageLoaderCacheSection is
 // an InputSection that gets merged into the __data OutputSection.
 class ImageLoaderCacheSection : public InputSection {
 public:

lld/MachO/SyntheticSections.cpp

if (!bindings.empty() || !definitions.empty())
     os << static_cast<uint8_t>(MachO::BIND_OPCODE_DONE);
 }
 
 void WeakBindingSection::writeTo(uint8_t *buf) const {
   memcpy(buf, contents.data(), contents.size());
 }
 
 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;
 }
 
 void macho::addNonLazyBindingEntries(const Symbol *sym,
                                      SectionPointerUnion section,
                                      uint64_t offset, int64_t addend) {
   if (auto *dysym = dyn_cast<DylibSymbol>(sym)) {
     in.binding->addEntry(dysym, section, offset, addend);
     if (dysym->isWeakDef())
       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);
   } else {
     // Undefined symbols are filtered out in scanRelocations(); we should never
     // get here
     llvm_unreachable("cannot bind to an undefined symbol");
   }
if (stubBinder == nullptr) {
     error("symbol dyld_stub_binder not found (normally in libSystem.dylib). "
           "Needed to perform lazy binding.");
     return;
   }
   stubBinder->refState = RefState::Strong;
   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() {
   segname = segment_names::data;
   name = "__data";
   uint8_t *arr = bAlloc.Allocate<uint8_t>(WordSize);
   memset(arr, 0, WordSize);
   data = {arr, WordSize};
if (in.stubs->addEntry(dysym)) {
                              sym->stubsIndex * WordSize);
         in.weakBinding->addEntry(sym, in.lazyPointers,
                                  sym->stubsIndex * WordSize);
       } else {
         in.lazyBinding->addEntry(dysym);
       }
     }
   } 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,
                                  sym->stubsIndex * WordSize);
       }
     }
   }
 }
 
 ExportSection::ExportSection()
     : LinkEditSection(segment_names::linkEdit, section_names::export_) {}
 
 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();
     }
   }
   size = trieBuilder.build();
 }
 
 void ExportSection::writeTo(uint8_t *buf) const { trieBuilder.writeTo(buf); }
if (auto *objFile = dyn_cast<ObjFile>(file)) {
             uint32_t strx = stringTableSection.addString(sym->getName());
             localSymbols.push_back({sym, strx});
           }
         }
       }
     }
   }
 
+  // __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)) {
       assert(defined->isExternal());
       externalSymbols.push_back({sym, strx});
     } else if (auto *dysym = dyn_cast<DylibSymbol>(sym)) {
       if (dysym->isReferenced())
         undefinedSymbols.push_back({sym, strx});
for (const StabsEntry &entry : stabs) {
     ++nList;
   }
 
   for (const SymtabEntry &entry : concat<const SymtabEntry>(
            localSymbols, externalSymbols, undefinedSymbols)) {
     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);
       nList->n_type = MachO::N_EXT;
       n_desc |= dysym->isWeakRef() ? MachO::N_WEAK_REF : 0;
       nList->n_desc = n_desc;
     }
     ++nList;

lld/test/MachO/dylink-lazy.s

 # RUN: (llvm-objdump -d --no-show-raw-insn --syms --rebase --bind --lazy-bind %t/dylink-lazy; \
 # RUN:  llvm-objdump -D --no-show-raw-insn %t/dylink-lazy) | FileCheck %s
 
 # RUN: %lld -pie -o %t/dylink-lazy-pie \
 # RUN:   -L%t -lhello -lgoodbye %t/dylink-lazy.o -lSystem
 # 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:]]
 # CHECK-NEXT:    callq 0x[[#%x, GOODBYE_STUB:]]
 
 ## Check that the rebase table is empty.
 # CHECK-LABEL: Rebase table:
 # CHECK-NEXT:  segment section address type

lld/test/MachO/private-extern.s

+# REQUIRES: x86
+
+# RUN: split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos \
+# RUN:     %t/basics.s -o %t/basics.o
+
+## Check that .private_extern symbols are marked as local in the symbol table
+## and aren't in the export trie.
+# RUN: %lld -dylib %t/basics.o -o %t/basics
+# RUN: llvm-objdump --syms --exports-trie %t/basics | \
+# RUN:     FileCheck --check-prefix=EXPORTS %s
+# RUN: llvm-nm -m %t/basics | FileCheck --check-prefix=EXPORTS-NM %s
+# EXPORTS-LABEL: SYMBOL TABLE:
+# EXPORTS-DAG:   [[#%x, FOO_ADDR:]] l {{.*}} _foo
+# EXPORTS-DAG:   [[#%x, BAR_ADDR:]] g {{.*}} _bar
+# EXPORTS-LABEL: Exports trie:
+# EXPORTS-NOT:   0x{{0*}}[[#%X, FOO_ADDR]] _foo
+# EXPORTS-DAG:   0x{{0*}}[[#%X, BAR_ADDR]] _bar
+# EXPORTS-NOT:   0x{{0*}}[[#%X, FOO_ADDR]] _foo
+# EXPORTS-NM-DAG: (__TEXT,__cstring) non-external (was a private external) _foo
+# EXPORTS-NM-DAG: (__TEXT,__cstring) external _bar
+
+#--- basics.s
+.section __TEXT,__cstring
+
+.globl _foo, _bar
+.private_extern _foo
+
+_foo:
+.asciz "Foo"
+
+_bar:
+.asciz "Bar"
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos \
+# RUN:     %t/strong-globl.s -o %t/strong-globl.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos \
+# RUN:     %t/weak-globl.s -o %t/weak-globl.o
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos \
+# RUN:     %t/strong-private.s -o %t/strong-private.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos \
+# RUN:     %t/weak-private.s -o %t/weak-private.o
+
+## weak + strong symbol takes privateness from strong symbol
+## - weak private extern + strong extern = strong extern (for both .o orderings)
+# RUN: %lld -dylib %t/weak-private.o %t/strong-globl.o -o %t/wpsg
+# RUN: llvm-nm -m %t/wpsg | FileCheck --check-prefix=EXTERNAL %s
+# RUN: %lld -dylib %t/strong-globl.o %t/weak-private.o -o %t/sgwp
+# RUN: llvm-nm -m %t/sgwp | FileCheck --check-prefix=EXTERNAL %s
+# EXTERNAL: (__TEXT,__text) external _foo
+## - weak extern + strong private extern = strong private extern
+##   (for both .o orderings)
+# RUN: %lld -dylib %t/weak-globl.o %t/strong-private.o -o %t/wgsp
+# RUN: llvm-nm -m %t/wgsp | FileCheck --check-prefix=NONEXTERNAL %s
+# RUN: %lld -dylib %t/strong-private.o %t/weak-globl.o -o %t/spwg
+# RUN: llvm-nm -m %t/spwg | FileCheck --check-prefix=NONEXTERNAL %s
+# NONEXTERNAL: (__TEXT,__text) non-external (was a private external) _foo
+
+## weak + weak symbol take weaker privateness
+## - weak extern + weak private extern = weak extern (both orders)
+# RUN: %lld -dylib %t/weak-private.o %t/weak-globl.o -o %t/wpwg
+# RUN: llvm-nm -m %t/wpwg | FileCheck --check-prefix=WEAK-EXTERNAL %s
+# RUN: %lld -dylib %t/weak-globl.o %t/weak-private.o -o %t/wgwp
+# RUN: llvm-nm -m %t/wgwp | FileCheck --check-prefix=WEAK-EXTERNAL %s
+# WEAK-EXTERNAL: (__TEXT,__text) weak external _foo
+## - weak private extern + weak private extern = weak private extern
+# RUN: %lld -dylib %t/weak-private.o %t/weak-private.o -o %t/wpwp
+# RUN: llvm-nm -m %t/wpwp | FileCheck --check-prefix=NONEXTERNAL %s
+
+#--- strong-globl.s
+.globl _foo
+_foo:
+  retq
+
+#--- weak-globl.s
+.globl _foo
+.weak_definition _foo
+_foo:
+  retq
+
+#--- strong-private.s
+.private_extern _foo
+.globl _foo
+_foo:
+  retq
+
+#--- weak-private.s
+.private_extern _foo
+.globl _foo
+.weak_definition _foo
+_foo:
+  retq
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos \
+# RUN:     %t/comm-small.s -o %t/comm-small.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos \
+# RUN:     %t/comm-large.s -o %t/comm-large.o
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos \
+# RUN:     %t/comm-small-private.s -o %t/comm-small-private.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos \
+# RUN:     %t/comm-large-private.s -o %t/comm-large-private.o
+
+## for common symbols the larger one wins.
+## - smaller private extern + larger extern = larger extern
+# RUN: %lld -dylib %t/comm-small-private.o %t/comm-large.o -o %t/cspcl
+# RUN: llvm-nm -m %t/cspcl | FileCheck --check-prefix=COMMON-EXTERNAL %s
+# RUN: %lld -dylib %t/comm-large.o %t/comm-small-private.o -o %t/clcsp
+# RUN: llvm-nm -m %t/clcsp | FileCheck --check-prefix=COMMON-EXTERNAL %s
+# COMMON-EXTERNAL: (__DATA,__common) external _foo
+## - smaller extern + larger private extern = larger private extern
+# RUN: %lld -dylib %t/comm-large-private.o %t/comm-small.o -o %t/clpcs
+# RUN: llvm-nm -m %t/clpcs | FileCheck --check-prefix=COMMON-NONEXTERNAL %s
+# RUN: %lld -dylib %t/comm-small.o %t/comm-large-private.o -o %t/csclp
+# RUN: llvm-nm -m %t/csclp | FileCheck --check-prefix=COMMON-NONEXTERNAL %s
+# COMMON-NONEXTERNAL: (__DATA,__common) non-external (was a private external) _foo
+
+# For common symbols with the same size, the privateness of the symbol seen
+# later wins (!).
+## - equal private extern + equal extern = equal extern (both orders)
+# RUN: %lld -dylib %t/comm-small-private.o %t/comm-small.o -o %t/cspcs
+# RUN: llvm-nm -m %t/cspcs | FileCheck --check-prefix=COMMON-EXTERNAL %s
+## - equal extern + equal private extern = equal private extern (both orders)
+# RUN: %lld -dylib %t/comm-small.o %t/comm-small-private.o -o %t/cscsp
+# RUN: llvm-nm -m %t/cscsp | FileCheck --check-prefix=COMMON-NONEXTERNAL %s
+## - equal private extern + equal private extern = equal private extern
+# RUN: %lld -dylib %t/comm-small-private.o %t/comm-small-private.o -o %t/cspcsp
+# RUN: llvm-nm -m %t/cspcsp | FileCheck --check-prefix=COMMON-NONEXTERNAL %s
+
+#--- comm-small.s
+.comm _foo,4,2
+
+#--- comm-large.s
+.comm _foo,8,3
+
+#--- comm-small-private.s
+.private_extern _foo
+.comm _foo,4,2
+
+#--- comm-large-private.s
+.private_extern _foo
+.comm _foo,8,3

lld/test/MachO/symtab.s

 # REQUIRES: x86
 # RUN: rm -rf %t && split-file %s %t
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/test.s -o %t/test.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/libfoo.s -o %t/libfoo.o
 # RUN: %lld -dylib %t/libfoo.o -o %t/libfoo.dylib
 # RUN: %lld -lSystem %t/test.o %t/libfoo.dylib -o %t/test
 
 # RUN: llvm-readobj --syms --macho-dysymtab %t/test | FileCheck %s
 # CHECK:      Symbols [
 # CHECK-NEXT:   Symbol {
-# CHECK-NEXT:     Name: _local (2)
+# CHECK-NEXT:     Name: _local
 # CHECK-NEXT:     Type: Section (0xE)
 # CHECK-NEXT:     Section: __data (0x4)
 # CHECK-NEXT:     RefType: UndefinedNonLazy (0x0)
 # CHECK-NEXT:     Flags [ (0x0)
 # CHECK-NEXT:     ]
 # CHECK-NEXT:     Value: 0x1{{[0-9a-f]*}}
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Symbol {
-# CHECK-NEXT:     Name: _main (9)
-# CHECK-NEXT:     Extern
+# CHECK-NEXT:     Name: __dyld_private
 # CHECK-NEXT:     Type: Section (0xE)
-# CHECK-NEXT:     Section: __text (0x1)
+# CHECK-NEXT:     Section: __data (0x4)
 # CHECK-NEXT:     RefType: UndefinedNonLazy (0x0)
 # CHECK-NEXT:     Flags [ (0x0)
 # CHECK-NEXT:     ]
 # CHECK-NEXT:     Value: 0x1{{[0-9a-f]*}}
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Symbol {
-# CHECK-NEXT:     Name: _external (55)
+# CHECK-NEXT:     Name: _main
 # CHECK-NEXT:     Extern
 # CHECK-NEXT:     Type: Section (0xE)
-# CHECK-NEXT:     Section: __data (0x4)
+# CHECK-NEXT:     Section: __text (0x1)
 # CHECK-NEXT:     RefType: UndefinedNonLazy (0x0)
 # CHECK-NEXT:     Flags [ (0x0)
 # CHECK-NEXT:     ]
 # CHECK-NEXT:     Value: 0x1{{[0-9a-f]*}}
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Symbol {
-# CHECK-NEXT:     Name: _external_weak (65)
+# CHECK-NEXT:     Name: _external
 # CHECK-NEXT:     Extern
 # CHECK-NEXT:     Type: Section (0xE)
-# CHECK-NEXT:     Section: __text (0x1)
+# CHECK-NEXT:     Section: __data (0x4)
 # CHECK-NEXT:     RefType: UndefinedNonLazy (0x0)
-# CHECK-NEXT:     Flags [ (0x80)
-# CHECK-NEXT:       WeakDef (0x80)
+# CHECK-NEXT:     Flags [ (0x0)
 # CHECK-NEXT:     ]
 # CHECK-NEXT:     Value: 0x1{{[0-9a-f]*}}
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Symbol {
-# CHECK-NEXT:     Name: __dyld_private (103)
+# CHECK-NEXT:     Name: _external_weak
 # CHECK-NEXT:     Extern
 # CHECK-NEXT:     Type: Section (0xE)
-# CHECK-NEXT:     Section: __data (0x4)
+# CHECK-NEXT:     Section: __text (0x1)
 # CHECK-NEXT:     RefType: UndefinedNonLazy (0x0)
-# CHECK-NEXT:     Flags [ (0x0)
+# CHECK-NEXT:     Flags [ (0x80)
+# CHECK-NEXT:       WeakDef (0x80)
 # CHECK-NEXT:     ]
 # CHECK-NEXT:     Value: 0x1{{[0-9a-f]*}}
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Symbol {
-# CHECK-NEXT:     Name: dyld_stub_binder (15)
+# CHECK-NEXT:     Name: dyld_stub_binder
 # CHECK-NEXT:     Extern
 # CHECK-NEXT:     Type: Undef (0x0)
 # CHECK-NEXT:     Section:  (0x0)
 # CHECK-NEXT:     RefType: UndefinedNonLazy (0x0)
 # CHECK-NEXT:     Flags [ (0x100)
 # CHECK-NEXT:     ]
 # CHECK-NEXT:     Value: 0x0
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Symbol {
-# CHECK-NEXT:     Name: _dynamic (80)
+# CHECK-NEXT:     Name: _dynamic
 # CHECK-NEXT:     Extern
 # CHECK-NEXT:     Type: Undef (0x0)
 # CHECK-NEXT:     Section:  (0x0)
 # CHECK-NEXT:     RefType: UndefinedNonLazy (0x0)
 # CHECK-NEXT:     Flags [ (0x200)
 # CHECK-NEXT:       AltEntry (0x200)
 # CHECK-NEXT:     ]
 # CHECK-NEXT:     Value: 0x0
 # CHECK-NEXT:   }
 # CHECK-NEXT: ]
 # CHECK-NEXT: Dysymtab {
 # CHECK-NEXT:   ilocalsym: 0
-# CHECK-NEXT:   nlocalsym: 1
-# CHECK-NEXT:   iextdefsym: 1
-# CHECK-NEXT:   nextdefsym: 4
+# CHECK-NEXT:   nlocalsym: 2
+# CHECK-NEXT:   iextdefsym: 2
+# CHECK-NEXT:   nextdefsym: 3
 # CHECK-NEXT:   iundefsym: 5
 # CHECK-NEXT:   nundefsym: 2
 
 ## Verify that the first entry in the StringTable is a space.
 # RUN: obj2yaml %t/test | FileCheck %s --check-prefix=YAML
 # YAML:      StringTable:
 # YAML-NEXT: ' '
 

lld/test/MachO/weak-private-extern.s

+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos %s -o %t.o
+# RUN: %lld -dylib %t.o -o %t.dylib -lSystem
+# RUN: llvm-objdump --macho --weak-bind %t.dylib | FileCheck %s
+# CHECK-NOT: __got
+# CHECK-NOT: __la_symbol_ptr
+
+# RUN: llvm-objdump --macho --all-headers %t.dylib | \
+# RUN:     FileCheck --check-prefix=HEADERS %s
+# HEADERS-NOT: WEAK_DEFINES
+# HEADERS-NOT: BINDS_TO_WEAK
+
+## Check that N_WEAK_DEF isn't set in the symbol table.
+## This is different from ld64, which makes privat extern weak symbols non-weak
+## for binds and relocations, but it still marks them as weak in the symbol table.
+## Since `nm -m` doesn't look at N_WEAK_DEF for N_PEXT symbols this is not
+## observable, but it feels slightly more correct.
+# RUN: obj2yaml %t.dylib | FileCheck --check-prefix=YAML %s
+# YAML-NOT: n_desc:          128
+# YAML-NOT: n_desc:          128
+
+.globl _use
+_use:
+  mov _weak_private_extern_gotpcrel@GOTPCREL(%rip), %rax
+  callq _weak_private_extern
+  retq
+
+.private_extern _weak_private_extern
+.globl _weak_private_extern
+.weak_definition _weak_private_extern
+_weak_private_extern:
+  retq
+
+.private_extern _weak_private_extern_gotpcrel
+.globl _weak_private_extern_gotpcrel
+.weak_definition _weak_private_extern_gotpcrel
+_weak_private_extern_gotpcrel:
+  .quad 0x1234