Merge pull request #16169 from atrick/verify-access

Improve -enable-verify-exclusivity.

Author: atrick

include/swift/SIL/MemAccessUtils.h

@@ -276,6 +276,11 @@ AccessedStorage findAccessedStorage(SILValue sourceAddr);
 /// storage.
 AccessedStorage findAccessedStorageOrigin(SILValue sourceAddr);
 
+/// Return true if the given address operand is used by a memory operation that
+/// initializes the memory at that address, implying that the previous value is
+/// uninitialized.
+bool memInstMustInitialize(Operand *memOper);
+
 /// Return true if the given address producer may be the source of a formal
 /// access (a read or write of a potentially aliased, user visible variable).
 ///

lib/SIL/MemAccessUtils.cpp

@@ -13,6 +13,7 @@
 #define DEBUG_TYPE "sil-access-utils"
 
 #include "swift/SIL/MemAccessUtils.h"
+#include "swift/SIL/SILUndef.h"
 
 using namespace swift;
 
@@ -115,6 +116,30 @@ static void checkSwitchEnumBlockArg(SILPHIArgument *arg) {
   }
 }
 
+/// Return true if the given address value is produced by a special address
+/// producer that is only used for local initialization, not formal access.
+static bool isAddressForLocalInitOnly(SILValue sourceAddr) {
+  switch (sourceAddr->getKind()) {
+  default:
+    return false;
+
+  // Value to address conversions: the operand is the non-address source
+  // value. These allow local mutation of the value but should never be used
+  // for formal access of an lvalue.
+  case ValueKind::OpenExistentialBoxInst:
+  case ValueKind::ProjectExistentialBoxInst:
+    return true;
+
+  // Self-evident local initialization.
+  case ValueKind::InitEnumDataAddrInst:
+  case ValueKind::InitExistentialAddrInst:
+  case ValueKind::AllocExistentialBoxInst:
+  case ValueKind::AllocValueBufferInst:
+  case ValueKind::ProjectValueBufferInst:
+    return true;
+  }
+}
+
 AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
   SILValue address = sourceAddr;
   while (true) {
@@ -127,9 +152,12 @@ AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
     // Handle other unidentified address sources.
     switch (address->getKind()) {
     default:
+      if (isAddressForLocalInitOnly(address))
+        return AccessedStorage(address, AccessedStorage::Unidentified);
       return AccessedStorage();
 
     case ValueKind::PointerToAddressInst:
+    case ValueKind::SILUndef:
       return AccessedStorage(address, AccessedStorage::Unidentified);
 
     // A block argument may be a box value projected out of
@@ -181,22 +209,6 @@ AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
     case ValueKind::IndexAddrInst:
       address = cast<SingleValueInstruction>(address)->getOperand(0);
       continue;
-
-    // Value to address conversions: the operand is the non-address source
-    // value. These allow local mutation of the value but should never be used
-    // for formal access of an lvalue.
-    case ValueKind::OpenExistentialBoxInst:
-    case ValueKind::ProjectExistentialBoxInst:
-    case ValueKind::ProjectValueBufferInst:
-      return AccessedStorage(address, AccessedStorage::Unidentified);
-
-    // Local initialization: these cases are skipped.
-    case ValueKind::InitEnumDataAddrInst:
-    case ValueKind::InitExistentialAddrInst:
-    case ValueKind::AllocExistentialBoxInst:
-    case ValueKind::AllocValueBufferInst:
-    case ValueKind::SILUndef:
-      return AccessedStorage(address, AccessedStorage::Unidentified);
     }
   }
 }
@@ -231,6 +243,35 @@ static bool isScratchBuffer(SILValue value) {
     value->getType().getSwiftRValueType());
 }
 
