Merge pull request swiftlang#79191 from aschwaighofer/access_enforcement_fixes

AccessEnforcement: Fix analysis to include mayReleases as potentially executing unknown code

Author: aschwaighofer

include/swift/AST/KnownStdlibTypes.def

@@ -50,6 +50,8 @@ KNOWN_STDLIB_TYPE_DECL(StaticString, NominalTypeDecl, 0)
 KNOWN_STDLIB_TYPE_DECL(Substring, NominalTypeDecl, 0)
 KNOWN_STDLIB_TYPE_DECL(Array, NominalTypeDecl, 1)
 KNOWN_STDLIB_TYPE_DECL(ArraySlice, NominalTypeDecl, 1)
+KNOWN_STDLIB_TYPE_DECL(_ArrayBuffer, NominalTypeDecl, 1)
+KNOWN_STDLIB_TYPE_DECL(_ContiguousArrayBuffer, NominalTypeDecl, 1)
 KNOWN_STDLIB_TYPE_DECL(_ContiguousArrayStorage, ClassDecl, 1)
 KNOWN_STDLIB_TYPE_DECL(Set, NominalTypeDecl, 1)
 KNOWN_STDLIB_TYPE_DECL(Sequence, NominalTypeDecl, 1)

include/swift/SILOptimizer/Analysis/AccessStorageAnalysis.h

