[ownership] Add a new ReborrowVerifier #34438

meg-gupta · 2020-10-26T18:28:53Z

This updates how we model reborrow's lifetimes for ownership verification.
Today we follow and combine a borrow's lifetime through phi args as well.
Owned values lifetimes end at a phi arg. This discrepency in modeling
lifetimes leads to the OwnershipVerifier raising errors incorrectly for
cases such as this, where the borrow and the base value do not dominate
the end_borrow:

bb0:
  cond_br undef, bb1, bb2
bb1:
  %copy0 = copy_value %0
  %borrow0 = begin_borrow %copy0
  br bb3(%borrow0, %copy0)
bb2:
  %copy1 = copy_value %1
  %borrow1 = begin_borrow %copy1
  br bb3(%borrow1, %copy1)
bb3(%borrow, %baseVal):
  end_borrow %borrow
  destroy_value %baseVal

This PR adds a new ReborrowVerifier. The ownership verifier collects borrow's
lifetime ending users and populates the worklist of the ReborrowVerifier
with reborrows and the corresponding base value.
ReborrowVerifier then verifies that the lifetime of the reborrow is
within the lifetime of the base value.

gottesmm · 2020-10-26T19:58:37Z

include/swift/SIL/ReborrowVerifier.h

+  bool verify();
+
+  // Add a guaranteed phiArg and its base value to the worklist for verification
+  void add(SILPhiArgument *phiArg, SILValue baseVal) {


Does this need to be in SIL? Can we hide it as a library detail?

I'll update it.

meg-gupta · 2020-10-26T20:58:36Z

@swift-ci test

gottesmm

Some initial comments around perf/etc and some things for me that I need to think about. I want to look through this again before this goes in.

lib/SIL/Verifier/ReborrowVerifierPrivate.h

gottesmm · 2020-10-26T21:32:18Z

lib/SIL/Verifier/ReborrowVerifierPrivate.h

+// this.
+class ReborrowVerifier {
+  /// worklist of reborrows and their base value
+  llvm::SmallVector<std::pair<SILPhiArgument *, SILValue>, 8> worklist;


You don't need llvm:: on SmallVector.

gottesmm · 2020-10-26T21:32:34Z

lib/SIL/Verifier/ReborrowVerifierPrivate.h

+// lies within the lifetime of its base value. It uses LinearLifetimeChecker for
+// this.
+class ReborrowVerifier {
+  /// worklist of reborrows and their base value


Caps plz. I would also put space in between ivars.

gottesmm · 2020-10-26T21:34:28Z

lib/SIL/Verifier/ReborrowVerifier.cpp

+    auto item = worklist.pop_back_val();
+    auto *phiArg = item.first;
+    auto baseVal = item.second;
+    SmallVector<Operand *, 4> phiArgUses(phiArg->getUses());


I would hoist these small things out. That way if we go big, we do not need to reallocate.

gottesmm · 2020-10-26T21:37:36Z

lib/SIL/Verifier/ReborrowVerifier.cpp

@@ -0,0 +1,63 @@
+//===--- ReborrowVerifier.cpp ----------------------------------------===//


This header is messed up. I think it isn't 80 columns. Also can you change the copyright year to 2020?

gottesmm · 2020-10-26T21:38:30Z

include/swift/SIL/LinearLifetimeChecker.h

@@ -50,6 +50,7 @@ class LinearLifetimeChecker {
  class ErrorBuilder;

 private:
+  friend class ReborrowVerifier;


Rather than doing this, can you expose a higher level API on LinearLifetimeChecker? Maybe an asserting variant of checkUses?

Am doing this to pass LinearLifetimeChecker::ErrorBuilder with continuous error number with a common errorMessageCounter. I would have used the public LinearLifetimeChecker ::validateLifetime but would loose the nice error numbering.
What do you think about exposing a version of LinearLifetimeChecker ::validateLifetime that has an errorBuilder parameter ?

gottesmm · 2020-10-26T21:38:54Z

include/swift/SIL/SILValue.h

@@ -285,6 +285,12 @@ class ValueBase : public SILNode, public SILAllocated<ValueBase> {
  /// and it has a single consuming user. Returns .none otherwise.
  inline Operand *getSingleConsumingUse() const;

+  inline SmallVector<Operand *, 4>


Can you use an iterator here instead of returning a SmallVector?

Also why do you need the verifying argument?

When verifying with OwnershipVerifierTextualErrorDumper - we don't want this assert to fire: https://github.com/apple/swift/blob/58d07a4efd0d9e5a38dc4b0d029d6f80d7152a62/include/swift/SIL/SILValue.h#L586. That reminds me that callers should pass true only when verifying with OwnershipVerifierTextualErrorDumper.

gottesmm · 2020-10-26T21:39:22Z

lib/SIL/Utils/OwnershipUtils.cpp

-
-          foundUses.push_back(op);
-        });
+        [&](Operand *op) { foundUses.push_back(op); });
  }


I need to think about this. Just to make sure it works in my brain.

gottesmm · 2020-10-26T21:40:25Z

lib/SIL/Verifier/LinearLifetimeChecker.cpp

+              }
+              if (!isa<TermInst>(&i)) {
+                return true;
+              }


