Undo Force Load + Incremental Ban on Darwin Platforms

CodaFi · CodaFi · commit f6b2294d5ac9 · 2021-05-24T15:53:13.000-07:00
Gather 'round to hear tell of the saga of autolinking in incremental
mode.

In the beginning, there was Swift code, and there was Objective-C code.
To make one import bind two languages, a twinned Swift module named the same as an
Objective-C module could be imported as an overlay. But all was not
well, for an overlay could be created which had no Swift content, yet
required Swift symbols. And there was much wailing and gnashing of teeth
as loaders everywhere disregarded loading these content-less Swift
libraries.

So, a solution was found - a magical symbol _swift_FORCE_LOAD_$_&lt;MODULE&gt;
that forced the loaders to heed the dependency on a Swift library
regardless of its content. It was a constant with common linkage, and it
was good. But, along came COFF which needed to support autolinking but
had no support for such matters. It did, however, have support for
COMDAT sections into which we placed the symbol. Immediately, a darkness
fell across the land as the windows linker loudly proclaimed it had
discovered a contradiction: "_swift_FORCE_LOAD_$_&lt;MODULE&gt; cannot be
a constant!", it said, gratingly, "for this value requires rebasing."
Undeterred, we switched to a function instead, and the windows linker
happily added a level of indirection to its symbol resolution procedure
and all was right with the world.

But this definition was not all right. In order to support multiple
translation units emitting it, and to prevent the linker from dead
stripping it, Weak ODR linkage was used. Weak ODR linkage has the nasty
side effect of pessimizing load times since the dynamic linker must
assume that loading a later library could produce a more definitive
definition for the symbol.

A compromise was drawn up: To keep load times low, external linkage was
used. To keep the linker from complaining about multiple strong
definitions for the same symbol, the first translation unit in the
module was nominated to recieve the magic symbol. But one final problem
remained:

Incremental builds allow for files to be added or removed during the
build procedure. The placement of the symbol was therefore dependent
entirely upon the order of files passed at the command line. This was no
good, so a decree was set forth that using -autolink-force-load and
-incremental together was a criminal offense.

So we must compromise once more: Return to a symbol with common linkage,
but only on Mach-O targets. Preserve the existing COMDAT-friendly
approach everywhere else.

This concludes our tale.

