Skip to content

Improve -enable-verify-exclusivity. #16169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 26, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions include/swift/SIL/MemAccessUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,11 @@ AccessedStorage findAccessedStorage(SILValue sourceAddr);
/// storage.
AccessedStorage findAccessedStorageOrigin(SILValue sourceAddr);

/// Return true if the given address operand is used by a memory operation that
/// initializes the memory at that address, implying that the previous value is
/// uninitialized.
bool memInstMustInitialize(Operand *memOper);

/// Return true if the given address producer may be the source of a formal
/// access (a read or write of a potentially aliased, user visible variable).
///
Expand Down
83 changes: 61 additions & 22 deletions lib/SIL/MemAccessUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#define DEBUG_TYPE "sil-access-utils"

#include "swift/SIL/MemAccessUtils.h"
#include "swift/SIL/SILUndef.h"

using namespace swift;

Expand Down Expand Up @@ -115,6 +116,30 @@ static void checkSwitchEnumBlockArg(SILPHIArgument *arg) {
}
}

/// Return true if the given address value is produced by a special address
/// producer that is only used for local initialization, not formal access.
static bool isAddressForLocalInitOnly(SILValue sourceAddr) {
switch (sourceAddr->getKind()) {
default:
return false;

// Value to address conversions: the operand is the non-address source
// value. These allow local mutation of the value but should never be used
// for formal access of an lvalue.
case ValueKind::OpenExistentialBoxInst:
case ValueKind::ProjectExistentialBoxInst:
return true;

// Self-evident local initialization.
case ValueKind::InitEnumDataAddrInst:
case ValueKind::InitExistentialAddrInst:
case ValueKind::AllocExistentialBoxInst:
case ValueKind::AllocValueBufferInst:
case ValueKind::ProjectValueBufferInst:
return true;
}
}

AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
SILValue address = sourceAddr;
while (true) {
Expand All @@ -127,9 +152,12 @@ AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
// Handle other unidentified address sources.
switch (address->getKind()) {
default:
if (isAddressForLocalInitOnly(address))
return AccessedStorage(address, AccessedStorage::Unidentified);
return AccessedStorage();

case ValueKind::PointerToAddressInst:
case ValueKind::SILUndef:
return AccessedStorage(address, AccessedStorage::Unidentified);

// A block argument may be a box value projected out of
Expand Down Expand Up @@ -181,22 +209,6 @@ AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
case ValueKind::IndexAddrInst:
address = cast<SingleValueInstruction>(address)->getOperand(0);
continue;

// Value to address conversions: the operand is the non-address source
// value. These allow local mutation of the value but should never be used
// for formal access of an lvalue.
case ValueKind::OpenExistentialBoxInst:
case ValueKind::ProjectExistentialBoxInst:
case ValueKind::ProjectValueBufferInst:
return AccessedStorage(address, AccessedStorage::Unidentified);

// Local initialization: these cases are skipped.
case ValueKind::InitEnumDataAddrInst:
case ValueKind::InitExistentialAddrInst:
case ValueKind::AllocExistentialBoxInst:
case ValueKind::AllocValueBufferInst:
case ValueKind::SILUndef:
return AccessedStorage(address, AccessedStorage::Unidentified);
}
}
}
Expand Down Expand Up @@ -231,6 +243,35 @@ static bool isScratchBuffer(SILValue value) {
value->getType().getSwiftRValueType());
}

bool swift::memInstMustInitialize(Operand *memOper) {
SILValue address = memOper->get();
SILInstruction *memInst = memOper->getUser();

switch (memInst->getKind()) {
default:
return false;

case SILInstructionKind::CopyAddrInst: {
auto *CAI = cast<CopyAddrInst>(memInst);
return CAI->getDest() == address && CAI->isInitializationOfDest();
}
case SILInstructionKind::InitExistentialAddrInst:
case SILInstructionKind::InitEnumDataAddrInst:
case SILInstructionKind::InjectEnumAddrInst:
return true;

case SILInstructionKind::StoreInst:
return cast<StoreInst>(memInst)->getOwnershipQualifier()
== StoreOwnershipQualifier::Init;

case SILInstructionKind::StoreWeakInst:
return cast<StoreWeakInst>(memInst)->isInitializationOfDest();

case SILInstructionKind::StoreUnownedInst:
return cast<StoreUnownedInst>(memInst)->isInitializationOfDest();
}
}

