From 82b08c572295129cfb4d762a6e0453d078ab0a79 Mon Sep 17 00:00:00 2001 From: Michael Gottesman Date: Mon, 4 May 2020 13:31:25 -0700 Subject: [PATCH] [ownership] Add new pass OwnershipVerifierTextualErrorDumper and use it in ownership verifier FileCheck tests instead of SILVerifier. This will make it easier for me with a few further refactors to make the ownership verifier testing mode emit per function error numbers instead of the global error number that it is emitting now. The reason why this is necessary is that today, the verification by -sil-verify-all causes the errors to be emitted. That verification is done on a per value level, rather than a per function level, so it is hard to get per function error numbers without doing unprincipled things like propagating around state saying what the current function being verified is. This pass instead will let me make the error counter be per ErrorBuilder which are created per function. One thing to be aware of is that this /will/ cause SILValue::verifyOwnership to not emit any output when the testing flag is enabled. This is to ensure I only do not get duplicate textual error messages from the SILVerifier. --- include/swift/SIL/SILFunction.h | 7 ++ .../swift/SILOptimizer/PassManager/Passes.def | 2 + lib/SIL/Verifier/SILOwnershipVerifier.cpp | 89 ++++++++++++++----- lib/SILOptimizer/UtilityPasses/CMakeLists.txt | 1 + .../OwnershipVerifierTextualErrorDumper.cpp | 49 ++++++++++ test/SIL/ownership-verifier/arguments.sil | 2 +- .../borrow_scope_introducing_operands.sil | 2 +- .../disable_verifier_using_semantic_tag.sil | 2 +- .../ownership-verifier/interior_pointer.sil | 2 +- test/SIL/ownership-verifier/leaks.sil | 2 +- test/SIL/ownership-verifier/over_consume.sil | 2 +- .../subobject_borrowing.sil | 2 +- 12 files changed, 135 insertions(+), 27 deletions(-) create mode 100644 lib/SILOptimizer/UtilityPasses/OwnershipVerifierTextualErrorDumper.cpp diff --git a/include/swift/SIL/SILFunction.h b/include/swift/SIL/SILFunction.h index 6213188af2fba..7ffe87bc892d3 100644 --- a/include/swift/SIL/SILFunction.h +++ b/include/swift/SIL/SILFunction.h @@ -1052,6 +1052,13 @@ class SILFunction /// invariants. void verify(bool SingleFunction = true) const; + /// Run the SIL ownership verifier to check for ownership invariant failures. + /// + /// NOTE: The ownership verifier is always run when performing normal IR + /// verification, so this verification can be viewed as a subset of + /// SILFunction::verify. + void verifyOwnership(DeadEndBlocks *deadEndBlocks = nullptr) const; + /// Verify that all non-cond-br critical edges have been split. /// /// This is a fast subset of the checks performed in the SILVerifier. diff --git a/include/swift/SILOptimizer/PassManager/Passes.def b/include/swift/SILOptimizer/PassManager/Passes.def index 55f12b2d7d8d5..eb13c359e8062 100644 --- a/include/swift/SILOptimizer/PassManager/Passes.def +++ b/include/swift/SILOptimizer/PassManager/Passes.def @@ -314,6 +314,8 @@ PASS(UsePrespecialized, "use-prespecialized", "Use Pre-Specialized Functions") PASS(OwnershipDumper, "ownership-dumper", "Print Ownership information for Testing") +PASS(OwnershipVerifierTextualErrorDumper, "ownership-verifier-textual-error-dumper", + "Run ownership verification on all functions, emitting FileCheck-able textual errors instead of asserting") PASS(SemanticARCOpts, "semantic-arc-opts", "Semantic ARC Optimization") PASS(SimplifyUnreachableContainingBlocks, "simplify-unreachable-containing-blocks", diff --git a/lib/SIL/Verifier/SILOwnershipVerifier.cpp b/lib/SIL/Verifier/SILOwnershipVerifier.cpp index bbce868ac244b..817381749f2dc 100644 --- a/lib/SIL/Verifier/SILOwnershipVerifier.cpp +++ b/lib/SIL/Verifier/SILOwnershipVerifier.cpp @@ -882,12 +882,41 @@ void SILInstruction::verifyOperandOwnership() const { } } +static void verifySILValueHelper(const SILFunction *f, SILValue value, + DeadEndBlocks *deadEndBlocks) { + assert(!isa(value) && + "We assume we are always passed arguments or instruction results"); + + // If the given function has unqualified ownership or we have been asked by + // the user not to verify this function, there is nothing to verify. + if (!f->hasOwnership() || !f->shouldVerifyOwnership()) + return; + + using BehaviorKind = LinearLifetimeChecker::ErrorBehaviorKind; + Optional errorBuilder; + if (IsSILOwnershipVerifierTestingEnabled) { + errorBuilder.emplace(*f, BehaviorKind::PrintMessageAndReturnFalse); + } else { + errorBuilder.emplace(*f, BehaviorKind::PrintMessageAndAssert); + } + + SmallPtrSet liveBlocks; + if (deadEndBlocks) { + SILValueOwnershipChecker(*deadEndBlocks, value, *errorBuilder, liveBlocks) + .check(); + } else { + DeadEndBlocks deadEndBlocks(f); + SILValueOwnershipChecker(deadEndBlocks, value, *errorBuilder, liveBlocks) + .check(); + } +} + void SILValue::verifyOwnership(DeadEndBlocks *deadEndBlocks) const { if (DisableOwnershipVerification) return; // Do not validate SILUndef values. - if (isa(Value)) + if (isa(*this)) return; #ifdef NDEBUG @@ -898,8 +927,8 @@ void SILValue::verifyOwnership(DeadEndBlocks *deadEndBlocks) const { // that if we run into something that we do not understand, we do not assert // in user code even tohugh we aren't going to actually verify (the default // behavior when -sil-verify-all is disabled). - auto *Mod = Value->getModule(); - if (!Mod || !Mod->getOptions().VerifyAll) + auto *mod = Value->getModule(); + if (!mod || !mod->getOptions().VerifyAll) return; #endif @@ -911,31 +940,51 @@ void SILValue::verifyOwnership(DeadEndBlocks *deadEndBlocks) const { } } + // If we are testing the verifier, bail so we only print errors once when + // performing a full verification a function at a time by the + // OwnershipVerifierStateDumper pass, instead of additionally in the + // SILBuilder and in the actual SIL verifier that may be run by sil-opt. + if (IsSILOwnershipVerifierTestingEnabled) + return; + // Since we do not have SILUndef, we now know that getFunction() should return // a real function. Assert in case this assumption is no longer true. - SILFunction *f = (*this)->getFunction(); + auto *f = (*this)->getFunction(); assert(f && "Instructions and arguments should have a function"); + verifySILValueHelper(f, *this, deadEndBlocks); +} + +void SILFunction::verifyOwnership(DeadEndBlocks *deadEndBlocks) const { + if (DisableOwnershipVerification) + return; + +#ifdef NDEBUG + // When compiling without asserts enabled, only verify ownership if + // -sil-verify-all is set. + // + // NOTE: We purposely return if we do can not look up a module here to ensure + // that if we run into something that we do not understand, we do not assert + // in user code even tohugh we aren't going to actually verify (the default + // behavior when -sil-verify-all is disabled). + auto *mod = getParent(); + if (!mod || !mod->getOptions().VerifyAll) + return; +#endif // If the given function has unqualified ownership or we have been asked by // the user not to verify this function, there is nothing to verify. - if (!f->hasOwnership() || !f->shouldVerifyOwnership()) + if (!hasOwnership() || !shouldVerifyOwnership()) return; - using BehaviorKind = LinearLifetimeChecker::ErrorBehaviorKind; - Optional errorBuilder; - if (IsSILOwnershipVerifierTestingEnabled) { - errorBuilder.emplace(*f, BehaviorKind::PrintMessageAndReturnFalse); - } else { - errorBuilder.emplace(*f, BehaviorKind::PrintMessageAndAssert); - } + for (auto &block : *this) { + for (auto *arg : block.getArguments()) { + verifySILValueHelper(this, arg, deadEndBlocks); + } - SmallPtrSet liveBlocks; - if (deadEndBlocks) { - SILValueOwnershipChecker(*deadEndBlocks, *this, *errorBuilder, liveBlocks) - .check(); - } else { - DeadEndBlocks deadEndBlocks(f); - SILValueOwnershipChecker(deadEndBlocks, *this, *errorBuilder, liveBlocks) - .check(); + for (auto &inst : block) { + for (auto result : inst.getResults()) { + verifySILValueHelper(this, result, deadEndBlocks); + } + } } } diff --git a/lib/SILOptimizer/UtilityPasses/CMakeLists.txt b/lib/SILOptimizer/UtilityPasses/CMakeLists.txt index b7472977050d7..24f268320b8da 100644 --- a/lib/SILOptimizer/UtilityPasses/CMakeLists.txt +++ b/lib/SILOptimizer/UtilityPasses/CMakeLists.txt @@ -31,4 +31,5 @@ silopt_register_sources( SimplifyUnreachableContainingBlocks.cpp StripDebugInfo.cpp OwnershipDumper.cpp + OwnershipVerifierTextualErrorDumper.cpp ) diff --git a/lib/SILOptimizer/UtilityPasses/OwnershipVerifierTextualErrorDumper.cpp b/lib/SILOptimizer/UtilityPasses/OwnershipVerifierTextualErrorDumper.cpp new file mode 100644 index 0000000000000..167b8e584cc96 --- /dev/null +++ b/lib/SILOptimizer/UtilityPasses/OwnershipVerifierTextualErrorDumper.cpp @@ -0,0 +1,49 @@ +//===--- OwnershipVerifierStateDumper.cpp ---------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors +// Licensed under Apache License v2.0 with Runtime Library Exception +// +// See https://swift.org/LICENSE.txt for license information +// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors +// +//===----------------------------------------------------------------------===// +/// +/// \file +/// +/// This is a simple utility pass that verifies the ownership of all SILValue in +/// a module with a special flag set that causes the verifier to emit textual +/// errors instead of asserting. This is done one function at a time so that we +/// can number the errors as we emit them so in FileCheck tests, we can be 100% +/// sure we exactly matched the number of errors. +/// +//===----------------------------------------------------------------------===// + +#include "swift/SIL/BasicBlockUtils.h" +#include "swift/SIL/SILFunction.h" +#include "swift/SIL/SILInstruction.h" +#include "swift/SILOptimizer/PassManager/Passes.h" +#include "swift/SILOptimizer/PassManager/Transforms.h" + +using namespace swift; + +//===----------------------------------------------------------------------===// +// Top Level Entrypoint +//===----------------------------------------------------------------------===// + +namespace { + +class OwnershipVerifierTextualErrorDumper : public SILFunctionTransform { + void run() override { + SILFunction *f = getFunction(); + DeadEndBlocks deadEndBlocks(f); + f->verifyOwnership(&deadEndBlocks); + } +}; + +} // end anonymous namespace + +SILTransform *swift::createOwnershipVerifierTextualErrorDumper() { + return new OwnershipVerifierTextualErrorDumper(); +} diff --git a/test/SIL/ownership-verifier/arguments.sil b/test/SIL/ownership-verifier/arguments.sil index 0978148817bc1..0d2c02327791a 100644 --- a/test/SIL/ownership-verifier/arguments.sil +++ b/test/SIL/ownership-verifier/arguments.sil @@ -1,4 +1,4 @@ -// RUN: %target-sil-opt -module-name Swift -sil-ownership-verifier-enable-testing -enable-sil-verify-all=0 %s -o /dev/null 2>&1 | %FileCheck %s +// RUN: %target-sil-opt -module-name Swift -sil-ownership-verifier-enable-testing -ownership-verifier-textual-error-dumper -enable-sil-verify-all=0 %s -o /dev/null 2>&1 | %FileCheck %s // REQUIRES: asserts // This is a test that verifies ownership behavior around arguments that should diff --git a/test/SIL/ownership-verifier/borrow_scope_introducing_operands.sil b/test/SIL/ownership-verifier/borrow_scope_introducing_operands.sil index 5f738b0adc60c..2aa91496230c3 100644 --- a/test/SIL/ownership-verifier/borrow_scope_introducing_operands.sil +++ b/test/SIL/ownership-verifier/borrow_scope_introducing_operands.sil @@ -1,4 +1,4 @@ -// RUN: %target-sil-opt -sil-ownership-verifier-enable-testing -enable-sil-verify-all=0 -o /dev/null %s 2>&1 | %FileCheck %s +// RUN: %target-sil-opt -sil-ownership-verifier-enable-testing -ownership-verifier-textual-error-dumper -enable-sil-verify-all=0 -o /dev/null %s 2>&1 | %FileCheck %s // REQUIRES: asserts // This file tests that when we emit an error, it is a true negative. This is diff --git a/test/SIL/ownership-verifier/disable_verifier_using_semantic_tag.sil b/test/SIL/ownership-verifier/disable_verifier_using_semantic_tag.sil index 6bc788272470a..b7ba3d3564f60 100644 --- a/test/SIL/ownership-verifier/disable_verifier_using_semantic_tag.sil +++ b/test/SIL/ownership-verifier/disable_verifier_using_semantic_tag.sil @@ -1,4 +1,4 @@ -// RUN: %target-sil-opt -sil-ownership-verifier-enable-testing -enable-sil-verify-all=0 %s -o /dev/null 2>&1 | %FileCheck %s +// RUN: %target-sil-opt -sil-ownership-verifier-enable-testing -ownership-verifier-textual-error-dumper -enable-sil-verify-all=0 %s -o /dev/null 2>&1 | %FileCheck %s // REQUIRES: asserts // This test makes sure that the ownership verifier can be disabled on specific diff --git a/test/SIL/ownership-verifier/interior_pointer.sil b/test/SIL/ownership-verifier/interior_pointer.sil index b82eb54e782f3..4ce8fd58249f5 100644 --- a/test/SIL/ownership-verifier/interior_pointer.sil +++ b/test/SIL/ownership-verifier/interior_pointer.sil @@ -1,4 +1,4 @@ -// RUN: %target-sil-opt -sil-ownership-verifier-enable-testing -enable-sil-verify-all=0 %s -o /dev/null 2>&1 | %FileCheck %s +// RUN: %target-sil-opt -sil-ownership-verifier-enable-testing -ownership-verifier-textual-error-dumper -enable-sil-verify-all=0 %s -o /dev/null 2>&1 | %FileCheck %s // REQUIRES: asserts sil_stage canonical diff --git a/test/SIL/ownership-verifier/leaks.sil b/test/SIL/ownership-verifier/leaks.sil index a3d45849f5c91..e680576e80e19 100644 --- a/test/SIL/ownership-verifier/leaks.sil +++ b/test/SIL/ownership-verifier/leaks.sil @@ -1,4 +1,4 @@ -// RUN: %target-sil-opt -module-name Swift -sil-ownership-verifier-enable-testing -enable-sil-verify-all=0 %s -o /dev/null 2>&1 | %FileCheck %s +// RUN: %target-sil-opt -module-name Swift -sil-ownership-verifier-enable-testing -ownership-verifier-textual-error-dumper -enable-sil-verify-all=0 %s -o /dev/null 2>&1 | %FileCheck %s // REQUIRES: asserts // This file is meant to contain dataflow tests that are true leaks. It is diff --git a/test/SIL/ownership-verifier/over_consume.sil b/test/SIL/ownership-verifier/over_consume.sil index 556800a619eca..d59ade297caf3 100644 --- a/test/SIL/ownership-verifier/over_consume.sil +++ b/test/SIL/ownership-verifier/over_consume.sil @@ -1,4 +1,4 @@ -// RUN: %target-sil-opt -sil-ownership-verifier-enable-testing -enable-sil-verify-all=0 -o /dev/null 2>&1 %s | %FileCheck %s +// RUN: %target-sil-opt -sil-ownership-verifier-enable-testing -ownership-verifier-textual-error-dumper -enable-sil-verify-all=0 -o /dev/null %s 2>&1 | %FileCheck %s // REQUIRES: asserts sil_stage canonical diff --git a/test/SIL/ownership-verifier/subobject_borrowing.sil b/test/SIL/ownership-verifier/subobject_borrowing.sil index 29dae9141bc95..040e7230a0fd7 100644 --- a/test/SIL/ownership-verifier/subobject_borrowing.sil +++ b/test/SIL/ownership-verifier/subobject_borrowing.sil @@ -1,4 +1,4 @@ -// RUN: %target-sil-opt -sil-ownership-verifier-enable-testing -enable-sil-verify-all=0 -o /dev/null 2>&1 %s | %FileCheck %s +// RUN: %target-sil-opt -sil-ownership-verifier-enable-testing -ownership-verifier-textual-error-dumper -enable-sil-verify-all=0 -o /dev/null %s 2>&1 | %FileCheck %s // REQUIRES: asserts sil_stage canonical