SIL Verifier: implement load-borrow-immutability checkin in the swift verifier

Author: eeckstein

SwiftCompilerSources/Sources/Optimizer/Utilities/Verifier.swift

@@ -54,6 +54,24 @@ private extension Instruction {
   }
 }
 
+private extension Argument {
+  func verify(_ context: FunctionPassContext) {
+    if let phi = Phi(self), phi.value.ownership == .guaranteed {
+      var forwardingBorrowedFromFound = false
+      for use in phi.value.uses {
+        require(use.instruction is BorrowedFromInst,
+                     "guaranteed phi: \(self)\n has non borrowed-from use: \(use)")
+        if use.index == 0 {
+          require(!forwardingBorrowedFromFound, "phi \(self) has multiple forwarding borrowed-from uses")
+          forwardingBorrowedFromFound = true
+        }
+      }
+      require (forwardingBorrowedFromFound,
+                   "missing forwarding borrowed-from user of guaranteed phi \(phi)")
+    }
+  }
+}
+
 extension BorrowedFromInst : VerifiableInstruction {
   func verify(_ context: FunctionPassContext) {
     var computedEVs = Stack<Value>(context)
@@ -74,20 +92,121 @@ extension BorrowedFromInst : VerifiableInstruction {
   }
 }
 
-private extension Argument {
+extension LoadBorrowInst : VerifiableInstruction {
   func verify(_ context: FunctionPassContext) {
-    if let phi = Phi(self), phi.value.ownership == .guaranteed {
-      var forwardingBorrowedFromFound = false
-      for use in phi.value.uses {
-        require(use.instruction is BorrowedFromInst,
-                     "guaranteed phi: \(self)\n has non borrowed-from use: \(use)")
-        if use.index == 0 {
-          require(!forwardingBorrowedFromFound, "phi \(self) has multiple forwarding borrowed-from uses")
-          forwardingBorrowedFromFound = true
+    if isUnchecked {
+      return
+    }
+
+    var mutatingInstructions = MutatingUsesWalker(context)
+    defer { mutatingInstructions.deinitialize() }
+
+    mutatingInstructions.findMutatingUses(of: self.address)
+    mutatingInstructions.verifyNoMutatingUsesInLiferange(of: self)
+  }
+}
+
+// Used to check if any instruction is mutating the memory location within the liferange of a `load_borrow`.
+// Note that it is not checking if an instruction _may_ mutate the memory, but it's checking if any instruction
+// _definitely_ will mutate the memory.
+// Otherwise the risk would be too big for false alarms. It also means that this verification is not perfect and
+// might miss some subtle violations.
+private struct MutatingUsesWalker : AddressDefUseWalker {
+  let context: FunctionPassContext
+  var mutatingInstructions: InstructionSet
+
+  init(_ context: FunctionPassContext) {
+    self.context = context
+    self.mutatingInstructions = InstructionSet(context)
+  }
+
+  mutating func deinitialize() {
+    self.mutatingInstructions.deinitialize()
+  }
+
+  mutating func findMutatingUses(of address: Value) {
+    _ = walkDownUses(ofAddress: address, path: UnusedWalkingPath())
+  }
+
+  mutating func verifyNoMutatingUsesInLiferange(of load: LoadBorrowInst) {
+    // The liferange of a `load_borrow` can end in a branch instruction. In such cases we handle the re-borrow liferanges
+    // (starting at the `borrowed_from` in the successor block) separately.
+    // This worklist contains the starts of the individual linear liferanges,
+    // i.e. `load_borrow` and `borrowed_from` instructions.
+    var linearLiferanges = SpecificInstructionWorklist<SingleValueInstruction>(context)
+    defer { linearLiferanges.deinitialize() }
+
+    linearLiferanges.pushIfNotVisited(load)
+    while let startInst = linearLiferanges.pop() {
+      verifyNoMutatingUsesInLinearLiferange(of: startInst)
+
+      for use in startInst.uses {
+        if let phi = Phi(using: use) {
+          linearLiferanges.pushIfNotVisited(phi.borrowedFrom!)
         }
       }
-      require (forwardingBorrowedFromFound,
-                   "missing forwarding borrowed-from user of guaranteed phi \(phi)")
+    }
+  }
+
+  private mutating func verifyNoMutatingUsesInLinearLiferange(of startInst: SingleValueInstruction) {
+    assert(startInst is LoadBorrowInst || startInst is BorrowedFromInst)
+
+    var instWorklist = InstructionWorklist(context)
+    defer { instWorklist.deinitialize() }
+
+    // It would be good enough to only consider `end_borrow`s (and branches), but adding all users doesn't harm.
+    for use in startInst.uses {
+      instWorklist.pushPredecessors(of: use.instruction, ignoring: startInst)
+    }
+
+    while let inst = instWorklist.pop() {
+      require(!mutatingInstructions.contains(inst),
+              "Load borrow invalidated by a local write", atInstruction: inst)
+      instWorklist.pushPredecessors(of: inst, ignoring: startInst)
+    }
+  }
+
+  mutating func leafUse(address: Operand, path: UnusedWalkingPath) -> WalkResult {
+    if address.isMutatedAddress {
+      mutatingInstructions.insert(address.instruction)
+    }
+    return .continueWalk
+  }
+}
+
+private extension Operand {
+  // Returns true if the operand's instruction is definitely writing to the operand's address value.
+  var isMutatedAddress: Bool {
+    assert(value.type.isAddress)
+    switch instruction {
+    case let store as StoringInstruction:
+      return self == store.destinationOperand
+    case let copy as SourceDestAddrInstruction:
+      if self == copy.destinationOperand {
+        return true
+      } else if self == copy.sourceOperand && copy.isTakeOfSrc {
+        return true
+      }
+      return false
+    case let load as LoadInst:
+      return load.loadOwnership == .take
+    case let partialApply as PartialApplyInst:
+      if !partialApply.isOnStack,
+         let convention = partialApply.convention(of: self)
+      {
+        switch convention {
+        case .indirectIn, .indirectInGuaranteed:
+          // Such operands are consumed by the `partial_apply` and therefore cound as "written".
+          return true
+        default:
+          return false
+        }
+      }
+      return false
+    case is DestroyAddrInst, is IsUniqueInst:
+      return true
+    default:
+      return false
     }
   }
 }

lib/SIL/Verifier/CMakeLists.txt

@@ -1,6 +1,5 @@
 target_sources(swiftSIL PRIVATE
   DebugInfoVerifier.cpp
-  LoadBorrowImmutabilityChecker.cpp
   LinearLifetimeChecker.cpp
   MemoryLifetimeVerifier.cpp
   GuaranteedPhiVerifier.cpp