Serialization: Serialize shared thunks referenced from fragile functions

If a thunk is referenced from two different functions, the thunk inherits
the fragile attribute from the first function that forced it to be emitted.

This is wrong, in case the first function might not be fragile, while
the second one is. Copying the fragile attribute to an existing thunk when
checking if it has already been emitted is also wrong, because the thunk
might reference another thunk, and so on.

The correct fix is to have SIL serialization serialize the transitive
closure of all fragile functions and thunks referenced from fragile
functions. Re-work SIL function serialization to use a worklist so that
we can do this.

Part of https://bugs.swift.org/browse/SR-267.

Author: slavapestov

lib/SIL/SILVerifier.cpp

@@ -912,28 +912,33 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
 
     case SILLinkage::Shared:
     case SILLinkage::SharedExternal:
-        // This handles some kind of generated functions, like constructors
-        // of clang imported types.
-        // TODO: check why those functions are not fragile anyway and make
-        // a less conservative check here.
-        return true;
+      // This allows fragile functions to reference thunks, generated
+      // methods on Clang types, and optimizer specializations.
+      return true;
 
     case SILLinkage::Public:
     case SILLinkage::PublicExternal:
       return true;
     }
   }
 
+  bool isValidLinkageForFragileRef(const SILFunction *RefF) {
+    if (RefF->isFragile() ||
+        RefF->isExternalDeclaration()) {
+      return true;
+    }
+
+    return isValidLinkageForFragileRef(RefF->getLinkage());
+  }
+
   void checkFunctionRefInst(FunctionRefInst *FRI) {
     auto fnType = requireObjectType(SILFunctionType, FRI,
                                     "result of function_ref");
     require(!fnType->getExtInfo().hasContext(),
             "function_ref should have a context-free function result");
     if (F.isFragile()) {
       SILFunction *RefF = FRI->getReferencedFunction();
-      require(RefF->isFragile()
-                || isValidLinkageForFragileRef(RefF->getLinkage())
-                || RefF->isExternalDeclaration(),
+      require(isValidLinkageForFragileRef(RefF),
               "function_ref inside fragile function cannot "
               "reference a private or hidden symbol");
     }

lib/Serialization/SerializeSIL.cpp

@@ -157,10 +157,14 @@ namespace {
     uint32_t /*DeclID*/ NextDefaultWitnessTableID = 1;
 
     /// Give each SILBasicBlock a unique ID.
-    llvm::DenseMap<const SILBasicBlock*, unsigned> BasicBlockMap;
+    llvm::DenseMap<const SILBasicBlock *, unsigned> BasicBlockMap;
 
-    /// Functions that we've emitted a reference to.
-    llvm::SmallSet<const SILFunction *, 16> FuncsToDeclare;
+    /// Functions that we've emitted a reference to. If the key maps
+    /// to true, we want to emit a declaration only.
+    llvm::DenseMap<const SILFunction *, bool> FuncsToEmit;
+
+    /// Additional functions we might need to serialize.
+    llvm::SmallVector<const SILFunction *, 16> Worklist;
 
     std::array<unsigned, 256> SILAbbrCodes;
     template <typename Layout>
@@ -175,6 +179,12 @@ namespace {
 
     bool ShouldSerializeAll;
 
+    void addMandatorySILFunction(const SILFunction *F,
+                                 bool emitDeclarationsForOnoneSupport);
+    void addReferencedSILFunction(const SILFunction *F,
+                                  bool DeclOnly = false);
+    void processSILFunctionWorklist();
+
     /// Helper function to update ListOfValues for MethodInst. Format:
     /// Attr, SILDeclRef (DeclID, Kind, uncurryLevel, IsObjC), and an operand.
     void handleMethodInst(const MethodInst *MI, SILValue operand,
@@ -207,7 +217,7 @@ namespace {
 
     /// Helper function to determine if given the current state of the
     /// deserialization if the function body for F should be deserialized.
-    bool shouldEmitFunctionBody(const SILFunction &F);
+    bool shouldEmitFunctionBody(const SILFunction *F);
 
   public:
     SILSerializer(Serializer &S, ASTContext &Ctx,
@@ -218,6 +228,69 @@ namespace {
   };
 } // end anonymous namespace
 
+void SILSerializer::addMandatorySILFunction(const SILFunction *F,
+                                            bool emitDeclarationsForOnoneSupport) {
+  // If this function is not fragile, don't do anything.
+  if (!shouldEmitFunctionBody(F))
+    return;
+
+  auto iter = FuncsToEmit.find(F);
+  if (iter != FuncsToEmit.end()) {
+    // We've already visited this function. Make sure that we decided
+    // to emit its body the first time around.
+    assert(iter->second == emitDeclarationsForOnoneSupport
+           && "Already emitting declaration");
+    return;
+  }
+
+  // We haven't seen this function before. Record that we want to
+  // emit its body, and add it to the worklist.
+  FuncsToEmit[F] = emitDeclarationsForOnoneSupport;
+  if (!emitDeclarationsForOnoneSupport)
+    Worklist.push_back(F);
+}
+
+void SILSerializer::addReferencedSILFunction(const SILFunction *F,
+                                             bool DeclOnly) {
+  assert(F != nullptr);
+
+  if (FuncsToEmit.count(F) > 0)
+    return;
+
+  // We haven't seen this function before. Let's see if we should
+  // serialize the body or just the declaration.
+  if (shouldEmitFunctionBody(F)) {
+    FuncsToEmit[F] = false;
+    Worklist.push_back(F);
+    return;
+  }
+
+  // If we referenced a non-fragile shared function from a fragile
+  // function, serialize it too. In practice, it will either be a
+  // thunk, or an optimizer specialization. In both cases, we don't
+  // have enough information at the time we emit the function to
+  // know if it should be marked fragile or not.
+  if (F->getLinkage() == SILLinkage::Shared && !DeclOnly) {
+    FuncsToEmit[F] = false;
+    Worklist.push_back(F);
+    return;
+  }
+
+  // Ok, we just need to emit a declaration.
+  FuncsToEmit[F] = true;
+}
+
+void SILSerializer::processSILFunctionWorklist() {
+  while(Worklist.size() > 0) {
+    const SILFunction *F = Worklist.back();
+    Worklist.pop_back();
+    assert(F != nullptr);
+
+    assert(FuncsToEmit.count(F) > 0);
+    writeSILFunction(*F, FuncsToEmit[F]);
+  }
+}
+
 /// We enumerate all values in a SILFunction beforehand to correctly
 /// handle forward references of values.
 ValueID SILSerializer::addValueRef(const ValueBase *Val) {
@@ -943,7 +1016,7 @@ void SILSerializer::writeSILInstruction(const SILInstruction &SI) {
         S.addIdentifierRef(Ctx.getIdentifier(ReferencedFunction->getName())));
 
     // Make sure we declare the referenced function.
-    FuncsToDeclare.insert(ReferencedFunction);
+    addReferencedSILFunction(ReferencedFunction);
     break;
   }
   case ValueKind::DeallocPartialRefInst:
@@ -1558,7 +1631,7 @@ void SILSerializer::writeSILVTable(const SILVTable &vt) {
   for (auto &entry : vt.getEntries()) {
     SmallVector<ValueID, 4> ListOfValues;
     handleSILDeclRef(S, entry.first, ListOfValues);
-    FuncsToDeclare.insert(entry.second);
+    addReferencedSILFunction(entry.second, true);
     // Each entry is a pair of SILDeclRef and SILFunction.
     VTableEntryLayout::emitRecord(Out, ScratchRecord,
         SILAbbrCodes[VTableEntryLayout::Code],
@@ -1619,9 +1692,9 @@ void SILSerializer::writeSILWitnessTable(const SILWitnessTable &wt) {
     auto &methodWitness = entry.getMethodWitness();
     SmallVector<ValueID, 4> ListOfValues;
     handleSILDeclRef(S, methodWitness.Requirement, ListOfValues);
-    FuncsToDeclare.insert(methodWitness.Witness);
     IdentifierID witnessID = 0;
     if (SILFunction *witness = methodWitness.Witness) {
+      addReferencedSILFunction(witness, true);
       witnessID = S.addIdentifierRef(Ctx.getIdentifier(witness->getName()));
     }
     WitnessMethodEntryLayout::emitRecord(Out, ScratchRecord,
@@ -1656,7 +1729,7 @@ writeSILDefaultWitnessTable(const SILDefaultWitnessTable &wt) {
     SmallVector<ValueID, 4> ListOfValues;
     handleSILDeclRef(S, entry.getRequirement(), ListOfValues);
     SILFunction *witness = entry.getWitness();
-    FuncsToDeclare.insert(witness);
+    addReferencedSILFunction(witness, true);
     IdentifierID witnessID = S.addIdentifierRef(
         Ctx.getIdentifier(witness->getName()));
     DefaultWitnessTableEntryLayout::emitRecord(Out, ScratchRecord,
@@ -1668,17 +1741,19 @@ writeSILDefaultWitnessTable(const SILDefaultWitnessTable &wt) {
 }
 
 /// Helper function for whether to emit a function body.
-bool SILSerializer::shouldEmitFunctionBody(const SILFunction &F) {
+bool SILSerializer::shouldEmitFunctionBody(const SILFunction *F) {
+  // If we are asked to serialize everything, go ahead and do it.
+  if (ShouldSerializeAll)
+    return true;
+
   // If F is a declaration, it has no body to emit...
-  if (F.isExternalDeclaration())
+  if (F->isExternalDeclaration())
     return false;
 
   // If F is transparent, we should always emit its body.
-  if (F.isFragile())
+  if (F->isFragile())
     return true;
 
-  // Otherwise serialize the body of the function only if we are asked to
-  // serialize everything.
   return false;
 }
 
@@ -1769,29 +1844,33 @@ void SILSerializer::writeSILBlock(const SILModule *SILMod) {
   // Go through all the SILFunctions in SILMod and write out any
   // mandatory function bodies.
   for (const SILFunction &F : *SILMod) {
-    if (shouldEmitFunctionBody(F) || ShouldSerializeAll) {
-      if (emitDeclarationsForOnoneSupport) {
-        // Only declarations of whitelisted pre-specializations from with
-        // public linkage need to be serialized as they will be used
-        // by UsePrespecializations pass during -Onone compilation to
-        // check for availability of concrete pre-specializations.
-        if (!hasPublicVisibility(F.getLinkage()) ||
-            !isWhitelistedSpecialization(F.getName()))
-            continue;
-      }
-      writeSILFunction(F, emitDeclarationsForOnoneSupport);
+    if (emitDeclarationsForOnoneSupport) {
+      // Only declarations of whitelisted pre-specializations from with
+      // public linkage need to be serialized as they will be used
+      // by UsePrespecializations pass during -Onone compilation to
+      // check for availability of concrete pre-specializations.
+      if (!hasPublicVisibility(F.getLinkage()) ||
+          !isWhitelistedSpecialization(F.getName()))
+        continue;
     }
-  }
 
-  if (ShouldSerializeAll)
-    return;
+    addMandatorySILFunction(&F, emitDeclarationsForOnoneSupport);
+    processSILFunctionWorklist();
+  }
 
   // Now write function declarations for every function we've
   // emitted a reference to without emitting a function body for.
   for (const SILFunction &F : *SILMod) {
-    if (!shouldEmitFunctionBody(F) && FuncsToDeclare.count(&F))
+    auto iter = FuncsToEmit.find(&F);
+    if (iter != FuncsToEmit.end() && iter->second) {
+      assert((emitDeclarationsForOnoneSupport ||
+              !shouldEmitFunctionBody(&F)) &&
+             "Should have emitted function body earlier");
       writeSILFunction(F, true);
+    }
   }
+
+  assert(Worklist.empty() && "Did not emit everything in worklist");
 }
 
 void SILSerializer::writeSILModule(const SILModule *SILMod) {

validation-test/Evolution/Inputs/function_change_transparent_body.swift

@@ -1,4 +1,12 @@
 
+public func getVersion() -> Int {
+#if BEFORE
+  return 0
+#else
+  return 1
+#endif
+}
+
 @_transparent public func getBuildVersion() -> Int {
 #if BEFORE
   return 0
@@ -7,15 +15,29 @@
 #endif
 }
 
-@_transparent public func getFunction(x: Int) -> Int -> Int {
+public func getFunction(x: Int) -> Int -> Int {
+  // Force a re-abstraction thunk for (T -> T) => (Int -> Int) to be
+  // emitted from a non-transparent context first
+
+#if BEFORE
+  func id(y: Int) -> Int { return x * y }
+#else
+  func id<T>(t: T) -> T { return t }
+#endif
+
+  return id
+}
+
+@_transparent public func getTransparentFunction(x: Int) -> Int -> Int {
   // The mangled name and calling convention of the local function
   // will change -- so we must serialize it and inline it into
   // the calling module
 
 #if BEFORE
   func id(y: Int) -> Int { return x * y }
 #else
-  func id(y: Int) -> Int { return y }
+  func id<T>(t: T) -> T { return t }
 #endif
+
   return id
 }

validation-test/Evolution/test_function_change_transparent_body.swift

@@ -28,12 +28,22 @@ ChangeTransparentBodyTest.test("ChangeTransparentBody") {
 
 }
 
+ChangeTransparentBodyTest.test("ChangeNonTransparentClosure") {
+
+  if getVersion() == 0 {
+    expectEqual(202, getFunction(2)(101))
+  } else {
+    expectEqual(101, getFunction(2)(101))
+  }
+
+}
+
 ChangeTransparentBodyTest.test("ChangeTransparentClosure") {
 
 #if BEFORE
-  expectEqual(202, getFunction(2)(101))
+  expectEqual(202, getTransparentFunction(2)(101))
 #else
-  expectEqual(101, getFunction(2)(101))
+  expectEqual(101, getTransparentFunction(2)(101))
 #endif
 
 }