From 911b8d11a7c847c536d6fb82b2883cc88d416503 Mon Sep 17 00:00:00 2001 From: Jens Johansen Date: Mon, 10 Jun 2024 13:02:28 +0000 Subject: [PATCH] [vm] RecordCoverageInstr isn't dead code Dead Code Elimination can remove `RecordCoverageInstr`, but shouldn't. This CL makes it stay by returning true in `HasUnknownSideEffects` making `MayHaveVisibleEffect` return true, making Dead Code Elimination keep it. Note that `RecordCoverageInstr::Canonicalize` will still let it go away if the position is already covered which is what we want. Bug: https://github.com/dart-lang/sdk/issues/42061 TEST=pkg/vm_service/test/coverage_closure_call_after_optimization_test.dart,vm/cc/IL_RecordCoverageSurvivesOptimizations Change-Id: Ifd72f9071a51924fd71f3dae91687acb1467047d Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/370220 Reviewed-by: Slava Egorov Commit-Queue: Jens Johansen --- ..._closure_call_after_optimization_test.dart | 147 ++++++++++++++++++ runtime/vm/compiler/backend/il.h | 1 + runtime/vm/compiler/backend/il_test.cc | 29 ++++ runtime/vm/compiler/backend/il_test_helper.cc | 6 +- runtime/vm/compiler/compiler_pass.cc | 7 +- runtime/vm/compiler/compiler_pass.h | 9 +- 6 files changed, 195 insertions(+), 4 deletions(-) create mode 100644 pkg/vm_service/test/coverage_closure_call_after_optimization_test.dart diff --git a/pkg/vm_service/test/coverage_closure_call_after_optimization_test.dart b/pkg/vm_service/test/coverage_closure_call_after_optimization_test.dart new file mode 100644 index 00000000000..618e97fa75c --- /dev/null +++ b/pkg/vm_service/test/coverage_closure_call_after_optimization_test.dart @@ -0,0 +1,147 @@ +// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'dart:developer'; + +import 'package:test/test.dart'; +import 'package:vm_service/vm_service.dart'; + +import 'common/service_test_common.dart'; +import 'common/test_helper.dart'; + +@pragma('vm:never-inline') +String leafFunction(void Function() f, bool intoIf) { + if (intoIf) { + f(); + } + return 'some constant'; +} + +const optimizationCounterThreshold = 10; + +void testFunction() { + debugger(); + // If we do `optimizationCounterThreshold - 2` here optimization doesn't kick + // in and the test (which otherwise currently fails) passes. + for (int i = 0; i < optimizationCounterThreshold; i++) { + leafFunction(() {}, false); + } + // Assuming `leafFunction` is optimized now, does coverage still work? + // Note that I via `--print_flow_graph --print_flow_graph_optimized \ + // --print-flow-graph-filter=leafFunction` can see that it is, but that + // `func.code?.isOptimized` is false for whatever reason. + leafFunction(() {}, true); + debugger(); +} + +var tests = [ + hasStoppedAtBreakpoint, + (VmService service, IsolateRef isolateRef) async { + final isolateId = isolateRef.id!; + final isolate = await service.getIsolate(isolateId); + final stack = await service.getStack(isolateId); + + // Make sure we are in the right place. + final frames = stack.frames!; + expect(frames.length, greaterThanOrEqualTo(1)); + expect(frames[0].function!.name, 'testFunction'); + + final rootLib = + await service.getObject(isolateId, isolate.rootLib!.id!) as Library; + final funcRef = + rootLib.functions!.singleWhere((f) => f.name == 'leafFunction'); + final func = await service.getObject(isolateId, funcRef.id!) as Func; + + final expectedRange = { + 'scriptIndex': 0, + 'startPos': 399, + 'endPos': 535, + 'compiled': true, + 'coverage': { + 'hits': [], + 'misses': [399, 501], + }, + }; + + final location = func.location!; + final report = await service.getSourceReport( + isolateId, + [SourceReportKind.kCoverage], + scriptId: location.script!.id!, + tokenPos: location.tokenPos, + endTokenPos: location.endTokenPos, + forceCompile: true, + ); + + final ranges = report.ranges!; + final scripts = report.scripts!; + expect(ranges.length, 1); + expect(ranges[0].toJson(), expectedRange); + expect(scripts.length, 1); + expect( + scripts[0].uri, + endsWith('coverage_closure_call_after_optimization_test.dart'), + ); + }, + resumeIsolate, + hasStoppedAtBreakpoint, + (VmService service, IsolateRef isolateRef) async { + final isolateId = isolateRef.id!; + final isolate = await service.getIsolate(isolateId); + final stack = await service.getStack(isolateId); + + // Make sure we are in the right place. + final frames = stack.frames!; + expect(frames.length, greaterThanOrEqualTo(1)); + expect(frames[0].function!.name, 'testFunction'); + + final rootLib = + await service.getObject(isolateId, isolate.rootLib!.id!) as Library; + final funcRef = + rootLib.functions!.singleWhere((f) => f.name == 'leafFunction'); + final func = await service.getObject(isolateId, funcRef.id!) as Func; + + final expectedRange = { + 'scriptIndex': 0, + 'startPos': 399, + 'endPos': 535, + 'compiled': true, + 'coverage': { + 'hits': [399, 501], + 'misses': [], + }, + }; + + final location = func.location!; + final report = await service.getSourceReport( + isolateId, + [SourceReportKind.kCoverage], + scriptId: location.script!.id!, + tokenPos: location.tokenPos, + endTokenPos: location.endTokenPos, + forceCompile: true, + ); + + final ranges = report.ranges!; + final scripts = report.scripts!; + expect(ranges.length, 1); + expect(ranges[0].toJson(), expectedRange); + expect(scripts.length, 1); + expect( + scripts[0].uri, + endsWith('coverage_closure_call_after_optimization_test.dart'), + ); + }, +]; + +void main(List args) => runIsolateTests( + args, + tests, + 'coverage_closure_call_after_optimization_test.dart', + testeeConcurrent: testFunction, + extraArgs: [ + '--deterministic', + '--optimization-counter-threshold=$optimizationCounterThreshold', + ], + ); diff --git a/runtime/vm/compiler/backend/il.h b/runtime/vm/compiler/backend/il.h index d38c12232f4..8d569f7020e 100644 --- a/runtime/vm/compiler/backend/il.h +++ b/runtime/vm/compiler/backend/il.h @@ -7166,6 +7166,7 @@ class RecordCoverageInstr : public TemplateInstruction<0, NoThrow> { virtual TokenPosition token_pos() const { return token_pos_; } virtual bool ComputeCanDeoptimize() const { return false; } virtual bool HasUnknownSideEffects() const { return false; } + virtual bool MayHaveVisibleEffect() const { return true; } virtual Instruction* Canonicalize(FlowGraph* flow_graph); #define FIELD_LIST(F) \ diff --git a/runtime/vm/compiler/backend/il_test.cc b/runtime/vm/compiler/backend/il_test.cc index c42feef803c..8f0233534ee 100644 --- a/runtime/vm/compiler/backend/il_test.cc +++ b/runtime/vm/compiler/backend/il_test.cc @@ -1728,4 +1728,33 @@ ISOLATE_UNIT_TEST_CASE(IL_TestIntInstr) { } } +// This is a smoke test which verifies that RecordCoverage instruction is not +// accidentally removed by some overly eager optimization. +ISOLATE_UNIT_TEST_CASE(IL_RecordCoverageSurvivesOptimizations) { + using compiler::BlockBuilder; + SetFlagScope sfs(&FLAG_reorder_basic_blocks, false); + + TestPipeline pipeline(CompilerPass::kJIT, [&]() { + FlowGraphBuilderHelper H(/*num_parameters=*/0); + + { + BlockBuilder builder(H.flow_graph(), + H.flow_graph()->graph_entry()->normal_entry()); + const auto& coverage_array = Array::Handle(Array::New(1)); + coverage_array.SetAt(0, Smi::Handle(Smi::New(0))); + builder.AddInstruction( + new RecordCoverageInstr(coverage_array, 0, InstructionSource())); + builder.AddReturn(new Value(H.flow_graph()->constant_null())); + } + + H.FinishGraph(); + return H.flow_graph(); + }); + + auto flow_graph = pipeline.RunPasses({}); + + // RecordCoverage instruction should remain in the graph. + EXPECT(flow_graph->graph_entry()->normal_entry()->next()->IsRecordCoverage()); +} + } // namespace dart diff --git a/runtime/vm/compiler/backend/il_test_helper.cc b/runtime/vm/compiler/backend/il_test_helper.cc index 162b7000a8b..7c88eda6481 100644 --- a/runtime/vm/compiler/backend/il_test_helper.cc +++ b/runtime/vm/compiler/backend/il_test_helper.cc @@ -120,6 +120,9 @@ FlowGraph* TestPipeline::RunPasses( const bool optimized = true; const intptr_t osr_id = Compiler::kNoOSRDeoptId; + // We assume that prebuilt graph is already in SSA form so we should + // avoid running ComputeSSA on it (it will just crash). + const bool is_ssa = (flow_graph_ != nullptr); if (flow_graph_ == nullptr) { auto pipeline = CompilationPipeline::New(zone, function_); @@ -163,7 +166,8 @@ FlowGraph* TestPipeline::RunPasses( if (passes.size() > 0) { flow_graph_ = CompilerPass::RunPipelineWithPasses(pass_state_, passes); } else { - flow_graph_ = CompilerPass::RunPipeline(mode_, pass_state_); + flow_graph_ = CompilerPass::RunPipeline(mode_, pass_state_, + /*compute_ssa=*/!is_ssa); } pass_state_->call_specializer = nullptr; } diff --git a/runtime/vm/compiler/compiler_pass.cc b/runtime/vm/compiler/compiler_pass.cc index 310344c9128..1384153d95f 100644 --- a/runtime/vm/compiler/compiler_pass.cc +++ b/runtime/vm/compiler/compiler_pass.cc @@ -300,8 +300,11 @@ void CompilerPass::RunInliningPipeline(PipelineMode mode, } FlowGraph* CompilerPass::RunPipeline(PipelineMode mode, - CompilerPassState* pass_state) { - INVOKE_PASS(ComputeSSA); + CompilerPassState* pass_state, + bool compute_ssa) { + if (compute_ssa) { + INVOKE_PASS(ComputeSSA); + } INVOKE_PASS_AOT(ApplyClassIds); INVOKE_PASS_AOT(TypePropagation); INVOKE_PASS(ApplyICData); diff --git a/runtime/vm/compiler/compiler_pass.h b/runtime/vm/compiler/compiler_pass.h index bdf84b24cbc..92d8449f654 100644 --- a/runtime/vm/compiler/compiler_pass.h +++ b/runtime/vm/compiler/compiler_pass.h @@ -166,10 +166,17 @@ class CompilerPass { // the old invariant that the FlowGraph stored in the CompilerPassState was // always updated, never entirely replaced. // + // By default pipeline assumes that input graph is not in SSA form yet and + // will invoke |ComputeSSA| pass on it. |ComputeSSA| is not idempotent and + // will crash if invoked on a graph which is already in SSA form. To avoid + // that you can set |compute_ssa| to |false|. + // // To make sure callers are updated properly, these methods also return // the final FlowGraph and we add a check that callers use this result. DART_WARN_UNUSED_RESULT - static FlowGraph* RunPipeline(PipelineMode mode, CompilerPassState* state); + static FlowGraph* RunPipeline(PipelineMode mode, + CompilerPassState* state, + bool compute_ssa = true); DART_WARN_UNUSED_RESULT static FlowGraph* RunPipelineWithPasses( CompilerPassState* state,