Skip to content

Commit 6e3f56f

Browse files
committed
Fix exclusivity diagnostics to be aware of [dynamically_replaceable].
Previously, any function marked [dynamically_replaceable] that was partially applied and captured by address would not be diagnosed. This is a rare thing. For example: struct S { var x = 0 public mutating func testCallDynamic() { dynamic func bar(_ i: inout Int) { i = 1 x = 2 } bar(&x) } } Fixes <rdar://problem/50972786> Fix exclusivity diagnostics to be aware of [dynamically_replaceable].
1 parent a3de97b commit 6e3f56f

File tree

2 files changed

+90
-13
lines changed

2 files changed

+90
-13
lines changed

lib/SILOptimizer/Mandatory/DiagnoseStaticExclusivity.cpp

Lines changed: 49 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -695,18 +695,10 @@ struct AccessState {
695695
};
696696
} // namespace
697697

698-
/// For each argument in the range of the callee arguments being applied at the
699-
/// given apply site, use the summary analysis to determine whether the
700-
/// arguments will be accessed in a way that conflicts with any currently in
701-
/// progress accesses. If so, diagnose.
702-
static void checkCaptureAccess(ApplySite Apply, AccessState &State) {
703-
SILFunction *Callee = Apply.getCalleeFunction();
704-
if (!Callee || Callee->empty())
705-
return;
706-
707-
const AccessSummaryAnalysis::FunctionSummary &FS =
708-
State.ASA->getOrCreateSummary(Callee);
709-
698+
// Find conflicting access on each argument using AccessSummaryAnalysis.
699+
static void
700+
checkCaptureAccess(ApplySite Apply, AccessState &State,
701+
const AccessSummaryAnalysis::FunctionSummary &FS) {
710702
for (unsigned ArgumentIndex : range(Apply.getNumArguments())) {
711703

712704
unsigned CalleeIndex =
@@ -724,7 +716,7 @@ static void checkCaptureAccess(ApplySite Apply, AccessState &State) {
724716
SILValue Argument = Apply.getArgument(ArgumentIndex);
725717
assert(Argument->getType().isAddress());
726718

727-
// A valid AccessedStorage should alway sbe found because Unsafe accesses
719+
// A valid AccessedStorage should always be found because Unsafe accesses
728720
// are not tracked by AccessSummaryAnalysis.
729721
const AccessedStorage &Storage = findValidAccessedStorage(Argument);
730722
auto AccessIt = State.Accesses->find(Storage);
@@ -739,6 +731,50 @@ static void checkCaptureAccess(ApplySite Apply, AccessState &State) {
739731
}
740732
}
741733

734+
/// For each argument in the range of the callee arguments being applied at the
735+
/// given apply site, use the summary analysis to determine whether the
736+
/// arguments will be accessed in a way that conflicts with any currently in
737+
/// progress accesses. If so, diagnose.
738+
static void checkCaptureAccess(ApplySite Apply, AccessState &State) {
739+
// A callee may be nullptr or empty for various reasons, such as being
740+
// dynamically replaceable.
741+
SILFunction *Callee = Apply.getCalleeFunction();
742+
if (Callee && !Callee->empty()) {
743+
checkCaptureAccess(Apply, State, State.ASA->getOrCreateSummary(Callee));
744+
return;
745+
}
746+
// In the absence of AccessSummaryAnalysis, conservatively assume by-address
747+
// captures are fully accessed by the callee.
748+
for (Operand &argOper : Apply.getArgumentOperands()) {
749+
auto convention = Apply.getArgumentConvention(argOper);
750+
if (convention != SILArgumentConvention::Indirect_InoutAliasable)
751+
continue;
752+
753+
// A valid AccessedStorage should always be found because Unsafe accesses
754+
// are not tracked by AccessSummaryAnalysis.
755+
const AccessedStorage &Storage = findValidAccessedStorage(argOper.get());
756+
757+
// Are there any accesses in progress at the time of the call?
758+
auto AccessIt = State.Accesses->find(Storage);
759+
if (AccessIt == State.Accesses->end())
760+
continue;
761+
762+
// The unknown argument access is considered a modify of the root subpath.
763+
auto argAccess = RecordedAccess(SILAccessKind::Modify, Apply.getLoc(),
764+
State.ASA->getSubPathTrieRoot());
765+
766+
// Construct a conflicting RecordedAccess if one doesn't already exist.
767+
const AccessInfo &info = AccessIt->getSecond();
768+
auto inProgressAccess =
769+
shouldReportAccess(info, SILAccessKind::Modify, argAccess.getSubPath());
770+
if (!inProgressAccess)
771+
continue;
772+
773+
State.ConflictingAccesses.emplace_back(Storage, *inProgressAccess,
774+
argAccess);
775+
}
776+
}
777+
742778
/// If the given values has a SILFunctionType or an Optional<SILFunctionType>,
743779
/// return the SILFunctionType. Otherwise, return an invalid type.
744780
static CanSILFunctionType getSILFunctionTypeForValue(SILValue arg) {

test/SILOptimizer/exclusivity_static_diagnostics.sil

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1421,3 +1421,44 @@ bb0(%0 : @guaranteed $@convention(block) () -> ()):
14211421
%v = tuple ()
14221422
return %v : $()
14231423
}
1424+
1425+
//-----------------------------------------------------------------------------
1426+
// Test dynamically replaceable. Diagnostics should not be able to
1427+
// make assumptions about the accesses in the callee function.
1428+
1429+
struct TestDynamic {
1430+
@_hasStorage @_hasInitialValue var x: Builtin.Int64 { get set }
1431+
@_hasStorage @_hasInitialValue var y: Builtin.Int64 { get set }
1432+
public mutating func foo()
1433+
init(x: Builtin.Int64, y: Builtin.Int64)
1434+
init()
1435+
}
1436+
1437+
@_hasStorage @_hasInitialValue var s: S { get set }
1438+
1439+
sil hidden [ossa] @testCallDynamic : $@convention(method) (@inout TestDynamic) -> () {
1440+
bb0(%0 : $*TestDynamic):
1441+
%access = begin_access [modify] [unknown] %0 : $*TestDynamic // expected-error {{overlapping accesses, but modification requires exclusive access; consider copying to a local variable}}
1442+
%sea = struct_element_addr %access : $*TestDynamic, #TestDynamic.x
1443+
%f = dynamic_function_ref @testDynamicLocal : $@convention(thin) (@inout Builtin.Int64, @inout_aliasable TestDynamic) -> ()
1444+
%call = apply %f(%sea, %0) : $@convention(thin) (@inout Builtin.Int64, @inout_aliasable TestDynamic) -> () // expected-note {{conflicting access is here}}
1445+
end_access %access : $*TestDynamic
1446+
%v = tuple ()
1447+
return %v : $()
1448+
}
1449+
1450+
// This callee actually accesses different subpaths, but the caller's
1451+
// diagnostic should not be able to see that.
1452+
sil private [dynamically_replacable] [ossa] @testDynamicLocal : $@convention(thin) (@inout Builtin.Int64, @inout_aliasable TestDynamic) -> () {
1453+
bb0(%0 : $*Builtin.Int64, %1 : $*TestDynamic):
1454+
%literal = integer_literal $Builtin.Int64, 1
1455+
%access1 = begin_access [modify] [unknown] %0 : $*Builtin.Int64
1456+
assign %literal to %access1 : $*Builtin.Int64
1457+
end_access %access1 : $*Builtin.Int64
1458+
%access2 = begin_access [modify] [unknown] %1 : $*TestDynamic
1459+
%sea = struct_element_addr %access2 : $*TestDynamic, #TestDynamic.y
1460+
assign %literal to %sea : $*Builtin.Int64
1461+
end_access %access2 : $*TestDynamic
1462+
%19 = tuple ()
1463+
return %19 : $()
1464+
}

0 commit comments

Comments
 (0)