rdar://77803299
diff --git a/include/swift/AST/DiagnosticsDriver.def b/include/swift/AST/DiagnosticsDriver.def
@@ -119,9 +119,6 @@ ERROR(error_input_changed_during_build,none,
 ERROR(error_conflicting_options, none,
       "conflicting options '%0' and '%1'",
       (StringRef, StringRef))
-ERROR(error_option_not_supported, none,
-      "'%0' is not supported with '%1'",
-      (StringRef, StringRef))
 ERROR(error_requirement_not_met, none,
       "'%0' requires '%1'",
       (StringRef, StringRef))
diff --git a/include/swift/IRGen/Linking.h b/include/swift/IRGen/Linking.h
@@ -1417,6 +1417,7 @@ struct IRLinkage {
   static const IRLinkage InternalWeakODR;
   static const IRLinkage Internal;
 
+  static const IRLinkage ExternalCommon;
   static const IRLinkage ExternalImport;
   static const IRLinkage ExternalWeakImport;
   static const IRLinkage ExternalExport;
diff --git a/lib/Driver/Driver.cpp b/lib/Driver/Driver.cpp
@@ -251,27 +251,6 @@ static void validateSearchPathArgs(DiagnosticEngine &diags,
   }
 }
 
-static void validateAutolinkingArgs(DiagnosticEngine &diags,
-                                    const ArgList &args,
-                                    const llvm::Triple &T) {
-  auto *forceLoadArg = args.getLastArg(options::OPT_autolink_force_load);
-  if (!forceLoadArg)
-    return;
-  auto *incrementalArg = args.getLastArg(options::OPT_incremental);
-  if (!incrementalArg)
-    return;
-
-  if (T.supportsCOMDAT())
-    return;
-
-  // Note: -incremental can itself be overridden by other arguments later
-  // on, but since -autolink-force-load is a rare and not-really-recommended
-  // option it's not worth modeling that complexity here (or moving the
-  // check somewhere else).
-  diags.diagnose(SourceLoc(), diag::error_option_not_supported,
-                 forceLoadArg->getSpelling(), incrementalArg->getSpelling());
-}
-
 /// Perform miscellaneous early validation of arguments.
 static void validateArgs(DiagnosticEngine &diags, const ArgList &args,
                          const llvm::Triple &T) {
@@ -282,7 +261,6 @@ static void validateArgs(DiagnosticEngine &diags, const ArgList &args,
   validateDebugInfoArgs(diags, args);
   validateCompilationConditionArgs(diags, args);
   validateSearchPathArgs(diags, args);
-  validateAutolinkingArgs(diags, args, T);
   validateVerifyIncrementalDependencyArgs(diags, args);
 }
 
diff --git a/lib/FrontendTool/TBD.cpp b/lib/FrontendTool/TBD.cpp
@@ -84,7 +84,8 @@ static bool validateSymbols(DiagnosticEngine &diags,
     if (auto GV = dyn_cast<llvm::GlobalValue>(value)) {
       // Is this a symbol that should be listed?
       auto externallyVisible =
-          GV->hasExternalLinkage() && !GV->hasHiddenVisibility();
+          (GV->hasExternalLinkage() || GV->hasCommonLinkage())
+        && !GV->hasHiddenVisibility();
       if (!GV->isDeclaration() && externallyVisible) {
         // Is it listed?
         if (!symbolSet.erase(name))
diff --git a/lib/IRGen/IRGenModule.cpp b/lib/IRGen/IRGenModule.cpp
@@ -1300,18 +1300,6 @@ static bool replaceModuleFlagsEntry(llvm::LLVMContext &Ctx,
   llvm_unreachable("Could not replace old linker options entry?");
 }
 
-/// Returns true if the object file generated by \p IGM will be the "first"
-/// object file in the module. This lets us determine where to put a symbol
-/// that must be unique.
-static bool isFirstObjectFileInModule(IRGenModule &IGM) {
-  if (IGM.getSILModule().isWholeModule())
-    return IGM.IRGen.getPrimaryIGM() == &IGM;
-
-  auto *file = cast<FileUnit>(IGM.getSILModule().getAssociatedContext());
-  auto *containingModule = file->getParentModule();
-  return containingModule->getFiles().front() == file;
-}
-
 static bool
 doesTargetAutolinkUsingAutolinkExtract(const SwiftTargetInfo &TargetInfo,
                                        const llvm::Triple &Triple) {
@@ -1486,6 +1474,47 @@ AutolinkKind AutolinkKind::create(const SwiftTargetInfo &TargetInfo,
   return AutolinkKind::LLVMLinkerOptions;
 }
 
+static llvm::GlobalObject *createForceImportThunk(IRGenModule &IGM) {
+  llvm::SmallString<64> buf;
+  encodeForceLoadSymbolName(buf, IGM.IRGen.Opts.ForceLoadSymbolName);
+  if (IGM.Triple.isOSBinFormatMachO()) {
+    // On Mach-O targets, emit a common force-load symbol that resolves to
+    // a global variable so it will be coalesced at static link time into a
+    // single external symbol.
+    //
+    // Looks like C's tentative definitions are good for something after all.
+    auto ForceImportThunk =
+        new llvm::GlobalVariable(IGM.Module,
+                                 IGM.Int1Ty,
+                                 /*isConstant=*/false,
+                                 llvm::GlobalValue::CommonLinkage,
+                                 llvm::Constant::getNullValue(IGM.Int1Ty),
+                                 buf.str());
+    ApplyIRLinkage(IRLinkage::ExternalCommon).to(ForceImportThunk);
+    IGM.addUsedGlobal(ForceImportThunk);
+    return ForceImportThunk;
+  } else {
+    // On all other targets, emit an external symbol that resolves to a
+    // function definition. On Windows, linkonce_odr is basically a no-op and
+    // the COMDAT we set by applying linkage gives us the desired coalescing
+    // behavior.
+    auto ForceImportThunk =
+        llvm::Function::Create(llvm::FunctionType::get(IGM.VoidTy, false),
+                               llvm::GlobalValue::LinkOnceODRLinkage, buf,
+                               &IGM.Module);
+    ForceImportThunk->setAttributes(IGM.constructInitialAttributes());
+    ApplyIRLinkage(IRLinkage::ExternalExport).to(ForceImportThunk);
+    if (IGM.Triple.supportsCOMDAT())
+      if (auto *GO = cast<llvm::GlobalObject>(ForceImportThunk))
+        GO->setComdat(IGM.Module.getOrInsertComdat(ForceImportThunk->getName()));
+
+    auto BB = llvm::BasicBlock::Create(IGM.getLLVMContext(), "", ForceImportThunk);
+    llvm::IRBuilder<> IRB(BB);
+    IRB.CreateRetVoid();
+    return ForceImportThunk;
+  }
+}
+
 void IRGenModule::emitAutolinkInfo() {
   auto Autolink =
       AutolinkKind::create(TargetInfo, Triple, IRGen.Opts.LLVMLTOKind);
@@ -1504,23 +1533,8 @@ void IRGenModule::emitAutolinkInfo() {
 
   Autolink.writeEntries(Entries, Metadata, *this);
 
-  if (!IRGen.Opts.ForceLoadSymbolName.empty() &&
-      (Triple.supportsCOMDAT() || isFirstObjectFileInModule(*this))) {
-    llvm::SmallString<64> buf;
-    encodeForceLoadSymbolName(buf, IRGen.Opts.ForceLoadSymbolName);
-    auto ForceImportThunk =
-        llvm::Function::Create(llvm::FunctionType::get(VoidTy, false),
-                               llvm::GlobalValue::ExternalLinkage, buf,
-                               &Module);
-    ForceImportThunk->setAttributes(constructInitialAttributes());
-    ApplyIRLinkage(IRLinkage::ExternalExport).to(ForceImportThunk);
-    if (Triple.supportsCOMDAT())
-      if (auto *GO = cast<llvm::GlobalObject>(ForceImportThunk))
-        GO->setComdat(Module.getOrInsertComdat(ForceImportThunk->getName()));
-
-    auto BB = llvm::BasicBlock::Create(getLLVMContext(), "", ForceImportThunk);
-    llvm::IRBuilder<> IRB(BB);
-    IRB.CreateRetVoid();
+  if (!IRGen.Opts.ForceLoadSymbolName.empty()) {
+    (void) createForceImportThunk(*this);
   }
 }
 
diff --git a/lib/IRGen/Linking.cpp b/lib/IRGen/Linking.cpp
@@ -50,6 +50,12 @@ const IRLinkage IRLinkage::Internal = {
   llvm::GlobalValue::DefaultStorageClass,
 };
 
+const IRLinkage IRLinkage::ExternalCommon = {
+  llvm::GlobalValue::CommonLinkage,
+  llvm::GlobalValue::DefaultVisibility,
+  llvm::GlobalValue::DLLExportStorageClass,
+};
+
 const IRLinkage IRLinkage::ExternalImport = {
   llvm::GlobalValue::ExternalLinkage,
   llvm::GlobalValue::DefaultVisibility,
diff --git a/lib/TBDGen/TBDGen.cpp b/lib/TBDGen/TBDGen.cpp
@@ -1186,9 +1186,6 @@ static bool hasLinkerDirective(Decl *D) {
 }
 
 void TBDGenVisitor::visitFile(FileUnit *file) {
-  if (file == SwiftModule->getFiles()[0])
-    addFirstFileSymbols();
-
   SmallVector<Decl *, 16> decls;
   file->getTopLevelDecls(decls);
 
@@ -1202,6 +1199,9 @@ void TBDGenVisitor::visitFile(FileUnit *file) {
 }
 
 void TBDGenVisitor::visit(const TBDGenDescriptor &desc) {
+  // Add any autolinking force_load symbols.
+  addFirstFileSymbols();
+  
   if (auto *singleFile = desc.getSingleFile()) {
     assert(SwiftModule == singleFile->getParentModule() &&
            "mismatched file and module");
@@ -1230,6 +1230,7 @@ void TBDGenVisitor::visit(const TBDGenDescriptor &desc) {
     // Diagnose module name that cannot be found
     ctx.Diags.diagnose(SourceLoc(), diag::unknown_swift_module_name, Name);
   }
+
   // Collect symbols in each module.
   llvm::for_each(Modules, [&](ModuleDecl *M) {
     for (auto *file : M->getFiles()) {
diff --git a/test/Driver/autolink-force-load-comdat.swift b/test/Driver/autolink-force-load-comdat.swift
diff --git a/test/Driver/autolink-force-load-no-comdat.swift b/test/Driver/autolink-force-load-no-comdat.swift
diff --git a/test/IRGen/autolink-force-link.swift b/test/IRGen/autolink-force-link.swift
@@ -3,27 +3,25 @@
 // RUN: %swift -disable-legacy-type-info -target x86_64-apple-macosx10.9 -parse-stdlib -autolink-force-load -module-name TEST -module-link-name TEST -emit-ir %s %S/../Inputs/empty.swift | %FileCheck -check-prefix=CHECK-WMO %s
 
 // CHECK-WMO: source_filename = "<swift-imported-modules>"
-// CHECK-WMO: define void @"_swift_FORCE_LOAD_$_TEST"()
+// CHECK-WMO: @"_swift_FORCE_LOAD_$_TEST"
 // CHECK-WMO-NOT: source_filename
 
 
 // RUN: %swift -disable-legacy-type-info -target x86_64-apple-macosx10.9 -parse-stdlib -autolink-force-load -module-name TEST -module-link-name TEST -emit-ir -num-threads 1 %s %S/../Inputs/empty.swift | %FileCheck -check-prefix=CHECK-WMO-THREADED %s
 
 // CHECK-WMO-THREADED: source_filename = "<swift-imported-modules>"
-// CHECK-WMO-THREADED: define void @"_swift_FORCE_LOAD_$_TEST"()
+// CHECK-WMO-THREADED: @"_swift_FORCE_LOAD_$_TEST"
 // CHECK-WMO-THREADED: source_filename = "<swift-imported-modules>"
-// CHECK-WMO-THREADED-NOT: _swift_FORCE_LOAD_$_TEST
-// CHECK-WMO-THREADED-NOT: source_filename
-
+// CHECK-WMO-THREADED: @"_swift_FORCE_LOAD_$_TEST"
 
 // RUN: %swift -disable-legacy-type-info -target x86_64-apple-macosx10.9 -parse-stdlib -autolink-force-load -module-name TEST -module-link-name TEST -emit-ir -primary-file %s %S/../Inputs/empty.swift | %FileCheck -check-prefix=CHECK-SINGLE-FILE-FIRST %s
 // RUN: %swift -disable-legacy-type-info -target x86_64-apple-macosx10.9 -parse-stdlib -autolink-force-load -module-name TEST -module-link-name TEST -emit-ir %S/../Inputs/empty.swift -primary-file %s | %FileCheck -check-prefix=CHECK-SINGLE-FILE-SECOND %s
 
 // CHECK-SINGLE-FILE-FIRST: source_filename = "<swift-imported-modules>"
-// CHECK-SINGLE-FILE-FIRST: define void @"_swift_FORCE_LOAD_$_TEST"()
+// CHECK-SINGLE-FILE-FIRST: @"_swift_FORCE_LOAD_$_TEST"
 // CHECK-SINGLE-FILE-FIRST-NOT: source_filename
 
 // CHECK-SINGLE-FILE-SECOND: source_filename = "<swift-imported-modules>"
-// CHECK-SINGLE-FILE-SECOND-NOT: _swift_FORCE_LOAD_$_TEST
+// CHECK-SINGLE-FILE-SECOND: @"_swift_FORCE_LOAD_$_TEST"
 // CHECK-SINGLE-FILE-SECOND-NOT: source_filename
 
diff --git a/test/Incremental/Inputs/autolinking/AutolinkingTest.framework/AutolinkingTest b/test/Incremental/Inputs/autolinking/AutolinkingTest.framework/AutolinkingTest
diff --git a/test/Incremental/Inputs/autolinking/AutolinkingTest.framework/Headers/AutolinkingTest.h b/test/Incremental/Inputs/autolinking/AutolinkingTest.framework/Headers/AutolinkingTest.h
@@ -0,0 +1,3 @@
+struct Test {
+  int value;
+};
diff --git a/test/Incremental/Inputs/autolinking/AutolinkingTest.framework/Modules/module.modulemap b/test/Incremental/Inputs/autolinking/AutolinkingTest.framework/Modules/module.modulemap
@@ -0,0 +1,5 @@
+framework module AutolinkingTest {
+  umbrella header "AutolinkingTest.h"
+  export *
+  module * { export * }
+}
diff --git a/test/Incremental/Inputs/autolinking/autolinking-other.swift b/test/Incremental/Inputs/autolinking/autolinking-other.swift
@@ -0,0 +1 @@
+
diff --git a/test/Incremental/Inputs/autolinking/ofm.json b/test/Incremental/Inputs/autolinking/ofm.json
@@ -0,0 +1,18 @@
+{
+  "autolinking.swift": {
+    "object": "./autolinking.o",
+    "swift-dependencies": "./autolinking.swiftdeps"
+  },
+  "autolinking-overlay.swift": {
+    "object": "./autolinking-overlay.o",
+    "swift-dependencies": "./autolinking-overlay.swiftdeps"
+  },
+  "autolinking-other.swift": {
+    "object": "./autolinking-other.o",
+    "swift-dependencies": "./autolinking-other.swiftdeps"
+  },
+  "": {
+    "swift-dependencies": "./autolinking~buildrecord.swiftdeps"
+  }
+}
+
diff --git a/test/Incremental/autolinking-overlay.swift b/test/Incremental/autolinking-overlay.swift
@@ -0,0 +1,19 @@
+// This test shares inputs with its cousin autolinking.swift
+// This one specifically tests an overlay-esque build.
+
+// RUN: %empty-directory(%t)
+// RUN: cp %s %t
+// RUN: cp -r %S/Inputs/autolinking/* %t
+// RUN: cd %t
+
+// RUN: %target-build-swift-dylib(%t/%target-library-name(AutolinkingTest)) -autolink-force-load -module-link-name swiftAutolinkingTest -incremental -driver-show-incremental -module-name AutolinkingTest -output-file-map ofm.json -F %t -import-underlying-module autolinking-overlay.swift autolinking-other.swift
+
+// Make sure `swift_FORCE_LOAD_$_swiftAutolinkingTest` appears in all objects
+// RUN: llvm-readobj -symbols -coff-exports %t/autolinking-overlay.o | %FileCheck %s
+// RUN: llvm-readobj -symbols -coff-exports %t/autolinking-other.o | %FileCheck %s
+// RUN: llvm-readobj -symbols -coff-exports %t/%target-library-name(AutolinkingTest) | %FileCheck %s
+
+// Emulate an overlay build by importing content from the underlying module.
+extension Test { }
+
+// CHECK: _swift_FORCE_LOAD_$_swiftAutolinkingTest
diff --git a/test/Incremental/autolinking.swift b/test/Incremental/autolinking.swift
@@ -0,0 +1,16 @@
+// This test shares inputs with its cousin autolinking-overlay.swift
+// This one specifically tests the bare minimum: two empty swift files.
+
+// RUN: %empty-directory(%t)
+// RUN: cp %s %t
+// RUN: cp -r %S/Inputs/autolinking/* %t
+// RUN: cd %t
+
+// RUN: %target-build-swift-dylib(%t/%target-library-name(AutolinkingTest)) -autolink-force-load -module-link-name swiftAutolinkingTest -incremental -driver-show-incremental -module-name AutolinkingTest -output-file-map ofm.json autolinking.swift autolinking-other.swift
+
+// Make sure `swift_FORCE_LOAD_$_swiftAutolinkingTest` appears in all objects
+// RUN: llvm-readobj -symbols -coff-exports %t/autolinking.o | %FileCheck %s
+// RUN: llvm-readobj -symbols -coff-exports %t/autolinking-other.o | %FileCheck %s
+// RUN: llvm-readobj -symbols -coff-exports %t/%target-library-name(AutolinkingTest) | %FileCheck %s
+
+// CHECK: _swift_FORCE_LOAD_$_swiftAutolinkingTest
diff --git a/test/Serialization/autolinking.swift b/test/Serialization/autolinking.swift
@@ -40,12 +40,8 @@ import someModule
 // NO-FORCE-LOAD-NOT: FORCE_LOAD
 // NO-FORCE-LOAD-NOT -lmodule
 // NO-FORCE-LOAD-NOT -lmagic
-// FORCE-LOAD: define{{( dllexport)?}} void @"_swift_FORCE_LOAD_$_module"() {{(#[0-9]+)?( comdat)?}} {
-// FORCE-LOAD:   ret void
-// FORCE-LOAD: }
-// FORCE-LOAD-HEX: define{{( dllexport)?}} void @"_swift_FORCE_LOAD_$306d6f64756c65"() {{(#[0-9]+)?( comdat)?}} {
-// FORCE-LOAD-HEX:   ret void
-// FORCE-LOAD-HEX: }
+// FORCE-LOAD: @"_swift_FORCE_LOAD_$_module"
+// FORCE-LOAD-HEX: @"_swift_FORCE_LOAD_$306d6f64756c65"
 
 // NO-FORCE-LOAD-CLIENT-NOT: FORCE_LOAD
 // FORCE-LOAD-CLIENT: @"_swift_FORCE_LOAD_$_module_$_autolinking" = weak_odr hidden constant void ()* @"_swift_FORCE_LOAD_$_module"
