[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 <vegorov@google.com>
Commit-Queue: Jens Johansen <jensj@google.com>

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 = <IsolateTest>[
+  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<String> args) => runIsolateTests(
+      args,
+      tests,
+      'coverage_closure_call_after_optimization_test.dart',
+      testeeConcurrent: testFunction,
+      extraArgs: [
+        '--deterministic',
+        '--optimization-counter-threshold=$optimizationCounterThreshold',
+      ],
+    );

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)                                                          \

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<bool> 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

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;
   }

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);

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,