Merge pull request #40287 from gottesmm/pr-8bb19fcfe173f5e7b33ddd95dcbc5566be2262b8

[sil] Ban casting AnyObject with a guaranteed ownership forwarding checked_cast_br and fix up semantic-arc-opts given that.

Author: gottesmm

include/swift/SIL/SILInstruction.h

@@ -1057,16 +1057,40 @@ class CopyLikeInstruction {
 /// initializes the kind field on our object is run before our constructor runs.
 class OwnershipForwardingMixin {
   ValueOwnershipKind ownershipKind;
+  bool directlyForwards;
 
 protected:
   OwnershipForwardingMixin(SILInstructionKind kind,
-                           ValueOwnershipKind ownershipKind)
-      : ownershipKind(ownershipKind) {
+                           ValueOwnershipKind ownershipKind,
+                           bool isDirectlyForwarding = true)
+      : ownershipKind(ownershipKind), directlyForwards(isDirectlyForwarding) {
     assert(isa(kind) && "Invalid subclass?!");
     assert(ownershipKind && "invalid forwarding ownership");
+    assert((directlyForwards || ownershipKind != OwnershipKind::Guaranteed) &&
+           "Non directly forwarding instructions can not forward guaranteed "
+           "ownership");
   }
 
 public:
+  /// If an instruction is directly forwarding, then any operand op whose
+  /// ownership it forwards into a result r must have the property that op and r
+  /// are "rc identical". This means that they are representing the same set of
+  /// underlying lifetimes (plural b/c of aggregates).
+  ///
+  /// An instruction that is not directly forwarding, can not have guaranteed
+  /// ownership since without direct forwarding, there isn't necessarily any
+  /// connection in between the operand's lifetime and the value's lifetime.
+  ///
+  /// An example of this is checked_cast_br where when performing the following:
+  ///
+  ///   __SwiftValue(AnyHashable(Klass())) to OtherKlass()
+  ///
+  /// we will look through the __SwiftValue(AnyHashable(X)) any just cast Klass
+  /// to OtherKlass. This means that the result argument would no longer be
+  /// rc-identical to the operand and default case and thus we can not propagate
+  /// forward any form of guaranteed ownership.
+  bool isDirectlyForwarding() const { return directlyForwards; }
+
   /// Forwarding ownership is determined by the forwarding instruction's
   /// constant ownership attribute. If forwarding ownership is owned, then the
   /// instruction moves an owned operand to its result, ending its lifetime. If
@@ -1081,6 +1105,9 @@ class OwnershipForwardingMixin {
     return ownershipKind;
   }
   void setForwardingOwnershipKind(ValueOwnershipKind newKind) {
+    assert((isDirectlyForwarding() || newKind != OwnershipKind::Guaranteed) &&
+           "Non directly forwarding instructions can not forward guaranteed "
+           "ownership");
     ownershipKind = newKind;
   }
 
@@ -7927,9 +7954,10 @@ class OwnershipForwardingTermInst : public TermInst,
 protected:
   OwnershipForwardingTermInst(SILInstructionKind kind,
                               SILDebugLocation debugLoc,
-                              ValueOwnershipKind ownershipKind)
+                              ValueOwnershipKind ownershipKind,
+                              bool isDirectlyForwarding = true)
       : TermInst(kind, debugLoc),
-        OwnershipForwardingMixin(kind, ownershipKind) {
+        OwnershipForwardingMixin(kind, ownershipKind, isDirectlyForwarding) {
     assert(classof(kind));
   }
 
@@ -8909,7 +8937,13 @@ class CheckedCastBranchInst final
                         ValueOwnershipKind forwardingOwnershipKind)
       : UnaryInstructionWithTypeDependentOperandsBase(
             DebugLoc, Operand, TypeDependentOperands, SuccessBB, FailureBB,
-            Target1Count, Target2Count, forwardingOwnershipKind),
+            Target1Count, Target2Count, forwardingOwnershipKind,
+            // We are always directly forwarding unless we are casting an
+            // AnyObject. This is b/c an AnyObject could contain a boxed
+            // AnyObject(Class()) that we unwrap as part of the cast. In such a
+            // case, we would return a different value and potentially end the
+            // lifetime of the operand value.
+            !Operand->getType().isAnyObject()),
         DestLoweredTy(DestLoweredTy), DestFormalTy(DestFormalTy),
         IsExact(IsExact) {}
 

lib/SIL/Utils/MemAccessUtils.cpp