@@ -31,6 +31,7 @@
 namespace swift {
 
 class BasicCalleeAnalysis;
+class DestructorAnalysis;
 
 /// Information about a formal access within a function pertaining to a
 /// particular AccessStorage location.
@@ -202,7 +203,7 @@ class AccessStorageResult {
 
   /// Record any access scopes entered by the given single SIL instruction. 'I'
   /// must not be a FullApply; use mergeFromApply instead.
-  void analyzeInstruction(SILInstruction *I);
+  void analyzeInstruction(SILInstruction *I, DestructorAnalysis *DA);
 
   void print(raw_ostream &os) const;
   void dump() const;
@@ -317,8 +318,8 @@ class FunctionAccessStorage {
   /// Analyze the side-effects of a single SIL instruction \p I.
   /// Visited callees are added to \p BottomUpOrder until \p RecursionDepth
   /// reaches MaxRecursionDepth.
-  void analyzeInstruction(SILInstruction *I) {
-    accessResult.analyzeInstruction(I);
+  void analyzeInstruction(SILInstruction *I, DestructorAnalysis *DA) {
+    accessResult.analyzeInstruction(I, DA);
   }
 
   void print(raw_ostream &os) const { accessResult.print(os); }
@@ -380,6 +381,10 @@ class AccessStorageAnalysis : public BottomUpIPAnalysis {
   /// Callee analysis, used for determining the callees at call sites.
   BasicCalleeAnalysis *BCA;
 
+  /// Destructor analysis, used for determined which releases are harmless wrt
+  /// to their side-effects.
+  DestructorAnalysis *DA;
+
 public:
   AccessStorageAnalysis()
       : BottomUpIPAnalysis(SILAnalysisKind::AccessStorage) {}
@@ -412,6 +417,8 @@ class AccessStorageAnalysis : public BottomUpIPAnalysis {
 
   BasicCalleeAnalysis *getBasicCalleeAnalysis() { return BCA; }
 
+  DestructorAnalysis *getDestructorAnalysis() { return DA; }
+
   virtual void initialize(SILPassManager *PM) override;
 
   /// Invalidate all information in this analysis.

include/swift/SILOptimizer/Utils/InstOptUtils.h

@@ -35,6 +35,7 @@ namespace swift {
 class DominanceInfo;
 class DeadEndBlocks;
 class BasicCalleeAnalysis;
+class DestructorAnalysis;
 template <class T> class NullablePtr;
 
 /// Transform a Use Range (Operand*) into a User Range (SILInstruction *)
@@ -621,6 +622,9 @@ bool findUnreferenceableStorage(StructDecl *decl, SILType structType,
 
 SILValue getInitOfTemporaryAllocStack(AllocStackInst *asi);
 
+bool isDestructorSideEffectFree(SILInstruction *mayRelease,
+                                DestructorAnalysis *DA);
+
 } // end namespace swift
 
 #endif // SWIFT_SILOPTIMIZER_UTILS_INSTOPTUTILS_H

lib/SILOptimizer/Analysis/AccessStorageAnalysis.cpp

@@ -15,7 +15,9 @@
 #include "swift/Basic/Assertions.h"
 #include "swift/SILOptimizer/Analysis/AccessStorageAnalysis.h"
 #include "swift/SILOptimizer/Analysis/BasicCalleeAnalysis.h"
+#include "swift/SILOptimizer/Analysis/DestructorAnalysis.h"
 #include "swift/SILOptimizer/Analysis/FunctionOrder.h"
+#include "swift/SILOptimizer/Utils/InstOptUtils.h"
 #include "swift/SILOptimizer/PassManager/PassManager.h"
 
 using namespace swift;
@@ -313,13 +315,16 @@ void AccessStorageResult::visitBeginAccess(B *beginAccess) {
     result.first->mergeFrom(storageAccess);
 }
 
-void AccessStorageResult::analyzeInstruction(SILInstruction *I) {
+void AccessStorageResult::analyzeInstruction(SILInstruction *I,
+                                             DestructorAnalysis *DA) {
   assert(!FullApplySite::isa(I) && "caller should merge");
 
   if (auto *BAI = dyn_cast<BeginAccessInst>(I))
     visitBeginAccess(BAI);
   else if (auto *BUAI = dyn_cast<BeginUnpairedAccessInst>(I))
     visitBeginAccess(BUAI);
+  else if (I->mayRelease() && !isDestructorSideEffectFree(I, DA))
+    setWorstEffects();
 }
 
 void StorageAccessInfo::print(raw_ostream &os) const {
@@ -393,6 +398,7 @@ bool FunctionAccessStorage::summarizeCall(FullApplySite fullApply) {
 void AccessStorageAnalysis::initialize(
     SILPassManager *PM) {
   BCA = PM->getAnalysis<BasicCalleeAnalysis>();
+  DA = PM->getAnalysis<DestructorAnalysis>();
 }
 
 void AccessStorageAnalysis::invalidate() {
@@ -449,7 +455,7 @@ void AccessStorageAnalysis::analyzeFunction(
       if (auto fullApply = FullApplySite::isa(&I))
         analyzeCall(functionInfo, fullApply, bottomUpOrder, recursionDepth);
       else
-        functionInfo->functionEffects.analyzeInstruction(&I);
+        functionInfo->functionEffects.analyzeInstruction(&I, DA);
     }
   }
   LLVM_DEBUG(llvm::dbgs() << "  << finished " << F->getName() << '\n');

lib/SILOptimizer/Analysis/DestructorAnalysis.cpp

@@ -43,6 +43,13 @@ bool DestructorAnalysis::cacheResult(CanType Type, bool Result) {
   return Result;
 }
 
+static bool isKnownSafeStdlibContainerType(CanType Ty) {
+  return Ty->isArray() ||
+    Ty->is_ArrayBuffer() ||
+    Ty->is_ContiguousArrayBuffer() ||
+    Ty->isDictionary();
+}
+
 bool DestructorAnalysis::isSafeType(CanType Ty) {
   // Don't visit types twice.
   auto CachedRes = Cached.find(Ty);
@@ -68,7 +75,8 @@ bool DestructorAnalysis::isSafeType(CanType Ty) {
   //   * or all stored properties are safe types.
   if (auto *Struct = Ty->getStructOrBoundGenericStruct()) {
 
-    if (implementsDestructorSafeContainerProtocol(Struct) &&
+    if ((implementsDestructorSafeContainerProtocol(Struct) ||
+         isKnownSafeStdlibContainerType(Ty)) &&
         areTypeParametersSafe(Ty))
       return cacheResult(Ty, true);
 

lib/SILOptimizer/Transforms/AccessEnforcementOpts.cpp

@@ -88,6 +88,7 @@
 #include "swift/SILOptimizer/Analysis/DominanceAnalysis.h"
 #include "swift/SILOptimizer/Analysis/LoopRegionAnalysis.h"
 #include "swift/SILOptimizer/PassManager/Transforms.h"
+#include "swift/SILOptimizer/Utils/InstOptUtils.h"
 #include "swift/SILOptimizer/Utils/InstructionDeleter.h"
 #include "swift/SILOptimizer/Utils/OwnershipOptUtils.h"
 #include "llvm/ADT/MapVector.h"
@@ -633,7 +634,7 @@ void AccessConflictAndMergeAnalysis::propagateAccessSetsBottomUp(
           }
           // FIXME: Treat may-release conservatively in the analysis itself by
           // adding a mayRelease flag, in addition to the unidentified flag.
-          accessResult.analyzeInstruction(&instr);
+          accessResult.analyzeInstruction(&instr, ASA->getDestructorAnalysis());
         }
       }
     }
@@ -838,7 +839,8 @@ void AccessConflictAndMergeAnalysis::localDataFlowInBlock(RegionState &state,
       visitFullApply(fullApply, state);
       continue;
     }
-    if (instr.mayRelease()) {
+    if (instr.mayRelease() &&
+        !isDestructorSideEffectFree(&instr, ASA->getDestructorAnalysis())) {
       visitMayRelease(&instr, state);
     }
   }

lib/SILOptimizer/Utils/InstOptUtils.cpp

@@ -35,6 +35,7 @@
 #include "swift/SILOptimizer/Analysis/ArraySemantic.h"
 #include "swift/SILOptimizer/Analysis/BasicCalleeAnalysis.h"
 #include "swift/SILOptimizer/Analysis/DominanceAnalysis.h"
+#include "swift/SILOptimizer/Analysis/DestructorAnalysis.h"
 #include "swift/SILOptimizer/OptimizerBridging.h"
 #include "swift/SILOptimizer/Utils/CFGOptUtils.h"
 #include "swift/SILOptimizer/Utils/DebugOptUtils.h"
@@ -2470,3 +2471,130 @@ SILValue swift::getInitOfTemporaryAllocStack(AllocStackInst *asi) {
     return findRootValueForTupleTempAllocation(asi, state);
   return findRootValueForNonTupleTempAllocation(asi, state);
 }
+
+SILType getTypeOfLoadOfArrayOperandStorage(SILValue val) {
+  // The projection should look something like this:
+  // %29 = struct_element_addr %28 : $*Array<UInt8>, #Array._buffer
+  // %30 = struct_element_addr %29 : $*_ArrayBuffer<UInt8>, #_ArrayBuffer._storage
+  // %31 = struct_element_addr %30 : $*_BridgeStorage<__ContiguousArrayStorageBase>, #_BridgeStorage.rawValue
+  // %32 = load %31 : $*Builtin.BridgeObject
+
+  // We can strip casts and init_existential_ref leading to a load.
+  if (auto initExistRef = dyn_cast<InitExistentialRefInst>(val))
+    val = initExistRef->getOperand();
+  auto ld = dyn_cast<LoadInst>(stripCasts(val));
+  if (!ld)
+    return SILType();
+
+  auto opd = ld->getOperand();
+  auto opdTy = opd->getType();
+  if (opdTy.getObjectType() !=
+      SILType::getBridgeObjectType(opdTy.getASTContext()))
+    return SILType();
+
+  auto bridgedStoragePrj = dyn_cast<StructElementAddrInst>(opd);
+  if (!bridgedStoragePrj)
+    return SILType();
+
+  auto arrayBufferStoragePrj =
+    dyn_cast<StructElementAddrInst>(bridgedStoragePrj->getOperand());
+  if (!arrayBufferStoragePrj)
+    return SILType();
+
+  // If successfull return _ArrayBuffer<UInt8>.
+  return arrayBufferStoragePrj->getOperand()->getType().getObjectType();
+}
+
+static bool isBoxTypeWithoutSideEffectsOnRelease(SILFunction *f,
+                                                 DestructorAnalysis *DA,
+                                                 SILType ty) {
+  auto silBoxedTy = ty.getSILBoxFieldType(f);
+  if (silBoxedTy && !DA->mayStoreToMemoryOnDestruction(silBoxedTy))
+   return true;
+  return false;
+}
+
+
+static bool isReleaseOfClosureWithoutSideffects(SILFunction *f,
+                                                DestructorAnalysis *DA,
+                                                SILValue opd) {
+  auto fnTy = dyn_cast<SILFunctionType>(opd->getType().getASTType());
+  if (!fnTy)
+    return false;
+
+  if (fnTy->isNoEscape() &&
+      fnTy->getRepresentation() == SILFunctionType::Representation::Thick)
+    return true;
+
+  auto pa = dyn_cast<PartialApplyInst>(lookThroughOwnershipInsts(opd));
+  if (!pa)
+    return false;
+
+  // Check that all captured argument types are "trivial".
+  for (auto &opd: pa->getArgumentOperands()) {
+    auto OpdTy = opd.get()->getType().getObjectType();
+    if (!DA->mayStoreToMemoryOnDestruction(OpdTy))
+      continue;
+    if (isBoxTypeWithoutSideEffectsOnRelease(f, DA, OpdTy))
+      continue;
+    return false;
+  }
+
+  return true;
+}
+
+bool swift::isDestructorSideEffectFree(SILInstruction *mayRelease,
+                                       DestructorAnalysis *DA) {
+  switch (mayRelease->getKind()) {
+  case SILInstructionKind::DestroyValueInst:
+  case SILInstructionKind::StrongReleaseInst:
+  case SILInstructionKind::ReleaseValueInst: {
+    auto opd = mayRelease->getOperand(0);
+    auto opdTy = opd->getType();
+    if (!DA->mayStoreToMemoryOnDestruction(opdTy))
+      return true;
+
+    auto arrayTy = getTypeOfLoadOfArrayOperandStorage(opd);
+    if (arrayTy &&  !DA->mayStoreToMemoryOnDestruction(arrayTy))
+      return true;
+
+    if (isReleaseOfClosureWithoutSideffects(mayRelease->getFunction(), DA, opd))
+      return true;
+
+    if (isBoxTypeWithoutSideEffectsOnRelease(mayRelease->getFunction(), DA,
+                                             opdTy))
+      return true;
+
+    return false;
+  }
+  case SILInstructionKind::BuiltinInst: {
+    auto *builtin = cast<BuiltinInst>(mayRelease);
+    switch (builtin->getBuiltinInfo().ID) {
+    case BuiltinValueKind::CopyArray:
+    case BuiltinValueKind::TakeArrayNoAlias:
+    case BuiltinValueKind::TakeArrayFrontToBack:
+    case BuiltinValueKind::TakeArrayBackToFront:
+      return true; // nothing is released, harmless regardless of type
+    case BuiltinValueKind::AssignCopyArrayNoAlias:
+    case BuiltinValueKind::AssignCopyArrayFrontToBack:
+    case BuiltinValueKind::AssignCopyArrayBackToFront:
+    case BuiltinValueKind::AssignTakeArray:
+    case BuiltinValueKind::DestroyArray: {
+      SubstitutionMap substitutions = builtin->getSubstitutions();
+      auto eltTy = substitutions.getReplacementTypes()[0];
+      return !DA->mayStoreToMemoryOnDestruction(
+        builtin->getFunction()->getLoweredType(eltTy));
+      // Only harmless if the array element type destruction is harmless.
+    }
+    default:
+      break;
+    }
+    return false;
+  }
+  // Unhandled instruction.
+  default:
+    return false;
+  }
+
+  return false;
+}

test/SILOptimizer/access_enforcement_opts.sil

@@ -976,17 +976,17 @@ bb0:
 
 
 // public func testOldToNewMapReadMayRelease() {
-// Checks merging 2 out of 3 scopes resulting in a larger read scope
-// Due to a MayRelease instruction before the 3rd scope
+// Checks merging 3 of 3 scopes resulting in a larger read scope
+// There is a MayRelease instruction but we recognize it to be harmless
 //
 // CHECK-LABEL: sil @testOldToNewMapReadMayRelease : $@convention(thin) () -> () {
 // CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
 // CHECK-NEXT: [[BEGIN:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
 // CHECK-NEXT: load [[BEGIN]] : $*X
 // CHECK-NEXT: load [[BEGIN]] : $*X
-// CHECK-NEXT: end_access [[BEGIN]] : $*X
 // CHECK-NEXT: strong_release
-// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
+// CHECK-NEXT: load [[BEGIN]] : $*X
+// CHECK-NEXT: end_access [[BEGIN]] : $*X
 // CHECK-LABEL: } // end sil function 'testOldToNewMapReadMayRelease'
 sil @testOldToNewMapReadMayRelease : $@convention(thin) () -> () {
 bb0:
@@ -1711,3 +1711,32 @@ bb0(%0 : $RefElemNoConflictClass, %1 : $any Error):
   %10 = tuple ()
   return %10 : $()
 }
+
+public class MayHaveDeinit {
+  deinit
+}
+
+sil @releaseArg : $@convention(thin) (@owned MayHaveDeinit) -> () {
+bb0(%0 : $MayHaveDeinit):
+  release_value %0 : $MayHaveDeinit
+  %2 = tuple ()
+  return %2 : $()
+}
+
+sil @testSummarizeFunction : $@convention(method) (@guaranteed TestClass, @owned MayHaveDeinit) -> () {
+bb0(%0 : $TestClass, %1: $MayHaveDeinit):
+  %2 = ref_element_addr %0 : $TestClass, #TestClass.flags
+  %3 = begin_access [modify] [dynamic] %2 : $*Int64
+  %4 = integer_literal $Builtin.Int64, 3
+  %5 = struct $Int64 (%4 : $Builtin.Int64)
+  store %5 to %3 : $*Int64
+  %6 = function_ref @releaseArg : $@convention(thin) (@owned MayHaveDeinit) -> ()
+  %7 = apply %6(%1) : $@convention(thin) (@owned MayHaveDeinit) -> ()
+  end_access %3 : $*Int64
+  %12 = tuple ()
+  return %12 : $()
+}
+
+// CHECK-LABEL: sil @testSummarizeFunction
+// CHECK-NOT: begin_access {{.*}}no_nested_conflict
+// CHECK: } // end sil function 'testSummarizeFunction'

test/SILOptimizer/access_enforcement_opts_ossa.sil

@@ -1006,9 +1006,9 @@ bb0:
 // CHECK-NEXT: [[BEGIN:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
 // CHECK-NEXT: load [trivial] [[BEGIN]] : $*X
 // CHECK-NEXT: load [trivial] [[BEGIN]] : $*X
-// CHECK-NEXT: end_access [[BEGIN]] : $*X
 // CHECK-NEXT: destroy_value
-// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
+// CHECK-NEXT: load [trivial] [[BEGIN]] : $*X
+// CHECK-NEXT: end_access [[BEGIN]] : $*X
 // CHECK-LABEL: } // end sil function 'testOldToNewMapReadMayRelease'
 sil [ossa] @testOldToNewMapReadMayRelease : $@convention(thin) () -> () {
 bb0: