Merge pull request #25190 from slavapestov/vtable-derived-more-visibl…

…e-than-base-5.1

SILGen: Correctly emit vtables when an override is more visible than the base [5.1]

include/swift/AST/Decl.h

@@ -5728,6 +5728,11 @@ class AbstractFunctionDecl : public GenericContext, public ValueDecl {
     return Bits.AbstractFunctionDecl.NeedsNewVTableEntry;
   }
 
+  bool isEffectiveLinkageMoreVisibleThan(ValueDecl *other) const {
+    return (std::min(getEffectiveAccess(), AccessLevel::Public) >
+            std::min(other->getEffectiveAccess(), AccessLevel::Public));
+  }
+
   bool isSynthesized() const {
     return Bits.AbstractFunctionDecl.Synthesized;
   }

include/swift/SIL/SILVTableVisitor.h

@@ -86,8 +86,7 @@ template <class T> class SILVTableVisitor {
   void maybeAddMethod(FuncDecl *fd) {
     assert(!fd->hasClangNode());
 
-    SILDeclRef constant(fd, SILDeclRef::Kind::Func);
-    maybeAddEntry(constant, constant.requiresNewVTableEntry());
+    maybeAddEntry(SILDeclRef(fd, SILDeclRef::Kind::Func));
   }
 
   void maybeAddConstructor(ConstructorDecl *cd) {
@@ -97,19 +96,38 @@ template <class T> class SILVTableVisitor {
     // The initializing entry point for designated initializers is only
     // necessary for super.init chaining, which is sufficiently constrained
     // to never need dynamic dispatch.
-    SILDeclRef constant(cd, SILDeclRef::Kind::Allocator);
-    maybeAddEntry(constant, constant.requiresNewVTableEntry());    
+    maybeAddEntry(SILDeclRef(cd, SILDeclRef::Kind::Allocator));
   }
 
-  void maybeAddEntry(SILDeclRef declRef, bool needsNewEntry) {
+  void maybeAddEntry(SILDeclRef declRef) {
     // Introduce a new entry if required.
-    if (needsNewEntry)
+    if (declRef.requiresNewVTableEntry())
       asDerived().addMethod(declRef);
 
     // Update any existing entries that it overrides.
     auto nextRef = declRef;
     while ((nextRef = nextRef.getNextOverriddenVTableEntry())) {
       auto baseRef = nextRef.getOverriddenVTableEntry();
+
+      // If A.f() is overridden by B.f() which is overridden by
+      // C.f(), it's possible that C.f() is not visible from C.
+      // In this case, we pretend that B.f() is the least derived
+      // method with a vtable entry in the override chain.
+      //
+      // This works because we detect the possibility of this
+      // happening when we emit B.f() and do two things:
+      // - B.f() always gets a new vtable entry, even if it is
+      //   ABI compatible with A.f()
+      // - The vtable thunk for the override of A.f() in B does a
+      //   vtable dispatch to the implementation of B.f() for the
+      //   concrete subclass, so a subclass of B only needs to
+      //   replace the vtable entry for B.f(); a call to A.f()
+      //   will correctly dispatch to the implementation of B.f()
+      //   in the subclass.
+      if (!baseRef.getDecl()->isAccessibleFrom(
+            declRef.getDecl()->getDeclContext()))
+        break;
+
       asDerived().addMethodOverride(baseRef, declRef);
       nextRef = baseRef;
     }

lib/AST/Decl.cpp

@@ -6184,6 +6184,12 @@ static bool requiresNewVTableEntry(const AbstractFunctionDecl *decl) {
     }
   }
 
+  // If the base is less visible than the override, we might need a vtable
+  // entry since callers of the override might not be able to see the base
+  // at all.
+  if (decl->isEffectiveLinkageMoreVisibleThan(base))
+    return true;
+
   // If the method overrides something, we only need a new entry if the
   // override has a more general AST type. However an abstraction
   // change is OK; we don't want to add a whole new vtable entry just

lib/IRGen/Linking.cpp

@@ -426,16 +426,7 @@ SILLinkage LinkEntity::getLinkage(ForDefinition_t forDefinition) const {
   switch (getKind()) {
   case Kind::DispatchThunk:
   case Kind::DispatchThunkInitializer:
-  case Kind::DispatchThunkAllocator: {
-    auto *decl = getDecl();
-
-    // Protocol requirements don't have their own access control
-    if (auto *proto = dyn_cast<ProtocolDecl>(decl->getDeclContext()))
-      decl = proto;
-
-    return getSILLinkage(getDeclLinkage(decl), forDefinition);
-  }
-
+  case Kind::DispatchThunkAllocator:
   case Kind::MethodDescriptor:
   case Kind::MethodDescriptorInitializer:
   case Kind::MethodDescriptorAllocator: {
@@ -445,14 +436,6 @@ SILLinkage LinkEntity::getLinkage(ForDefinition_t forDefinition) const {
     if (auto *proto = dyn_cast<ProtocolDecl>(decl->getDeclContext()))
       decl = proto;
 
-    // Method descriptors for internal class initializers can be referenced
-    // from outside the module.
-    if (auto *ctor = dyn_cast<ConstructorDecl>(decl)) {
-      auto *classDecl = cast<ClassDecl>(ctor->getDeclContext());
-      if (classDecl->getEffectiveAccess() == AccessLevel::Open)
-        decl = classDecl;
-    }
-
     return getSILLinkage(getDeclLinkage(decl), forDefinition);
   }
 

lib/SIL/SILDeclRef.cpp

@@ -852,6 +852,8 @@ SILDeclRef SILDeclRef::getNextOverriddenVTableEntry() const {
     if (overridden.kind == SILDeclRef::Kind::Initializer) {
       return SILDeclRef();
     }
+
+    // Overrides of @objc dynamic declarations are not in the vtable.
     if (overridden.getDecl()->isObjCDynamic()) {
       return SILDeclRef();
     }

lib/SIL/TypeLowering.cpp

@@ -2412,6 +2412,9 @@ TypeConverter::checkFunctionForABIDifferences(SILFunctionType *fnTy1,
   if (fnTy1->getNumResults() != fnTy2->getNumResults())
     return ABIDifference::NeedsThunk;
 
+  if (fnTy1->getNumYields() != fnTy2->getNumYields())
+    return ABIDifference::NeedsThunk;
+
   // If we don't have a context but the other type does, we'll return
   // ABIDifference::ThinToThick below.
   if (fnTy1->getExtInfo().hasContext() &&
@@ -2426,8 +2429,22 @@ TypeConverter::checkFunctionForABIDifferences(SILFunctionType *fnTy1,
       return ABIDifference::NeedsThunk;
 
     if (checkForABIDifferences(result1.getSILStorageType(),
-                             result2.getSILStorageType(),
-              /*thunk iuos*/ fnTy1->getLanguage() == SILFunctionLanguage::Swift)
+                               result2.getSILStorageType(),
+             /*thunk iuos*/ fnTy1->getLanguage() == SILFunctionLanguage::Swift)
+        != ABIDifference::Trivial)
+      return ABIDifference::NeedsThunk;
+  }
+
+  for (unsigned i : indices(fnTy1->getYields())) {
+    auto yield1 = fnTy1->getYields()[i];
+    auto yield2 = fnTy2->getYields()[i];
+
+    if (yield1.getConvention() != yield2.getConvention())
+      return ABIDifference::NeedsThunk;
+
+    if (checkForABIDifferences(yield1.getSILStorageType(),
+                               yield2.getSILStorageType(),
+             /*thunk iuos*/ fnTy1->getLanguage() == SILFunctionLanguage::Swift)
         != ABIDifference::Trivial)
       return ABIDifference::NeedsThunk;
   }

lib/SILGen/SILGenFunction.h

@@ -682,11 +682,17 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
   /// \param inputOrigType Abstraction pattern of base class method
   /// \param inputSubstType Formal AST type of base class method
   /// \param outputSubstType Formal AST type of derived class method
-  void emitVTableThunk(SILDeclRef derived,
+  /// \param baseLessVisibleThanDerived If true, the thunk does a
+  /// double dispatch to the derived method's vtable entry, so that if
+  /// the derived method has an override that cannot access the base,
+  /// calls to the base dispatch to the correct method.
+  void emitVTableThunk(SILDeclRef base,
+                       SILDeclRef derived,
                        SILFunction *implFn,
                        AbstractionPattern inputOrigType,
                        CanAnyFunctionType inputSubstType,
-                       CanAnyFunctionType outputSubstType);
+                       CanAnyFunctionType outputSubstType,
+                       bool baseLessVisibleThanDerived);
 
   //===--------------------------------------------------------------------===//
   // Control flow

lib/SILGen/SILGenPoly.cpp

@@ -3541,11 +3541,13 @@ SILGenFunction::emitTransformedValue(SILLocation loc, RValue &&v,
 //===----------------------------------------------------------------------===//
 
 void
-SILGenFunction::emitVTableThunk(SILDeclRef derived,
+SILGenFunction::emitVTableThunk(SILDeclRef base,
+                                SILDeclRef derived,
                                 SILFunction *implFn,
                                 AbstractionPattern inputOrigType,
                                 CanAnyFunctionType inputSubstType,
-                                CanAnyFunctionType outputSubstType) {
+                                CanAnyFunctionType outputSubstType,
+                                bool baseLessVisibleThanDerived) {
   auto fd = cast<AbstractFunctionDecl>(derived.getDecl());
 
   SILLocation loc(fd);
@@ -3554,13 +3556,21 @@ SILGenFunction::emitVTableThunk(SILDeclRef derived,
   cleanupLoc.markAutoGenerated();
   Scope scope(Cleanups, cleanupLoc);
 
-  auto fTy = implFn->getLoweredFunctionType();
-
+  SmallVector<ManagedValue, 8> thunkArgs;
+  collectThunkParams(loc, thunkArgs);
+
+  CanSILFunctionType derivedFTy;
+  if (baseLessVisibleThanDerived) {
+    derivedFTy = SGM.Types.getConstantOverrideType(derived);
+  } else {
+    derivedFTy = SGM.Types.getConstantInfo(derived).SILFnType;
+  }
+
   SubstitutionMap subs;
   if (auto *genericEnv = fd->getGenericEnvironment()) {
     F.setGenericEnvironment(genericEnv);
     subs = getForwardingSubstitutionMap();
-    fTy = fTy->substGenericArgs(SGM.M, subs);
+    derivedFTy = derivedFTy->substGenericArgs(SGM.M, subs);
 
     inputSubstType = cast<FunctionType>(
         cast<GenericFunctionType>(inputSubstType)
@@ -3573,42 +3583,92 @@ SILGenFunction::emitVTableThunk(SILDeclRef derived,
   // Emit the indirect return and arguments.
   auto thunkTy = F.getLoweredFunctionType();
 
-  SmallVector<ManagedValue, 8> thunkArgs;
-  collectThunkParams(loc, thunkArgs);
-
   SmallVector<ManagedValue, 8> substArgs;
 
   AbstractionPattern outputOrigType(outputSubstType);
 
   // Reabstract the arguments.
-  TranslateArguments(*this, loc, thunkArgs, substArgs, fTy->getParameters())
+  TranslateArguments(*this, loc, thunkArgs, substArgs,
+                     derivedFTy->getParameters())
     .translate(inputOrigType,
                inputSubstType.getParams(),
                outputOrigType,
                outputSubstType.getParams());
 
+  auto coroutineKind = F.getLoweredFunctionType()->getCoroutineKind();
+
   // Collect the arguments to the implementation.
   SmallVector<SILValue, 8> args;
 
-  // First, indirect results.
-  ResultPlanner resultPlanner(*this, loc);
-  resultPlanner.plan(outputOrigType.getFunctionResultType(),
-                     outputSubstType.getResult(),
-                     inputOrigType.getFunctionResultType(),
-                     inputSubstType.getResult(),
-                     fTy, thunkTy, args);
+  Optional<ResultPlanner> resultPlanner;
+
+  if (coroutineKind == SILCoroutineKind::None) {
+    // First, indirect results.
+    resultPlanner.emplace(*this, loc);
+    resultPlanner->plan(outputOrigType.getFunctionResultType(),
+                        outputSubstType.getResult(),
+                        inputOrigType.getFunctionResultType(),
+                        inputSubstType.getResult(),
+                        derivedFTy, thunkTy, args);
+  }
 
   // Then, the arguments.
-  forwardFunctionArguments(*this, loc, fTy, substArgs, args);
+  forwardFunctionArguments(*this, loc, derivedFTy, substArgs, args);
 
   // Create the call.
-  auto implRef = B.createFunctionRefFor(loc, implFn);
-  SILValue implResult = emitApplyWithRethrow(loc, implRef,
-                                SILType::getPrimitiveObjectType(fTy),
-                                subs, args);
+  SILValue derivedRef;
+  if (baseLessVisibleThanDerived) {
+    // See the comment in SILVTableVisitor.h under maybeAddMethod().
+    auto selfValue = thunkArgs.back().getValue();
+    auto derivedTy = SGM.Types.getConstantOverrideType(derived);
+    derivedRef = emitClassMethodRef(loc, selfValue, derived, derivedTy);
+  } else {
+    derivedRef = B.createFunctionRefFor(loc, implFn);
+  }
 
-  // Reabstract the return.
-  SILValue result = resultPlanner.execute(implResult);
+  SILValue result;
+
+  switch (coroutineKind) {
+  case SILCoroutineKind::None: {
+    auto implResult =
+      emitApplyWithRethrow(loc, derivedRef,
+                            SILType::getPrimitiveObjectType(derivedFTy),
+                            subs, args);
+
+    // Reabstract the return.
+    result = resultPlanner->execute(implResult);
+    break;
+  }
+
+  case SILCoroutineKind::YieldOnce: {
+    SmallVector<SILValue, 4> derivedYields;
+    auto tokenAndCleanup =
+        emitBeginApplyWithRethrow(loc, derivedRef,
+                                  SILType::getPrimitiveObjectType(derivedFTy),
+                                  subs, args, derivedYields);
+    auto overrideSubs = SubstitutionMap::getOverrideSubstitutions(
+        base.getDecl(), derived.getDecl(), /*derivedSubs=*/subs);
+
+    YieldInfo derivedYieldInfo(SGM, derived, derivedFTy, subs);
+    YieldInfo baseYieldInfo(SGM, base, thunkTy, overrideSubs);
+
+    translateYields(*this, loc, derivedYields, derivedYieldInfo, baseYieldInfo);
+
+    // Kill the abort cleanup without emitting it.
+    Cleanups.setCleanupState(tokenAndCleanup.second, CleanupState::Dead);
+
+    // End the inner coroutine normally.
+    emitEndApplyWithRethrow(loc, tokenAndCleanup.first);
+
+    result = B.createTuple(loc, {});
+    break;
+  }
+
+  case SILCoroutineKind::YieldMany:
+    SGM.diagnose(loc, diag::unimplemented_generator_witnesses);
+    result = B.createTuple(loc, {});
+    break;
+  }
 
   scope.pop();
   B.createReturn(loc, result);

lib/SILGen/SILGenType.cpp

@@ -42,8 +42,8 @@ SILGenModule::emitVTableMethod(ClassDecl *theClass,
                                SILDeclRef derived, SILDeclRef base) {
   assert(base.kind == derived.kind);
 
-  auto *baseDecl = base.getDecl();
-  auto *derivedDecl = derived.getDecl();
+  auto *baseDecl = cast<AbstractFunctionDecl>(base.getDecl());
+  auto *derivedDecl = cast<AbstractFunctionDecl>(derived.getDecl());
 
   // Note: We intentionally don't support extension members here.
   //
@@ -80,8 +80,11 @@ SILGenModule::emitVTableMethod(ClassDecl *theClass,
   // If the member is dynamic, reference its dynamic dispatch thunk so that
   // it will be redispatched, funneling the method call through the runtime
   // hook point.
-  if (derivedDecl->isObjCDynamic()
-      && derived.kind != SILDeclRef::Kind::Allocator) {
+  bool usesObjCDynamicDispatch =
+      (derivedDecl->isObjCDynamic() &&
+       derived.kind != SILDeclRef::Kind::Allocator);
+
+  if (usesObjCDynamicDispatch) {
     implFn = getDynamicThunk(derived, Types.getConstantInfo(derived).SILFnType);
     implLinkage = SILLinkage::Public;
   } else {
@@ -93,6 +96,12 @@ SILGenModule::emitVTableMethod(ClassDecl *theClass,
   if (derived == base)
     return SILVTable::Entry(base, implFn, implKind, implLinkage);
 
+  // If the base method is less visible than the derived method, we need
+  // a thunk.
+  bool baseLessVisibleThanDerived =
+    (derivedDecl->isEffectiveLinkageMoreVisibleThan(baseDecl) &&
+     !usesObjCDynamicDispatch);
+
   // Determine the derived thunk type by lowering the derived type against the
   // abstraction pattern of the base.
   auto baseInfo = Types.getConstantInfo(base);
@@ -104,7 +113,8 @@ SILGenModule::emitVTableMethod(ClassDecl *theClass,
   // The override member type is semantically a subtype of the base
   // member type. If the override is ABI compatible, we do not need
   // a thunk.
-  if (M.Types.checkFunctionForABIDifferences(derivedInfo.SILFnType,
+  if (!baseLessVisibleThanDerived &&
+      M.Types.checkFunctionForABIDifferences(derivedInfo.SILFnType,
                                              overrideInfo.SILFnType)
       == TypeConverter::ABIDifference::Trivial)
     return SILVTable::Entry(base, implFn, implKind, implLinkage);
@@ -138,10 +148,13 @@ SILGenModule::emitVTableMethod(ClassDecl *theClass,
       IsBare, IsNotTransparent, IsNotSerialized, IsNotDynamic);
   thunk->setDebugScope(new (M) SILDebugScope(loc, thunk));
 
+  PrettyStackTraceSILFunction trace("generating vtable thunk", thunk);
+
   SILGenFunction(*this, *thunk, theClass)
-    .emitVTableThunk(derived, implFn, basePattern,
+    .emitVTableThunk(base, derived, implFn, basePattern,
                      overrideInfo.LoweredType,
-                     derivedInfo.LoweredType);
+                     derivedInfo.LoweredType,
+                     baseLessVisibleThanDerived);
 
   return SILVTable::Entry(base, thunk, implKind, implLinkage);
 }