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

[Nico Weber via Phabricator via llvm-commits]

thakis created this revision.
thakis added a reviewer: lld-macho.
Herald added a reviewer: int3.
Herald added a reviewer: gkm.
Herald added a project: lld-macho.
thakis requested review of this revision.

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)

It also makes the candidate fix in comment 3 of PR50373 "just" a 4% performance
regression instead of a 9.4% performance regression.

No behavior change.


https://reviews.llvm.org/D102661

Files:
  lld/MachO/Driver.cpp
  lld/MachO/SyntheticSections.cpp


Index: lld/MachO/SyntheticSections.cpp
===================================================================
--- lld/MachO/SyntheticSections.cpp
+++ lld/MachO/SyntheticSections.cpp
@@ -566,31 +566,11 @@
 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;
 }
 
@@ -598,7 +578,6 @@
   trieBuilder.setImageBase(in.header->addr);
   for (const Symbol *sym : symtab->getSymbols()) {
     if (const auto *defined = dyn_cast<Defined>(sym)) {
-      validateExportSymbol(defined);
       if (!shouldExportSymbol(defined))
         continue;
       trieBuilder.addSymbol(*defined);
Index: lld/MachO/Driver.cpp
===================================================================
--- lld/MachO/Driver.cpp
+++ lld/MachO/Driver.cpp
@@ -1196,6 +1196,27 @@
     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);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D102661.346031.patch
Type: text/x-patch
Size: 3079 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210518/dcd26247/attachment.bin>