Merge pull request swiftlang#24947 from atrick/fix-dynamic-exclusivity

Fix exclusivity diagnostics to be aware of [dynamically_replaceable].

Author: atrick

include/swift/AST/DiagnosticsParse.def

@@ -731,6 +731,8 @@ ERROR(expected_type_after_arrow,none,
 ERROR(function_type_argument_label,none,
       "function types cannot have argument labels; use '_' before %0",
       (Identifier))
+ERROR(expected_dynamic_func_attr,none,
+      "expected a dynamically_replaceable function", ())
 
 // Enum Types
 ERROR(expected_expr_enum_case_raw_value,PointsToFirstBadToken,

include/swift/SIL/SILFunction.h

@@ -590,7 +590,6 @@ class SILFunction
     return IsDynamicallyReplaceable_t(IsDynamicReplaceable);
   }
   void setIsDynamic(IsDynamicallyReplaceable_t value = IsDynamic) {
-    assert(IsDynamicReplaceable == IsNotDynamic && "already set");
     IsDynamicReplaceable = value;
     assert(!Transparent || !IsDynamicReplaceable);
   }

lib/ParseSIL/ParseSIL.cpp

@@ -2722,6 +2722,14 @@ bool SILParser::parseSILInstruction(SILBuilder &B) {
     if (parseSILFunctionRef(InstLoc, Fn) ||
         parseSILDebugLocation(InstLoc, B))
       return true;
+    // Set a forward reference's dynamic property for the first time.
+    if (!Fn->isDynamicallyReplaceable()) {
+      if (!Fn->empty()) {
+        P.diagnose(P.Tok, diag::expected_dynamic_func_attr);
+        return true;
+      }
+      Fn->setIsDynamic();
+    }
     ResultVal = B.createDynamicFunctionRef(InstLoc, Fn);
     break;
   }

lib/SILOptimizer/Mandatory/DiagnoseStaticExclusivity.cpp

@@ -695,18 +695,10 @@ struct AccessState {
 };
 } // namespace
 
