Coverage reports differ when --optimization-counter-threshold=-1 is used

[a-siva]

The CFE team has made the change to use RecordCoverage to InstanceCall and StaticCall (see https://dart-review.googlesource.com/c/sdk/+/370501)

There seems to be a difference between the coverage results on normal runs Vs runs with the option --optimization-counter-threshold=-1

Applying the following patch makes the results match up

diff --git a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
index 46c46cd1e1f..49c75a002e1 100644
--- a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
+++ b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
@@ -62,6 +62,7 @@ FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfFieldInitializer() {
   Fragment body(normal_entry);
   body += B->CheckStackOverflowInPrologue(field_helper.position_);
   body += SetupCapturedParameters(parsed_function()->function());
+  body += RecordCoverage(field_helper.position_);
   body += BuildExpression();  // read initializer.
   body += Return(TokenPosition::kNoSource);

@@ -655,7 +656,7 @@ Fragment StreamingFlowGraphBuilder::BuildFunctionBody(
     const Function& dart_function,
     LocalVariable* first_parameter,
     bool constructor) {
-  Fragment body;
+  Fragment body = RecordCoverage(dart_function.token_pos());

   // TODO(27590): Currently the [VariableDeclaration]s from the
   // initializers will be visible inside the entire body of the constructor.

It is unclear why this is needed.

[a-siva]

//cc @liamappelbe @jensjoha

[alexmarkov]

https://dart-review.googlesource.com/c/sdk/+/370501 added coverage instructions at call sites (at the InstanceCall and StaticCall instructions), so the new coverage instructions effectively replace ICData usage for coverage. However, the missing part is the replacement for function usage counters (Function::WasExecuted), which are incremented during call and used in the coverage reporting, but they are not be updated if call target is inlined during optimizations. Adding RecordCoverage instructions in the prologue should solve this problem. We can also append those instructions to flow graph conditionally (if the position is not covered yet; only when building flow graph during inlining).

Closing this issue as duplicate to #42061.

[jensjoha]

But doesn't --optimization-counter-threshold=-1 disable optimizations?

[alexmarkov]

That's right. However, --optimization-counter-threshold=-1 also affects Function::usage_counter() / Function::WasExecuted(): usage counter is not bumped during calls if optimizing JIT is disabled entirely:

sdk/runtime/vm/compiler/stub_code_compiler_x64.cc

Lines 2313 to 2326 in f9cd243

	void StubCodeCompiler::GenerateUsageCounterIncrement(Register temp_reg) {
	if (FLAG_precompiled_mode) {
	__ Breakpoint();
	return;
	}
	if (FLAG_optimization_counter_threshold >= 0) {
	Register func_reg = temp_reg;
	ASSERT(func_reg != IC_DATA_REG);
	__ Comment("Increment function counter");
	__ movq(func_reg,
	FieldAddress(IC_DATA_REG, target::ICData::owner_offset()));
	__ incl(FieldAddress(func_reg, target::Function::usage_counter_offset()));
	}
	}

That's a yet another reason to replace unreliable function usage counters with explicit RecordCoverage instructions when calculating coverage.

[jensjoha]

Thanks for the explanation!
Should we just apply the patch at the top then?

[alexmarkov]

@jensjoha Yes, we can apply your patch - adding RecordCoverage instructions to the function prologues should be a clear improvement. However, we should also consider cleaning up old coverage machinery which is based on ICData and function usage counters and replacing it with RecordCoverage instructions entirely. I'm not sure if the above patch would be enough to drop old coverage entirely.