Enable strict verification of begin_access patterns in all SIL passes. (#17534)

atrick · web-flow · commit 8d41d6ef5f5e · 2018-06-27T23:40:52.000-07:00
* Teach findAccessedStorage about global addressors.

AccessedStorage now properly represents access to global variables, even if they
haven't been fully optimized down to global_addr instructions.

This is essential for optimizing dynamic exclusivity checks. As a
verified SIL property, all access to globals and class properties
needs to be identifiable.

* Add stronger SILVerifier support for formal access.

Ensure that all formal access follows recognizable patterns
at all points in the SIL pipeline.

This is important to run acccess enforcement optimization late in the pipeline.
diff --git a/include/swift/SIL/SILGlobalVariable.h b/include/swift/SIL/SILGlobalVariable.h
@@ -68,7 +68,8 @@ class SILGlobalVariable
   /// once (either in its declaration, or once later), making it immutable.
   unsigned IsLet : 1;
 
-  /// The VarDecl associated with this SILGlobalVariable. For debugger purpose.
+  /// The VarDecl associated with this SILGlobalVariable. Must by nonnull for
+  /// language-level global variables.
   VarDecl *VDecl;
 
   /// Whether or not this is a declaration.
diff --git a/lib/SIL/MemAccessUtils.cpp b/lib/SIL/MemAccessUtils.cpp
@@ -13,6 +13,7 @@
 #define DEBUG_TYPE "sil-access-utils"
 
 #include "swift/SIL/MemAccessUtils.h"
+#include "swift/SIL/SILGlobalVariable.h"
 #include "swift/SIL/SILUndef.h"
 
 using namespace swift;
@@ -28,6 +29,14 @@ AccessedStorage::Kind AccessedStorage::classify(SILValue base) {
     return Stack;
   case ValueKind::GlobalAddrInst:
     return Global;
+  case ValueKind::ApplyInst: {
+    FullApplySite apply(cast<ApplyInst>(base));
+    if (auto *funcRef = apply.getReferencedFunction()) {
+      if (getVariableOfGlobalInit(funcRef))
+        return Global;
+    }
+    return Unidentified;
+  }
   case ValueKind::RefElementAddrInst:
     return Class;
   // A function argument is effectively a nested access, enforced
@@ -67,7 +76,19 @@ AccessedStorage::AccessedStorage(SILValue base, Kind kind) {
     paramIndex = cast<SILFunctionArgument>(base)->getIndex();
     break;
   case Global:
-    global = cast<GlobalAddrInst>(base)->getReferencedGlobal();
+    if (auto *GAI = dyn_cast<GlobalAddrInst>(base))
+      global = GAI->getReferencedGlobal();
+    else {
+      FullApplySite apply(cast<ApplyInst>(base));
+      auto *funcRef = apply.getReferencedFunction();
+      assert(funcRef);
+      global = getVariableOfGlobalInit(funcRef);
+      assert(global);
+      // Require a decl for all formally accessed globals defined in this
+      // module. (Access of globals defined elsewhere has Unidentified storage).
+      // AccessEnforcementWMO requires this.
+      assert(global->getDecl());
+    }
     break;
   case Class: {
     // Do a best-effort to find the identity of the object being projected
@@ -149,6 +170,17 @@ void AccessedStorage::print(raw_ostream &os) const {
 
 void AccessedStorage::dump() const { print(llvm::dbgs()); }
 
+// Return true if the given apply invokes a global addressor defined in another
+// module.
+static bool isExternalGlobalAddressor(ApplyInst *AI) {
+  FullApplySite apply(AI);
+  auto *funcRef = apply.getReferencedFunction();
+  if (!funcRef)
+    return false;
+  
+  return funcRef->isGlobalInit() && funcRef->isExternalDeclaration();
+}
+
 // Given an address base is a block argument, verify that it is actually a box
 // projected from a switch_enum. This is a valid pattern at any SIL stage
 // resulting in a block-type phi. In later SIL stages, the optimizer may form
@@ -186,12 +218,16 @@ static bool isAddressForLocalInitOnly(SILValue sourceAddr) {
   }
 }
 
+// AccessEnforcementWMO makes a strong assumption that all accesses are either
+// identified or are *not* accessing a global variable or class property defined
+// in this module. Consequently, we cannot simply bail out on
+// PointerToAddressInst as an Unidentified access.
 AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
   SILValue address = sourceAddr;
   while (true) {
     AccessedStorage::Kind kind = AccessedStorage::classify(address);
-    // First handle identified cases: these are always valid as the base of a
-    // formal access.
+    // First handle identified cases: these are always valid as the base of
+    // a formal access.
     if (kind != AccessedStorage::Unidentified)
       return AccessedStorage(address, kind);
 
@@ -202,10 +238,16 @@ AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
         return AccessedStorage(address, AccessedStorage::Unidentified);
       return AccessedStorage();
 
-    case ValueKind::PointerToAddressInst:
     case ValueKind::SILUndef:
       return AccessedStorage(address, AccessedStorage::Unidentified);
 
+    case ValueKind::ApplyInst:
+      if (isExternalGlobalAddressor(cast<ApplyInst>(address)))
+        return AccessedStorage(address, AccessedStorage::Unidentified);
+
+      // Don't currently allow any other calls to return an accessed address.
+      return AccessedStorage();
+
     // A block argument may be a box value projected out of
     // switch_enum. Address-type block arguments are not allowed.
     case ValueKind::SILPHIArgument:
@@ -218,17 +260,22 @@ AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
     // Load a box from an indirect payload of an opaque enum.
     // We must have peeked past the project_box earlier in this loop.
     // (the indirectness makes it a box, the load is for address-only).
-    // 
+    //
     // %payload_adr = unchecked_take_enum_data_addr %enum : $*Enum, #Enum.case
     // %box = load [take] %payload_adr : $*{ var Enum }
     //
     // FIXME: this case should go away with opaque values.
+    //
+    // Otherwise return invalid AccessedStorage.
     case ValueKind::LoadInst: {
-      assert(address->getType().is<SILBoxType>());
-      address = cast<LoadInst>(address)->getOperand();
-      assert(isa<UncheckedTakeEnumDataAddrInst>(address));
-      continue;
+      if (address->getType().is<SILBoxType>()) {
+        address = cast<LoadInst>(address)->getOperand();
+        assert(isa<UncheckedTakeEnumDataAddrInst>(address));
+        continue;
+      }
+      return AccessedStorage();
     }
+
     // Inductive cases:
     // Look through address casts to find the source address.
     case ValueKind::MarkUninitializedInst:
@@ -249,6 +296,30 @@ AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
       address = cast<SingleValueInstruction>(address)->getOperand(0);
       continue;
 
+    // Access to a Builtin.RawPointer. Treat this like the inductive cases
+    // above because some RawPointer's originate from identified locations. See
+    // the special case for global addressors, which return RawPointer above.
+    //
+    // If the inductive search does not find a valid addressor, it will
+    // eventually reach the default case that returns in invalid location. This
+    // is correct for RawPointer because, although accessing a RawPointer is
+    // legal SIL, there is no way to guarantee that it doesn't access class or
+    // global storage, so returning a valid unidentified storage object would be
+    // incorrect. It is the caller's responsibility to know that formal access
+    // to such a location can be safely ignored.
+    //
+    // For example:
+    //
+    // - KeyPath Builtins access RawPointer. However, the caller can check
+    // that the access `isFromBuilin` and ignore the storage.
+    //
+    // - lldb generates RawPointer access for debugger variables, but SILGen
+    // marks debug VarDecl access as 'Unsafe' and SIL passes don't need the
+    // AccessedStorage for 'Unsafe' access.
+    case ValueKind::PointerToAddressInst:
+      address = cast<SingleValueInstruction>(address)->getOperand(0);
+      continue;
+
     // Subobject projections.
     case ValueKind::StructElementAddrInst:
     case ValueKind::TupleElementAddrInst:
diff --git a/lib/SIL/SILVerifier.cpp b/lib/SIL/SILVerifier.cpp
@@ -1581,17 +1581,21 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
             "category");
   }
 
+  template <class AI>
+  void checkAccessEnforcement(AI *AccessInst) {
+    if (AccessInst->getModule().getStage() != SILStage::Raw) {
+      require(AccessInst->getEnforcement() != SILAccessEnforcement::Unknown,
+              "access must have known enforcement outside raw stage");
+    }
+  }
+
   void checkBeginAccessInst(BeginAccessInst *BAI) {
-    auto sourceOper = BAI->getOperand();
-    requireSameType(BAI->getType(), sourceOper->getType(),
+    requireSameType(BAI->getType(), BAI->getSource()->getType(),
                     "result must be same type as operand");
     require(BAI->getType().isAddress(),
             "begin_access operand must have address type");
 
-    if (BAI->getModule().getStage() != SILStage::Raw) {
-      require(BAI->getEnforcement() != SILAccessEnforcement::Unknown,
-              "access must have known enforcement outside raw stage");
-    }
+    checkAccessEnforcement(BAI);
 
     switch (BAI->getAccessKind()) {
     case SILAccessKind::Init:
@@ -1605,10 +1609,25 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
       break;
     }
 
-    // For dynamic Read/Modify access, AccessEnforcementWMO assumes a valid
-    // AccessedStorage and runs very late in the optimizer pipeline.
-    // TODO: eventually, make this true for any kind of access.
-    findAccessedStorage(sourceOper);
+    // Verify that all formal accesses patterns are recognized as part of a
+    // whitelist. The presence of an unknown pattern means that analysis will
+    // silently fail, and the compiler may be introducing undefined behavior
+    // with no other way to detect it.
+    //
+    // For example, AccessEnforcementWMO runs very late in the
+    // pipeline and assumes valid storage for all dynamic Read/Modify access. It
+    // also requires that Unidentified access fit a whitelist on known
+    // non-internal globals or class properties.
+    //
+    // First check that findAccessedStorage returns without asserting. For
+    // Unsafe enforcement, that is sufficient. For any other enforcement
+    // level also require that it returns a valid AccessedStorage object.
+    // Unsafe enforcement is used for some unrecognizable access patterns,
+    // like debugger variables. The compiler never cares about the source of
+    // those accesses.
+    AccessedStorage storage = findAccessedStorage(BAI->getSource());
+    if (BAI->getEnforcement() != SILAccessEnforcement::Unsafe)
+      require(storage, "Unknown formal access pattern");
   }
 
   void checkEndAccessInst(EndAccessInst *EAI) {
@@ -1623,20 +1642,43 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     }
   }
 
-  void checkBeginUnpairedAccessInst(BeginUnpairedAccessInst *I) {
-    require(I->getEnforcement() != SILAccessEnforcement::Unknown,
+  void checkBeginUnpairedAccessInst(BeginUnpairedAccessInst *BUAI) {
+    require(BUAI->getEnforcement() != SILAccessEnforcement::Unknown,
             "unpaired access can never use unknown enforcement");
-    require(I->getSource()->getType().isAddress(),
+    require(BUAI->getSource()->getType().isAddress(),
             "address operand must have address type");
-    requireAddressType(BuiltinUnsafeValueBufferType, I->getBuffer(),
+    requireAddressType(BuiltinUnsafeValueBufferType, BUAI->getBuffer(),
                        "scratch buffer operand");
+
+    checkAccessEnforcement(BUAI);
+
+    switch (BUAI->getAccessKind()) {
+    case SILAccessKind::Init:
+    case SILAccessKind::Deinit:
+      require(BUAI->getEnforcement() == SILAccessEnforcement::Static,
+              "init/deinit accesses cannot use non-static enforcement");
+      break;
+    case SILAccessKind::Read:
+    case SILAccessKind::Modify:
+      break;
+    }
+
+    // First check that findAccessedStorage never asserts.
+    AccessedStorage storage = findAccessedStorage(BUAI->getSource());
+    // Only allow Unsafe and Builtin access to have invalid storage.
+    if (BUAI->getEnforcement() != SILAccessEnforcement::Unsafe
+        && !BUAI->isFromBuiltin()) {
+      require(storage, "Unknown formal access pattern");
+    }
   }
 
   void checkEndUnpairedAccessInst(EndUnpairedAccessInst *I) {
     require(I->getEnforcement() != SILAccessEnforcement::Unknown,
             "unpaired access can never use unknown enforcement");
     requireAddressType(BuiltinUnsafeValueBufferType, I->getBuffer(),
                        "scratch buffer operand");
+
+    checkAccessEnforcement(I);
   }
 
   void checkStoreInst(StoreInst *SI) {
diff --git a/lib/SILOptimizer/Analysis/AccessSummaryAnalysis.cpp b/lib/SILOptimizer/Analysis/AccessSummaryAnalysis.cpp
@@ -68,13 +68,15 @@ void AccessSummaryAnalysis::processArgument(FunctionInfo *info,
     switch (user->getKind()) {
     case SILInstructionKind::BeginAccessInst: {
       auto *BAI = cast<BeginAccessInst>(user);
-      const IndexTrieNode *subPath = findSubPathAccessed(BAI);
-      summary.mergeWith(BAI->getAccessKind(), BAI->getLoc(), subPath);
-      // We don't add the users of the begin_access to the worklist because
-      // even if these users eventually begin an access to the address
-      // or a projection from it, that access can't begin more exclusive
-      // access than this access -- otherwise it will be diagnosed
-      // elsewhere.
+      if (BAI->getEnforcement() != SILAccessEnforcement::Unsafe) {
+        const IndexTrieNode *subPath = findSubPathAccessed(BAI);
+        summary.mergeWith(BAI->getAccessKind(), BAI->getLoc(), subPath);
+        // We don't add the users of the begin_access to the worklist because
+        // even if these users eventually begin an access to the address
+        // or a projection from it, that access can't begin more exclusive
+        // access than this access -- otherwise it will be diagnosed
+        // elsewhere.
+      }
       break;
     }
     case SILInstructionKind::EndUnpairedAccessInst:
diff --git a/lib/SILOptimizer/Mandatory/AccessMarkerElimination.cpp b/lib/SILOptimizer/Mandatory/AccessMarkerElimination.cpp
@@ -32,9 +32,6 @@ using namespace swift;
 // This temporary option allows markers during optimization passes. Enabling
 // this flag causes this pass to preserve only dynamic checks when dynamic
 // checking is enabled. Otherwise, this pass removes all checks.
-//
-// This is currently unsupported because tail duplication results in
-// address-type block arguments.
 llvm::cl::opt<bool> EnableOptimizedAccessMarkers(
     "sil-optimized-access-markers", llvm::cl::init(true),
     llvm::cl::desc("Enable memory access markers during optimization passes."));
diff --git a/lib/SILOptimizer/Mandatory/DiagnoseStaticExclusivity.cpp b/lib/SILOptimizer/Mandatory/DiagnoseStaticExclusivity.cpp
@@ -811,6 +811,9 @@ static void checkForViolationAtApply(ApplySite Apply, AccessState &State) {
 static void checkForViolationsAtInstruction(SILInstruction &I,
                                             AccessState &State) {
   if (auto *BAI = dyn_cast<BeginAccessInst>(&I)) {
+    if (BAI->getEnforcement() == SILAccessEnforcement::Unsafe)
+      return;
+
     SILAccessKind Kind = BAI->getAccessKind();
     const AccessedStorage &Storage =
       findValidAccessedStorage(BAI->getSource());
@@ -826,6 +829,9 @@ static void checkForViolationsAtInstruction(SILInstruction &I,
   }
 
   if (auto *EAI = dyn_cast<EndAccessInst>(&I)) {
+    if (EAI->getBeginAccess()->getEnforcement() == SILAccessEnforcement::Unsafe)
+      return;
+
     auto It =
       State.Accesses->find(findValidAccessedStorage(EAI->getSource()));
     AccessInfo &Info = It->getSecond();
@@ -1080,6 +1086,9 @@ static void checkAccessedAddress(Operand *memOper, StorageMap &Accesses) {
   // Otherwise, the address base should be an in-scope begin_access.
   if (storage.getKind() == AccessedStorage::Nested) {
     auto *BAI = cast<BeginAccessInst>(storage.getValue());
+    if (BAI->getEnforcement() == SILAccessEnforcement::Unsafe)
+      return;
+
     const AccessedStorage &Storage = findValidAccessedStorage(BAI->getSource());
     AccessInfo &Info = Accesses[Storage];
     if (!Info.hasAccessesInProgress())
diff --git a/test/SILOptimizer/accessed_storage_analysis.sil b/test/SILOptimizer/accessed_storage_analysis.sil
diff --git a/test/SILOptimizer/exclusivity_static_diagnostics.sil b/test/SILOptimizer/exclusivity_static_diagnostics.sil
