Merge pull request swiftlang#8954 from rjmccall/writeback-conflict-access-markers

Fix the writeback-conflict diagnostic to look through access markers

Author: rjmccall

lib/SILGen/LValue.h

@@ -497,22 +497,7 @@ struct LLVM_LIBRARY_VISIBILITY ExclusiveBorrowFormalAccess : FormalAccess {
         component(std::move(comp)), base(base), materialized(materialized) {}
 
   void diagnoseConflict(const ExclusiveBorrowFormalAccess &rhs,
-                        SILGenFunction &SGF) const {
-    // If the two writebacks we're comparing are of different kinds (e.g.
-    // ownership conversion vs a computed property) then they aren't the
-    // same and thus cannot conflict.
-    if (component->getKind() != rhs.component->getKind())
-      return;
-
-    // If the lvalues don't have the same base value, then they aren't the same.
-    // Note that this is the primary source of false negative for this
-    // diagnostic.
-    if (base.getValue() != rhs.base.getValue())
-      return;
-
-    component->diagnoseWritebackConflict(rhs.component.get(), loc, rhs.loc,
-                                         SGF);
-  }
+                        SILGenFunction &SGF) const;
 
   void performWriteback(SILGenFunction &SGF, bool isFinal) {
     Scope S(SGF.Cleanups, CleanupLocation::get(loc));

lib/SILGen/RValue.cpp

@@ -690,3 +690,30 @@ ManagedValue RValue::materialize(SILGenFunction &SGF, SILLocation loc) && {
   std::move(*this).forwardInto(SGF, loc, temp.get());
   return temp->getManagedAddress();
 }
+
+bool RValue::isObviouslyEqual(const RValue &rhs) const {
+  assert(isComplete() && rhs.isComplete() && "Comparing incomplete rvalues");
+
+  // Compare the count of elements instead of the type.
+  if (values.size() != rhs.values.size())
+    return false;
+
+  return std::equal(values.begin(), values.end(), rhs.values.begin(),
+                [](const ManagedValue &lhs, const ManagedValue &rhs) -> bool {
+                  return areObviouslySameValue(lhs.getValue(), rhs.getValue());
+                });
+}
+
+static SILValue getCanonicalValueSource(SILValue value) {
+  while (true) {
+    if (auto access = dyn_cast<BeginAccessInst>(value)) {
+      value = access->getSource();
+    } else {
+      return value;
+    }
+  }
+}
+
+bool RValue::areObviouslySameValue(SILValue lhs, SILValue rhs) {
+  return getCanonicalValueSource(lhs) == getCanonicalValueSource(rhs);
+}

lib/SILGen/RValue.h

@@ -222,19 +222,8 @@ class RValue {
     return RValue(*this, SGF, l);
   }
 
-  bool isObviouslyEqual(const RValue &rhs) const {
-    assert(isComplete() && rhs.isComplete() && "Comparing incomplete rvalues");
-
-    // Compare the count of elements instead of the type.
-    if (values.size() != rhs.values.size())
-      return false;
-
-    return std::equal(values.begin(), values.end(), rhs.values.begin(),
-                  [](const ManagedValue &lhs, const ManagedValue &rhs) -> bool {
-                    return lhs.getValue() == rhs.getValue() &&
-                    lhs.getCleanup() == rhs.getCleanup();
-                  });
-  }
+  static bool areObviouslySameValue(SILValue lhs, SILValue rhs);
+  bool isObviouslyEqual(const RValue &rhs) const;
 };
 
 } // end namespace Lowering

lib/SILGen/SILGenApply.cpp

