Merge pull request #60889 from gottesmm/initial-move-only-address-checker

[move-only] Initial patch for move only address checking.

Author: gottesmm

docs/SIL.rst

@@ -2593,9 +2593,9 @@ only values" that are guaranteed to never be copied. This is enforced by:
 * Having SILGen emit copies as it normally does.
 
 * Use OSSA canonicalization to eliminate copies that aren't needed semantically
-  due to consuming uses of the value. This is implemented by the pass guaranteed
-  pass "MoveOnlyChecker". Emit errors on any of the consuming uses that we found
-  in said pass.
+  due to consuming uses of the value. This is implemented by the guaranteed
+  passes "MoveOnlyObjectChecker" and "MoveOnlyAddressChecker". We will emit
+  errors on any of the consuming uses that we found in said pass.
 
 Assuming that no errors are emitted, we can then conclude before we reach
 canonical SIL that the value was never copied and thus is a "move only value"
@@ -2667,7 +2667,7 @@ rather than a viral type level annotation that would constrain the type system.
 As mentioned above trivial move only wrapped types are actually
 non-trivial. This is because in SIL ownership is tied directly to
 non-trivialness so unless we did that we could not track ownership
-accurately. This is loss of triviality is not an issue for most of the pipeline
+accurately. This loss of triviality is not an issue for most of the pipeline
 since we eliminate all move only wrapper types for trivial types during the
 guaranteed optimizations after we have run various ownership checkers but before
 we have run diagnostics for trivial types (e.x.: DiagnosticConstantPropagation).

include/swift/AST/DiagnosticsSIL.def

@@ -727,13 +727,25 @@ ERROR(noimplicitcopy_used_on_generic_or_existential, none,
 // move only checker diagnostics
 ERROR(sil_moveonlychecker_owned_value_consumed_more_than_once, none,
       "'%0' consumed more than once", (StringRef))
+ERROR(sil_moveonlychecker_value_used_after_consume, none,
+      "'%0' used after consume. Lifetime extension of variable requires a copy", (StringRef))
 ERROR(sil_moveonlychecker_guaranteed_value_consumed, none,
       "'%0' has guaranteed ownership but was consumed", (StringRef))
+ERROR(sil_moveonlychecker_inout_not_reinitialized_before_end_of_function, none,
+      "'%0' consumed but not reinitialized before end of function", (StringRef))
+ERROR(sil_moveonlychecker_value_consumed_in_a_loop, none,
+      "'%0' consumed by a use in a loop", (StringRef))
+
 NOTE(sil_moveonlychecker_consuming_use_here, none,
      "consuming use", ())
-ERROR(sil_moveonlychecker_not_understand_mark_move, none,
+NOTE(sil_moveonlychecker_nonconsuming_use_here, none,
+     "non-consuming use", ())
+ERROR(sil_moveonlychecker_not_understand_no_implicit_copy, none,
       "Usage of @noImplicitCopy that the move checker does not know how to "
       "check!", ())
+ERROR(sil_moveonlychecker_not_understand_moveonly, none,
+      "Usage of a move only type that the move checker does not know how to "
+      "check!", ())
 
 // move kills copyable values checker diagnostics
 ERROR(sil_movekillscopyablevalue_value_consumed_more_than_once, none,

include/swift/SIL/BasicBlockData.h

@@ -213,6 +213,32 @@ class BasicBlockData {
     }
     return data[getIndex(block)];
   }
+
+  /// Look up the state associated with \p block. Returns nullptr upon failure.
+  NullablePtr<Data> get(SILBasicBlock *block) const {
+    if (block->index < 0)
+      return {nullptr};
+    Data *d = &const_cast<BasicBlockData *>(this)->data[getIndex(block)];
+    return NullablePtr<Data>(d);
+  }
+
+  /// If \p block is a new block, i.e. created after this BasicBlockData was
+  /// constructed, creates a new Data by calling
+  /// Data(std::forward<ArgTypes>(Args)...).
+  template <typename... ArgTypes>
+  std::pair<Data *, bool> try_emplace(SILBasicBlock *block,
+                                      ArgTypes &&...Args) {
+    if (block->index != 0) {
+      return {&data[getIndex(block)], false};
+    }
+
+    assert(validForBlockOrder == function->BlockListChangeIdx &&
+           "BasicBlockData invalid because the function's block list changed");
+    validForBlockOrder = ++function->BlockListChangeIdx;
+    block->index = data.size();
+    data.emplace_back(std::forward<ArgTypes>(Args)...);
+    return {&data.back(), true};
+  }
 };
 
 } // end swift namespace

