From ae04531b5f52fac103683e9b0d8b50593f278300 Mon Sep 17 00:00:00 2001 From: Andrew Trick Date: Thu, 23 Jan 2020 14:59:32 -0800 Subject: [PATCH 1/2] Fix EscapeAnalysis value_to_bridge_object and strong_copy_unowned_value. Noticed via code inspection. This could potentially miscompile, but we haven't seen that happen to my knowledge. Both value_to_bridge_object and strong_copy_XXX need to escape their resulting value. The implementation seemed to assume that it is conservatively correct simply to avoid building a connection graph node for an value. This is *not* true. Any value that has a pointer type requires a connection graph node. The only way to be conservative is to create the value node *and* point it to an escaping content node. We can always declare that certain special types are not considered pointer types, but then we need to handle all conversions from those types to pointer types by escaping the resulting pointer. BridgeObjects are often on the performance-critical path. --- lib/SILOptimizer/Analysis/EscapeAnalysis.cpp | 12 +++--- test/SILOptimizer/escape_analysis.sil | 44 ++++++++++++++++++++ 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/lib/SILOptimizer/Analysis/EscapeAnalysis.cpp b/lib/SILOptimizer/Analysis/EscapeAnalysis.cpp index 9a7c9ff814832..e7e8636fd30f6 100644 --- a/lib/SILOptimizer/Analysis/EscapeAnalysis.cpp +++ b/lib/SILOptimizer/Analysis/EscapeAnalysis.cpp @@ -2020,12 +2020,9 @@ void EscapeAnalysis::analyzeInstruction(SILInstruction *I, ConGraph->getNode(cast(I)); return; -#define UNCHECKED_REF_STORAGE(Name, ...) \ - case SILInstructionKind::StrongCopy##Name##ValueInst: #define ALWAYS_OR_SOMETIMES_LOADABLE_CHECKED_REF_STORAGE(Name, ...) \ case SILInstructionKind::Name##RetainInst: \ - case SILInstructionKind::StrongRetain##Name##Inst: \ - case SILInstructionKind::StrongCopy##Name##ValueInst: + case SILInstructionKind::StrongRetain##Name##Inst: #include "swift/AST/ReferenceStorage.def" case SILInstructionKind::DeallocStackInst: case SILInstructionKind::StrongRetainInst: @@ -2043,8 +2040,11 @@ void EscapeAnalysis::analyzeInstruction(SILInstruction *I, case SILInstructionKind::SetDeallocatingInst: case SILInstructionKind::FixLifetimeInst: case SILInstructionKind::ClassifyBridgeObjectInst: - case SILInstructionKind::ValueToBridgeObjectInst: - // These instructions don't have any effect on escaping. + // Early bailout: These instructions never produce a pointer value and + // have no escaping effect on their operands. + assert(!llvm::any_of(I->getResults(), [this](SILValue result) { + return isPointer(result); + })); return; #define ALWAYS_OR_SOMETIMES_LOADABLE_CHECKED_REF_STORAGE(Name, ...) \ diff --git a/test/SILOptimizer/escape_analysis.sil b/test/SILOptimizer/escape_analysis.sil index 46af879e675bd..eddfac11b7f09 100644 --- a/test/SILOptimizer/escape_analysis.sil +++ b/test/SILOptimizer/escape_analysis.sil @@ -1844,3 +1844,47 @@ bb3(%4 : $AnyObject): %5 = tuple () return %5 : $() } + +// Test value_to_bridge_object merged with a local reference. A graph node +// must be created for all values involved, and they must point to an +// escaping content node. +// CHECK-LABEL: CG of testValueToBridgeObject +// CHECK-NEXT: Val [ref] %1 Esc: , Succ: (%5.1) +// CHECK-NEXT: Val [ref] %5 Esc: , Succ: (%5.1) +// CHECK-NEXT: Con [int] %5.1 Esc: G, Succ: (%5.2) +// CHECK-NEXT: Con %5.2 Esc: G, Succ: +// CHECK-NEXT: Val [ref] %7 Esc: , Succ: (%5.1), %1, %5 +// CHECK-NEXT: Ret [ref] return Esc: , Succ: %7 +// CHECK-LABEL: End +sil @testValueToBridgeObject : $@convention(thin) (Builtin.Word) -> C { +bb0(%0 : $Builtin.Word): + %1 = alloc_ref $C + cond_br undef, bb1, bb2 + +bb1: + %derived = ref_to_bridge_object %1 : $C, %0 : $Builtin.Word + br bb3(%derived : $Builtin.BridgeObject) + +bb2: + %5 = value_to_bridge_object %0 : $Builtin.Word + br bb3(%5 : $Builtin.BridgeObject) + +bb3(%7 : $Builtin.BridgeObject): + %result = bridge_object_to_ref %7 : $Builtin.BridgeObject to $C + return %result : $C +} + +// Test strong_copy_unowned_value returned. It must be marked escaping, +// not simply "returned", because it's reference is formed from a +// function argument. +// CHECK-LABEL: CG of testStrongCopyUnowned +// CHECK-NEXT: Val [ref] %1 Esc: , Succ: (%1.1) +// CHECK-NEXT: Con [int] %1.1 Esc: G, Succ: (%1.2) +// CHECK-NEXT: Con %1.2 Esc: G, Succ: +// CHECK-NEXT: Ret [ref] return Esc: , Succ: %1 +// CHECK-LABEL: End +sil [ossa] @testStrongCopyUnowned : $@convention(thin) (@guaranteed @sil_unowned Builtin.NativeObject) -> @owned Builtin.NativeObject { +bb0(%0 : @guaranteed $@sil_unowned Builtin.NativeObject): + %1 = strong_copy_unowned_value %0 : $@sil_unowned Builtin.NativeObject + return %1 : $Builtin.NativeObject +} From 06645d3c0f1e7912a35e79ddd2fbd3271084b8b8 Mon Sep 17 00:00:00 2001 From: Andrew Trick Date: Thu, 23 Jan 2020 17:43:19 -0800 Subject: [PATCH 2/2] Add EscapeAnalysis verification to catch unmapped SILValues. This verification would have prevented the following recent miscompilation: commit fbe38ce78d97df804fbf2b3a939d22e2f0013308 Fix EscapeAnalysis losing precision during merge. --- .../SILOptimizer/Analysis/EscapeAnalysis.h | 34 ++----- lib/SILOptimizer/Analysis/EscapeAnalysis.cpp | 89 +++++++++++++++---- 2 files changed, 79 insertions(+), 44 deletions(-) diff --git a/include/swift/SILOptimizer/Analysis/EscapeAnalysis.h b/include/swift/SILOptimizer/Analysis/EscapeAnalysis.h index 832e40214e64b..c1d405c4d7f42 100644 --- a/include/swift/SILOptimizer/Analysis/EscapeAnalysis.h +++ b/include/swift/SILOptimizer/Analysis/EscapeAnalysis.h @@ -660,7 +660,7 @@ class EscapeAnalysis : public BottomUpIPAnalysis { : F(F), EA(EA), isSummaryGraph(isSummaryGraph) {} /// Returns true if the connection graph is empty. - bool isEmpty() { + bool isEmpty() const { return Values2Nodes.empty() && Nodes.empty() && UsePoints.empty(); } @@ -906,11 +906,10 @@ class EscapeAnalysis : public BottomUpIPAnalysis { /// Dump the connection graph to a DOT file for remote debugging. void dumpCG() const; - /// Checks if the graph is OK. - void verify(bool allowMerge = false) const; + /// Checks if the graph is valid and complete. + void verify() const; - /// Just verifies the graph structure. This function can also be called - /// during the graph is modified, e.g. in mergeAllScheduledNodes(). + /// Just verifies the graph nodes. void verifyStructure(bool allowMerge = false) const; friend struct ::CGForDotView; @@ -1045,8 +1044,9 @@ class EscapeAnalysis : public BottomUpIPAnalysis { /// If \p ai is an optimizable @_semantics("array.uninitialized") call, return /// valid call information. - ArrayUninitCall canOptimizeArrayUninitializedCall(ApplyInst *ai, - ConnectionGraph *conGraph); + ArrayUninitCall + canOptimizeArrayUninitializedCall(ApplyInst *ai, + const ConnectionGraph *conGraph); /// Return true of this tuple_extract is the result of an optimizable /// @_semantics("array.uninitialized") call. @@ -1174,25 +1174,9 @@ class EscapeAnalysis : public BottomUpIPAnalysis { virtual bool needsNotifications() override { return true; } - virtual void verify() const override { -#ifndef NDEBUG - for (auto Iter : Function2Info) { - FunctionInfo *FInfo = Iter.second; - FInfo->Graph.verify(); - FInfo->SummaryGraph.verify(); - } -#endif - } - - virtual void verify(SILFunction *F) const override { -#ifndef NDEBUG - if (FunctionInfo *FInfo = Function2Info.lookup(F)) { - FInfo->Graph.verify(); - FInfo->SummaryGraph.verify(); - } -#endif - } + virtual void verify() const override; + virtual void verify(SILFunction *F) const override; }; } // end namespace swift diff --git a/lib/SILOptimizer/Analysis/EscapeAnalysis.cpp b/lib/SILOptimizer/Analysis/EscapeAnalysis.cpp index e7e8636fd30f6..dbfa9931c9cb1 100644 --- a/lib/SILOptimizer/Analysis/EscapeAnalysis.cpp +++ b/lib/SILOptimizer/Analysis/EscapeAnalysis.cpp @@ -609,7 +609,7 @@ void EscapeAnalysis::ConnectionGraph::mergeAllScheduledNodes() { // To pointsTo-> NodeB (but still has a pointsTo edge to NodeB) while (!ToMerge.empty()) { if (EnableInternalVerify) - verify(/*allowMerge=*/true); + verifyStructure(true /*allowMerge*/); CGNode *From = ToMerge.pop_back_val(); CGNode *To = From->getMergeTarget(); @@ -745,7 +745,6 @@ void EscapeAnalysis::ConnectionGraph::mergeAllScheduledNodes() { From->isMerged = true; if (From->mappedValue) { - // If possible, transfer 'From's mappedValue to 'To' for clarity. Any // values previously mapped to 'From' but not transferred to 'To's // mappedValue must remain mapped to 'From'. Lookups on those values will // find 'To' via the mergeTarget. Dropping a value's mapping is illegal @@ -762,7 +761,7 @@ void EscapeAnalysis::ConnectionGraph::mergeAllScheduledNodes() { From->pointsTo = nullptr; } if (EnableInternalVerify) - verify(/*allowMerge=*/true); + verifyStructure(true /*allowMerge*/); } // As a result of a merge, update the pointsTo field of initialNode and @@ -1574,21 +1573,40 @@ bool CGNode::matchPointToOfDefers(bool allowMerge) const { return true; } -void EscapeAnalysis::ConnectionGraph::verify(bool allowMerge) const { +void EscapeAnalysis::ConnectionGraph::verify() const { #ifndef NDEBUG - verifyStructure(allowMerge); + // Invalidating EscapeAnalysis clears the connection graph. + if (isEmpty()) + return; - // Check graph invariants - for (CGNode *Nd : Nodes) { - // ConnectionGraph invariant #4: For any node N, all paths starting at N - // which consist of only defer-edges and a single trailing points-to edge - // must lead to the same - assert(Nd->matchPointToOfDefers(allowMerge)); - if (Nd->mappedValue && !(allowMerge && Nd->isMerged)) { - assert(Nd == Values2Nodes.lookup(Nd->mappedValue)); - assert(EA->isPointer(Nd->mappedValue)); - // Nodes must always be mapped from the pointer root value. - assert(Nd->mappedValue == EA->getPointerRoot(Nd->mappedValue)); + verifyStructure(); + + // Verify that all pointer nodes are still mapped, otherwise the process of + // merging nodes may have lost information. + for (SILBasicBlock &BB : *F) { + for (auto &I : BB) { + if (isNonWritableMemoryAddress(&I)) + continue; + + if (auto ai = dyn_cast(&I)) { + if (EA->canOptimizeArrayUninitializedCall(ai, this).isValid()) + continue; + } + for (auto result : I.getResults()) { + if (EA->getPointerBase(result)) + continue; + + if (!EA->isPointer(result)) + continue; + + if (!Values2Nodes.lookup(result)) { + llvm::dbgs() << "No CG mapping for "; + result->dumpInContext(); + llvm::dbgs() << " in:\n"; + F->dump(); + llvm_unreachable("Missing escape connection graph mapping"); + } + } } } #endif @@ -1597,6 +1615,7 @@ void EscapeAnalysis::ConnectionGraph::verify(bool allowMerge) const { void EscapeAnalysis::ConnectionGraph::verifyStructure(bool allowMerge) const { #ifndef NDEBUG for (CGNode *Nd : Nodes) { + // Verify the graph structure... if (Nd->isMerged) { assert(Nd->mergeTo); assert(!Nd->pointsTo); @@ -1625,6 +1644,19 @@ void EscapeAnalysis::ConnectionGraph::verifyStructure(bool allowMerge) const { } if (Nd->isInterior()) assert(Nd->pointsTo && "Interior content node requires a pointsTo node"); + + // ConnectionGraph invariant #4: For any node N, all paths starting at N + // which consist of only defer-edges and a single trailing points-to edge + // must lead to the same + assert(Nd->matchPointToOfDefers(allowMerge)); + + // Verify the node to value mapping... + if (Nd->mappedValue && !(allowMerge && Nd->isMerged)) { + assert(Nd == Values2Nodes.lookup(Nd->mappedValue)); + assert(EA->isPointer(Nd->mappedValue)); + // Nodes must always be mapped from the pointer root value. + assert(Nd->mappedValue == EA->getPointerRoot(Nd->mappedValue)); + } } #endif } @@ -1780,8 +1812,8 @@ bool EscapeAnalysis::buildConnectionGraphForDestructor( } EscapeAnalysis::ArrayUninitCall -EscapeAnalysis::canOptimizeArrayUninitializedCall(ApplyInst *ai, - ConnectionGraph *conGraph) { +EscapeAnalysis::canOptimizeArrayUninitializedCall( + ApplyInst *ai, const ConnectionGraph *conGraph) { ArrayUninitCall call; // This must be an exact match so we don't accidentally optimize // "array.uninitialized_intrinsic". @@ -2425,7 +2457,7 @@ void EscapeAnalysis::recompute(FunctionInfo *Initial) { if (BottomUpOrder.wasRecomputedWithCurrentUpdateID(FInfo)) { FInfo->Graph.computeUsePoints(); FInfo->Graph.verify(); - FInfo->SummaryGraph.verify(); + FInfo->SummaryGraph.verifyStructure(); } } } @@ -2766,6 +2798,25 @@ void EscapeAnalysis::handleDeleteNotification(SILNode *node) { } } +void EscapeAnalysis::verify() const { +#ifndef NDEBUG + for (auto Iter : Function2Info) { + FunctionInfo *FInfo = Iter.second; + FInfo->Graph.verify(); + FInfo->SummaryGraph.verifyStructure(); + } +#endif +} + +void EscapeAnalysis::verify(SILFunction *F) const { +#ifndef NDEBUG + if (FunctionInfo *FInfo = Function2Info.lookup(F)) { + FInfo->Graph.verify(); + FInfo->SummaryGraph.verifyStructure(); + } +#endif +} + SILAnalysis *swift::createEscapeAnalysis(SILModule *M) { return new EscapeAnalysis(M); }