[lld-macho] `-exported_symbols` should hide symbols before LTO runs

[int3]

Summary

We were previously doing it after LTO, which did have the desired effect
of having the un-exported symbols marked as private extern in the final
output binary, but doing it before LTO creates more optimization
opportunities.

One observable difference is that LTO can now elide un-exported symbols
entirely, so they may not even be present as private externs in the
output.

This is also what ld64 implements.

[int3]

lld/MachO/Driver.cpp

1645–1648

had to move these up as well as they need to run before handleExplicitExports

[int3]

tweak comment

[int3]

update

[thevinster]

lld/MachO/Driver.cpp

540

Instead of needing to set didCompile on every loop iteration, we could extract lto->compile into another variable and set didCompile based on the size of that variable? It's minor but feels cleaner this way.

[int3]

lld/MachO/Driver.cpp

540

good idea :)

lld/MachO/Driver.cpp

 // we don't bother to do lazily because the initialization is fast.
 static void initLLVM() {
   InitializeAllTargets();
   InitializeAllTargetMCs();
   InitializeAllAsmPrinters();
   InitializeAllAsmParsers();
 }
 
-static void compileBitcodeFiles() {
+static bool compileBitcodeFiles() {
   TimeTraceScope timeScope("LTO");
   auto *lto = make<BitcodeCompiler>();
   for (InputFile *file : inputFiles)
     if (auto *bitcodeFile = dyn_cast<BitcodeFile>(file))
       if (!file->lazy)
         lto->add(*bitcodeFile);
 
-  for (ObjFile *file : lto->compile())
+  std::vector<ObjFile *> compiled = lto->compile();
+  for (ObjFile *file : compiled)
     inputFiles.insert(file);
+
+  return !compiled.empty();
 }
 
 // Replaces common symbols with defined symbols residing in __common sections.
 // This function must be called after all symbol names are resolved (i.e. after
 // all InputFiles have been loaded.) As a result, later operations won't see
 // any CommonSymbols.
 static void replaceCommonSymbols() {
   TimeTraceScope timeScope("Replace common symbols");
static void referenceStubBinder() {
   // dyld_stub_binder is in libSystem.dylib, which is usually linked in. This
   // isn't needed for correctness, but the presence of that symbol suppresses
   // "no symbols" diagnostics from `nm`.
   // StubHelperSection::setup() adds a reference and errors out if
   // dyld_stub_binder doesn't exist in case it is actually needed.
   symtab->addUndefined("dyld_stub_binder", /*file=*/nullptr, /*isWeak=*/false);
 }
 
+static void createAliases() {
+  for (const auto &pair : config->aliasedSymbols) {
+    if (const auto &sym = symtab->find(pair.first)) {
+      if (const auto &defined = dyn_cast<Defined>(sym)) {
+        symtab->aliasDefined(defined, pair.second);
+        continue;
+      }
+    }
+
+    warn("undefined base symbol '" + pair.first + "' for alias '" +
+         pair.second + "'\n");
+  }
+}
+
+static void handleExplicitExports() {
+  if (config->hasExplicitExports) {
+    parallelForEach(symtab->getSymbols(), [](Symbol *sym) {
+      if (auto *defined = dyn_cast<Defined>(sym)) {
+        StringRef symbolName = defined->getName();
+        if (config->exportedSymbols.match(symbolName)) {
+          if (defined->privateExtern) {
+            if (defined->weakDefCanBeHidden) {
+              // weak_def_can_be_hidden symbols behave similarly to
+              // private_extern symbols in most cases, except for when
+              // it is explicitly exported.
+              // The former can be exported but the latter cannot.
+              defined->privateExtern = false;
+            } else {
+              warn("cannot export hidden symbol " + toString(*defined) +
+                   "\n>>> defined in " + toString(defined->getFile()));
+            }
+          }
+        } else {
+          defined->privateExtern = true;
+        }
+      }
+    });
+  } else if (!config->unexportedSymbols.empty()) {
+    parallelForEach(symtab->getSymbols(), [](Symbol *sym) {
+      if (auto *defined = dyn_cast<Defined>(sym))
+        if (config->unexportedSymbols.match(defined->getName()))
+          defined->privateExtern = true;
+    });
+  }
+}
+
 bool macho::link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
                  llvm::raw_ostream &stderrOS, bool exitEarly,
                  bool disableOutput) {
   // This driver-specific context will be freed later by lldMain().
   auto *ctx = new CommonLinkerContext;
 
   ctx->e.initialize(stdoutOS, stderrOS, exitEarly, disableOutput);
   ctx->e.cleanupCallback = []() {
if (config->timeTraceEnabled)
     // Parse LTO options.
     if (const Arg *arg = args.getLastArg(OPT_mcpu))
       parseClangOption(saver().save("-mcpu=" + StringRef(arg->getValue())),
                        arg->getSpelling());
 
     for (const Arg *arg : args.filtered(OPT_mllvm))
       parseClangOption(arg->getValue(), arg->getSpelling());
 
-    compileBitcodeFiles();
+    createSyntheticSections();
+    createSyntheticSymbols();
+
+    createAliases();
+    // If we are in "explicit exports" mode, hide everything that isn't
+    // explicitly exported. Do this before running LTO so that LTO can better
+    // optimize.
+    handleExplicitExports();
+    // LTO may emit a non-hidden (extern) object file symbol even if the
+    // corresponding bitcode symbol is hidden. In particular, this happens for
+    // cross-module references to hidden symbols under ThinLTO. Thus, if we
+    // compiled any bitcode files, we must redo the symbol hiding.
+    if (compileBitcodeFiles())
+      handleExplicitExports();
     replaceCommonSymbols();
 
     StringRef orderFile = args.getLastArgValue(OPT_order_file);
     if (!orderFile.empty())
       priorityBuilder.parseOrderFile(orderFile);
 
     referenceStubBinder();
 
     // FIXME: should terminate the link early based on errors encountered so
     // far?
 
-    createSyntheticSections();
-    createSyntheticSymbols();
-
-    for (const auto &pair : config->aliasedSymbols) {
-      if (const auto &sym = symtab->find(pair.first)) {
-        if (const auto &defined = dyn_cast<Defined>(sym)) {
-          symtab->aliasDefined(defined, pair.second);
-          continue;
-        }
-      }
-
-      warn("undefined base symbol '" + pair.first + "' for alias '" +
-           pair.second + "'\n");
-    }
-
-    if (config->hasExplicitExports) {
-      parallelForEach(symtab->getSymbols(), [](Symbol *sym) {
-        if (auto *defined = dyn_cast<Defined>(sym)) {
-          StringRef symbolName = defined->getName();
-          if (config->exportedSymbols.match(symbolName)) {
-            if (defined->privateExtern) {
-              if (defined->weakDefCanBeHidden) {
-                // weak_def_can_be_hidden symbols behave similarly to
-                // private_extern symbols in most cases, except for when
-                // it is explicitly exported.
-                // The former can be exported but the latter cannot.
-                defined->privateExtern = false;
-              } else {
-                warn("cannot export hidden symbol " + toString(*defined) +
-                     "\n>>> defined in " + toString(defined->getFile()));
-              }
-            }
-          } else {
-            defined->privateExtern = true;
-          }
-        }
-      });
-    } else if (!config->unexportedSymbols.empty()) {
-      parallelForEach(symtab->getSymbols(), [](Symbol *sym) {
-        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/test/MachO/lto-explicit-exports.ll

+; REQUIRES: x86
+; RUN: rm -rf %t; split-file %s %t
+
+;; Check that `-exported_symbol` causes all non-exported symbols to be marked
+;; as hidden before LTO. We don't want to downgrade them to private extern only
+;; after LTO runs as that likely causes LTO to miss optimization opportunities.
+
+; RUN: llvm-as %t/foo.ll -o %t/foo.o
+; RUN: llvm-as %t/refs-foo.ll -o %t/refs-foo.o
+
+; RUN: %lld -lSystem -dylib %t/foo.o %t/refs-foo.o -o %t/test-fulllto \
+; RUN:  -save-temps -exported_symbol _refs_foo -exported_symbol _same_module_caller
+
+; RUN: llvm-dis %t/test-fulllto.0.2.internalize.bc -o - | FileCheck %s --check-prefix=FULLLTO
+; RUN: llvm-objdump --macho --syms %t/test-fulllto | FileCheck %s --check-prefix=FULLLTO-SYMS
+
+; FULLLTO: define internal void @foo()
+; FULLLTO: define internal void @same_module_callee()
+; FULLLTO: define dso_local void @same_module_caller()
+; FULLLTO: define dso_local void @refs_foo()
+
+;; LTO is able to elide the hidden symbols, and they will be entirely absent
+;; from the final symbol table.
+
+; FULLLTO-SYMS:       SYMBOL TABLE:
+; FULLLTO-SYMS:       g     F __TEXT,__text _same_module_caller
+; FULLLTO-SYMS:       g     F __TEXT,__text _refs_foo
+; FULLLTO-SYMS:       *UND* dyld_stub_binder
+; FULLLTO-SYMS-EMPTY:
+
+;; ThinLTO is unable to internalize symbols that are referenced from another
+;; module. Verify that we still mark the final symbol as private extern.
+
+; RUN: opt -module-summary %t/foo.ll -o %t/foo.thinlto.o
+; RUN: opt -module-summary %t/refs-foo.ll -o %t/refs-foo.thinlto.o
+
+; RUN: %lld -lSystem -dylib %t/foo.thinlto.o %t/refs-foo.thinlto.o -o %t/test-thinlto \
+; RUN:  -save-temps -exported_symbol _refs_foo -exported_symbol _same_module_caller
+
+; RUN: llvm-dis %t/foo.thinlto.o.2.internalize.bc -o - | FileCheck %s --check-prefix=THINLTO-FOO
+; RUN: llvm-dis %t/refs-foo.thinlto.o.2.internalize.bc -o - | FileCheck %s --check-prefix=THINLTO-REFS-FOO
+; RUN: llvm-objdump --macho --syms %t/test-thinlto | FileCheck %s --check-prefix=THINLTO-SYMS
+
+; THINLTO-FOO: define dso_local void @foo()
+; THINLTO-FOO: define internal void @same_module_callee()
+; THINLTO-REFS-FOO: declare dso_local void @foo()
+; THINLTO-REFS-FOO: define dso_local void @refs_foo()
+
+; THINLTO-SYMS: l     F __TEXT,__text _foo
+; THINLTO-SYMS: g     F __TEXT,__text _same_module_caller
+; THINLTO-SYMS: g     F __TEXT,__text _refs_foo
+
+;--- foo.ll
+
+target triple = "x86_64-apple-macosx10.15.0"
+target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+
+define void @foo() {
+  ret void
+}
+
+define void @same_module_callee() {
+  ret void
+}
+
+define void @same_module_caller() {
+  call void @same_module_callee()
+  ret void
+}
+
+;--- refs-foo.ll
+
+target triple = "x86_64-apple-macosx10.15.0"
+target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+
+declare void @foo()
+
+define void @refs_foo() {
+  call void @foo()
+  ret void
+}