include/swift/SIL/PrunedLiveness.h

@@ -556,9 +556,141 @@ struct PrunedLivenessBoundary {
                ArrayRef<SILBasicBlock *> postDomBlocks);
 };
 
+//===----------------------------------------------------------------------===//
+//                      Field Sensitive Pruned Liveness
+//===----------------------------------------------------------------------===//
+
 /// Given a type T and a descendent field F in T's type tree, then the
 /// sub-element number of F is the first leaf element of the type tree in its
 /// linearized representation.
+///
+/// Linearized Representation of Structs/Tuples
+/// -------------------------------------------
+///
+/// For structs/tuples, the linearized representation is just an array with one
+/// element for each leaf element. Thus if we have a struct of the following
+/// sort:
+///
+/// ```
+/// struct Pair {
+///   var lhs: Int
+///   var rhs: Int
+/// }
+///
+/// struct MyStruct {
+///   var firstField: Int
+///   var pairField: Pair
+///   var tupleField: (Int, Int)
+/// }
+/// ```
+///
+/// the linearized representation of MyStruct's type tree leaves would be:
+///
+/// ```
+/// [firstField, pairField.lhs, pairField.rhs, tupleField.0, tupleField.1]
+/// ```
+///
+/// So if one had an uninitialized myStruct and initialized pairField, one would
+/// get the following bit set of liveness:
+///
+/// ```
+/// [0, 1, 1, 0, 0]
+/// ```
+///
+/// Linearized Representation of Enums
+/// ----------------------------------
+///
+/// Since enums are sum types, an enum can require different numbers of bits in
+/// its linearized representation depending on the payload of the case that the
+/// enum is initialized to. To work around this problem in our representation,
+/// we always store enough bits for the max sized payload of all cases of the
+/// enum and add an additional last bit for the discriminator. Any extra bits
+/// that may be needed (e.x.: we are processing a enum case with a smaller
+/// payload) are always assumed to be set to the same value that the
+/// discriminator bit is set to. This representation allows us to track liveness
+/// trading off the ability to determine information about the actual case that
+/// we are tracking. Since we just care about liveness, this is a trade off that
+/// we are willing to make since our goal (computing liveness) is still solved.
+///
+/// With the above paragraph in mind, an example of the bit layout of an enum
+/// looks as follows:
+///
+/// ```
+/// [ PAYLOAD BITS | EXTRA_TOP_LEVEL_BITS | DISCRIMINATOR_BIT ]
+/// ```
+///
+/// Notice how placing the discriminator bit last ensures that separately the
+/// payload and the extra top level bits/discriminator bit are both contiguous
+/// in the representation. This ensures that we can test both that the payload
+/// is live and separately that the discriminator/extra top level bits are live
+/// with a single contiguous range of bits. This is important since field
+/// sensitive liveness can only compute liveness for contiguous ranges of bits.
+///
+/// Lets look at some examples, starting with E:
+///
+/// ```
+/// enum E {
+/// case firstCase
+/// case secondCase(Int)
+/// case thirdCase(Pair)
+/// }
+/// ```
+///
+/// The linearized representation of E would be three slots since the payload
+/// with the largest linearized representation is Pair:
+///
+///            ----- |E| --------
+///           /                  \
+///          /                    \
+///         v                      v
+///      | Pair |          | Discriminator |
+///       /    \
+///      /      \
+///     /        \
+///    v          v
+/// | LHS |    | RHS |
+///
+/// This in term would mean the following potential bit representations given
+/// various cases/states of deinitialization of the payload.
+///
+/// ```
+/// firstCase inited:            [1, 1, 1]
+/// firstCase deinited:          [0, 0, 0]
+///
+/// secondCase inited:           [1, 1, 1]
+/// secondCase payload deinited: [0, 1, 1]
+/// secondCase deinited:         [0, 0, 0]
+///
+/// thirdCase inited:            [1, 1, 1]
+/// thirdCase payload deinited:  [0, 0, 1]
+/// thirdCase deinited:          [0, 0, 0]
+/// ```
+///
+/// Now lets consider an enum without any payload cases. Given such an enum:
+///
+/// ```
+/// enum E2 {
+/// case firstCase
+/// case secondCase
+/// case thirdCase
+/// }
+/// ```
+///
+/// we would only use a single bit in our linearized representation, just for
+/// the discriminator value.
+///
+/// Enums and Partial Initialization
+/// --------------------------------
+///
+/// One property of our representation of structs and tuples is that a code
+/// generator can reinitialize a struct/tuple completely just by re-initializing
+/// each of its sub-types individually. This is not possible for enums in our
+/// representation since if one just took the leaf nodes for the payload, one
+/// would not update the bit for the enum case itself and any additional spare
+/// bits. Luckily for us, this is actually impossible to do in SIL since it is
+/// impossible to dynamically change the payload of an enum without destroying
+/// the original enum and its payload since that would be a verifier caught
+/// leak.
 struct SubElementNumber {
   unsigned number;
 

include/swift/SILOptimizer/PassManager/Passes.def

@@ -454,6 +454,8 @@ PASS(AssemblyVisionRemarkGenerator, "assembly-vision-remark-generator",
      "Emit assembly vision remarks that provide source level guidance of where runtime calls ended up")
 PASS(MoveOnlyObjectChecker, "sil-move-only-object-checker",
      "Pass that enforces move only invariants on raw SIL for objects")
+PASS(MoveOnlyAddressChecker, "sil-move-only-address-checker",
+     "Pass that enforces move only invariants on raw SIL for addresses")
 PASS(MoveKillsCopyableValuesChecker, "sil-move-kills-copyable-values-checker",
      "Pass that checks that any copyable (non-move only) value that is passed "
      "to _move do not have any uses later than the _move")

lib/SIL/Utils/MemAccessUtils.cpp

@@ -1368,12 +1368,14 @@ class AccessPathVisitor : public FindAccessVisitorImpl<AccessPathVisitor> {
     } else {
       // Ignore everything in getAccessProjectionOperand that is an access
       // projection with no affect on the access path.
-      assert(isa<OpenExistentialAddrInst>(projectedAddr)
-             || isa<InitEnumDataAddrInst>(projectedAddr)
-             || isa<UncheckedTakeEnumDataAddrInst>(projectedAddr)
+      assert(isa<OpenExistentialAddrInst>(projectedAddr) ||
+             isa<InitEnumDataAddrInst>(projectedAddr) ||
+             isa<UncheckedTakeEnumDataAddrInst>(projectedAddr)
              // project_box is not normally an access projection but we treat it
              // as such when it operates on unchecked_take_enum_data_addr.
-             || isa<ProjectBoxInst>(projectedAddr));
+             || isa<ProjectBoxInst>(projectedAddr)
+             // Ignore mark_must_check, we just look through it when we see it.
+             || isa<MarkMustCheckInst>(projectedAddr));
     }
     return sourceAddr->get();
   }
