From 84b1976061c51dec09d3f4f2d1da2712f116a6f5 Mon Sep 17 00:00:00 2001 From: Alastair Donaldson Date: Thu, 26 Sep 2019 10:57:05 +0100 Subject: [PATCH] spirv-fuzz: do not allow a dead break to target an unreachable block (#2917) Because dominance information becomes a bit unreliable when blocks are unreachable, this change makes it so that the 'dead break' transformation will not introduce a break to an unreachable block. Fixes #2907. --- source/fuzz/fuzzer_util.cpp | 7 +++ source/fuzz/fuzzer_util.h | 5 ++ source/fuzz/transformation_add_dead_break.cpp | 7 +++ .../fuzz/transformation_add_dead_continue.cpp | 7 ++- .../transformation_add_dead_break_test.cpp | 49 +++++++++++++++++++ 5 files changed, 71 insertions(+), 4 deletions(-) diff --git a/source/fuzz/fuzzer_util.cpp b/source/fuzz/fuzzer_util.cpp index a573563e..2ffe8154 100644 --- a/source/fuzz/fuzzer_util.cpp +++ b/source/fuzz/fuzzer_util.cpp @@ -300,6 +300,13 @@ bool NewEdgeRespectsUseDefDominance(opt::IRContext* context, return true; } +bool BlockIsReachableInItsFunction(opt::IRContext* context, + opt::BasicBlock* bb) { + auto enclosing_function = bb->GetParent(); + return context->GetDominatorAnalysis(enclosing_function) + ->Dominates(enclosing_function->entry().get(), bb); +} + } // namespace fuzzerutil } // namespace fuzz diff --git a/source/fuzz/fuzzer_util.h b/source/fuzz/fuzzer_util.h index e9a62bd8..7aebc285 100644 --- a/source/fuzz/fuzzer_util.h +++ b/source/fuzz/fuzzer_util.h @@ -82,6 +82,11 @@ bool NewEdgeRespectsUseDefDominance(opt::IRContext* context, opt::BasicBlock* bb_from, opt::BasicBlock* bb_to); +// Returns true if and only if there is a path to |bb| from the entry block of +// the function that contains |bb|. +bool BlockIsReachableInItsFunction(opt::IRContext* context, + opt::BasicBlock* bb); + } // namespace fuzzerutil } // namespace fuzz diff --git a/source/fuzz/transformation_add_dead_break.cpp b/source/fuzz/transformation_add_dead_break.cpp index f6e6f23e..b244cf48 100644 --- a/source/fuzz/transformation_add_dead_break.cpp +++ b/source/fuzz/transformation_add_dead_break.cpp @@ -138,6 +138,13 @@ bool TransformationAddDeadBreak::IsApplicable( return false; } + if (!fuzzerutil::BlockIsReachableInItsFunction(context, bb_to)) { + // If the target of the break is unreachable, we conservatively do not + // allow adding a dead break, to avoid the compilations that arise due to + // the lack of sensible dominance information for unreachable blocks. + return false; + } + // Check that |message_.from_block| ends with an unconditional branch. if (bb_from->terminator()->opcode() != SpvOpBranch) { // The block associated with the id does not end with an unconditional diff --git a/source/fuzz/transformation_add_dead_continue.cpp b/source/fuzz/transformation_add_dead_continue.cpp index 3723a2e8..e644b882 100644 --- a/source/fuzz/transformation_add_dead_continue.cpp +++ b/source/fuzz/transformation_add_dead_continue.cpp @@ -85,10 +85,9 @@ bool TransformationAddDeadContinue::IsApplicable( auto continue_block = context->cfg()->block(loop_header)->ContinueBlockId(); - if (!context->GetDominatorAnalysis(bb_from->GetParent()) - ->Dominates(loop_header, continue_block)) { - // If the loop's continue block is not dominated by the loop header, the - // continue block is unreachable. In that case, we conservatively do not + if (!fuzzerutil::BlockIsReachableInItsFunction( + context, context->cfg()->block(continue_block))) { + // If the loop's continue block is unreachable, we conservatively do not // allow adding a dead continue, to avoid the compilations that arise due to // the lack of sensible dominance information for unreachable blocks. return false; diff --git a/test/fuzz/transformation_add_dead_break_test.cpp b/test/fuzz/transformation_add_dead_break_test.cpp index 63b6f34a..1dd0c9d4 100644 --- a/test/fuzz/transformation_add_dead_break_test.cpp +++ b/test/fuzz/transformation_add_dead_break_test.cpp @@ -2559,6 +2559,55 @@ TEST(TransformationAddDeadBreakTest, RespectDominanceRules8) { ASSERT_FALSE(bad_transformation.IsApplicable(context.get(), fact_manager)); } +TEST(TransformationAddDeadBreakTest, + BreakWouldDisobeyDominanceBlockOrderingRules) { + std::string shader = R"( + OpCapability Shader + %1 = OpExtInstImport "GLSL.std.450" + OpMemoryModel Logical GLSL450 + OpEntryPoint Fragment %4 "main" + OpExecutionMode %4 OriginUpperLeft + OpSource ESSL 310 + %2 = OpTypeVoid + %3 = OpTypeFunction %2 + %6 = OpTypeBool + %9 = OpConstantTrue %6 + %4 = OpFunction %2 None %3 + %5 = OpLabel + OpBranch %10 + %10 = OpLabel + OpLoopMerge %16 %15 None + OpBranch %11 + %11 = OpLabel + OpSelectionMerge %14 None + OpBranchConditional %9 %12 %13 + %14 = OpLabel + OpBranch %15 + %12 = OpLabel + OpBranch %16 + %13 = OpLabel + OpBranch %16 + %15 = OpLabel + OpBranch %10 + %16 = OpLabel + OpReturn + OpFunctionEnd + )"; + + const auto env = SPV_ENV_UNIVERSAL_1_3; + const auto consumer = nullptr; + const auto context = BuildModule(env, consumer, shader, kFuzzAssembleOption); + ASSERT_TRUE(IsValid(env, context.get())); + + FactManager fact_manager; + + // Bad because 14 comes before 12 in the module, and 14 has no predecessors. + // This means that an edge from 12 to 14 will lead to 12 dominating 14, which + // is illegal if 12 appears after 14. + auto bad_transformation = TransformationAddDeadBreak(12, 14, true, {}); + ASSERT_FALSE(bad_transformation.IsApplicable(context.get(), fact_manager)); +} + } // namespace } // namespace fuzz } // namespace spvtools