IRGen/SIL: Fix IR linkage computation for inlined function references from modules imported @_weakLinked.

tshortli · tshortli · commit 40eb1422bb71 · 2022-08-19T09:56:45.000-07:00
Include the parent `ModuleDecl` when serializing a `SILFunction` so that it is available on deserialized functions even though the full `DeclContext` is not present. With the parent module always available we can reliably compute whether the `SILFunction` comes from a module that was imported `@_weakLinked`.

Serialize the `DeclContext` member of `SILFunction` so that it can be used to look up the module that a function belongs to in order to compute weak import status.

Resolves rdar://98521248
diff --git a/include/swift/AST/Module.h b/include/swift/AST/Module.h
@@ -685,9 +685,8 @@ class ModuleDecl
   // Is \p spiGroup accessible as an explicitly imported SPI from this module?
   bool isImportedAsSPI(Identifier spiGroup, const ModuleDecl *fromModule) const;
 
-  /// Is \p targetDecl from a module that is imported as \c @_weakLinked from
-  /// this module?
-  bool isImportedAsWeakLinked(const Decl *targetDecl) const;
+  /// Is \p module imported as \c @_weakLinked from this module?
+  bool isImportedAsWeakLinked(const ModuleDecl *module) const;
 
   /// \sa getImportedModules
   enum class ImportFilterKind {
diff --git a/include/swift/SIL/SILFunction.h b/include/swift/SIL/SILFunction.h
@@ -19,6 +19,7 @@
 
 #include "swift/AST/ASTNode.h"
 #include "swift/AST/Availability.h"
+#include "swift/AST/Module.h"
 #include "swift/AST/ResilienceExpansion.h"
 #include "swift/Basic/ProfileCounter.h"
 #include "swift/Basic/SwiftObjectHeader.h"
@@ -64,11 +65,6 @@ enum IsDistributed_t {
   IsNotDistributed,
   IsDistributed,
 };
-enum IsWeakImported_t {
-  IsNotWeakImported,
-  IsWeakImportedByModule,
-  IsAlwaysWeakImported,
-};
 
 enum class PerformanceConstraints : uint8_t {
   None = 0,
@@ -222,6 +218,10 @@ class SILFunction
   /// The AST decl context of the function.
   DeclContext *DeclCtxt = nullptr;
 
+  /// The module that defines this function. This member should only be set as
+  /// a fallback when a \c DeclCtxt is unavailable.
+  ModuleDecl *ParentModule = nullptr;
+
   /// The profiler for instrumentation based profiling, or null if profiling is
   /// disabled.
   SILProfiler *Profiler = nullptr;
@@ -322,8 +322,9 @@ class SILFunction
   /// would indicate.
   unsigned HasCReferences : 1;
 
-  /// Whether cross-module references to this function should use weak linking.
-  unsigned IsWeakImported : 2;
+  /// Whether cross-module references to this function should always use weak
+  /// linking.
+  unsigned IsAlwaysWeakImported : 1;
 
   /// Whether the implementation can be dynamically replaced.
   unsigned IsDynamicReplaceable : 1;
@@ -801,19 +802,11 @@ class SILFunction
 
   /// Returns whether this function's symbol must always be weakly referenced
   /// across module boundaries.
-  bool isAlwaysWeakImported() const {
-    return IsWeakImported == IsWeakImported_t::IsAlwaysWeakImported;
-  }
-
-  /// Returns whether this function's symbol was referenced by a module that
-  /// imports the defining module \c @_weakLinked.
-  bool isWeakImportedByModule() const {
-    return IsWeakImported == IsWeakImported_t::IsWeakImportedByModule;
-  }
+  bool isAlwaysWeakImported() const { return IsAlwaysWeakImported; }
 
-  void setIsWeakImported(IsWeakImported_t value) { IsWeakImported = value; }
+  void setIsAlwaysWeakImported(bool value) { IsAlwaysWeakImported = value; }
 
-  bool isWeakImported() const;
+  bool isWeakImported(ModuleDecl *module) const;
 
   /// Returns whether this function implementation can be dynamically replaced.
   IsDynamicallyReplaceable_t isDynamicallyReplaceable() const {
@@ -960,6 +953,18 @@ class SILFunction
     DeclCtxt = (DS ? DebugScope->Loc.getAsDeclContext() : nullptr);
   }
 
+  /// Returns the module that defines this function.
+  ModuleDecl *getParentModule() const {
+    return DeclCtxt ? DeclCtxt->getParentModule() : ParentModule;
+  }
+
+  /// Sets \c ParentModule as fallback if \c DeclCtxt is not available to
+  /// provide the parent module.
+  void setParentModule(ModuleDecl *module) {
+    assert(!DeclCtxt && "already have a DeclCtxt");
+    ParentModule = module;
+  }
+
   /// Initialize the debug scope for debug info on SIL level
   /// (-sil-based-debuginfo).
   void setSILDebugScope(const SILDebugScope *DS) {
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
@@ -1002,7 +1002,7 @@ bool Decl::isWeakImported(ModuleDecl *fromModule) const {
   if (isAlwaysWeakImported())
     return true;
 
-  if (fromModule->isImportedAsWeakLinked(this))
+  if (fromModule->isImportedAsWeakLinked(this->getModuleContext()))
     return true;
 
   auto availability = getAvailabilityForLinkage();
diff --git a/lib/AST/Module.cpp b/lib/AST/Module.cpp
@@ -2663,10 +2663,9 @@ bool ModuleDecl::isImportedAsSPI(Identifier spiGroup,
   return importedSPIGroups.count(spiGroup);
 }
 
-bool ModuleDecl::isImportedAsWeakLinked(const Decl *targetDecl) const {
-  const auto *declaringModule = targetDecl->getModuleContext();
+bool ModuleDecl::isImportedAsWeakLinked(const ModuleDecl *module) const {
   for (auto file : getFiles()) {
-    if (file->importsModuleAsWeakLinked(declaringModule))
+    if (file->importsModuleAsWeakLinked(module))
       return true;
   }
   return false;
diff --git a/lib/IRGen/GenDecl.cpp b/lib/IRGen/GenDecl.cpp
@@ -2520,7 +2520,8 @@ Address IRGenModule::getAddrOfSILGlobalVariable(SILGlobalVariable *var,
     // FIXME: We should be able to set the linkage unconditionally here but
     //        some fixes are needed for Cxx interop.
     if (auto globalVar = dyn_cast<llvm::GlobalVariable>(addr)) {
-      if (getSwiftModule()->isImportedAsWeakLinked(var->getDecl()))
+      auto varModule = var->getDecl()->getModuleContext();
+      if (getSwiftModule()->isImportedAsWeakLinked(varModule))
         globalVar->setLinkage(link.getLinkage());
     }
 
@@ -3275,13 +3276,16 @@ llvm::Function *IRGenModule::getAddrOfSILFunction(
     fn = dyn_cast<llvm::Function>(clangAddr->stripPointerCasts());
 
     if (fn) {
-      // Override the linkage computed by Clang if the decl is from another
-      // module that imported @_weakLinked.
-      //
-      // FIXME: We should be able to set the linkage unconditionally here but
-      //        some fixes are needed for Cxx interop.
-      if (!forDefinition && f->isWeakImportedByModule())
-        fn->setLinkage(link.getLinkage());
+      if (!forDefinition) {
+        // Override the linkage computed by Clang if the decl is from another
+        // module that imported @_weakLinked.
+        //
+        // FIXME: We should be able to set the linkage unconditionally here but
+        //        some fixes are needed for Cxx interop.
+        if (auto *DC = f->getDeclContext())
+          if (getSwiftModule()->isImportedAsWeakLinked(DC->getParentModule()))
+            fn->setLinkage(link.getLinkage());
+      }
 
       // If we have a function, move it to the appropriate position.
       if (hasOrderNumber) {
diff --git a/lib/IRGen/Linking.cpp b/lib/IRGen/Linking.cpp
@@ -1177,7 +1177,7 @@ bool LinkEntity::isWeakImported(ModuleDecl *module) const {
   case Kind::DynamicallyReplaceableFunctionVariable:
   case Kind::SILFunction:
   case Kind::DistributedAccessor: {
-    return getSILFunction()->isWeakImported();
+    return getSILFunction()->isWeakImported(module);
   }
 
   case Kind::AssociatedConformanceDescriptor:
diff --git a/lib/SIL/IR/Linker.cpp b/lib/SIL/IR/Linker.cpp
@@ -16,10 +16,7 @@
 /// deserializing functions, vtables and witness tables.
 ///
 /// The behavior of the linker is controlled by a LinkMode value. The LinkMode
-/// has three possible values:
-///
-/// - LinkNone: The linker does not deserialize anything. This is only used for
-///   debugging and testing purposes, and never during normal operation.
+/// has two possible values:
 ///
 /// - LinkNormal: The linker deserializes bodies for declarations that must be
 ///   emitted into the client because they do not have definitions available
diff --git a/lib/SIL/IR/SILFunction.cpp b/lib/SIL/IR/SILFunction.cpp
@@ -182,7 +182,7 @@ void SILFunction::init(
   this->InlineStrategy = inlineStrategy;
   this->Linkage = unsigned(Linkage);
   this->HasCReferences = false;
-  this->IsWeakImported = false;
+  this->IsAlwaysWeakImported = false;
   this->IsDynamicReplaceable = isDynamic;
   this->ExactSelfClass = isExactSelfClass;
   this->IsDistributed = isDistributed;
@@ -260,7 +260,7 @@ void SILFunction::createSnapshot(int id) {
   newSnapshot->perfConstraints = perfConstraints;
   newSnapshot->GlobalInitFlag = GlobalInitFlag;
   newSnapshot->HasCReferences = HasCReferences;
-  newSnapshot->IsWeakImported = IsWeakImported;
+  newSnapshot->IsAlwaysWeakImported = IsAlwaysWeakImported;
   newSnapshot->HasOwnership = HasOwnership;
   newSnapshot->IsWithoutActuallyEscapingThunk = IsWithoutActuallyEscapingThunk;
   newSnapshot->OptMode = OptMode;
@@ -452,9 +452,10 @@ bool SILFunction::isTypeABIAccessible(SILType type) const {
   return getModule().isTypeABIAccessible(type, TypeExpansionContext(*this));
 }
 
-bool SILFunction::isWeakImported() const {
-  if (isWeakImportedByModule())
-    return true;
+bool SILFunction::isWeakImported(ModuleDecl *module) const {
+  if (auto *parent = getParentModule())
+    if (module->isImportedAsWeakLinked(parent))
+      return true;
 
   // For imported functions check the Clang declaration.
   if (ClangNodeOwner)
diff --git a/lib/SIL/IR/SILFunctionBuilder.cpp b/lib/SIL/IR/SILFunctionBuilder.cpp
@@ -312,14 +312,7 @@ SILFunction *SILFunctionBuilder::getOrCreateFunction(
       F->setClangNodeOwner(decl);
 
     F->setAvailabilityForLinkage(decl->getAvailabilityForLinkage());
-
-    if (decl->isAlwaysWeakImported()) {
-      F->setIsWeakImported(IsWeakImported_t::IsAlwaysWeakImported);
-    } else if (decl->isWeakImported(mod.getSwiftModule())) {
-      F->setIsWeakImported(IsWeakImported_t::IsWeakImportedByModule);
-    } else {
-      F->setIsWeakImported(IsWeakImported_t::IsNotWeakImported);
-    }
+    F->setIsAlwaysWeakImported(decl->isAlwaysWeakImported());
 
     if (auto *accessor = dyn_cast<AccessorDecl>(decl)) {
       auto *storage = accessor->getStorage();
diff --git a/lib/SIL/Parser/ParseSIL.cpp b/lib/SIL/Parser/ParseSIL.cpp
@@ -6514,9 +6514,7 @@ bool SILParserState::parseDeclSIL(Parser &P) {
     if (!objCReplacementFor.empty())
       FunctionState.F->setObjCReplacement(objCReplacementFor);
     FunctionState.F->setSpecialPurpose(specialPurpose);
-    FunctionState.F->setIsWeakImported(
-        isWeakImported ? IsWeakImported_t::IsAlwaysWeakImported
-                       : IsWeakImported_t::IsNotWeakImported);
+    FunctionState.F->setIsAlwaysWeakImported(isWeakImported);
     FunctionState.F->setAvailabilityForLinkage(availability);
     FunctionState.F->setWithoutActuallyEscapingThunk(
       isWithoutActuallyEscapingThunk);
diff --git a/lib/Serialization/DeserializeSIL.cpp b/lib/Serialization/DeserializeSIL.cpp
@@ -519,6 +519,7 @@ SILDeserializer::readSILFunctionChecked(DeclID FID, SILFunction *existingFn,
   (void)kind;
 
   DeclID clangNodeOwnerID;
+  ModuleID parentModuleID;
   TypeID funcTyID;
   IdentifierID replacedFunctionID;
   IdentifierID usedAdHocWitnessFunctionID;
@@ -538,7 +539,7 @@ SILDeserializer::readSILFunctionChecked(DeclID FID, SILFunction *existingFn,
       hasQualifiedOwnership, isWeakImported, LIST_VER_TUPLE_PIECES(available),
       isDynamic, isExactSelfClass, isDistributed, funcTyID,
       replacedFunctionID, usedAdHocWitnessFunctionID,
-      genericSigID, clangNodeOwnerID, SemanticsIDs);
+      genericSigID, clangNodeOwnerID, parentModuleID, SemanticsIDs);
 
   if (funcTyID == 0) {
     MF->fatal("SILFunction typeID is 0");
@@ -661,9 +662,7 @@ SILDeserializer::readSILFunctionChecked(DeclID FID, SILFunction *existingFn,
     fn->setEffectsKind(EffectsKind(effect));
     fn->setOptimizationMode(OptimizationMode(optimizationMode));
     fn->setPerfConstraints((PerformanceConstraints)perfConstr);
-    fn->setIsWeakImported(isWeakImported
-                              ? IsWeakImported_t::IsAlwaysWeakImported
-                              : IsWeakImported_t::IsNotWeakImported);
+    fn->setIsAlwaysWeakImported(isWeakImported);
     fn->setClassSubclassScope(SubclassScope(subclassScope));
     fn->setHasCReferences(bool(hasCReferences));
 
@@ -705,12 +704,16 @@ SILDeserializer::readSILFunctionChecked(DeclID FID, SILFunction *existingFn,
     fn->setWasDeserializedCanonical();
 
   fn->setBare(IsBare);
-  const SILDebugScope *DS = fn->getDebugScope();
-  if (!DS) {
-    DS = new (SILMod) SILDebugScope(loc, fn);
+  if (!fn->getDebugScope()) {
+    const SILDebugScope *DS = new (SILMod) SILDebugScope(loc, fn);
     fn->setDebugScope(DS);
   }
 
+  // If we don't already have a DeclContext to use to find a parent module,
+  // attempt to deserialize a parent module reference directly.
+  if (!fn->getDeclContext() && parentModuleID)
+    fn->setParentModule(MF->getModule(parentModuleID));
+
   // Read and instantiate the specialize attributes.
   bool shouldAddSpecAttrs = fn->getSpecializeAttrs().empty();
   bool shouldAddEffectAttrs = !fn->hasArgumentEffects();
@@ -3014,6 +3017,7 @@ bool SILDeserializer::hasSILFunction(StringRef Name,
   // TODO: If this results in any noticeable performance problems, Cache the
   // linkage to avoid re-reading it from the bitcode each time?
   DeclID clangOwnerID;
+  ModuleID parentModuleID;
   TypeID funcTyID;
   IdentifierID replacedFunctionID;
   IdentifierID usedAdHocWitnessFunctionID;
@@ -3033,7 +3037,7 @@ bool SILDeserializer::hasSILFunction(StringRef Name,
       hasQualifiedOwnership, isWeakImported, LIST_VER_TUPLE_PIECES(available),
       isDynamic, isExactSelfClass, isDistributed, funcTyID,
       replacedFunctionID, usedAdHocWitnessFunctionID,
-      genericSigID, clangOwnerID, SemanticsIDs);
+      genericSigID, clangOwnerID, parentModuleID, SemanticsIDs);
   auto linkage = fromStableSILLinkage(rawLinkage);
   if (!linkage) {
     LLVM_DEBUG(llvm::dbgs() << "invalid linkage code " << rawLinkage
diff --git a/lib/Serialization/ModuleFormat.h b/lib/Serialization/ModuleFormat.h
@@ -58,7 +58,7 @@ const uint16_t SWIFTMODULE_VERSION_MAJOR = 0;
 /// describe what change you made. The content of this comment isn't important;
 /// it just ensures a conflict if two people change the module format.
 /// Don't worry about adhering to the 80-column limit for this line.
-const uint16_t SWIFTMODULE_VERSION_MINOR = 701; // opened archetype serialization
+const uint16_t SWIFTMODULE_VERSION_MINOR = 702; // SILFunction DeclCtxt
 
 /// A standard hash seed used for all string hashes in a serialized module.
 ///
diff --git a/lib/Serialization/SILFormat.h b/lib/Serialization/SILFormat.h
@@ -287,6 +287,7 @@ namespace sil_block {
                      DeclIDField,  // SILFunction name or 0 (used ad-hoc requirement witness function)
                      GenericSignatureIDField,
                      DeclIDField, // ClangNode owner
+                     ModuleIDField, // Parent ModuleDecl *
                      BCArray<IdentifierIDField> // Semantics Attribute
                      // followed by specialize and argument effects attributes
                      // followed by generic param list, if any
diff --git a/lib/Serialization/SerializeSIL.cpp b/lib/Serialization/SerializeSIL.cpp
@@ -459,6 +459,10 @@ void SILSerializer::writeSILFunction(const SILFunction &F, bool DeclOnly) {
   if (F.hasClangNode())
     clangNodeOwnerID = S.addDeclRef(F.getClangNodeOwner());
 
+  ModuleID parentModuleID;
+  if (auto *parentModule = F.getParentModule())
+    parentModuleID = S.addModuleRef(parentModule);
+
   IdentifierID replacedFunctionID = 0;
   if (auto *fun = F.getDynamicallyReplacedFunction()) {
     addReferencedSILFunction(fun, true);
@@ -505,7 +509,7 @@ void SILSerializer::writeSILFunction(const SILFunction &F, bool DeclOnly) {
       (unsigned)F.isExactSelfClass(),
       (unsigned)F.isDistributed(),
       FnID, replacedFunctionID, usedAdHocWitnessFunctionID,
-      genericSigID, clangNodeOwnerID, SemanticsIDs);
+      genericSigID, clangNodeOwnerID, parentModuleID, SemanticsIDs);
 
   F.visitArgEffects(
     [&](int effectIdx, bool isDerived, SILFunction::ArgEffectKind) {
diff --git a/test/IRGen/weaklinked_import.swift b/test/IRGen/weaklinked_import.swift
@@ -159,10 +159,7 @@ protocol RefinesP: BaseP {}
 extension S: RefinesP {}
 
 func testInlining() {
-  // FIXME: usableFromInlineFn() should be extern_weak but isn't because
-  //        inlining doesn't respect @_weakLinked import yet.
-
   // CHECK-DAG: define linkonce_odr hidden {{.+}} @"$s24weaklinked_import_helper22alwaysEmitIntoClientFnyyF"()
-  // CHECK-DAG: declare swiftcc {{.+}} @"$s24weaklinked_import_helper18usableFromInlineFnyyF"
+  // CHECK-DAG: declare extern_weak {{.+}} @"$s24weaklinked_import_helper18usableFromInlineFnyyF"
   alwaysEmitIntoClientFn()
 }
diff --git a/test/IRGen/weaklinked_import_inlining.swift b/test/IRGen/weaklinked_import_inlining.swift
@@ -0,0 +1,39 @@
+// RUN: %empty-directory(%t)
+// RUN: split-file %s %t
+
+// RUN: %target-swift-frontend -emit-module -emit-module-path %t/weaklinked_import_helper.swiftmodule -parse-as-library %S/Inputs/weaklinked_import_helper.swift -enable-library-evolution
+// RUN: %target-swift-frontend -emit-module -emit-module-path %t/Intermediate.swiftmodule -parse-as-library %t/Intermediate.swift -I %t -enable-library-evolution
+//
+// RUN: %target-swift-frontend -primary-file %t/Client.swift -I %t -emit-ir | %FileCheck %s
+
+// UNSUPPORTED: OS=windows-msvc
+
+//--- Intermediate.swift
+
+import weaklinked_import_helper
+
+public func hasDefaultArgument(_ s: S = S()) { }
+
+@_alwaysEmitIntoClient
+public func aeicFuncCallingFn() {
+  fn()
+}
+
+//--- Client.swift
+
+import Intermediate
+@_weakLinked import weaklinked_import_helper
+
+// Symbols from `weaklinked_import_helper` should have weak linkage even
+// when the references to them are inlined from `Intermediate`, which imported
+// `weaklinked_import_helper` without `@_weakLinked`.
+
+func testDefaultArguments() {
+  // CHECK: declare extern_weak {{.+}} @"$s24weaklinked_import_helper1SVMa"
+  hasDefaultArgument()
+}
+
+func testAlwaysEmitIntoClient() {
+  // CHECK-DAG: declare extern_weak {{.+}} @"$s24weaklinked_import_helper2fnyyF"()
+  aeicFuncCallingFn()
+}
diff --git a/test/IRGen/weaklinked_import_transitive.swift b/test/IRGen/weaklinked_import_transitive.swift
@@ -12,8 +12,9 @@
 import intermediate
 import weaklinked_import_helper
 
-// Symbols from weaklinked_import_helper should be strong, despite intermediate
-// importining the module @_weakLinked.
+// The `@_weakLinked` attribute on the import of `weaklinked_import_helper` in
+// the module `intermediate` should not apply transitively to the import from
+// this module.
 
 // CHECK-DAG: declare swiftcc {{.+}} @"$s24weaklinked_import_helper2fnyyF"()
 fn()

Original file line number	Diff line number	Diff line change
`@@ -2663,10 +2663,9 @@ bool ModuleDecl::isImportedAsSPI(Identifier spiGroup,`
`2663`	`2663`	`return importedSPIGroups.count(spiGroup);`
`2664`	`2664`	`}`
`2665`	`2665`
`2666`		`-bool ModuleDecl::isImportedAsWeakLinked(const Decl *targetDecl) const {`
`2667`		`- const auto *declaringModule = targetDecl->getModuleContext();`
	`2666`	`+bool ModuleDecl::isImportedAsWeakLinked(const ModuleDecl *module) const {`
`2668`	`2667`	`for (auto file : getFiles()) {`
`2669`		`- if (file->importsModuleAsWeakLinked(declaringModule))`
	`2668`	`+ if (file->importsModuleAsWeakLinked(module))`
`2670`	`2669`	`return true;`
`2671`	`2670`	`}`
`2672`	`2671`	`return false;`
Original file line number	Diff line number	Diff line change
`@@ -1177,7 +1177,7 @@ bool LinkEntity::isWeakImported(ModuleDecl *module) const {`
`1177`	`1177`	`case Kind::DynamicallyReplaceableFunctionVariable:`
`1178`	`1178`	`case Kind::SILFunction:`
`1179`	`1179`	`case Kind::DistributedAccessor: {`
`1180`		`- return getSILFunction()->isWeakImported();`
	`1180`	`+ return getSILFunction()->isWeakImported(module);`
`1181`	`1181`	`}`
`1182`	`1182`
`1183`	`1183`	`case Kind::AssociatedConformanceDescriptor:`