@@ -1862,7 +1864,14 @@ AccessPathDefUseTraversal::visitSingleValueUser(SingleValueInstruction *svi,
     return IgnoredUse;
   }
 
-  // MARK: Access projections
+  case SILInstructionKind::MarkMustCheckInst: {
+    // Mark must check goes on the project_box, so it isn't a ref.
+    assert(!dfs.isRef());
+    pushUsers(svi, dfs);
+    return IgnoredUse;
+  }
+
+    // MARK: Access projections
 
   case SILInstructionKind::StructElementAddrInst:
   case SILInstructionKind::TupleElementAddrInst:

lib/SIL/Utils/OwnershipUtils.cpp

@@ -936,7 +936,7 @@ swift::findTransitiveUsesForAddress(SILValue projectedAddress,
         isa<InitExistentialAddrInst>(user) || isa<InitEnumDataAddrInst>(user) ||
         isa<BeginAccessInst>(user) || isa<TailAddrInst>(user) ||
         isa<IndexAddrInst>(user) || isa<StoreBorrowInst>(user) ||
-        isa<UncheckedAddrCastInst>(user)) {
+        isa<UncheckedAddrCastInst>(user) || isa<MarkMustCheckInst>(user)) {
       transitiveResultUses(op);
       continue;
     }

lib/SIL/Utils/PrunedLiveness.cpp

@@ -11,10 +11,12 @@
 //===----------------------------------------------------------------------===//
 
 #include "swift/SIL/PrunedLiveness.h"
+#include "swift/AST/TypeExpansionContext.h"
 #include "swift/Basic/Defer.h"
 #include "swift/SIL/BasicBlockDatastructures.h"
 #include "swift/SIL/BasicBlockUtils.h"
 #include "swift/SIL/OwnershipUtils.h"
+#include "swift/SIL/SILInstruction.h"
 
 using namespace swift;
 
@@ -406,8 +408,26 @@ TypeSubElementCount::TypeSubElementCount(SILType type, SILModule &mod,
     return;
   }
 