+bool swift::memInstMustInitialize(Operand *memOper) {
+  SILValue address = memOper->get();
+  SILInstruction *memInst = memOper->getUser();
+
+  switch (memInst->getKind()) {
+  default:
+    return false;
+
+  case SILInstructionKind::CopyAddrInst: {
+    auto *CAI = cast<CopyAddrInst>(memInst);
+    return CAI->getDest() == address && CAI->isInitializationOfDest();
+  }
+  case SILInstructionKind::InitExistentialAddrInst:
+  case SILInstructionKind::InitEnumDataAddrInst:
+  case SILInstructionKind::InjectEnumAddrInst:
+    return true;
+
+  case SILInstructionKind::StoreInst:
+    return cast<StoreInst>(memInst)->getOwnershipQualifier()
+           == StoreOwnershipQualifier::Init;
+
+  case SILInstructionKind::StoreWeakInst:
+    return cast<StoreWeakInst>(memInst)->isInitializationOfDest();
+
+  case SILInstructionKind::StoreUnownedInst:
+    return cast<StoreUnownedInst>(memInst)->isInitializationOfDest();
+  }
+}
+
 bool swift::isPossibleFormalAccessBase(const AccessedStorage &storage,
                                        SILFunction *F) {
   switch (storage.getKind()) {
@@ -259,6 +300,9 @@ bool swift::isPossibleFormalAccessBase(const AccessedStorage &storage,
     return isPossibleFormalAccessBase(nestedStorage, F);
   }
   case AccessedStorage::Unidentified:
+    if (isAddressForLocalInitOnly(storage.getValue()))
+      return false;
+
     if (isa<SILPHIArgument>(storage.getValue())) {
       checkSwitchEnumBlockArg(cast<SILPHIArgument>(storage.getValue()));
       return false;
@@ -268,12 +312,7 @@ bool swift::isPossibleFormalAccessBase(const AccessedStorage &storage,
     if (isa<PointerToAddressInst>(storage.getValue()))
       return false;
 
-    // Existential initialization is just initialization.
-    if (isa<InitExistentialAddrInst>(storage.getValue()))
-      return false;
-
-    // Enum initialization is just initialization.
-    if (isa<InitEnumDataAddrInst>(storage.getValue()))
+    if (isa<SILUndef>(storage.getValue()))
       return false;
 
     if (isScratchBuffer(storage.getValue()))

lib/SILOptimizer/Mandatory/DiagnoseStaticExclusivity.cpp

@@ -889,6 +889,11 @@ static void checkAccessedAddress(Operand *memOper, StorageMap &Accesses) {
     abort();
   };
 
+  // If the memory instruction is only used for initialization, it doesn't need
+  // an access marker.
+  if (memInstMustInitialize(memOper))
+    return;
+
   if (auto apply = ApplySite::isa(memInst)) {
     SILArgumentConvention conv =
         apply.getArgumentConvention(apply.getCalleeArgIndex(*memOper));
@@ -922,8 +927,9 @@ static void checkAccessedAddress(Operand *memOper, StorageMap &Accesses) {
   if (!storage || !isPossibleFormalAccessBase(storage, memInst->getFunction()))
     return;
 
-  // Skip local non-lvalue accesses. There are many local initialization
-  // patterns that don't require access markers.
+  // A box or stack variable may represent lvalues, but they can only conflict
+  // with call sites in the same scope. Some initialization patters (stores to
+  // the local value) aren't protected by markers, so we need this check.
   if (!isa<ApplySite>(memInst)
       && (storage.getKind() == AccessedStorage::Box
           || storage.getKind() == AccessedStorage::Stack)) {

test/SILOptimizer/access_marker_verify.swift

@@ -1,7 +1,7 @@
 
 // RUN: %target-swift-frontend -module-name access_marker_verify -enable-verify-exclusivity -enforce-exclusivity=checked -enable-sil-ownership -emit-silgen -swift-version 4 -parse-as-library %s | %FileCheck %s
-// RUN: %target-swift-frontend -module-name access_marker_verify -enable-verify-exclusivity -enforce-exclusivity=checked -enable-sil-ownership -Onone -emit-sil -swift-version 4 -parse-as-library %s
-// RUN: %target-swift-frontend -module-name access_marker_verify -enable-verify-exclusivity -enforce-exclusivity=checked -enable-sil-ownership -O -emit-sil -swift-version 4 -parse-as-library %s
+// RUN: %target-swift-frontend -module-name access_marker_verify -enable-verify-exclusivity -enforce-exclusivity=checked -enable-sil-ownership -Onone -emit-sil -swift-version 4 -parse-as-library %s -o /dev/null
+// RUN: %target-swift-frontend -module-name access_marker_verify -enable-verify-exclusivity -enforce-exclusivity=checked -enable-sil-ownership -O -emit-sil -swift-version 4 -parse-as-library %s -o /dev/null
 // REQUIRES: asserts
 
 // Test the combination of SILGen + DiagnoseStaticExclusivity with verification.
@@ -995,3 +995,30 @@ func testPointerInit(x: Int, y: UnsafeMutablePointer<Int>) {
 // CHECK-NOT: begin_access
 // CHECK: assign %0 to [[ADR]] : $*Int
 // CHECK-LABEL: } // end sil function '$S20access_marker_verify15testPointerInit1x1yySi_SpySiGtF'
+
+// Verification should ignore the address operand of init_existential_addr.
+class testInitExistentialGlobal {
+  static var testProperty: P = StructP()
+}
+// CHECK-LABEL: sil private @globalinit{{.*}} : $@convention(c) () -> () {
+// CHECK:   alloc_global @$S20access_marker_verify25testInitExistentialGlobalC0D8PropertyAA1P_pvpZ
+// CHECK:   [[GADR:%.*]] = global_addr @$S20access_marker_verify25testInitExistentialGlobalC0D8PropertyAA1P_pvpZ : $*P
+// CHECK:   [[EADR:%.*]] = init_existential_addr [[GADR]] : $*P, $StructP
+// CHECK:   %{{.*}} = apply %{{.*}}({{.*}}) : $@convention(method) (@thin StructP.Type) -> StructP
+// CHECK:   store %{{.*}} to [trivial] [[EADR]] : $*StructP
+// CHECK-LABEL: } // end sil function 'globalinit
+
+public enum SomeError: Swift.Error {
+    case error
+}
+
+// Verification should ignore addresses produced by project_existential_box.
+public func testInitBox() throws {
+    throw SomeError.error
+}
+// CHECK-LABEL: sil @$S20access_marker_verify11testInitBoxyyKF : $@convention(thin) () -> @error Error {
+// CHECK: [[BOXALLOC:%.*]] = alloc_existential_box $Error, $SomeError
+// CHECK: [[PROJ:%.*]] = project_existential_box $SomeError in [[BOXALLOC]] : $Error
+// CHECK: store %{{.*}} to [trivial] [[PROJ]] : $*SomeError
+// CHECK: throw [[BOXALLOC]] : $Error
+// CHECK-LABEL: } // end sil function '$S20access_marker_verify11testInitBoxyyKF'