[lld/mac] Propagate -(un)exported_symbol(s_list) to privateExtern in Driver

[thakis]

Summary

That way, it's done only once instead of every time shouldExportSymbol() is
called.

Possibly a bit faster:

% ministat at_main at_symtodo
x at_main
+ at_symtodo
    N           Min           Max        Median           Avg        Stddev
x  30     3.9732189      4.114846      4.024621     4.0304692   0.037022865
+  30       3.93766     4.0510042     3.9973931      3.991469   0.028472565
Difference at 95.0% confidence
        -0.0390002 +/- 0.0170714
        -0.967635% +/- 0.423559%
        (Student's t, pooled s = 0.0330256)

In other runs with n=30 it makes no perf difference, so maybe it's just noise.
But being able to quickly and conveniently answer "is this symbol exported?"
is useful for fixing PR50373 and for implementing -dead_strip, so this seems
like a good change regardless.

No behavior change.

[int3]

lld/MachO/Driver.cpp

1197

I assume that we are handling exportedSymbols after this line so we can export/unexported synthetic symbols... do we want to have a test for this to check ld64 parity? (If you think that's too obscure to bother with that's fair too)

[thakis]

Thanks! Landed with the tweak you suggested on D102662

lld/MachO/Driver.cpp

1197

__mh_execute_header is the only visible synthetic symbol, so it's really just for that. ld64 does mark that symbol as internal with -Wl,-exported_symbol,_main so that's why I put this here, but adding a test for it felt a bit frivolous to me :D

lld/MachO/Driver.cpp

for (const CachedHashStringRef &cachedName :
         if (const auto *undefined = dyn_cast<Undefined>(sym))
           treatUndefinedSymbol(*undefined, "-exported_symbol(s_list)");
     }
 
     // FIXME: should terminate the link early based on errors encountered so
     // far?
 
     createSyntheticSections();
     createSyntheticSymbols();
 
+    if (!config->exportedSymbols.empty()) {
+      for (Symbol *sym : symtab->getSymbols()) {
+        if (auto *defined = dyn_cast<Defined>(sym)) {
+          StringRef symbolName = defined->getName();
+          if (config->exportedSymbols.match(symbolName)) {
+            if (defined->privateExtern) {
+              error("cannot export hidden symbol " + symbolName +
+                    "\n>>> defined in " + toString(defined->getFile()));
+            }
+          } else {
+            defined->privateExtern = true;
+          }
+        }
+      }
+    } else if (!config->unexportedSymbols.empty()) {
+      for (Symbol *sym : symtab->getSymbols())
+        if (auto *defined = dyn_cast<Defined>(sym))
+          if (config->unexportedSymbols.match(defined->getName()))
+            defined->privateExtern = true;
+    }
+
     for (const Arg *arg : args.filtered(OPT_sectcreate)) {
       StringRef segName = arg->getValue(0);
       StringRef sectName = arg->getValue(1);
       StringRef fileName = arg->getValue(2);
       Optional<MemoryBufferRef> buffer = readFile(fileName);
       if (buffer)
         inputFiles.insert(make<OpaqueFile>(*buffer, segName, sectName));
     }

lld/MachO/SyntheticSections.cpp

os << flags << sym.getName() << '\0'
      << static_cast<uint8_t>(BIND_OPCODE_DO_BIND)
      << static_cast<uint8_t>(BIND_OPCODE_DONE);
   return opstreamOffset;
 }
 
 ExportSection::ExportSection()
     : LinkEditSection(segment_names::linkEdit, section_names::export_) {}
 
-static void validateExportSymbol(const Defined *defined) {
-  StringRef symbolName = defined->getName();
-  if (defined->privateExtern && config->exportedSymbols.match(symbolName))
-    error("cannot export hidden symbol " + symbolName + "\n>>> defined in " +
-          toString(defined->getFile()));
-}
-
-static bool shouldExportSymbol(const Defined *defined) {
-  if (defined->privateExtern) {
-    assert(defined->isExternal() && "invalid input file");
-    return false;
-  }
-  // TODO: Is this a performance bottleneck? If a build has mostly
-  // global symbols in the input but uses -exported_symbols to filter
-  // out most of them, then it would be better to set the value of
-  // privateExtern at parse time instead of calling
-  // exportedSymbols.match() more than once.
-  //
-  // Measurements show that symbol ordering (which again looks up
-  // every symbol in a hashmap) is the biggest bottleneck when linking
-  // chromium_framework, so this will likely be worth optimizing.
-  if (!config->exportedSymbols.empty())
-    return config->exportedSymbols.match(defined->getName());
-  if (!config->unexportedSymbols.empty())
-    return !config->unexportedSymbols.match(defined->getName());
-  return true;
-}
-
 void ExportSection::finalizeContents() {
   trieBuilder.setImageBase(in.header->addr);
   for (const Symbol *sym : symtab->getSymbols()) {
     if (const auto *defined = dyn_cast<Defined>(sym)) {
-      validateExportSymbol(defined);
-      if (!shouldExportSymbol(defined))
+      if (defined->privateExtern)
         continue;
       trieBuilder.addSymbol(*defined);
       hasWeakSymbol = hasWeakSymbol || sym->isWeakDef();
     }
   }
   size = trieBuilder.build();
 }
 
template <class LP> void SymtabSectionImpl<LP>::writeTo(uint8_t *buf) const {
   }
 
   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 (!shouldExportSymbol(defined)) {
+      if (defined->privateExtern) {
         // Private external -- dylib scoped symbol.
         // Promote to non-external at link time.
         scope = N_PEXT;
       } else if (defined->isExternal()) {
         // Normal global symbol.
         scope = N_EXT;
       } else {
         // TU-local symbol from localSymbols.