-/// For each argument in the range of the callee arguments being applied at the
-/// given apply site, use the summary analysis to determine whether the
-/// arguments will be accessed in a way that conflicts with any currently in
-/// progress accesses. If so, diagnose.
-static void checkCaptureAccess(ApplySite Apply, AccessState &State) {
-  SILFunction *Callee = Apply.getCalleeFunction();
-  if (!Callee || Callee->empty())
-    return;
-
-  const AccessSummaryAnalysis::FunctionSummary &FS =
-      State.ASA->getOrCreateSummary(Callee);
-
+// Find conflicting access on each argument using AccessSummaryAnalysis.
+static void
+checkCaptureAccess(ApplySite Apply, AccessState &State,
+                   const AccessSummaryAnalysis::FunctionSummary &FS) {
   for (unsigned ArgumentIndex : range(Apply.getNumArguments())) {
 
     unsigned CalleeIndex =
@@ -724,7 +716,7 @@ static void checkCaptureAccess(ApplySite Apply, AccessState &State) {
     SILValue Argument = Apply.getArgument(ArgumentIndex);
     assert(Argument->getType().isAddress());
 
-    // A valid AccessedStorage should alway sbe found because Unsafe accesses
+    // A valid AccessedStorage should always be found because Unsafe accesses
     // are not tracked by AccessSummaryAnalysis.
     const AccessedStorage &Storage = findValidAccessedStorage(Argument);
     auto AccessIt = State.Accesses->find(Storage);
@@ -739,6 +731,50 @@ static void checkCaptureAccess(ApplySite Apply, AccessState &State) {
   }
 }
 
+/// For each argument in the range of the callee arguments being applied at the
+/// given apply site, use the summary analysis to determine whether the
+/// arguments will be accessed in a way that conflicts with any currently in
+/// progress accesses. If so, diagnose.
+static void checkCaptureAccess(ApplySite Apply, AccessState &State) {
+  // A callee may be nullptr or empty for various reasons, such as being
+  // dynamically replaceable.
+  SILFunction *Callee = Apply.getCalleeFunction();
+  if (Callee && !Callee->empty()) {
+    checkCaptureAccess(Apply, State, State.ASA->getOrCreateSummary(Callee));
+    return;
+  }
+  // In the absence of AccessSummaryAnalysis, conservatively assume by-address
+  // captures are fully accessed by the callee.
+  for (Operand &argOper : Apply.getArgumentOperands()) {
+    auto convention = Apply.getArgumentConvention(argOper);
+    if (convention != SILArgumentConvention::Indirect_InoutAliasable)
+      continue;
+
+    // A valid AccessedStorage should always be found because Unsafe accesses
+    // are not tracked by AccessSummaryAnalysis.
+    const AccessedStorage &Storage = findValidAccessedStorage(argOper.get());
+
+    // Are there any accesses in progress at the time of the call?
+    auto AccessIt = State.Accesses->find(Storage);
+    if (AccessIt == State.Accesses->end())
+      continue;
+
+    // The unknown argument access is considered a modify of the root subpath.
+    auto argAccess = RecordedAccess(SILAccessKind::Modify, Apply.getLoc(),
+                                    State.ASA->getSubPathTrieRoot());
+
+    // Construct a conflicting RecordedAccess if one doesn't already exist.
+    const AccessInfo &info = AccessIt->getSecond();
+    auto inProgressAccess =
+        shouldReportAccess(info, SILAccessKind::Modify, argAccess.getSubPath());
+    if (!inProgressAccess)
+      continue;
+
+    State.ConflictingAccesses.emplace_back(Storage, *inProgressAccess,
+                                           argAccess);
+  }
+}
+
 /// If the given values has a SILFunctionType or an Optional<SILFunctionType>,
 /// return the SILFunctionType. Otherwise, return an invalid type.
 static CanSILFunctionType getSILFunctionTypeForValue(SILValue arg) {

test/SILOptimizer/exclusivity_static_diagnostics.sil

@@ -1421,3 +1421,44 @@ bb0(%0 : @guaranteed $@convention(block) () -> ()):
   %v = tuple ()
   return %v : $()
 }
+
+//-----------------------------------------------------------------------------
+// Test dynamically replaceable. Diagnostics should not be able to
+// make assumptions about the accesses in the callee function.
+
+struct TestDynamic {
+  @_hasStorage @_hasInitialValue var x: Builtin.Int64 { get set }
+  @_hasStorage @_hasInitialValue var y: Builtin.Int64 { get set }
+  public mutating func foo()
+  init(x: Builtin.Int64, y: Builtin.Int64)
+  init()
+}
+
+@_hasStorage @_hasInitialValue var s: S { get set }
+
+sil hidden [ossa] @testCallDynamic : $@convention(method) (@inout TestDynamic) -> () {
+bb0(%0 : $*TestDynamic):
+  %access = begin_access [modify] [unknown] %0 : $*TestDynamic // expected-error {{overlapping accesses, but modification requires exclusive access; consider copying to a local variable}}
+  %sea = struct_element_addr %access : $*TestDynamic, #TestDynamic.x
+  %f = dynamic_function_ref @testDynamicLocal : $@convention(thin) (@inout Builtin.Int64, @inout_aliasable TestDynamic) -> ()
+  %call = apply %f(%sea, %0) : $@convention(thin) (@inout Builtin.Int64, @inout_aliasable TestDynamic) -> () // expected-note {{conflicting access is here}}
+  end_access %access : $*TestDynamic
+  %v = tuple ()
+  return %v : $()
+}
+
+// This callee actually accesses different subpaths, but the caller's
+// diagnostic should not be able to see that.
+sil private [dynamically_replacable] [ossa] @testDynamicLocal : $@convention(thin) (@inout Builtin.Int64, @inout_aliasable TestDynamic) -> () {
+bb0(%0 : $*Builtin.Int64, %1 : $*TestDynamic):
+  %literal = integer_literal $Builtin.Int64, 1
+  %access1 = begin_access [modify] [unknown] %0 : $*Builtin.Int64
+  assign %literal to %access1 : $*Builtin.Int64
+  end_access %access1 : $*Builtin.Int64
+  %access2 = begin_access [modify] [unknown] %1 : $*TestDynamic
+  %sea = struct_element_addr %access2 : $*TestDynamic, #TestDynamic.y
+  assign %literal to %sea : $*Builtin.Int64
+  end_access %access2 : $*TestDynamic
+  %19 = tuple ()
+  return %19 : $()
+}