-  // If this isn't a tuple or struct, it is a single element. This was our
-  // default value, so we can just return.
+  // If we have an enum, we add one for tracking if the base enum is set and use
+  // the remaining bits for the max sized payload. This ensures that if we have
+  // a smaller sized payload, we still get all of the bits set, allowing for a
+  // homogeneous representation.
+  if (auto *enumDecl = type.getEnumOrBoundGenericEnum()) {
+    unsigned numElements = 0;
+    for (auto *eltDecl : enumDecl->getAllElements()) {
+      if (!eltDecl->hasAssociatedValues())
+        continue;
+      numElements = std::max(
+          numElements,
+          unsigned(TypeSubElementCount(
+              type.getEnumElementType(eltDecl, mod, context), mod, context)));
+    }
+    number = numElements + 1;
+    return;
+  }
+
+  // If this isn't a tuple, struct, or enum, it is a single element. This was
+  // our default value, so we can just return.
 }
 
 Optional<SubElementNumber>
@@ -463,8 +483,30 @@ SubElementNumber::compute(SILValue projectionDerivedFromRoot,
       continue;
     }
 
-    // This fails when we visit unchecked_take_enum_data_addr. We should just
-    // add support for enums.
+    // In the case of enums, we note that our representation is:
+    //
+    //                   ---------|Enum| ---
+    //                  /                   \
+    //                 /                     \
+    //                v                       v
+    //  |Bits for Max Sized Payload|    |Discrim Bit|
+    //
+    // So our payload is always going to start at the current field number since
+    // we are the left most child of our parent enum. So we just need to look
+    // through to our parent enum.
+    if (auto *enumData = dyn_cast<UncheckedTakeEnumDataAddrInst>(
+            projectionDerivedFromRoot)) {
+      projectionDerivedFromRoot = enumData->getOperand();
+      continue;
+    }
+
+    // Init enum data addr is treated like unchecked take enum data addr.
+    if (auto *initData =
+            dyn_cast<InitEnumDataAddrInst>(projectionDerivedFromRoot)) {
+      projectionDerivedFromRoot = initData->getOperand();
+      continue;
+    }
+
 #ifndef NDEBUG
     if (!isa<InitExistentialAddrInst>(projectionDerivedFromRoot)) {
       llvm::errs() << "Unknown access path instruction!\n";

lib/SIL/Verifier/LoadBorrowImmutabilityChecker.cpp

@@ -169,6 +169,17 @@ bool GatherWritesVisitor::visitUse(Operand *op, AccessUseType useTy) {
     }
     return true;
 
+  case SILInstructionKind::ExplicitCopyAddrInst:
+    if (cast<ExplicitCopyAddrInst>(user)->getDest() == op->get()) {
+      writeAccumulator.push_back(op);
+      return true;
+    }
+    // This operand is the copy source. Check if it is taken.
+    if (cast<ExplicitCopyAddrInst>(user)->isTakeOfSrc()) {
+      writeAccumulator.push_back(op);
+    }
+    return true;
+
   case SILInstructionKind::MarkUnresolvedMoveAddrInst:
     if (cast<MarkUnresolvedMoveAddrInst>(user)->getDest() == op->get()) {
       writeAccumulator.push_back(op);

lib/SILGen/SILGenBuilder.cpp

@@ -946,7 +946,8 @@ ManagedValue SILGenBuilder::createGuaranteedCopyableToMoveOnlyWrapperValue(
 ManagedValue
 SILGenBuilder::createMarkMustCheckInst(SILLocation loc, ManagedValue value,
                                        MarkMustCheckInst::CheckKind kind) {
-  assert(value.isPlusOne(SGF) && "Argument must be at +1!");
+  assert((value.isPlusOne(SGF) || value.isLValue()) &&
+         "Argument must be at +1 or be an inout!");
   CleanupCloner cloner(*this, value);
   auto *mdi = SILBuilder::createMarkMustCheckInst(
       loc, value.forward(getSILGenFunction()), kind);

lib/SILGen/SILGenDecl.cpp

@@ -376,6 +376,11 @@ class LocalVariableInitialization : public SingleBufferInitialization {
     }
 
     Addr = SGF.B.createProjectBox(decl, Box, 0);
+    if (Addr->getType().isMoveOnly()) {
+      // TODO: Handle no implicit copy here.
+      Addr = SGF.B.createMarkMustCheckInst(
+          decl, Addr, MarkMustCheckInst::CheckKind::NoImplicitCopy);
+    }
 
     // Push a cleanup to destroy the local variable.  This has to be
     // inactive until the variable is initialized.