Don't run AccessEnforcementSelection on deserialized SIL and add verification.

This is a module pass, so was processing a combination of raw and deserialized
canonical SIL. The analysis makes structural assumptions that only hold prior to
mandatory inlining.

Add more verification of the structural properties.

Teach the pass to skip over canonical SIL, which only works because closures
must be serialized/deserialized in the same module as their parent.

Fixes <rdar://38042781> [Repl] crash while running SILOptimizer

Author: atrick

lib/SILOptimizer/Mandatory/AccessEnforcementSelection.cpp

@@ -244,6 +244,20 @@ void SelectEnforcement::analyzeUsesOfBox(SingleValueInstruction *source) {
   // capture and, for some reason, the closure is dead.
 }
 
+// Verify that accesses are not nested before mandatory inlining.
+// Closure captures should also not be nested within an access.
+static void checkUsesOfAccess(BeginAccessInst *access) {
+#ifndef NDEBUG
+  // These conditions are only true prior to mandatory inlining.
+  assert(!access->getFunction()->wasDeserializedCanonical());
+  for (auto *use : access->getUses()) {
+    auto user = use->getUser();
+    assert(!isa<BeginAccessInst>(user));
+    assert(!isa<PartialApplyInst>(user));
+  }
+#endif
+}
+
 void SelectEnforcement::analyzeProjection(ProjectBoxInst *projection) {
   for (auto *use : projection->getUses()) {
     auto user = use->getUser();
@@ -253,6 +267,8 @@ void SelectEnforcement::analyzeProjection(ProjectBoxInst *projection) {
       if (access->getEnforcement() == SILAccessEnforcement::Unknown)
         Accesses.push_back(access);
 
+      checkUsesOfAccess(access);
+
       continue;
     }
     if (isa<PartialApplyInst>(user))
@@ -564,16 +580,26 @@ void AccessEnforcementSelection::run() {
 void AccessEnforcementSelection::processFunction(SILFunction *F) {
   DEBUG(llvm::dbgs() << "Access Enforcement Selection in " << F->getName()
                      << "\n");
+
+  // This ModuleTransform needs to analyze closures and their parent scopes in
+  // the same pass, and the parent needs to be analyzed before the closure.
 #ifndef NDEBUG
   auto *CSA = getAnalysis<ClosureScopeAnalysis>();
   if (isNonEscapingClosure(F->getLoweredFunctionType())) {
     for (auto *scopeF : CSA->getClosureScopes(F)) {
       DEBUG(llvm::dbgs() << "  Parent scope: " << scopeF->getName() << "\n");
       assert(visited.count(scopeF));
+      // Closures must be defined in the same module as their parent scope.
+      assert(scopeF->wasDeserializedCanonical()
+             == F->wasDeserializedCanonical());
     }
   }
   visited.insert(F);
 #endif
+  // Deserialized functions, which have been mandatory inlined, no longer meet
+  // the structural requirements on access markers required by this pass.
+  if (F->wasDeserializedCanonical())
+    return;
 
   for (auto &bb : *F) {
     for (auto ii = bb.begin(), ie = bb.end(); ii != ie;) {

test/SILOptimizer/access_enforcement_selection.sil

@@ -228,3 +228,36 @@ bb0(%0 : @trivial $*Builtin.Int64, %1 : @trivial $Builtin.Int64):
   %closure = partial_apply %f(%0) : $@convention(thin) (@inout_aliasable Builtin.Int64) -> ()
   unreachable
 }
+
+sil [canonical] @serializedClosureCapturingByStorageAddress : $@convention(thin) (@inout_aliasable Builtin.Int64) -> () {
+bb0(%0 : @trivial $*Builtin.Int64):
+  %2 = begin_access [read] [unknown] %0 : $*Builtin.Int64
+  %3 = load [trivial] %2 : $*Builtin.Int64
+  end_access %2 : $*Builtin.Int64
+  %10 = tuple ()
+  return %10 : $()
+}
+
+// A begin_access may not be used by a partial_apply or a nested
+// begin_access prior to mandatory inlining. Nonetheless, this does
+// occur in deserialzied SIL. This SIL is only well-formed because the
+// function is marked [canonical].
+sil [canonical] @accessAroundClosure : $@convention(thin) () -> () {
+bb0:
+  %1 = alloc_box ${ var Builtin.Int64 }, var, name "x"
+  %2 = copy_value %1 : ${ var Builtin.Int64 }
+  %3 = project_box %1 : ${ var Builtin.Int64 }, 0
+  // outer access (presumably its uses were mandatory inlined)
+  %4 = begin_access [modify] [static] %3 : $*Builtin.Int64
+  %5 = function_ref @serializedClosureCapturingByStorageAddress : $@convention(thin) (@inout_aliasable Builtin.Int64) -> ()
+  %6 = partial_apply %5(%4) : $@convention(thin) (@inout_aliasable Builtin.Int64) -> ()
+  %7 = begin_access [modify] [static] %4 : $*Builtin.Int64
+  %8 = function_ref @takesInoutAndClosure : $@convention(thin) (@inout Builtin.Int64, @owned @callee_owned () -> ()) -> ()
+  %9 = apply %8(%7, %6) : $@convention(thin) (@inout Builtin.Int64, @owned @callee_owned () -> ()) -> ()
+  end_access %7 : $*Builtin.Int64
+  end_access %4 : $*Builtin.Int64
+  destroy_value %2 : ${ var Builtin.Int64 }
+  destroy_value %1 : ${ var Builtin.Int64 }
+  %10 = tuple ()
+  return %10 : $()
+}