From cc16ddfd136eb932cbfdedbc9ea20527d94f65be Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Sun, 7 Oct 2018 23:54:17 -0700 Subject: [PATCH] Revert "[SILOptimizer] Don't diagnose infinite recursion if a branch terminates (#19724)" This reverts commit e94450e840009180e1e5ed90bd89112499bc0927. rdar://45080912 --- docs/ARCOptimization.rst | 4 +- include/swift/SIL/SILBasicBlock.h | 5 +- include/swift/SIL/SILInstruction.h | 3 - .../swift/SILOptimizer/Analysis/ARCAnalysis.h | 3 + .../Analysis/ProgramTerminationAnalysis.h | 2 +- include/swift/Strings.h | 4 +- lib/SIL/SILBasicBlock.cpp | 75 ------------------- lib/SIL/SILInstructions.cpp | 24 ------ lib/SILOptimizer/Analysis/ARCAnalysis.cpp | 72 ++++++++++++++++++ .../Analysis/SideEffectAnalysis.cpp | 3 +- .../Mandatory/DiagnoseInfiniteRecursion.cpp | 13 +--- lib/SILOptimizer/Transforms/ARCCodeMotion.cpp | 2 +- lib/SILOptimizer/Transforms/SimplifyCFG.cpp | 2 +- stdlib/public/core/AssertCommon.swift | 26 ++++--- test/SILOptimizer/arcsequenceopts.sil | 2 +- test/SILOptimizer/infinite_recursion.swift | 43 +---------- .../retain_release_code_motion.sil | 2 +- test/SILOptimizer/side-effect.sil | 2 +- 18 files changed, 104 insertions(+), 183 deletions(-) diff --git a/docs/ARCOptimization.rst b/docs/ARCOptimization.rst index 698f4982a33..0c32738cf21 100644 --- a/docs/ARCOptimization.rst +++ b/docs/ARCOptimization.rst @@ -373,7 +373,7 @@ Semantic Tags ARC takes advantage of certain semantic tags. This section documents these semantics and their meanings. -programtermination_point +arc.programtermination_point ---------------------------- If this semantic tag is applied to a function, then we know that: @@ -396,7 +396,7 @@ scope's lifetime may never end. This means that: 1. While we can not ignore all such unreachable terminated blocks for ARC purposes for instance, if we sink a retain past a br into a non -programtermination_point block, we must sink the retain into the block. +arc.programtermination_point block, we must sink the retain into the block. 2. If we are able to infer that an object's lifetime scope would never end due to the unreachable/no-return function, then we do not need to end the lifetime diff --git a/include/swift/SIL/SILBasicBlock.h b/include/swift/SIL/SILBasicBlock.h index 926c354e26a..383c56b9ac6 100644 --- a/include/swift/SIL/SILBasicBlock.h +++ b/include/swift/SIL/SILBasicBlock.h @@ -370,12 +370,9 @@ public: /// no-return apply or builtin. bool isNoReturn() const; - /// Returns true if this block only contains a branch instruction. + /// Returns true if this instruction only contains a branch instruction. bool isTrampoline() const; - /// Returns true if this block traps without any side effects. - bool isProgramTerminationPoint() const; - /// Returns true if it is legal to hoist instructions into this block. /// /// Used by llvm::LoopInfo. diff --git a/include/swift/SIL/SILInstruction.h b/include/swift/SIL/SILInstruction.h index c4569ecae1b..4bc66b3e931 100644 --- a/include/swift/SIL/SILInstruction.h +++ b/include/swift/SIL/SILInstruction.h @@ -6645,9 +6645,6 @@ public: /// Returns true if this terminator exits the function. bool isFunctionExiting() const; - /// Returns true if this terminator terminates the program. - bool isProgramTerminating() const; - TermKind getTermKind() const { return TermKind(getKind()); } }; diff --git a/include/swift/SILOptimizer/Analysis/ARCAnalysis.h b/include/swift/SILOptimizer/Analysis/ARCAnalysis.h index bd578581142..3473fcc2d69 100644 --- a/include/swift/SILOptimizer/Analysis/ARCAnalysis.h +++ b/include/swift/SILOptimizer/Analysis/ARCAnalysis.h @@ -424,6 +424,9 @@ public: /// FinalRelease. bool getFinalReleasesForValue(SILValue Value, ReleaseTracker &Tracker); +/// Match a call to a trap BB with no ARC relevant side effects. +bool isARCInertTrapBB(const SILBasicBlock *BB); + /// Get the two result values of the builtin "unsafeGuaranteed" instruction. /// /// Gets the (GuaranteedValue, Token) tuple from a call to "unsafeGuaranteed" diff --git a/include/swift/SILOptimizer/Analysis/ProgramTerminationAnalysis.h b/include/swift/SILOptimizer/Analysis/ProgramTerminationAnalysis.h index 914a04a0a2a..8c835973107 100644 --- a/include/swift/SILOptimizer/Analysis/ProgramTerminationAnalysis.h +++ b/include/swift/SILOptimizer/Analysis/ProgramTerminationAnalysis.h @@ -39,7 +39,7 @@ class ProgramTerminationFunctionInfo { public: ProgramTerminationFunctionInfo(const SILFunction *F) { for (const auto &BB : *F) { - if (!BB.isProgramTerminationPoint()) + if (!isARCInertTrapBB(&BB)) continue; ProgramTerminatingBlocks.insert(&BB); } diff --git a/include/swift/Strings.h b/include/swift/Strings.h index ace5f724ac1..aba6a2b519d 100644 --- a/include/swift/Strings.h +++ b/include/swift/Strings.h @@ -77,8 +77,8 @@ constexpr static const char BUILTIN_TYPE_NAME_VEC[] = "Builtin.Vec"; constexpr static const char BUILTIN_TYPE_NAME_SILTOKEN[] = "Builtin.SILToken"; /// The name of the Builtin type for Word constexpr static const char BUILTIN_TYPE_NAME_WORD[] = "Builtin.Word"; -constexpr static StringLiteral SEMANTICS_PROGRAMTERMINATION_POINT = - "programtermination_point"; +constexpr static StringLiteral SEMANTICS_ARC_PROGRAMTERMINATION_POINT = + "arc.programtermination_point"; } // end namespace swift #endif // SWIFT_STRINGS_H diff --git a/lib/SIL/SILBasicBlock.cpp b/lib/SIL/SILBasicBlock.cpp index 4858d7f7af0..d271dc37da5 100644 --- a/lib/SIL/SILBasicBlock.cpp +++ b/lib/SIL/SILBasicBlock.cpp @@ -24,7 +24,6 @@ #include "swift/SIL/SILFunction.h" #include "swift/SIL/SILInstruction.h" #include "swift/SIL/SILModule.h" -#include "swift/Strings.h" using namespace swift; //===----------------------------------------------------------------------===// @@ -337,80 +336,6 @@ bool SILBasicBlock::isEntry() const { return this == &*getParent()->begin(); } -//===----------------------------------------------------------------------===// -// Program Termination Point Analysis -//===----------------------------------------------------------------------===// - -static bool ignorableApplyInstInUnreachableBlock(const ApplyInst *AI) { - const auto *Fn = AI->getReferencedFunction(); - if (!Fn) - return false; - - return Fn->hasSemanticsAttr(SEMANTICS_PROGRAMTERMINATION_POINT); -} - -static bool ignorableBuiltinInstInUnreachableBlock(const BuiltinInst *BI) { - const BuiltinInfo &BInfo = BI->getBuiltinInfo(); - if (BInfo.ID == BuiltinValueKind::CondUnreachable) - return true; - - const IntrinsicInfo &IInfo = BI->getIntrinsicInfo(); - if (IInfo.ID == llvm::Intrinsic::trap) - return true; - - return false; -} - -// Match a call to a basic block that's going to terminate -// with no side effects. -bool SILBasicBlock::isProgramTerminationPoint() const { - // Do a quick check at the beginning to make sure that our terminator is - // actually an unreachable. This ensures that in many cases this function will - // exit early and quickly. - auto II = rbegin(); - if (!isa(*II)) - return false; - - auto IE = rend(); - while (II != IE) { - // Ignore any instructions without side effects. - if (!II->mayHaveSideEffects()) { - ++II; - continue; - } - - // Ignore cond fail. - if (isa(*II)) { - ++II; - continue; - } - - // Check for apply insts that we can ignore. - if (auto *AI = dyn_cast(&*II)) { - if (ignorableApplyInstInUnreachableBlock(AI)) { - ++II; - continue; - } - } - - // Check for builtins that we can ignore. - if (auto *BI = dyn_cast(&*II)) { - if (ignorableBuiltinInstInUnreachableBlock(BI)) { - ++II; - continue; - } - } - - // If we can't ignore the instruction, return false. - return false; - } - - // Otherwise, we have an unreachable and every instruction is inert in an - // unreachable BB. - return true; -} - - /// Declared out of line so we can have a declaration of SILArgument. PhiArgumentArrayRef SILBasicBlock::getPhiArguments() const { return PhiArgumentArrayRef(getArguments(), [](SILArgument *arg) { diff --git a/lib/SIL/SILInstructions.cpp b/lib/SIL/SILInstructions.cpp index 6ddeab84b19..09512408553 100644 --- a/lib/SIL/SILInstructions.cpp +++ b/lib/SIL/SILInstructions.cpp @@ -1061,30 +1061,6 @@ bool TermInst::isFunctionExiting() const { llvm_unreachable("Unhandled TermKind in switch."); } -bool TermInst::isProgramTerminating() const { - switch (getTermKind()) { - case TermKind::BranchInst: - case TermKind::CondBranchInst: - case TermKind::SwitchValueInst: - case TermKind::SwitchEnumInst: - case TermKind::SwitchEnumAddrInst: - case TermKind::DynamicMethodBranchInst: - case TermKind::CheckedCastBranchInst: - case TermKind::CheckedCastValueBranchInst: - case TermKind::CheckedCastAddrBranchInst: - case TermKind::TryApplyInst: - case TermKind::YieldInst: - case TermKind::ReturnInst: - case TermKind::ThrowInst: - case TermKind::UnwindInst: - return false; - case TermKind::UnreachableInst: - return true; - } - - llvm_unreachable("Unhandled TermKind in switch."); -} - TermInst::SuccessorBlockArgumentsListTy TermInst::getSuccessorBlockArguments() const { function_ref op; diff --git a/lib/SILOptimizer/Analysis/ARCAnalysis.cpp b/lib/SILOptimizer/Analysis/ARCAnalysis.cpp index 77374f7403e..09d2ffadab8 100644 --- a/lib/SILOptimizer/Analysis/ARCAnalysis.cpp +++ b/lib/SILOptimizer/Analysis/ARCAnalysis.cpp @@ -1087,6 +1087,78 @@ bool swift::getFinalReleasesForValue(SILValue V, ReleaseTracker &Tracker) { return true; } +//===----------------------------------------------------------------------===// +// Leaking BB Analysis +//===----------------------------------------------------------------------===// + +static bool ignorableApplyInstInUnreachableBlock(const ApplyInst *AI) { + const auto *Fn = AI->getReferencedFunction(); + if (!Fn) + return false; + + return Fn->hasSemanticsAttr("arc.programtermination_point"); +} + +static bool ignorableBuiltinInstInUnreachableBlock(const BuiltinInst *BI) { + const BuiltinInfo &BInfo = BI->getBuiltinInfo(); + if (BInfo.ID == BuiltinValueKind::CondUnreachable) + return true; + + const IntrinsicInfo &IInfo = BI->getIntrinsicInfo(); + if (IInfo.ID == llvm::Intrinsic::trap) + return true; + + return false; +} + +/// Match a call to a trap BB with no ARC relevant side effects. +bool swift::isARCInertTrapBB(const SILBasicBlock *BB) { + // Do a quick check at the beginning to make sure that our terminator is + // actually an unreachable. This ensures that in many cases this function will + // exit early and quickly. + auto II = BB->rbegin(); + if (!isa(*II)) + return false; + + auto IE = BB->rend(); + while (II != IE) { + // Ignore any instructions without side effects. + if (!II->mayHaveSideEffects()) { + ++II; + continue; + } + + // Ignore cond fail. + if (isa(*II)) { + ++II; + continue; + } + + // Check for apply insts that we can ignore. + if (auto *AI = dyn_cast(&*II)) { + if (ignorableApplyInstInUnreachableBlock(AI)) { + ++II; + continue; + } + } + + // Check for builtins that we can ignore. + if (auto *BI = dyn_cast(&*II)) { + if (ignorableBuiltinInstInUnreachableBlock(BI)) { + ++II; + continue; + } + } + + // If we can't ignore the instruction, return false. + return false; + } + + // Otherwise, we have an unreachable and every instruction is inert from an + // ARC perspective in an unreachable BB. + return true; +} + //===----------------------------------------------------------------------===// // Analysis of builtin "unsafeGuaranteed" instructions //===----------------------------------------------------------------------===// diff --git a/lib/SILOptimizer/Analysis/SideEffectAnalysis.cpp b/lib/SILOptimizer/Analysis/SideEffectAnalysis.cpp index c87b9e73162..cda12534c0a 100644 --- a/lib/SILOptimizer/Analysis/SideEffectAnalysis.cpp +++ b/lib/SILOptimizer/Analysis/SideEffectAnalysis.cpp @@ -17,7 +17,6 @@ #include "swift/SILOptimizer/Analysis/BasicCalleeAnalysis.h" #include "swift/SILOptimizer/Analysis/FunctionOrder.h" #include "swift/SILOptimizer/PassManager/PassManager.h" -#include "swift/Strings.h" using namespace swift; @@ -322,7 +321,7 @@ FunctionSideEffectFlags *FunctionSideEffects::getEffectsOn(SILValue Addr) { // Return true if the given function has defined effects that were successfully // recorded in this FunctionSideEffects object. bool FunctionSideEffects::setDefinedEffects(SILFunction *F) { - if (F->hasSemanticsAttr(SEMANTICS_PROGRAMTERMINATION_POINT)) { + if (F->hasSemanticsAttr("arc.programtermination_point")) { Traps = true; return true; } diff --git a/lib/SILOptimizer/Mandatory/DiagnoseInfiniteRecursion.cpp b/lib/SILOptimizer/Mandatory/DiagnoseInfiniteRecursion.cpp index 39d88659ebe..c0e6ba49a8e 100644 --- a/lib/SILOptimizer/Mandatory/DiagnoseInfiniteRecursion.cpp +++ b/lib/SILOptimizer/Mandatory/DiagnoseInfiniteRecursion.cpp @@ -22,12 +22,10 @@ #include "swift/SIL/CFG.h" #include "swift/SIL/SILArgument.h" #include "swift/SIL/SILInstruction.h" -#include "swift/SILOptimizer/Analysis/ARCAnalysis.h" #include "swift/SILOptimizer/Analysis/PostOrderAnalysis.h" #include "swift/SILOptimizer/PassManager/Passes.h" #include "swift/SILOptimizer/PassManager/Transforms.h" #include "swift/SILOptimizer/Utils/Devirtualize.h" -#include "swift/Strings.h" #include "llvm/ADT/PostOrderIterator.h" #include "llvm/ADT/iterator_range.h" #include "llvm/Support/Debug.h" @@ -111,14 +109,6 @@ static bool hasInfinitelyRecursiveApply(SILFunction *targetFn) { while (!workList.empty()) { SILBasicBlock *curBlock = workList.pop_back_val(); - // If the block is "program terminating", meaning it traps without relevant - // side effects, then skip it. - // Note that this check happens _before_ the recursion check. This is - // deliberate, as we want to ignore recursive calls inside a program - // termination point. - if (curBlock->isProgramTerminationPoint()) - continue; - // We're looking for functions that are recursive on _all_ paths. If this // block is recursive, mark that we found recursion and check the next // block in the work list. @@ -129,8 +119,7 @@ static bool hasInfinitelyRecursiveApply(SILFunction *targetFn) { // If this block doesn't have a recursive call, and it exits the function, // then we know the function is not infinitely recursive. - TermInst *terminator = curBlock->getTerminator(); - if (terminator->isFunctionExiting() || terminator->isProgramTerminating()) + if (curBlock->getTerminator()->isFunctionExiting()) return false; // Otherwise, push the successors onto the stack if we haven't visited them. diff --git a/lib/SILOptimizer/Transforms/ARCCodeMotion.cpp b/lib/SILOptimizer/Transforms/ARCCodeMotion.cpp index 910a6c9ee20..7dd1f55d8a0 100644 --- a/lib/SILOptimizer/Transforms/ARCCodeMotion.cpp +++ b/lib/SILOptimizer/Transforms/ARCCodeMotion.cpp @@ -1074,7 +1074,7 @@ static void eliminateRetainsPrecedingProgramTerminationPoints(SILFunction *f) { if (auto apply = FullApplySite::isa(&*iter)) { SILFunction *callee = apply.getCalleeFunction(); if (!callee || - !callee->hasSemanticsAttr(SEMANTICS_PROGRAMTERMINATION_POINT)) { + !callee->hasSemanticsAttr(SEMANTICS_ARC_PROGRAMTERMINATION_POINT)) { continue; } } else { diff --git a/lib/SILOptimizer/Transforms/SimplifyCFG.cpp b/lib/SILOptimizer/Transforms/SimplifyCFG.cpp index f4318f2ace1..5648cf34879 100644 --- a/lib/SILOptimizer/Transforms/SimplifyCFG.cpp +++ b/lib/SILOptimizer/Transforms/SimplifyCFG.cpp @@ -3515,7 +3515,7 @@ bool SimplifyCFG::simplifyProgramTerminationBlock(SILBasicBlock *BB) { // use the analysis is because the CFG is likely to be invalidated right // after this pass, o we do not really get the benefit of reusing the // computation for the next iteration of the pass. - if (!BB->isProgramTerminationPoint()) + if (!isARCInertTrapBB(BB)) return false; // This is going to be the last basic block this program is going to execute diff --git a/stdlib/public/core/AssertCommon.swift b/stdlib/public/core/AssertCommon.swift index 73825d06aab..d305f5a5c6d 100644 --- a/stdlib/public/core/AssertCommon.swift +++ b/stdlib/public/core/AssertCommon.swift @@ -76,7 +76,6 @@ internal func _fatalErrorFlags() -> UInt32 { /// bloats code. @usableFromInline @inline(never) -@_semantics("programtermination_point") internal func _assertionFailure( _ prefix: StaticString, _ message: StaticString, file: StaticString, line: UInt, @@ -107,7 +106,6 @@ internal func _assertionFailure( /// bloats code. @usableFromInline @inline(never) -@_semantics("programtermination_point") internal func _assertionFailure( _ prefix: StaticString, _ message: String, file: StaticString, line: UInt, @@ -138,7 +136,6 @@ internal func _assertionFailure( /// bloats code. @usableFromInline @inline(never) -@_semantics("programtermination_point") internal func _assertionFailure( _ prefix: StaticString, _ message: String, flags: UInt32 @@ -164,18 +161,27 @@ internal func _assertionFailure( /// bloats code. @usableFromInline @inline(never) -@_semantics("programtermination_point") +@_semantics("arc.programtermination_point") internal func _fatalErrorMessage( _ prefix: StaticString, _ message: StaticString, file: StaticString, line: UInt, flags: UInt32 ) -> Never { + // This function breaks the infinite recursion detection pass by introducing + // an edge the pass doesn't look through. + func _withUTF8Buffer( + _ string: StaticString, + _ body: (UnsafeBufferPointer) -> R + ) -> R { + return string.withUTF8Buffer(body) + } + #if INTERNAL_CHECKS_ENABLED - prefix.withUTF8Buffer { + _withUTF8Buffer(prefix) { (prefix) in - message.withUTF8Buffer { + _withUTF8Buffer(message) { (message) in - file.withUTF8Buffer { + _withUTF8Buffer(file) { (file) in _swift_stdlib_reportFatalErrorInFile( prefix.baseAddress!, CInt(prefix.count), @@ -186,9 +192,9 @@ internal func _fatalErrorMessage( } } #else - prefix.withUTF8Buffer { + _withUTF8Buffer(prefix) { (prefix) in - message.withUTF8Buffer { + _withUTF8Buffer(message) { (message) in _swift_stdlib_reportFatalError( prefix.baseAddress!, CInt(prefix.count), @@ -265,7 +271,6 @@ func _undefined( /// in release builds anyway (old apps that are run on new OSs). @inline(never) @usableFromInline // COMPILER_INTRINSIC -@_semantics("programtermination_point") internal func _diagnoseUnexpectedEnumCaseValue( type: SwitchedValue.Type, rawValue: RawValue @@ -283,7 +288,6 @@ internal func _diagnoseUnexpectedEnumCaseValue( /// in release builds anyway (old apps that are run on new OSs). @inline(never) @usableFromInline // COMPILER_INTRINSIC -@_semantics("programtermination_point") internal func _diagnoseUnexpectedEnumCase( type: SwitchedValue.Type ) -> Never { diff --git a/test/SILOptimizer/arcsequenceopts.sil b/test/SILOptimizer/arcsequenceopts.sil index e0edb4aacaa..8543c514774 100644 --- a/test/SILOptimizer/arcsequenceopts.sil +++ b/test/SILOptimizer/arcsequenceopts.sil @@ -1553,7 +1553,7 @@ bb0(%0 : $<τ_0_0> { var τ_0_0 } ): return %2 : $() } -sil [_semantics "programtermination_point"] @fatalError : $@convention(thin) (StaticString, StaticString, StaticString) -> Never +sil [_semantics "arc.programtermination_point"] @fatalError : $@convention(thin) (StaticString, StaticString, StaticString) -> Never // CHECK-LABEL: sil @ignore_fatalErrorMsgBB : $@convention(thin) (Builtin.NativeObject) -> () { // CHECK-NOT: strong_retain diff --git a/test/SILOptimizer/infinite_recursion.swift b/test/SILOptimizer/infinite_recursion.swift index 48bdbd48852..e086a5f1da1 100644 --- a/test/SILOptimizer/infinite_recursion.swift +++ b/test/SILOptimizer/infinite_recursion.swift @@ -33,7 +33,7 @@ func f() { e() } func g() { // expected-warning {{all paths through this function will call itself}} while true { // expected-note {{condition always evaluates to true}} - g() + g() } g() // expected-warning {{will never be executed}} @@ -61,47 +61,6 @@ func k() -> Any { // expected-warning {{all paths through this function will ca return type(of: k()) } -@_silgen_name("exit") func exit(_: Int32) -> Never - -func l() { - guard Bool.random() else { - exit(0) // no warning; calling 'exit' terminates the program - } - l() -} - -func m() { // expected-warning {{all paths through this function will call itself}} - guard Bool.random() else { - fatalError() // we _do_ warn here, because fatalError is a programtermination_point - } - m() -} - -enum MyNever {} - -func blackHole() -> MyNever { // expected-warning {{all paths through this function will call itself}} - blackHole() -} - -@_semantics("programtermination_point") -func terminateMe() -> MyNever { - terminateMe() // no warning; terminateMe is a programtermination_point -} - -func n() -> MyNever { - if Bool.random() { - blackHole() // no warning; blackHole() will terminate the program - } - n() -} - -func o() -> MyNever { - if Bool.random() { - o() - } - blackHole() // no warning; blackHole() will terminate the program -} - class S { convenience init(a: Int) { // expected-warning {{all paths through this function will call itself}} self.init(a: a) diff --git a/test/SILOptimizer/retain_release_code_motion.sil b/test/SILOptimizer/retain_release_code_motion.sil index 5e16954dae0..487c6eab9e8 100644 --- a/test/SILOptimizer/retain_release_code_motion.sil +++ b/test/SILOptimizer/retain_release_code_motion.sil @@ -667,7 +667,7 @@ bb1: return %5 : $() } -sil [_semantics "programtermination_point"] @fatalError : $@convention(thin) () -> Never +sil [_semantics "arc.programtermination_point"] @fatalError : $@convention(thin) () -> Never // This should eliminate all retains except for the first one b/c of user. // CHECK-LABEL: sil @eliminate_retains_on_fatalError_path_single_bb : $@convention(thin) (@owned Builtin.NativeObject) -> () { diff --git a/test/SILOptimizer/side-effect.sil b/test/SILOptimizer/side-effect.sil index dfb0620d925..dfb547274c8 100644 --- a/test/SILOptimizer/side-effect.sil +++ b/test/SILOptimizer/side-effect.sil @@ -33,7 +33,7 @@ class X { init() } -sil [_semantics "programtermination_point"] @exitfunc : $@convention(thin) () -> Never +sil [_semantics "arc.programtermination_point"] @exitfunc : $@convention(thin) () -> Never sil [readnone] @pure_func : $@convention(thin) () -> () sil [releasenone] @releasenone_func : $@convention(thin) () -> () sil [readonly] @readonly_owned : $@convention(thin) (@owned X) -> ()