Merge pull request #4045 from eeckstein/leak3

SILOptimizer: fixed bugs which cause memory leaks.

Author: tkremenek

include/swift/SILOptimizer/Utils/Local.h

@@ -470,6 +470,7 @@ class CastOptimizer {
   /// into a bridged ObjC type.
   SILInstruction *
   optimizeBridgedSwiftToObjCCast(SILInstruction *Inst,
+      CastConsumptionKind ConsumptionKind,
       bool isConditional,
       SILValue Src,
       SILValue Dest,
@@ -531,6 +532,7 @@ class CastOptimizer {
   /// May change the control flow.
   SILInstruction *
   optimizeBridgedCasts(SILInstruction *Inst,
+      CastConsumptionKind ConsumptionKind,
       bool isConditional,
       SILValue Src,
       SILValue Dest,

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

@@ -156,7 +156,7 @@ class MemoryToRegisters {
   SILBuilder B;
 
   /// \brief Check if the AllocStackInst \p ASI is only written into.
-  bool isWriteOnlyAllocation(AllocStackInst *ASI, bool Promoted = false);
+  bool isWriteOnlyAllocation(AllocStackInst *ASI);
 
   /// \brief Promote all of the AllocStacks in a single basic block in one
   /// linear scan. Note: This function deletes all of the users of the
@@ -242,8 +242,7 @@ static bool isCaptured(AllocStackInst *ASI, bool &inSingleBlock) {
 }
 
 /// Returns true if the AllocStack is only stored into.
-bool MemoryToRegisters::isWriteOnlyAllocation(AllocStackInst *ASI,
-                                              bool Promoted) {
+bool MemoryToRegisters::isWriteOnlyAllocation(AllocStackInst *ASI) {
   // For all users of the AllocStack:
   for (auto UI = ASI->use_begin(), E = ASI->use_end(); UI != E; ++UI) {
     SILInstruction *II = UI->getUser();
@@ -259,15 +258,9 @@ bool MemoryToRegisters::isWriteOnlyAllocation(AllocStackInst *ASI,
 
     // If we haven't already promoted the AllocStack, we may see
     // DebugValueAddr uses.
-    if (!Promoted && isa<DebugValueAddrInst>(II))
+    if (isa<DebugValueAddrInst>(II))
       continue;
 
-    // Destroys of loadable types can be rewritten as releases, so
-    // they are fine.
-    if (auto *DAI = dyn_cast<DestroyAddrInst>(II))
-      if (!Promoted && DAI->getOperand()->getType().isLoadable(DAI->getModule()))
-        continue;
-
     // Can't do anything else with it.
     DEBUG(llvm::dbgs() << "*** AllocStack has non-write use: " << *II);
     return false;
@@ -874,8 +867,7 @@ bool MemoryToRegisters::run() {
       StackAllocationPromoter(ASI, DT, DomTreeLevels, B).run();
 
       // Make sure that all of the allocations were promoted into registers.
-      assert(isWriteOnlyAllocation(ASI, /* Promoted =*/ true) &&
-             "Non-write uses left behind");
+      assert(isWriteOnlyAllocation(ASI) && "Non-write uses left behind");
       // ... and erase the allocation.
       eraseUsesOfInstruction(ASI);
 

lib/SILOptimizer/Utils/Local.cpp

@@ -1597,6 +1597,7 @@ optimizeBridgedObjCToSwiftCast(SILInstruction *Inst,
 SILInstruction *
 CastOptimizer::
 optimizeBridgedSwiftToObjCCast(SILInstruction *Inst,
+                     CastConsumptionKind ConsumptionKind,
                      bool isConditional,
                      SILValue Src,
                      SILValue Dest,
@@ -1679,16 +1680,77 @@ optimizeBridgedSwiftToObjCCast(SILInstruction *Inst,
     Src = Builder.createLoad(Loc, Src);
   }
 
-  if (ParamTypes[0].getConvention() == ParameterConvention::Direct_Guaranteed)
+  // Compensate different owning conventions of the replaced cast instruction
+  // and the inserted convertion function.
+  bool needRetainBeforeCall = false;
+  bool needReleaseAfterCall = false;
+  bool needReleaseInSucc = false;
+  switch (ParamTypes[0].getConvention()) {
+    case ParameterConvention::Direct_Guaranteed:
+      switch (ConsumptionKind) {
+        case CastConsumptionKind::TakeAlways:
+          needReleaseAfterCall = true;
+          break;
+        case CastConsumptionKind::TakeOnSuccess:
+          needReleaseInSucc = true;
+          break;
+        case CastConsumptionKind::CopyOnSuccess:
+          // Conservatively insert a retain/release pair around the conversion
+          // function because the conversion function could decrement the
+          // (global) reference count of the source object.
+          //
+          // %src = load %global_var
+          // apply %conversion_func(@guaranteed %src)
+          //
+          // sil conversion_func {
+          //    %old_value = load %global_var
+          //    store %something_else, %global_var
+          //    strong_release %old_value
+          // }
+          needRetainBeforeCall = true;
+          needReleaseAfterCall = true;
+          break;
+      }
+      break;
+    case ParameterConvention::Direct_Owned:
+      // The Direct_Owned case is only handled for completeness. Currently this
+      // cannot appear, because the _bridgeToObjectiveC protocol witness method
+      // always receives the this pointer (= the source) as guaranteed.
+      switch (ConsumptionKind) {
+        case CastConsumptionKind::TakeAlways:
+          break;
+        case CastConsumptionKind::TakeOnSuccess:
+          needRetainBeforeCall = true;
+          needReleaseInSucc = true;
+          break;
+        case CastConsumptionKind::CopyOnSuccess:
+          needRetainBeforeCall = true;
+          break;
+      }
+      break;
+    case ParameterConvention::Direct_Unowned:
+      break;
+    case ParameterConvention::Indirect_In:
+    case ParameterConvention::Indirect_Inout:
+    case ParameterConvention::Indirect_InoutAliasable:
+    case ParameterConvention::Indirect_In_Guaranteed:
+    case ParameterConvention::Direct_Deallocating:
+      llvm_unreachable("unsupported convention for bridging conversion");
+  }
+
+  if (needRetainBeforeCall)
     Builder.createRetainValue(Loc, Src, Atomicity::Atomic);
 
   // Generate a code to invoke the bridging function.
   auto *NewAI = Builder.createApply(Loc, FnRef, SubstFnTy, ResultTy, Subs, Src,
                                     false);
 
-  if (ParamTypes[0].getConvention() == ParameterConvention::Direct_Guaranteed)
+  if (needReleaseAfterCall) {
     Builder.createReleaseValue(Loc, Src, Atomicity::Atomic);
-
+  } else if (needReleaseInSucc) {
+    SILBuilder SuccBuilder(SuccessBB->begin());
+    SuccBuilder.createReleaseValue(Loc, Src, Atomicity::Atomic);
+  }
   SILInstruction *NewI = NewAI;
 
   if (Dest) {
@@ -1721,6 +1783,7 @@ optimizeBridgedSwiftToObjCCast(SILInstruction *Inst,
 SILInstruction *
 CastOptimizer::
 optimizeBridgedCasts(SILInstruction *Inst,
+                     CastConsumptionKind ConsumptionKind,
                      bool isConditional,
                      SILValue Src,
                      SILValue Dest,
@@ -1786,7 +1849,8 @@ optimizeBridgedCasts(SILInstruction *Inst,
           target, BridgedSourceTy, BridgedTargetTy, SuccessBB, FailureBB);
     } else {
       // This is a Swift to ObjC cast
-      return optimizeBridgedSwiftToObjCCast(Inst, isConditional, Src, Dest, source,
+      return optimizeBridgedSwiftToObjCCast(Inst, ConsumptionKind,
+          isConditional, Src, Dest, source,
           target, BridgedSourceTy, BridgedTargetTy, SuccessBB, FailureBB);
     }
   }
@@ -1863,7 +1927,8 @@ simplifyCheckedCastAddrBranchInst(CheckedCastAddrBranchInst *Inst) {
     // To apply the bridged optimizations, we should
     // ensure that types are not existential,
     // and that not both types are classes.
-    BridgedI = optimizeBridgedCasts(Inst, true, Src, Dest, SourceType,
+    BridgedI = optimizeBridgedCasts(Inst, Inst->getConsumptionKind(),
+                                    true, Src, Dest, SourceType,
                                     TargetType, SuccessBB, FailureBB);
 
     if (!BridgedI) {
@@ -1975,7 +2040,8 @@ CastOptimizer::simplifyCheckedCastBranchInst(CheckedCastBranchInst *Inst) {
       auto Src = Inst->getOperand();
       auto Dest = SILValue();
       // To apply the bridged casts optimizations.
-      auto BridgedI = optimizeBridgedCasts(Inst, false, Src, Dest, SourceType,
+      auto BridgedI = optimizeBridgedCasts(Inst,
+          CastConsumptionKind::CopyOnSuccess, false, Src, Dest, SourceType,
           TargetType, nullptr, nullptr);
 
       if (BridgedI) {
@@ -2290,8 +2356,9 @@ optimizeUnconditionalCheckedCastInst(UnconditionalCheckedCastInst *Inst) {
     auto SourceType = LoweredSourceType.getSwiftRValueType();
     auto TargetType = LoweredTargetType.getSwiftRValueType();
     auto Src = Inst->getOperand();
-    auto NewI = optimizeBridgedCasts(Inst, false, Src, SILValue(), SourceType,
-        TargetType, nullptr, nullptr);
+    auto NewI = optimizeBridgedCasts(Inst, CastConsumptionKind::CopyOnSuccess,
+                                     false, Src, SILValue(), SourceType,
+                                     TargetType, nullptr, nullptr);
     if (NewI) {
       ReplaceInstUsesAction(Inst, NewI);
       EraseInstAction(Inst);
@@ -2406,14 +2473,25 @@ optimizeUnconditionalCheckedCastAddrInst(UnconditionalCheckedCastAddrInst *Inst)
     }
 
     if (ResultNotUsed) {
+      switch (Inst->getConsumptionKind()) {
+        case CastConsumptionKind::TakeAlways:
+        case CastConsumptionKind::TakeOnSuccess: {
+          SILBuilder B(Inst);
+          B.createDestroyAddr(Inst->getLoc(), Inst->getSrc());
+          break;
+        }
+        case CastConsumptionKind::CopyOnSuccess:
+          break;
+      }
       EraseInstAction(Inst);
       WillSucceedAction();
       return nullptr;
     }
 
     // Try to apply the bridged casts optimizations
-    auto NewI = optimizeBridgedCasts(Inst, false, Src, Dest, SourceType,
-                                         TargetType, nullptr, nullptr);
+    auto NewI = optimizeBridgedCasts(Inst, Inst->getConsumptionKind(),
+                                     false, Src, Dest, SourceType,
+                                     TargetType, nullptr, nullptr);
     if (NewI) {
         WillSucceedAction();
         return nullptr;

test/SILOptimizer/cast_folding_objc.swift

@@ -1,4 +1,4 @@
-// RUN: %target-swift-frontend -O -emit-sil %s | FileCheck %s
+// RUN: %target-swift-frontend -O -Xllvm -sil-disable-pass="Function Signature Optimization" -emit-sil %s | FileCheck %s
 // We want to check two things here:
 // - Correctness
 // - That certain "is" checks are eliminated based on static analysis at compile-time
@@ -51,6 +51,10 @@ func test0() -> Bool {
 // Check that this cast does not get eliminated, because
 // the compiler does not statically know if this object
 // is NSNumber can be converted into Int.
+
+// CHECK-LABEL: sil [noinline] @_TF17cast_folding_objc35testMayBeBridgedCastFromObjCtoSwiftFPs9AnyObject_Si
+// CHECK: unconditional_checked_cast_addr
+// CHECK: return
 @inline(never)
 public func testMayBeBridgedCastFromObjCtoSwift(_ o: AnyObject) -> Int {
   return o as! Int
@@ -59,6 +63,10 @@ public func testMayBeBridgedCastFromObjCtoSwift(_ o: AnyObject) -> Int {
 // Check that this cast does not get eliminated, because
 // the compiler does not statically know if this object
 // is NSString can be converted into String.
+
+// CHECK-LABEL: sil [noinline] @_TF17cast_folding_objc41testConditionalBridgedCastFromObjCtoSwiftFPs9AnyObject_GSqSS_
+// CHECK: unconditional_checked_cast_addr
+// CHECK: return
 @inline(never)
 public func testConditionalBridgedCastFromObjCtoSwift(_ o: AnyObject) -> String? {
   return o as? String
@@ -246,21 +254,14 @@ public func testBridgedCastFromObjCtoSwift(_ ns: NSString) -> String {
 }
 
 // Check that compiler understands that this cast always succeeds
-@inline(never)
-public func testBridgedCastFromSwiftToObjC(_ s: String) -> NSString {
-  return s as NSString
-}
 
-// CHECK-LABEL: sil [noinline] @_TTSf4g___TF17cast_folding_objc35testMayBeBridgedCastFromObjCtoSwiftFPs9AnyObject_Si
-// CHECK: unconditional_checked_cast_addr
-// CHECK: return
-
-// CHECK-LABEL: sil [noinline] @_TTSf4g___TF17cast_folding_objc41testConditionalBridgedCastFromObjCtoSwiftFPs9AnyObject_GSqSS_
-// CHECK: unconditional_checked_cast_addr
-// CHECK: return
-
-// CHECK-LABEL: sil [noinline] @_TTSf4gs___TF17cast_folding_objc30testBridgedCastFromSwiftToObjCFSSCSo8NSString
+// CHECK-LABEL: sil [noinline] @_TF17cast_folding_objc30testBridgedCastFromSwiftToObjCFSSCSo8NSString
 // CHECK-NOT: {{ cast}}
 // CHECK: function_ref @_TFE10FoundationSS19_bridgeToObjectiveC
 // CHECK: apply
 // CHECK: return
+@inline(never)
+public func testBridgedCastFromSwiftToObjC(_ s: String) -> NSString {
+  return s as NSString
+}
+

test/SILOptimizer/cast_foldings.sil

@@ -104,3 +104,16 @@ bb0:
   dealloc_stack %1 : $*S
   return %6 : $AnyObject
 }
+
+// CHECK-LABEL: sil @destroy_source_of_removed_cast
+// CHECK: destroy_addr %0
+sil @destroy_source_of_removed_cast : $@convention(thin) (@in AnyObject) -> () {
+bb0(%0 : $*AnyObject):
+  %2 = alloc_stack $Int
+  unconditional_checked_cast_addr take_always AnyObject in %0 : $*AnyObject to Int in %2 : $*Int
+  %3 = load %2 : $*Int
+  dealloc_stack %2 : $*Int
+  %r = tuple ()
+  return %r : $()
+}
+