Skip to content

Commit 2365176

Browse files
committed
[move-only] Add an error that is emitted if the move checker misses a copy and asks to file a bug.
1 parent 5f38f8e commit 2365176

File tree

5 files changed

+153
-86
lines changed

5 files changed

+153
-86
lines changed

include/swift/AST/DiagnosticsSIL.def

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -792,6 +792,8 @@ ERROR(sil_moveonlychecker_not_understand_consumable_and_assignable, none,
792792
ERROR(sil_moveonlychecker_not_understand_moveonly, none,
793793
"Usage of a move only type that the move checker does not know how to "
794794
"check!", ())
795+
ERROR(sil_moveonlychecker_missed_copy, none,
796+
"copy of noncopyable typed value. This is a compiler bug. Please file a bug with a small code example that triggers this behavior", ())
795797

796798
// move kills copyable values checker diagnostics
797799
ERROR(sil_movekillscopyablevalue_value_consumed_more_than_once, none,

lib/SILOptimizer/Mandatory/MoveOnlyAddressChecker.cpp

Lines changed: 107 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,62 @@ static bool isInOutDefThatNeedsEndOfFunctionLiveness(SILValue value) {
363363
return false;
364364
}
365365

366+
//===----------------------------------------------------------------------===//
367+
// MARK: Cleanup After Emitting Diagnostic
368+
//===----------------------------------------------------------------------===//
369+
370+
static bool cleanupAfterEmittingDiagnostic(SILFunction *fn) {
371+
bool changed = false;
372+
for (auto &block : *fn) {
373+
for (auto ii = block.begin(), ie = block.end(); ii != ie;) {
374+
auto *inst = &*ii;
375+
++ii;
376+
377+
// Convert load [copy] -> load_borrow + explicit_copy_value.
378+
if (auto *li = dyn_cast<LoadInst>(inst)) {
379+
if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Copy) {
380+
SILBuilderWithScope builder(li);
381+
auto *lbi = builder.createLoadBorrow(li->getLoc(), li->getOperand());
382+
auto *cvi = builder.createExplicitCopyValue(li->getLoc(), lbi);
383+
builder.createEndBorrow(li->getLoc(), lbi);
384+
li->replaceAllUsesWith(cvi);
385+
li->eraseFromParent();
386+
changed = true;
387+
}
388+
}
389+
390+
// Convert copy_addr !take of src to its explicit value form so we don't
391+
// error.
392+
if (auto *copyAddr = dyn_cast<CopyAddrInst>(inst)) {
393+
if (!copyAddr->isTakeOfSrc()) {
394+
SILBuilderWithScope builder(copyAddr);
395+
builder.createExplicitCopyAddr(
396+
copyAddr->getLoc(), copyAddr->getSrc(), copyAddr->getDest(),
397+
IsTake_t(copyAddr->isTakeOfSrc()),
398+
IsInitialization_t(copyAddr->isInitializationOfDest()));
399+
copyAddr->eraseFromParent();
400+
changed = true;
401+
}
402+
}
403+
404+
// Convert any copy_value of move_only type to explicit copy value.
405+
if (auto *cvi = dyn_cast<CopyValueInst>(inst)) {
406+
if (!cvi->getOperand()->getType().isMoveOnly())
407+
continue;
408+
SILBuilderWithScope b(cvi);
409+
auto *expCopy =
410+
b.createExplicitCopyValue(cvi->getLoc(), cvi->getOperand());
411+
cvi->replaceAllUsesWith(expCopy);
412+
cvi->eraseFromParent();
413+
changed = true;
414+
continue;
415+
}
416+
}
417+
}
418+
419+
return changed;
420+
}
421+
366422
//===----------------------------------------------------------------------===//
367423
// MARK: Use State
368424
//===----------------------------------------------------------------------===//
@@ -1584,52 +1640,7 @@ bool GlobalLivenessChecker::compute() {
15841640
//===----------------------------------------------------------------------===//
15851641

15861642
void MoveOnlyChecker::cleanupAfterEmittingDiagnostic() {
1587-
for (auto &block : *fn) {
1588-
for (auto ii = block.begin(), ie = block.end(); ii != ie;) {
1589-
auto *inst = &*ii;
1590-
++ii;
1591-
1592-
// Convert load [copy] -> load_borrow + explicit_copy_value.
1593-
if (auto *li = dyn_cast<LoadInst>(inst)) {
1594-
if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Copy) {
1595-
SILBuilderWithScope builder(li);
1596-
auto *lbi = builder.createLoadBorrow(li->getLoc(), li->getOperand());
1597-
auto *cvi = builder.createExplicitCopyValue(li->getLoc(), lbi);
1598-
builder.createEndBorrow(li->getLoc(), lbi);
1599-
li->replaceAllUsesWith(cvi);
1600-
li->eraseFromParent();
1601-
changed = true;
1602-
}
1603-
}
1604-
1605-
// Convert copy_addr !take of src to its explicit value form so we don't
1606-
// error.
1607-
if (auto *copyAddr = dyn_cast<CopyAddrInst>(inst)) {
1608-
if (!copyAddr->isTakeOfSrc()) {
1609-
SILBuilderWithScope builder(copyAddr);
1610-
builder.createExplicitCopyAddr(
1611-
copyAddr->getLoc(), copyAddr->getSrc(), copyAddr->getDest(),
1612-
IsTake_t(copyAddr->isTakeOfSrc()),
1613-
IsInitialization_t(copyAddr->isInitializationOfDest()));
1614-
copyAddr->eraseFromParent();
1615-
changed = true;
1616-
}
1617-
}
1618-
1619-
// Convert any copy_value of move_only type to explicit copy value.
1620-
if (auto *cvi = dyn_cast<CopyValueInst>(inst)) {
1621-
if (!cvi->getOperand()->getType().isMoveOnly())
1622-
continue;
1623-
SILBuilderWithScope b(cvi);
1624-
auto *expCopy =
1625-
b.createExplicitCopyValue(cvi->getLoc(), cvi->getOperand());
1626-
cvi->replaceAllUsesWith(expCopy);
1627-
cvi->eraseFromParent();
1628-
changed = true;
1629-
continue;
1630-
}
1631-
}
1632-
}
1643+
changed |= ::cleanupAfterEmittingDiagnostic(fn);
16331644
}
16341645

16351646
bool MoveOnlyChecker::searchForCandidateMarkMustChecks() {
@@ -2031,6 +2042,48 @@ bool MoveOnlyChecker::checkFunction() {
20312042
return changed;
20322043
}
20332044

2045+
//===----------------------------------------------------------------------===//
2046+
// MARK: Missed Copy Diagnostic
2047+
//===----------------------------------------------------------------------===//
2048+
2049+
/// A small diagnostic helper that causes us to emit a diagnostic error upon any
2050+
/// copies we did not eliminate and ask the user for a test case.
2051+
static bool checkForMissedCopies(SILFunction *fn) {
2052+
bool emittedDiagnostic = false;
2053+
DiagnosticEmitter diagnosticEmitter;
2054+
for (auto &block : *fn) {
2055+
for (auto &inst : block) {
2056+
if (auto *cvi = dyn_cast<CopyValueInst>(&inst)) {
2057+
if (cvi->getOperand()->getType().isMoveOnly()) {
2058+
diagnosticEmitter.emitCheckedMissedCopyError(cvi);
2059+
emittedDiagnostic = true;
2060+
}
2061+
continue;
2062+
}
2063+
2064+
if (auto *li = dyn_cast<LoadInst>(&inst)) {
2065+
if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Copy &&
2066+
li->getType().isMoveOnly()) {
2067+
diagnosticEmitter.emitCheckedMissedCopyError(li);
2068+
emittedDiagnostic = true;
2069+
}
2070+
continue;
2071+
}
2072+
2073+
if (auto *copyAddr = dyn_cast<CopyAddrInst>(&inst)) {
2074+
if (!copyAddr->isTakeOfSrc() &&
2075+
copyAddr->getSrc()->getType().isMoveOnly()) {
2076+
diagnosticEmitter.emitCheckedMissedCopyError(copyAddr);
2077+
emittedDiagnostic = true;
2078+
}
2079+
continue;
2080+
}
2081+
}
2082+
}
2083+
2084+
return emittedDiagnostic;
2085+
}
2086+
20342087
//===----------------------------------------------------------------------===//
20352088
// Top Level Entrypoint
20362089
//===----------------------------------------------------------------------===//
@@ -2058,47 +2111,15 @@ class MoveOnlyCheckerPass : public SILFunctionTransform {
20582111
auto *deAnalysis = getAnalysis<DeadEndBlocksAnalysis>()->get(fn);
20592112
auto *poa = getAnalysis<PostOrderAnalysis>();
20602113

2061-
if (MoveOnlyChecker(getFunction(), deAnalysis, domTree, poa)
2062-
.checkFunction()) {
2063-
invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
2114+
bool shouldInvalidate =
2115+
MoveOnlyChecker(getFunction(), deAnalysis, domTree, poa)
2116+
.checkFunction();
2117+
if (checkForMissedCopies(getFunction())) {
2118+
cleanupAfterEmittingDiagnostic(getFunction());
2119+
shouldInvalidate = true;
20642120
}
2065-
2066-
if (!getOptions().VerifyAll)
2067-
return;
2068-
2069-
// TODO: Enable this by default when verify all is disabled and make it a
2070-
// diagnostic error saying file a bug.
2071-
for (auto &block : *getFunction()) {
2072-
for (auto &inst : block) {
2073-
if (auto *cvi = dyn_cast<CopyValueInst>(&inst)) {
2074-
if (cvi->getOperand()->getType().isMoveOnly()) {
2075-
llvm::errs() << "Should have eliminated copy at this point: "
2076-
<< *cvi;
2077-
llvm::report_fatal_error("standard compiler error");
2078-
}
2079-
continue;
2080-
}
2081-
2082-
if (auto *li = dyn_cast<LoadInst>(&inst)) {
2083-
if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Copy &&
2084-
li->getType().isMoveOnly()) {
2085-
llvm::errs() << "Should have eliminated copy at this point: "
2086-
<< *li;
2087-
llvm::report_fatal_error("standard compiler error");
2088-
}
2089-
continue;
2090-
}
2091-
2092-
if (auto *copyAddr = dyn_cast<CopyAddrInst>(&inst)) {
2093-
if (!copyAddr->isTakeOfSrc() &&
2094-
copyAddr->getSrc()->getType().isMoveOnly()) {
2095-
llvm::errs() << "Should have eliminated copy at this point: "
2096-
<< *copyAddr;
2097-
llvm::report_fatal_error("standard compiler error");
2098-
}
2099-
continue;
2100-
}
2101-
}
2121+
if (shouldInvalidate) {
2122+
invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
21022123
}
21032124
}
21042125
};

lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,11 @@ void DiagnosticEmitter::emitCheckerDoesntUnderstandDiagnostic(
168168
emittedCheckerDoesntUnderstandDiagnostic = true;
169169
}
170170

171+
void DiagnosticEmitter::emitCheckedMissedCopyError(SILInstruction *copyInst) {
172+
diagnose(copyInst->getFunction()->getASTContext(), copyInst,
173+
diag::sil_moveonlychecker_missed_copy);
174+
}
175+
171176
//===----------------------------------------------------------------------===//
172177
// MARK: Object Diagnostics
173178
//===----------------------------------------------------------------------===//

lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,10 @@ class DiagnosticEmitter {
7171
return emittedCheckerDoesntUnderstandDiagnostic;
7272
}
7373

74+
/// Used at the end of the MoveOnlyAddressChecker to tell the user in a nice
75+
/// way to file a bug.
76+
void emitCheckedMissedCopyError(SILInstruction *copyInst);
77+
7478
void emitCheckerDoesntUnderstandDiagnostic(MarkMustCheckInst *markedValue);
7579
void emitObjectGuaranteedDiagnostic(MarkMustCheckInst *markedValue);
7680
void emitObjectOwnedDiagnostic(MarkMustCheckInst *markedValue);

test/SILOptimizer/moveonly_addresschecker_diagnostics.sil

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
// RUN: %target-sil-opt -sil-move-only-address-checker -enable-experimental-move-only -enable-experimental-feature MoveOnlyClasses -enable-sil-verify-all %s -verify
2+
// RUN: %target-sil-opt -sil-move-only-address-checker -enable-experimental-move-only -enable-experimental-feature MoveOnlyClasses -enable-sil-verify-all -move-only-diagnostics-silently-emit-diagnostics %s | %FileCheck %s
23

34
// TODO: Add FileCheck
45

@@ -58,11 +59,21 @@ public struct AggStruct {
5859
var pair: KlassPair
5960
}
6061

62+
@_moveOnly
63+
public enum EnumTy {
64+
case klass(NonTrivialStruct)
65+
case int(Builtin.Int32)
66+
67+
func doSomething() -> Bool { true }
68+
}
69+
6170
sil @get_aggstruct : $@convention(thin) () -> @owned AggStruct
6271
sil @nonConsumingUseKlass : $@convention(thin) (@guaranteed Klass) -> ()
6372
sil @classConsume : $@convention(thin) (@owned Klass) -> () // user: %18
6473
sil @copyableClassConsume : $@convention(thin) (@owned CopyableKlass) -> () // user: %24
6574
sil @copyableClassUseMoveOnlyWithoutEscaping : $@convention(thin) (@guaranteed CopyableKlass) -> () // user: %16
75+
sil @enumty_use : $@convention(thin) (@guaranteed EnumTy) -> ()
76+
sil @nontrivialstruct_use : $@convention(thin) (@guaranteed NonTrivialStruct) -> ()
6677

6778
///////////
6879
// Tests //
@@ -202,3 +213,27 @@ bb0(%arg : @owned $NonTrivialStruct, %arg1 : @owned $NonTrivialStruct):
202213
%33 = tuple ()
203214
return %33 : $()
204215
}
216+
217+
///////////////////////////////////////////////////////////////////////
218+
// MARK: Tests that make sure we emit a nice error for missed copies //
219+
///////////////////////////////////////////////////////////////////////
220+
221+
// CHECK-LABEL: sil [ossa] @missed_copy_diagnostic_test : $@convention(thin) (@guaranteed NonTrivialStruct, @in_guaranteed NonTrivialStruct) -> () {
222+
// CHECK: explicit_copy_value
223+
// CHECK: explicit_copy_value
224+
// CHECK: explicit_copy_addr
225+
// CHECK: } // end sil function 'missed_copy_diagnostic_test'
226+
sil [ossa] @missed_copy_diagnostic_test : $@convention(thin) (@guaranteed NonTrivialStruct, @in_guaranteed NonTrivialStruct) -> () {
227+
bb0(%arg0 : @guaranteed $NonTrivialStruct, %arg1 : $*NonTrivialStruct):
228+
%1 = copy_value %arg0 : $NonTrivialStruct // expected-error {{copy of noncopyable typed value. This is a compiler bug. Please file a bug with a small code example that triggers this behavior}}
229+
destroy_value %1 : $NonTrivialStruct
230+
%2 = load [copy] %arg1 : $*NonTrivialStruct // expected-error {{copy of noncopyable typed value. This is a compiler bug. Please file a bug with a small code example that triggers this behavior}}
231+
destroy_value %2 : $NonTrivialStruct
232+
%3 = alloc_stack $NonTrivialStruct
233+
copy_addr %arg1 to [init] %3 : $*NonTrivialStruct // expected-error {{copy of noncopyable typed value. This is a compiler bug. Please file a bug with a small code example that triggers this behavior}}
234+
destroy_addr %3 : $*NonTrivialStruct
235+
dealloc_stack %3 : $*NonTrivialStruct
236+
%9999 = tuple ()
237+
return %9999 : $()
238+
}
239+

0 commit comments

Comments
 (0)