Compiling with instrumentation does not generate unique coverage array per method

[drognanar]

Version Used: VS 17.14

Steps to Reproduce:

1. Download https://github.com/microsoft/vs-streamjsonrpc
2. Add <BuildingForLiveUnitTesting>true</BuildingForLiveUnitTesting> to the main Directory.Build.props, to compile all projects with LUT instrumentation and the /instrument:TestCoverage csc flag
3. After the build ildasm/disassemble the StreamJsonRpc.dll
4. There are multiple methods that use the same coverage payload index (in this case PayloadRoot[244] even though it should be unique per method). In the examples below it's both under StreamJsonRpc.JsonRpc class.

Expected Behavior:

All methods should have a unique PayloadRoot[index] index value.

Actual Behavior:

Multiple methods have the same PayloadRoot[index]. In this example multiple methods have PayloadRoot[244] and that's causing two methods to share the same coverage array.

[AArnott]

This bug is blocking my ability to use live unit testing where async methods are executed.

[RikkiGibson]

I cloned and added <BuildingForLiveUnitTesting>true</BuildingForLiveUnitTesting> to Directory.Build.props as instructed. However, the resulting dotnet build -bl did not pass /instrument:TestCoverage to the csc task for StreamJsonRpc.csproj.

I tried also adding <Instrument>TestCoverage</Instrument> to Directory.Build.props to address that. But that resulted in thousands of errors like the following: error CS0656: Missing compiler required member 'Microsoft.CodeAnalysis.Runtime.Instrumentation.CreatePayload'.

It looks like that is not a documented API. I'm unsure why the members are missing except that maybe each tool that uses the coverage feature is expected to include its own implementation. I ended up working around by reusing a stub from Roslyn's tests:

roslyn/src/Compilers/CSharp/Test/Emit/Emit/DynamicAnalysis/DynamicAnalysisResourceTests.cs

Lines 24 to 42 in 790f592

	namespace Microsoft.CodeAnalysis.Runtime
	{
	public static class Instrumentation
	{
	public static bool[] CreatePayload(System.Guid mvid, int methodToken, int fileIndex, ref bool[] payload, int payloadLength)
	{
	return payload;
	}
	public static bool[] CreatePayload(System.Guid mvid, int methodToken, int[] fileIndices, ref bool[] payload, int payloadLength)
	{
	return payload;
	}
	public static void FlushPayload()
	{
	}
	}
	}

This got me a dll with the expected instrumentation. I inspected overloads of indicated members JsonRpc.JsonRpc and JsonRpc.InvokeCoreAsync looking for duplicate methodTokens but didn't see them.

Replaying the compilation using a debug compiler with debugger attached also doesn't appear to have tripped any asserts.

---

To try and repro closer to the original scenario, I went back, #if false'd the Instrumentation stubs, opened the test project in VS 17.14 preview 6, setup LUT to look at JsonContractResolverTests.cs and started making edits. I didn't observe any obviously broken behaviors off the bat--tests reran after making edits, etc.

It's possible I didn't understand the particular failure mode that is occurring as a result of this. There are a few things we can do to continue to drive this:

1. I can review the compiler code which inserts instrumentation calls, and see how feasible it is to add an assertion that coverage arrays are unique (if such an assertion doesn't already exist).
2. If anybody does have a command line repro, or otherwise some repro with an associated binlog, please create a complog from it and attach it to this issue, so I can investigate using as close to your configuration as possible.

For reference, I am attaching the complog I produced by running the above build steps: build.zip

[AArnott]

Well it's certainly easy enough to repro within Visual Studio with Live Unit Testing actually running. If you need a command line equivalent for it I wouldn't know how to do that.

[drognanar]

@RikkiGibson perhaps instead of running dotnet build you need to open the VS developer prompt and run msbuild instead

if you check the binlog, you should see that the following files should be imported:

props: "C:\Program Files\Microsoft Visual Studio\2022\IntPreview\MSBuild\Current\Imports\Microsoft.Common.Props\ImportBefore\Microsoft.LiveUnitTesting.props"
targets: "C:\Program Files\Microsoft Visual Studio\2022\IntPreview\MSBuild\Current\Microsoft.Common.Targets\ImportAfter\Microsoft.LiveUnitTesting.targets"

these are the files that setup the instrumentation flag

I'll try to collect the complog later in the day

[drognanar]

here's the complog: repro.zip

[RikkiGibson]

I can repro from the complog. Thanks!

I tried building again with a debug compiler and hit an assertion failure:

roslyn/src/Compilers/CSharp/Portable/Lowering/ClosureConversion/SynthesizedClosureMethod.cs

Line 106 in 5b92584

Debug.Assert(!(originalMethod is LocalFunctionSymbol) || !originalMethod.IsStatic || IsStatic);

The originalMethod in this case is local function StreamJsonRpc.PipeMessageHandler.PipeMessageHandler(Stream, Stream, IJsonRpcMessageFormatter).DisposeStreamSafely(object).

I'll have to investigate more to understand the significance/impact of this. The assertion doesn't fail when compiler argument /instrument:TestCoverage is removed.

[RikkiGibson]

This does not repro when building with csc from .NET 9.0.203 (398aa31), but does repro when building with csc from .NET 9.0.301 (8edf7bc).

There are two suspicious areas interacting here.

1. CodeCoverageInstrumenter which inserts the usages of the coverage arrays during lowering.
2. ClosureConversion which converts lambdas and local functions to member methods with parameters/receivers holding captured variables.

The assert that is failing above, is that static local functions must compile to static methods, but, here the local function is actually capturing a coverage array, which forces us to break that invariant. (If we continue the build despite the assertion failure, even more different assertions start failing in the CodeGenerator, which seem to indicate that things are pretty deeply broken.)

I will check the history of the suspicious areas between the two commits for any changes that might have affected this scenario. Also, I will checkout the sources for the apparently working compiler from SDK 9.0.203, and see whether building the repro in debug mode fails the same assertions.

[RikkiGibson]

The above linked assertion actually still fails on the .NET 9.0.203 compiler, where the original bug does not repro. But, the later assertions in CodeGenerator do not fail.

It looks like a pre-existing bug involving static local functions being lowered to non-static methods, which is unrelated to the issue being reported here.

These are the assertions which appeared to only fail with the same compilers where the original reported bug was reproduced:

roslyn/src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs

Line 3532 in 637ee1e

Debug.Assert(node.Method.IsDefinition);

roslyn/src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs

Line 1263 in 637ee1e

Debug.Assert(!needDeclaration);

One of the methods which fails the assert in EmitExpression is Method System.Collections.Generic.IAsyncEnumerable<T>! StreamJsonRpc.JsonRpcExtensions.AsAsyncEnumerable<T>(this System.Collections.Generic.IEnumerable<T>! enumerable, System.Threading.CancellationToken cancellationToken).

[v-hozha1]

It works fine on the latest main build - Version 18.0.0 Preview 1.0 [10807.158.main]