@@ -1981,9 +1981,7 @@ static void beginInOutFormalAccesses(SILGenFunction &SGF,
   for (auto i = emittedInoutArgs.begin(), e = emittedInoutArgs.end();
          i != e; ++i) {
     for (auto j = emittedInoutArgs.begin(); j != i; ++j) {
-      // TODO: This uses exact SILValue equivalence to detect aliases,
-      // we could do something stronger here to catch other obvious cases.
-      if (i->first != j->first) continue;
+      if (!RValue::areObviouslySameValue(i->first, j->first)) continue;
 
       SGF.SGM.diagnose(i->second, diag::inout_argument_alias)
         .highlight(i->second.getSourceRange());

lib/SILGen/SILGenLValue.cpp

@@ -78,6 +78,28 @@ static void pushWriteback(SILGenFunction &SGF,
   cleanup.Depth = context.stable_begin();
 }
 
+void ExclusiveBorrowFormalAccess::diagnoseConflict(
+                                    const ExclusiveBorrowFormalAccess &rhs,
+                                    SILGenFunction &SGF) const {
+  // If the two writebacks we're comparing are of different kinds (e.g.
+  // ownership conversion vs a computed property) then they aren't the
+  // same and thus cannot conflict.
+  if (component->getKind() != rhs.component->getKind())
+    return;
+
+  // If the lvalues don't have the same base value (possibly null), then
+  // they aren't the same.  Note that this is the primary source of false
+  // negative for this diagnostic.
+  SILValue lhsBaseValue = base.getValue(), rhsBaseValue = rhs.base.getValue();
+  if (lhsBaseValue != rhsBaseValue &&
+      (!lhsBaseValue || !rhsBaseValue ||
+       !RValue::areObviouslySameValue(lhsBaseValue, rhsBaseValue))) {
+    return;
+  }
+
+  component->diagnoseWritebackConflict(rhs.component.get(), loc, rhs.loc, SGF);
+}
+
 //===----------------------------------------------------------------------===//
 
 static CanType getSubstFormalRValueType(Expr *expr) {

test/SILGen/writeback_conflict_diagnostics.swift

@@ -1,5 +1,5 @@
 // RUN: %target-swift-frontend %s -o /dev/null -emit-silgen -verify
-
+// RUN: %target-swift-frontend -enforce-exclusivity=checked %s -o /dev/null -emit-silgen -verify
 
 struct MutatorStruct {
   mutating func f(_ x : inout MutatorStruct) {}

test/SILOptimizer/exclusivity_static_diagnostics.swift

@@ -7,12 +7,16 @@ func takesTwoInouts<T>(_ p1: inout T, _ p2: inout T) { }
 func simpleInoutDiagnostic() {
   var i = 7
 
+  // expected-error@+4{{inout arguments are not allowed to alias each other}}
+  // expected-note@+3{{previous aliasing argument}}
   // expected-warning@+2{{simultaneous accesses to var 'i'; modification requires exclusive access}}
   // expected-note@+1{{conflicting access is here}}
   takesTwoInouts(&i, &i)
 }
 
 func inoutOnInoutParameter(p: inout Int) {
+  // expected-error@+4{{inout arguments are not allowed to alias each other}}
+  // expected-note@+3{{previous aliasing argument}}
   // expected-warning@+2{{simultaneous accesses to parameter 'p'; modification requires exclusive access}}
   // expected-note@+1{{conflicting access is here}}
   takesTwoInouts(&p, &p)
@@ -34,6 +38,8 @@ struct StructWithMutatingMethodThatTakesSelfInout {
   mutating func mutate(_ other: inout SomeClass) { }
 
   mutating func callMutatingMethodThatTakesSelfInout() {
+    // expected-error@+4{{inout arguments are not allowed to alias each other}}
+    // expected-note@+3{{previous aliasing argument}}
     // expected-warning@+2{{simultaneous accesses to parameter 'self'; modification requires exclusive access}}
     // expected-note@+1{{conflicting access is here}}
     mutate(&self)
@@ -74,6 +80,8 @@ class ClassWithFinalStoredProp {
 
 func violationWithGenericType<T>(_ p: T) {
   var local = p
+  // expected-error@+4{{inout arguments are not allowed to alias each other}}
+  // expected-note@+3{{previous aliasing argument}}
   // expected-warning@+2{{simultaneous accesses to var 'local'; modification requires exclusive access}}
   // expected-note@+1{{conflicting access is here}}
   takesTwoInouts(&local, &local)