bool swift::isPossibleFormalAccessBase(const AccessedStorage &storage,
SILFunction *F) {
switch (storage.getKind()) {
Expand Down Expand Up @@ -259,6 +300,9 @@ bool swift::isPossibleFormalAccessBase(const AccessedStorage &storage,
return isPossibleFormalAccessBase(nestedStorage, F);
}
case AccessedStorage::Unidentified:
if (isAddressForLocalInitOnly(storage.getValue()))
return false;

if (isa<SILPHIArgument>(storage.getValue())) {
checkSwitchEnumBlockArg(cast<SILPHIArgument>(storage.getValue()));
return false;
Expand All @@ -268,12 +312,7 @@ bool swift::isPossibleFormalAccessBase(const AccessedStorage &storage,
if (isa<PointerToAddressInst>(storage.getValue()))
return false;

// Existential initialization is just initialization.
if (isa<InitExistentialAddrInst>(storage.getValue()))
return false;

// Enum initialization is just initialization.
if (isa<InitEnumDataAddrInst>(storage.getValue()))
if (isa<SILUndef>(storage.getValue()))
return false;

if (isScratchBuffer(storage.getValue()))
Expand Down
10 changes: 8 additions & 2 deletions lib/SILOptimizer/Mandatory/DiagnoseStaticExclusivity.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -889,6 +889,11 @@ static void checkAccessedAddress(Operand *memOper, StorageMap &Accesses) {
abort();
};

// If the memory instruction is only used for initialization, it doesn't need
// an access marker.
if (memInstMustInitialize(memOper))
return;

if (auto apply = ApplySite::isa(memInst)) {
SILArgumentConvention conv =
apply.getArgumentConvention(apply.getCalleeArgIndex(*memOper));
Expand Down Expand Up @@ -922,8 +927,9 @@ static void checkAccessedAddress(Operand *memOper, StorageMap &Accesses) {
if (!storage || !isPossibleFormalAccessBase(storage, memInst->getFunction()))
return;

// Skip local non-lvalue accesses. There are many local initialization
// patterns that don't require access markers.
// A box or stack variable may represent lvalues, but they can only conflict
// with call sites in the same scope. Some initialization patters (stores to
// the local value) aren't protected by markers, so we need this check.
if (!isa<ApplySite>(memInst)
&& (storage.getKind() == AccessedStorage::Box
|| storage.getKind() == AccessedStorage::Stack)) {
Expand Down
31 changes: 29 additions & 2 deletions test/SILOptimizer/access_marker_verify.swift
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@

// RUN: %target-swift-frontend -module-name access_marker_verify -enable-verify-exclusivity -enforce-exclusivity=checked -enable-sil-ownership -emit-silgen -swift-version 4 -parse-as-library %s | %FileCheck %s
// RUN: %target-swift-frontend -module-name access_marker_verify -enable-verify-exclusivity -enforce-exclusivity=checked -enable-sil-ownership -Onone -emit-sil -swift-version 4 -parse-as-library %s
// RUN: %target-swift-frontend -module-name access_marker_verify -enable-verify-exclusivity -enforce-exclusivity=checked -enable-sil-ownership -O -emit-sil -swift-version 4 -parse-as-library %s
// RUN: %target-swift-frontend -module-name access_marker_verify -enable-verify-exclusivity -enforce-exclusivity=checked -enable-sil-ownership -Onone -emit-sil -swift-version 4 -parse-as-library %s -o /dev/null
// RUN: %target-swift-frontend -module-name access_marker_verify -enable-verify-exclusivity -enforce-exclusivity=checked -enable-sil-ownership -O -emit-sil -swift-version 4 -parse-as-library %s -o /dev/null
// REQUIRES: asserts

// Test the combination of SILGen + DiagnoseStaticExclusivity with verification.
Expand Down Expand Up @@ -995,3 +995,30 @@ func testPointerInit(x: Int, y: UnsafeMutablePointer<Int>) {
// CHECK-NOT: begin_access
// CHECK: assign %0 to [[ADR]] : $*Int
// CHECK-LABEL: } // end sil function '$S20access_marker_verify15testPointerInit1x1yySi_SpySiGtF'

// Verification should ignore the address operand of init_existential_addr.
class testInitExistentialGlobal {
static var testProperty: P = StructP()
}
// CHECK-LABEL: sil private @globalinit{{.*}} : $@convention(c) () -> () {
// CHECK: alloc_global @$S20access_marker_verify25testInitExistentialGlobalC0D8PropertyAA1P_pvpZ
// CHECK: [[GADR:%.*]] = global_addr @$S20access_marker_verify25testInitExistentialGlobalC0D8PropertyAA1P_pvpZ : $*P
// CHECK: [[EADR:%.*]] = init_existential_addr [[GADR]] : $*P, $StructP
// CHECK: %{{.*}} = apply %{{.*}}({{.*}}) : $@convention(method) (@thin StructP.Type) -> StructP
// CHECK: store %{{.*}} to [trivial] [[EADR]] : $*StructP
// CHECK-LABEL: } // end sil function 'globalinit

public enum SomeError: Swift.Error {
case error
}

// Verification should ignore addresses produced by project_existential_box.
public func testInitBox() throws {
throw SomeError.error
}
// CHECK-LABEL: sil @$S20access_marker_verify11testInitBoxyyKF : $@convention(thin) () -> @error Error {
// CHECK: [[BOXALLOC:%.*]] = alloc_existential_box $Error, $SomeError
// CHECK: [[PROJ:%.*]] = project_existential_box $SomeError in [[BOXALLOC]] : $Error
// CHECK: store %{{.*}} to [trivial] [[PROJ]] : $*SomeError
// CHECK: throw [[BOXALLOC]] : $Error
// CHECK-LABEL: } // end sil function '$S20access_marker_verify11testInitBoxyyKF'