I need to think about this. This is used for more than just the reborrow verifier.

test/SIL/ownership-verifier/borrow_extract_validate.sil

lib/SIL/Verifier/SILOwnershipVerifier.cpp

gottesmm · 2020-10-26T21:44:01Z

lib/SIL/Verifier/SILOwnershipVerifier.cpp

@@ -215,6 +233,79 @@ bool SILValueOwnershipChecker::isCompatibleDefUse(
  return false;
 }

+// Look for reborrows and find their base value and populate ReborrowVerifier's
+// worklist.
+void SILValueOwnershipChecker::populateReborrowVerifier(


I would stick this in ReborrowVerifier. SILValueOwnershipChecker doesn't need to know about this detail.

gottesmm · 2020-10-26T21:44:43Z

lib/SIL/Verifier/ReborrowVerifierPrivate.h

+    worklist.push_back(std::make_pair(phiArg, baseVal));
+  }
+
+  // check if the operand is visited


3 slashes for doxygen comment.

gottesmm · 2020-10-26T21:45:06Z

lib/SIL/Verifier/ReborrowVerifierPrivate.h

+  // check if the phi arg and base value are visited
+  bool isVisitedPhiArg(SILPhiArgument *phiArg, SILValue baseVal) {
+    auto itPhiArg = visitedPhiArgs.find(phiArg);
+    if (itPhiArg != visitedPhiArgs.end() && (*itPhiArg).second == baseVal)


You can just return this. That is:

return itPhiArg != visitedPhiArgs.end() && ...

gottesmm · 2020-10-26T21:46:02Z

lib/SIL/Verifier/ReborrowVerifierPrivate.h

+  /// ignore "leaks".
+  DeadEndBlocks &deadEndBlocks;
+  /// A cache map of borrow lifetime ending operands and their base value
+  llvm::DenseMap<Operand *, SILValue> visitedOps;


Do these need to be full on DenseMaps? Seems like they could be SmallDenseMaps.

swift-ci · 2020-10-26T21:47:05Z

Build failed
Swift Test Linux Platform
Git Sha - 48b505a73f0467e0c794b9053e2f2a2d27304085

swift-ci · 2020-10-26T23:39:02Z

Build failed
Swift Test OS X Platform
Git Sha - 48b505a73f0467e0c794b9053e2f2a2d27304085

gottesmm · 2020-10-26T23:51:01Z

include/swift/SIL/SILValue.h

-                 << ". Not in map!\n"
-                 << *this;
-    llvm_unreachable("standard error assertion");
+  if (!verifying) {


If we want to do this, why not pass back an optional?

gottesmm

Some more comments. I want to understand this more before it goes in/have my questions answered.

gottesmm · 2020-10-27T16:38:35Z

include/swift/SIL/SILValue.h

-#ifndef NDEBUG
-  if (!canAcceptKind(kind)) {
-    llvm::errs() << "Can not lookup lifetime constraint: " << kind
-                 << ". Not in map!\n"


I think this is needed here. Here is my suggestion, why don't we add two entry points, one that asserts and the other that returns an optional. That way the user can make the choice.

Right, I reverted to original

include/swift/SIL/SILValue.h

gottesmm · 2020-10-27T16:39:08Z

include/swift/SIL/SILValue.h

@@ -770,6 +781,71 @@ inline ValueBase::use_iterator ValueBase::use_end() const {
 inline iterator_range<ValueBase::use_iterator> ValueBase::getUses() const {
  return { use_begin(), use_end() };
 }
+
+class ConsumingUseIterator : public ValueBaseUseIterator {
+  public:


This needs to be git-clang-formated.

gottesmm · 2020-10-27T16:41:29Z

lib/SIL/Utils/OwnershipUtils.cpp

@@ -229,13 +229,6 @@ void BorrowingOperand::visitConsumingUsesOfBorrowIntroducingUserResults(
    // This enables one to walk the def-use chain of guaranteed phis for a
    // single guaranteed scope.
    value.visitLocalScopeEndingUses([&](Operand *valueUser) {
-      if (auto subBorrowScopeOp = BorrowingOperand::get(valueUser)) {
-        if (subBorrowScopeOp->consumesGuaranteedValues()) {
-          subBorrowScopeOp->visitUserResultConsumingUses(func);


The point of this method was to be able to chain through a piece-wise guaranteed lifetime. Given that we are no longer doing that I don't think that this method makes sense as written anymore.

I am leaving BorrowingOperand::visitConsumingUsesOfBorrowIntroducingUserResults unchanged now. It gets called transitively from BorrowedValue::areUsesWithinScope which gets used in SemanticARCOpt

gottesmm · 2020-10-27T16:42:36Z

lib/SIL/Utils/OwnershipUtils.cpp

  SmallVector<BorrowingOperand, 8> worklist;
  SmallPtrSet<Operand *, 8> visitedValue;
  worklist.push_back(*this);
  visitedValue.insert(op);
-  bool foundError = false;
+
  while (!worklist.empty()) {
    auto scopedOperand = worklist.pop_back_val();
    scopedOperand.visitConsumingUsesOfBorrowIntroducingUserResults(


I think this at this point should just be visitEndScopeInstructions, no?

yes, changed it : )

gottesmm · 2020-10-27T16:43:16Z

lib/SIL/Verifier/LinearLifetimeChecker.cpp

+                       }
+                       // guaranteed phi args are verifier as part of
+                       // ReborrowVerifier
+                       if (nonConsumingUse->get().getOwnershipKind() ==


I don't understand why this is here. This is a detail of the ReborrowVerifier leaking into the LinearLifetimeChecker. Can you elaborate here?

lib/SIL/Verifier/ReborrowVerifier.cpp

gottesmm · 2020-10-27T16:46:09Z

lib/SIL/Verifier/SILOwnershipVerifier.cpp

-    foundError |=
-        initialScopedOperand->getImplicitUses(nonLifetimeEndingUsers, &error);
+    SmallVector<Operand *, 4> borrowLifetimeEndingUsers;
+    initialScopedOperand->getImplicitUses(borrowLifetimeEndingUsers, &error);


My thought: why not just make the reborrow verifier a struct that is created here that takes the initialScopedOperand and verifies based off of that? Would be simpler and details wouldn't leak out.

lib/SIL/Verifier/SILOwnershipVerifier.cpp

meg-gupta · 2020-10-29T05:35:53Z

@swift-ci test

swift-ci · 2020-10-29T06:26:41Z

Build failed
Swift Test Linux Platform
Git Sha - 58f716ed345fd2e1c058962393d2177b4caf21e7

gottesmm · 2020-10-29T18:27:09Z

lib/SIL/Verifier/LinearLifetimeChecker.cpp

+                         return false;
+                       // Reborrows are currently represented as a consuming
+                       // use. Do not raise error for reborrows.
+                       if (isReborrowInstruction(&i))


I thought about this in a deeper manner. The condition we are really looking for here is if the consuming use and the non consuming use are the same instruction. We can just hoist that out of the if into an earlier if statement and continue. I.e.:

for (auto *nonConsumingUse : nonConsumingUsesInBlock) { auto *consumingUser = consumingUse->getUser(); if (nonConsumingUse->getUser() != consumingUser)) { if (std::find_if(... normal code ...)) { continue; } } else { if (isReborrowInstruction(consumingUser)) continue; } ... throw error ... }

I am also worried about baking in isReborrowInstruction into the verifier itself. It is meant to be a more general utility than this. Maybe add a flag that controls whether or not we allow for reborrows and have it disabled by default? Then at least we are only making this change where it is needed.

Another thing, on the reborrow failure path we could emit a better error as well (since it is not exactly an outside of lifetime use).

I refactored. But am not sure why would we need a flag ? Reborrows are never consuming

gottesmm · 2020-10-29T18:30:23Z

lib/SIL/Verifier/ReborrowVerifier.cpp

+  SmallVector<Operand *, 4> phiArgUses(phiArg->getUses().begin(),
+                                       phiArg->getUses().end());
+
+  // Verify whethre the guaranteed phi arg lies within the lifetime of the base


whethre -> whether

gottesmm

Looking much better! Some more smaller things.

I looked (but maybe I needed to look harder = p, so if I missed it feel free to ignore). Can you add multiple diamond tests. Something that does:

checked_cast_br -> switch_enum?

gottesmm · 2020-10-29T18:30:42Z

lib/SIL/Verifier/ReborrowVerifier.cpp

+                                              SILValue baseVal) {
+  SmallPtrSet<SILBasicBlock *, 4> visitedBlocks;
+  bool result = false;
+  SmallVector<Operand *, 4> phiArgUses(phiArg->getUses().begin(),


I think you can just pass in phiArg->getUses(). I would try it.

lib/SIL/Verifier/ReborrowVerifier.cpp

gottesmm · 2020-10-29T18:32:29Z

lib/SIL/Verifier/ReborrowVerifier.cpp

+    if (isVisitedOp(borrowLifetimeEndOp, baseVal))
+      continue;
+
+    // Process reborrow


I would add a TODO here that says if we add more reborrow instructions this should be abstracted onto a ReborrowingOperand ADT type so we can abstract over terminator vs non-terminator. I am fine doing that latter.

gottesmm · 2020-10-29T18:33:03Z

lib/SIL/Verifier/ReborrowVerifierPrivate.h

+
+// A guaranteed phi arg ends the borrow scope of its incoming value and begins a
+// new borrow scope. ReborrowVerifier validates the lifetime of the reborrow
+// lies within the lifetime of its base value. It uses LinearLifetimeChecker for


Three slashes here.

lib/SIL/Verifier/SILOwnershipVerifier.cpp

gottesmm · 2020-10-29T18:34:45Z

test/SIL/ownership-verifier/borrow_cast_validate.sil

@@ -0,0 +1,267 @@
+
+// RUN: %target-sil-opt -sil-ownership-verifier-enable-testing -ownership-verifier-textual-error-dumper -enable-sil-verify-all=0 %s -o /dev/null 2>&1 | %FileCheck %s


Nit: Can you put the RUN line on the top line and put a space in between the REQUIRES: asserts and the imports.

This updates how we model reborrow's lifetimes for ownership verification. Today we follow and combine a borrow's lifetime through phi args as well. Owned values lifetimes end at a phi arg. This discrepency in modeling lifetimes leads to the OwnershipVerifier raising errors incorrectly for cases such as this, where the borrow and the base value do not dominate the end_borrow: bb0: cond_br undef, bb1, bb2 bb1: %copy0 = copy_value %0 %borrow0 = begin_borrow %copy0 br bb3(%borrow0, %copy0) bb2: %copy1 = copy_value %1 %borrow1 = begin_borrow %copy1 br bb3(%borrow1, %copy1) bb3(%borrow, %baseVal): end_borrow %borrow destroy_value %baseVal This PR adds a new ReborrowVerifier. The ownership verifier collects borrow's lifetime ending users and populates the worklist of the ReborrowVerifier with reborrows and the corresponding base value. ReborrowVerifier then verifies that the lifetime of the reborrow is within the lifetime of the base value.

meg-gupta · 2020-10-30T04:26:36Z

@swift-ci test

swift-ci · 2020-10-30T05:46:56Z

Build failed
Swift Test Linux Platform
Git Sha - 601ea65

swift-ci · 2020-10-30T06:42:25Z

Build failed
Swift Test OS X Platform
Git Sha - 601ea65

gottesmm · 2020-11-02T19:48:51Z

@swift-ci test

swift-ci · 2020-11-02T22:28:14Z

Build failed
Swift Test OS X Platform
Git Sha - 601ea65

meg-gupta · 2020-11-02T23:26:15Z

@swift-ci test OS X Platform

gottesmm reviewed Oct 26, 2020

View reviewed changes

meg-gupta force-pushed the reborrowverifier branch from 76c1790 to 7fb3b79 Compare October 26, 2020 20:12

meg-gupta requested review from gottesmm and atrick October 26, 2020 20:12

meg-gupta force-pushed the reborrowverifier branch 2 times, most recently from f863ea3 to 48b505a Compare October 26, 2020 20:58

gottesmm reviewed Oct 26, 2020

View reviewed changes

test/SIL/ownership-verifier/borrow_extract_validate.sil Outdated Show resolved Hide resolved

gottesmm reviewed Oct 26, 2020

View reviewed changes

lib/SIL/Verifier/SILOwnershipVerifier.cpp Show resolved Hide resolved

gottesmm reviewed Oct 26, 2020

View reviewed changes

meg-gupta force-pushed the reborrowverifier branch from 48b505a to fd6e3b0 Compare October 26, 2020 23:43

gottesmm reviewed Oct 26, 2020

View reviewed changes

meg-gupta force-pushed the reborrowverifier branch from fd6e3b0 to 09c275f Compare October 27, 2020 04:36

gottesmm reviewed Oct 27, 2020

View reviewed changes

meg-gupta force-pushed the reborrowverifier branch 2 times, most recently from a1f7fc7 to 58f716e Compare October 29, 2020 00:35

gottesmm reviewed Oct 29, 2020

View reviewed changes

meg-gupta force-pushed the reborrowverifier branch from 58f716e to 601ea65 Compare October 30, 2020 04:21

meg-gupta merged commit 6c46841 into swiftlang:main Nov 3, 2020

meg-gupta deleted the reborrowverifier branch July 9, 2021 00:27

		@@ -0,0 +1,63 @@
		//===--- ReborrowVerifier.cpp ----------------------------------------===//

		@@ -0,0 +1,267 @@

		// RUN: %target-sil-opt -sil-ownership-verifier-enable-testing -ownership-verifier-textual-error-dumper -enable-sil-verify-all=0 %s -o /dev/null 2>&1 \| %FileCheck %s

[ownership] Add a new ReborrowVerifier #34438

[ownership] Add a new ReborrowVerifier #34438

Uh oh!

Conversation

meg-gupta commented Oct 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meg-gupta commented Oct 26, 2020

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Oct 26, 2020

Uh oh!

swift-ci commented Oct 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

meg-gupta commented Oct 29, 2020