@@ -877,9 +877,11 @@ SILValue swift::findOwnershipReferenceAggregate(SILValue ref) {
     if (auto *arg = dyn_cast<SILArgument>(root)) {
       if (auto *term = arg->getSingleTerminator()) {
         if (term->isTransformationTerminator()) {
-          assert(OwnershipForwardingTermInst::isa(term));
-          root = term->getOperand(0);
-          continue;
+          auto *ti = cast<OwnershipForwardingTermInst>(term);
+          if (ti->isDirectlyForwarding()) {
+            root = term->getOperand(0);
+            continue;
+          }
         }
       }
     }

lib/SIL/Utils/OwnershipUtils.cpp

@@ -29,121 +29,51 @@ bool swift::isValueAddressOrTrivial(SILValue v) {
          v.getOwnershipKind() == OwnershipKind::None;
 }
 
-// These operations forward both owned and guaranteed ownership.
-static bool isOwnershipForwardingInstructionKind(SILInstructionKind kind) {
-  switch (kind) {
-  case SILInstructionKind::TupleInst:
-  case SILInstructionKind::StructInst:
-  case SILInstructionKind::EnumInst:
-  case SILInstructionKind::DifferentiableFunctionInst:
-  case SILInstructionKind::LinearFunctionInst:
-  case SILInstructionKind::OpenExistentialRefInst:
-  case SILInstructionKind::UpcastInst:
-  case SILInstructionKind::UncheckedValueCastInst:
-  case SILInstructionKind::UncheckedRefCastInst:
-  case SILInstructionKind::ConvertFunctionInst:
-  case SILInstructionKind::RefToBridgeObjectInst:
-  case SILInstructionKind::BridgeObjectToRefInst:
-  case SILInstructionKind::UnconditionalCheckedCastInst:
-  case SILInstructionKind::UncheckedEnumDataInst:
-  case SILInstructionKind::SelectEnumInst:
-  case SILInstructionKind::SwitchEnumInst:
-  case SILInstructionKind::CheckedCastBranchInst:
-  case SILInstructionKind::DestructureStructInst:
-  case SILInstructionKind::DestructureTupleInst:
-  case SILInstructionKind::MarkDependenceInst:
-  case SILInstructionKind::InitExistentialRefInst:
-    return true;
-  default:
-    return false;
-  }
-}
-
-// These operations forward guaranteed ownership, but don't necessarily forward
-// owned values.
-static bool isGuaranteedForwardingInstructionKind(SILInstructionKind kind) {
-  switch (kind) {
-  case SILInstructionKind::TupleExtractInst:
-  case SILInstructionKind::StructExtractInst:
-  case SILInstructionKind::DifferentiableFunctionExtractInst:
-  case SILInstructionKind::LinearFunctionExtractInst:
-  case SILInstructionKind::OpenExistentialValueInst:
-  case SILInstructionKind::OpenExistentialBoxValueInst:
-    return true;
-  default:
-    return isOwnershipForwardingInstructionKind(kind);
-  }
-}
-
 bool swift::canOpcodeForwardGuaranteedValues(SILValue value) {
   // If we have an argument from a transforming terminator, we can forward
   // guaranteed.
   if (auto *arg = dyn_cast<SILArgument>(value))
     if (auto *ti = arg->getSingleTerminator())
-      if (ti->isTransformationTerminator()) {
-        assert(OwnershipForwardingMixin::isa(ti));
-        return true;
-      }
+      if (ti->isTransformationTerminator())
+        return OwnershipForwardingMixin::get(ti)->isDirectlyForwarding();
 
-  auto *inst = value->getDefiningInstruction();
-  if (!inst)
-    return false;
+  if (auto *inst = value->getDefiningInstruction())
+    if (auto *mixin = OwnershipForwardingMixin::get(inst))
+      return mixin->isDirectlyForwarding() &&
+             !isa<OwnedFirstArgForwardingSingleValueInst>(inst);
 
-  bool result = isGuaranteedForwardingInstructionKind(inst->getKind());
-  if (result) {
-    assert(!isa<OwnedFirstArgForwardingSingleValueInst>(inst));
-    assert(OwnershipForwardingMixin::isa(inst));
-  }
-  return result;
+  return false;
 }
 
 bool swift::canOpcodeForwardGuaranteedValues(Operand *use) {
-  auto *user = use->getUser();
-  bool result = isOwnershipForwardingInstructionKind(user->getKind());
-  if (result) {
-    assert(!isa<GuaranteedFirstArgForwardingSingleValueInst>(user));
-    assert(OwnershipForwardingMixin::isa(user));
-  }
-  return result;
-}
-
-static bool isOwnedForwardingValueKind(SILInstructionKind kind) {
-  switch (kind) {
-  case SILInstructionKind::MarkUninitializedInst:
-    return true;
-  default:
-    return isOwnershipForwardingInstructionKind(kind);
-  }
+  if (auto *mixin = OwnershipForwardingMixin::get(use->getUser()))
+    return mixin->isDirectlyForwarding() &&
+           !isa<OwnedFirstArgForwardingSingleValueInst>(use->getUser());
+  return false;
 }
 
 bool swift::canOpcodeForwardOwnedValues(SILValue value) {
   // If we have a SILArgument and we are the successor block of a transforming
   // terminator, we are fine.
   if (auto *arg = dyn_cast<SILPhiArgument>(value))
     if (auto *predTerm = arg->getSingleTerminator())
-      if (predTerm->isTransformationTerminator()) {
-        assert(OwnershipForwardingMixin::isa(predTerm));
-        return true;
-      }
-  auto *inst = value->getDefiningInstruction();
-  if (!inst)
-    return false;
+      if (predTerm->isTransformationTerminator())
+        return OwnershipForwardingMixin::get(predTerm)->isDirectlyForwarding();
 
-  bool result = isOwnedForwardingValueKind(inst->getKind());
-  if (result) {
-    assert(!isa<GuaranteedFirstArgForwardingSingleValueInst>(inst));
-    assert(OwnershipForwardingMixin::isa(inst));
-  }
-  return result;
+  if (auto *inst = value->getDefiningInstruction())
+    if (auto *mixin = OwnershipForwardingMixin::get(inst))
+      return mixin->isDirectlyForwarding() &&
+             !isa<GuaranteedFirstArgForwardingSingleValueInst>(inst);
+
+  return false;
 }
 
 bool swift::canOpcodeForwardOwnedValues(Operand *use) {
   auto *user = use->getUser();
-  bool result = isOwnershipForwardingInstructionKind(user->getKind());
-  if (result) {
-    assert(OwnershipForwardingMixin::isa(user));
-  }
-  return result;
+  if (auto *mixin = OwnershipForwardingMixin::get(user))
+    return mixin->isDirectlyForwarding() &&
+           !isa<GuaranteedFirstArgForwardingSingleValueInst>(user);
+  return false;
 }
 
 //===----------------------------------------------------------------------===//
@@ -980,7 +910,8 @@ bool swift::getAllBorrowIntroducingValues(SILValue inputValue,
       // predecessor terminator.
       auto *arg = cast<SILPhiArgument>(value);
       auto *termInst = arg->getSingleTerminator();
-      assert(termInst && termInst->isTransformationTerminator());
+      assert(termInst && termInst->isTransformationTerminator() &&
+             OwnershipForwardingMixin::get(termInst)->isDirectlyForwarding());
       assert(termInst->getNumOperands() == 1 &&
              "Transforming terminators should always have a single operand");
       worklist.push_back(termInst->getAllOperands()[0].get());
@@ -1031,7 +962,8 @@ BorrowedValue swift::getSingleBorrowIntroducingValue(SILValue inputValue) {
       // predecessor terminator.
       auto *arg = cast<SILPhiArgument>(currentValue);
       auto *termInst = arg->getSingleTerminator();
-      assert(termInst && termInst->isTransformationTerminator());
+      assert(termInst && termInst->isTransformationTerminator() &&
+             OwnershipForwardingMixin::get(termInst)->isDirectlyForwarding());
       assert(termInst->getNumOperands() == 1 &&
              "Transformation terminators should only have single operands");
       currentValue = termInst->getAllOperands()[0].get();
@@ -1083,7 +1015,8 @@ bool swift::getAllOwnedValueIntroducers(
       // predecessor terminator.
       auto *arg = cast<SILPhiArgument>(value);
       auto *termInst = arg->getSingleTerminator();
-      assert(termInst && termInst->isTransformationTerminator());
+      assert(termInst && termInst->isTransformationTerminator() &&
+             OwnershipForwardingMixin::get(termInst)->isDirectlyForwarding());
       assert(termInst->getNumOperands() == 1 &&
              "Transforming terminators should always have a single operand");
       worklist.push_back(termInst->getAllOperands()[0].get());
@@ -1127,10 +1060,11 @@ OwnedValueIntroducer swift::getSingleOwnedValueIntroducer(SILValue inputValue) {
       }
 
       // Otherwise, we should have a block argument that is defined by a single
-      // predecessor terminator.
+      // predecessor terminator and is directly forwarding.
       auto *arg = cast<SILPhiArgument>(currentValue);
       auto *termInst = arg->getSingleTerminator();
-      assert(termInst && termInst->isTransformationTerminator());
+      assert(termInst && termInst->isTransformationTerminator() &&
+             OwnershipForwardingMixin::get(termInst)->isDirectlyForwarding());
       assert(termInst->getNumOperands()
              - termInst->getNumTypeDependentOperands() == 1 &&
              "Transformation terminators should only have single operands");

lib/SIL/Verifier/SILVerifier.cpp

@@ -4048,6 +4048,24 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
                   CBI->getOperand().getOwnershipKind()),
               "failure dest block argument must have ownership compatible with "
               "the checked_cast_br operand");
+
+      // Do not allow for checked_cast_br to forward guaranteed ownership if the
+      // source type is an AnyObject.
+      //
+      // EXPLANATION: A checked_cast_br from an AnyObject may return a different
+      // object. This occurs for instance if one has an AnyObject that is a
+      // boxed AnyHashable (ClassType). This breaks with the guarantees of
+      // checked_cast_br guaranteed, so we ban it.
+      require(!CBI->getOperand()->getType().isAnyObject() ||
+                  CBI->getOperand().getOwnershipKind() !=
+                      OwnershipKind::Guaranteed,
+              "checked_cast_br with an AnyObject typed source cannot forward "
+              "guaranteed ownership");
+      require(CBI->isDirectlyForwarding() ||
+                  CBI->getOperand().getOwnershipKind() !=
+                      OwnershipKind::Guaranteed,
+              "If checked_cast_br is not directly forwarding, it can not have "
+              "guaranteed ownership");
     } else {
       require(CBI->getFailureBB()->args_empty(),
               "Failure dest of checked_cast_br must not take any argument in "

lib/SILGen/SILGenBuilder.cpp

@@ -536,6 +536,12 @@ void SILGenBuilder::createCheckedCastBranch(SILLocation loc, bool isExact,
                                             SILBasicBlock *falseBlock,
                                             ProfileCounter Target1Count,
                                             ProfileCounter Target2Count) {
+  // Check if our source type is AnyObject. In such a case, we need to ensure
+  // plus one our operand since SIL does not support guaranteed casts from an
+  // AnyObject.
+  if (op.getType().isAnyObject()) {
+    op = op.ensurePlusOne(SGF, loc);
+  }
   createCheckedCastBranch(loc, isExact, op.forward(SGF),
                           destLoweredTy, destFormalTy,
                           trueBlock, falseBlock,

lib/SILOptimizer/SemanticARC/OwnershipLiveRange.cpp

@@ -14,6 +14,7 @@
 #include "OwnershipPhiOperand.h"
 
 #include "swift/SIL/BasicBlockUtils.h"
+#include "swift/SIL/OwnershipUtils.h"
 
 using namespace swift;
 using namespace swift::semanticarc;
@@ -83,6 +84,20 @@ OwnershipLiveRange::OwnershipLiveRange(SILValue value)
       continue;
     }
 
+    // If we have a subclass of OwnershipForwardingMixin that doesnt directly
+    // forward its operand to the result, treat the use as an unknown consuming
+    // use.
+    //
+    // If we do not directly forward and we have an owned value (which we do
+    // here), we could get back a different value. Thus we can not transform
+    // such a thing from owned to guaranteed.
+    if (auto *i = OwnershipForwardingMixin::get(op->getUser())) {
+      if (!i->isDirectlyForwarding()) {
+        tmpUnknownConsumingUses.push_back(op);
+        continue;
+      }
+    }
+
     // Ok, this is a forwarding instruction whose ownership we can flip from
     // owned -> guaranteed.
     tmpForwardingConsumingUses.push_back(op);

test/SILGen/checked_cast_br_anyobject.swift

@@ -0,0 +1,44 @@
+// RUN: %target-swift-emit-silgen -module-name checked_cast_br_anyobject -parse-as-library -Xllvm -sil-full-demangle -enforce-exclusivity=checked %s
+
+public struct BridgedSwiftObject {
+    var swiftMetatype : UnsafeRawPointer
+    var refCounts : Int64
+}
+
+public typealias SwiftObject = UnsafeMutablePointer<BridgedSwiftObject>
+
+extension UnsafeMutablePointer where Pointee == BridgedSwiftObject {
+  init<T: AnyObject>(_ object: T) {
+    let ptr = Unmanaged.passUnretained(object).toOpaque()
+    self = ptr.bindMemory(to: BridgedSwiftObject.self, capacity: 1)
+  }
+  
+  func getAs<T: AnyObject>(_ objectType: T.Type) -> T {
+    return Unmanaged<T>.fromOpaque(self).takeUnretainedValue()
+  }
+}
+
+extension Optional where Wrapped == UnsafeMutablePointer<BridgedSwiftObject> {
+  func getAs<T: AnyObject>(_ objectType: T.Type) -> T? {
+    if let pointer = self {
+      return Unmanaged<T>.fromOpaque(pointer).takeUnretainedValue()
+    }
+    return nil
+  }
+}
+
+public class Klass {}
+public class Klass2 {}
+
+// Make sure that we do not crash when emitting this code!
+public func getValue(_ obj: UnsafeMutablePointer<BridgedSwiftObject>) -> AnyObject {
+    let v = obj.getAs(AnyObject.self)
+    switch v {
+    case let k as Klass:
+        return k
+    case let k as Klass2:
+        return k
+    default:
+        fatalError("unknown type")
+    }
+}