Runtime: Code coverage for System.Private.CoreLib is broken in CI

[stephentoub]

It appears we're no longer able to get code coverage of System.Private.CoreLib, even when using CoreCLROverridePath, even when using set COMPlus_ZapDisable=1.

cc: @mellinoe, @danmosemsft

[stephentoub]

It looks like an issue with PDBs. I was able to work around this by doing build windowsmscorlib release after doing my full coreclr build; presumably something about the crossgen step done in the regular coreclr build is messing with the PDBs in a way that prevents OpenCover from successfully using the PDB, and build windowsmscorlib release skips crossgen.

Unfortunately, while this allows OpenCover to successfully run and index the data from System.Private.CoreLib, I then hit a StackOverflowException in ReportGenerator consuming it 😦

[stephentoub]

I built https://github.com/icsharpcode/NRefactory/ from source, overwrote the binaries in my ReportGenerator package directory (corefx\packages\reportgenerator\2.5.0\tools), and the stack overflow went away. So I'm unblocked for now, but this is not a good state.

[danmosemsft]

Seems the latest release of NRefactory is 6.0.0 beta 1 (https://www.nuget.org/packages/ICSharpCode.NRefactory/6.0.0-beta1) ... if you see it work with binaries from that package, we could ask ReportGenerator folk to consider upgrading their dependency.

[stephentoub]

I tried that before I tried compiling from source... failed miserably.

[stephentoub]

Summarizing current workaround, to get code coverage for corelib, one needs to:

1. Build the coreclr repo
2. Build corelib without crossgen (build windowsmscorlib release)
3. Build the corefx repo with CoreCLROverridePath to target the local coreclr build
4. Build the https://github.com/icsharpcode/NRefactory/ repo
5. Overwrite the NRefactory binaries in the local directory containing the unzipped ReportGenerator binaries (e.g. corefx\packages\reportgenerator\2.5.0\tools) with those built from the NRefactory repo
6. Before running tests with /p:Coverage=true, set COMPlus_ZapDisable=1

[danmosemsft]

I see ReportGenerator distributes NRefactory within its package -- so we don't need NRefactory to update their package necessarily, we just need ReportGenerator to include an updated one. I see ReportGenerator 3.0 has just come out, but it has the identical NRefactory that 2.5 has. Worth opening an issue on the ReportGenerator repo to ask?

[stephentoub]

we just need ReportGenerator to include an updated one

ReportGenerator just references the NRefactory NuGet package, no? If it actually distributes the binary, then yeah, we should just ask ReportGenerator to include the latest from source.

[danmosemsft]

No, it distributes it.

[stephentoub]

https://github.com/danielpalme/ReportGenerator/issues/105

[danmosemsft]

I think 3.0.1 needs to go in two places

https://github.com/dotnet/buildtools/blob/master/src/Microsoft.DotNet.Build.Tasks/PackageFiles/CodeCoverage.targets#L9
https://github.com/dotnet/corefx/blob/7f1c480a6a280fbced29606120b507596bafbefc/external/test-runtime/XUnit.Runtime.depproj#L64

[danmosemsft]

I am trying to get coverage to work without building CoreCLR myself, instead using the corelib pdb that is now gotten automatically. Note this is a Windows style PDB not a portable PDB. I cannot get Cecil to load this System.Private.Corelib.pdb - debugging Cecil, it seems to think it does not match the dll, because the guid does not match; the guid it gets from the pdb seems right per Symchk, the guid it reads from the dll header is unexpected. Symchk and VS consider the pdb to be a match. My next guess is to try Cecil 0.10, but it is not a drop in replacement: it needs changes in opencover.

[danmosemsft]

Howver, I can verify that report generator works fine.

[danmosemsft]

It would be interesting to try stepping over the guid check to see whether the corelib pdb works correctly.

[stephentoub]

I am trying to get coverage to work without building CoreCLR myself, instead using the corelib pdb that is now gotten automatically.

What source would that match to / use to generate the rendered code coverage report?

[danmosemsft]

I am assuming it would at least show the bar graphs, then I was going to see whether there was a flag possibly to "remap" the source locations.

[johnnyRose]

@stephentoub any update on this? I haven't had luck with any of the workarounds. Or do you know of somewhere I can pull a recent coverage report for System.Private.CoreLib?

[weshaggard]

@danmosemsft what is the status of this?

[stephentoub]

My workarounds no longer appear to work. I can't get any code coverage for code in corelib. 😦

[ViktorHofer]

@danmosemsft is somebody working on this?

[danmosemsft]

Unfortunately not. At least OpenCover should soon support portable PDB clearly there's other issues to root cause here.

[danmosemsft]

Not required to ship --> Future.

[stephentoub]

😩

[danmosemsft]

@ahsonkhan

[danmosemsft]

Incidentally @ahsonkhan please update with what you find out. It would be great if you have time to at least figure out the area the problem is in.

[pjanotti]

This worked for me with the steps below (some other variations should also work, e.g.: copying the IL only CoreLib directly to testhost folder, using pdb2pdb, etc)

On CoreClr repo:

build -release -skipTests -- /p:DebugType=pdbonly
build -release -skipTests -windowsmscorlib -- /p:DebugType=pdbonly

The second step is important to get IL only of CoreLib on the folder that is going to be used in the CoreCLROverridePath, but as mentioned can be done in other ways.

On CoreFx repo:

build -- /p:CoreCLROverridePath=C:\s\coreclr\bin\Product\Windows_NT.x64.Release
build-tests -skipTests -- /p:CoreCLROverridePath=C:\s\coreclr\bin\Product\Windows_NT.x64.Release
cd src\System.Collections\tests
msbuild /t:RebuildAndTest /p:Coverage=True /p:CodeCoverageAssemblies="System.Private.CoreLib"

The instructions/wiki omit the part that ensures that we get the IL only CoreLib, I will update that after some of you have a chance to confirm the steps above.

[pjanotti]

Master of OpenCover w/ Mono.Cecil 0.10.0 as in https://github.com/OpenCover/opencover/pull/814 works fine with portable PDBs (including CoreLib with similar steps as above). However, Mono.Cecil 0.10.0 fails with pdbs from WindowsPDB (which is not an issue since we can remove that step from buildtools), Anyway I will follow up w/ Roslyn and Mono.Cecil to see what needs to be fixed to get it working.

[ahsonkhan]

build -release -skipTests -- /p:DebugType=pdbonly
build -release -skipTests -windowsmscorlib -- /p:DebugType=pdbonly

Why do we need the second step? Wouldn't the first build step cover building corelib as well?

I will update that after some of you have a chance to confirm the steps above.

Yep, just tried the steps you mentioned and I am able to see code coverage for corelib now.

@pjanotti, do you know what we need to do to get System.Private.CoreLib code coverage results working in CI?
https://github.com/dotnet/corefx#build--test-status
https://ci.dot.net/job/dotnet_corefx/job/master/job/code_coverage_windows/Code_Coverage_Report/

[danmosemsft]

@pjanotti seconded @ahsonkhan , it would be very helpful if you can make corelib coverage show up properly in https://ci.dot.net/job/dotnet_corefx/job/master/job/code_coverage_windows/Code_Coverage_Report/. I cannot think of why it should not work.

[danmosemsft]

I'm going to reopen this in the hope you can fix that.

[stephentoub]

I cannot think of why it should not work.

Presumably because a) the step to change the PDB isn't being done, and b) the source linked to from the PDB isn't available with those paths?

[pjanotti]

Why do we need the second step? Wouldn't the first build step cover building corelib as well?

Short answer:

The second step is important to get IL only of CoreLib on the folder that is going to be used in the CoreCLROverridePath, but as mentioned can be done in other ways.

Long answer:

By default build.cmd puts the native corelib (look at __BuildNativeCoreLib) at its bin/product, however, we want to use the IL version for code coverage (the native one gives wrong coverage numbers, which I assume is expected since it doesn't need to JIT everything). The second step puts the IL version at the proper bin/product and keep the steps simple (and is not very timing consuming). Perhaps the simplest alternative to run build again right now is too copy the IL version manually (the normal build just move it to the IL folder) to the test host folder (the PDB there should be already the correct one, since the PDB for the native corelib is named System.Private.CoreLib.ni.dll).

Assuming that code coverage in CI just does a test pass adding /p:Coverage=True /p:CodeCoverageAssemblies="System.Private.CoreLib" we just need to ensure that testhost has the IL version of corlib.

[pjanotti]

Side note: for the current version of OpenCover we can't use the portable PDB for corelib, that's why the instructions above add -- /p:DebugType=pdbonly to coreclr builds. Notice that, instead of building a Windows PDB directly in coreclr, we could generate a portable PDB with pdb2pdb but I didn't try that myself yet.

[pjanotti]

@danmosemsft the first thing to get this in CI is that we need to get testhost with the corelib IL, however, coreclr nuget packages don't come with it. If we do really want this in CI we need to either add it to the existing coreclr packages or create a new package(s) with it (a coverage package could potentially include managed sources too). If we just fiddle with PDB files without the correct corelib we'll get bogus numbers. Let me know if this is an investment that we want to do: it doesn't seem big but it adds stuff to coreclr nuget packages (currently about 2MB) or create new ones.

BTW, the coreclr symbol packages have a mismatch of PDBs for System.Private.CoreLib: it ships the native dll but its pdb is named System.Private.CoreLib.ni.pdb, the one named System.Private.CoreLib.pdb is actually for the IL version...

[danmosemsft]

We can't increase the size of the packages that customers get. @safern the package that you added Corelib symbols to, that's a transport package that doesn't go on nuget right? If so it seem we could put the IL into that one. (And the missing pdb)

Yes, I think this is worthwhile - it's important that it's easy to see what coverage we are getting.

[safern]

@safern the package that you added Corelib symbols to, that's a transport package that doesn't go on nuget right?

So in order to get the symbols I'm using the runtime package (i.e runtime.win-x64.Microsoft.NETCore.Runtime.CoreCLR) whenever we restore the CoreCLR metapackage. While talking to @pjanotti offline I think we can't use the transport package because we could end up growing the runtime package. What if we create a new package that contains the managed assembly and just push it to myget, then we can restore that package when trying to run code coverage and bin place it in the right location.

[jkotas]

I think we can't use the transport package because we could end up growing the runtime package

The transport packages are sliced and diced by 300+ lines of msbuild to produce the shipping bits: https://github.com/dotnet/core-setup/blob/master/src/pkg/projects/Microsoft.NETCore.App/src/Microsoft.NETCore.App.depproj . I do not think it would be a big deal to add a line here to filter out extra file for CoreLib.

[danmosemsft]

BTW @ahsonkhan @pjanotti if either of you have manually generated coverage for corelib from all the corefx tests, I would be interested to take a look if you could share the folder out to me.

[pjanotti]

@danmosemsft I will share the results of a innerloop pass.

[stephentoub]

Thanks for working on this, @pjanotti.

[danmosemsft]

+1 to the thanks, looking forward to this being completely seamless so

1. Rolling coverage runs include corelib
2. Local coverage runs include corelib - whether corelib is locally built or not
   All without any user intervention.

[pjanotti]

np, first step for automatic corelib coverage is to include IL version on the transport package.

Jan already indicated which file can be used to ensure that it doesn't get included in shipping bits, but :smile: @danmosemsft @safern can you save me some grepping to figure out where the contents of the transport package are defined? I suspect the the report won't be able to reach corelib sources at first, but let's get the IL corelib as first step.

I also started looking at the alternatives to get code coverage in Unix, I'll report in a few days about it.

[pjanotti]

Oh I see it is transform on top of the nuget specs done at build tools https://github.com/dotnet/buildtools/blob/3306ca2481414624a56f22e0ee28f7c08b93037e/src/Microsoft.DotNet.Build.Tasks.Packaging/src/NuGetPack.cs#L171

[pjanotti]

Adding a new folder and putting IL CoreLib and respective pdb on Microsoft.NETCore.Runtime.CoreClr package doesn't add it to the runtime package generated in core-setup because in https://github.com/dotnet/core-setup/blob/master/src/pkg/projects/Microsoft.NETCore.App/src/Microsoft.NETCore.App.depproj it only adds from specific folders (see snippet below). Since runtime.RID.Microsoft.NETCore.Runtime.CoreClr is not intended to be consumed directly it should be ok to proceed this way.

      <FilesToPackage Include="@(ReferenceCopyLocalPaths)">
        <!-- ResolveNugetPackageAssets doesn't preserve the asset type (native),
             calculate it by looking for native in the path -->
        <IsNative Condition="$([System.String]::new('%(Identity)').ToLowerInvariant().Replace('\', '/').Contains('/native/'))">true</IsNative>
      </FilesToPackage>
      <FilesToPackage>
        <TargetPath Condition="'%(FilesToPackage.IsNative)' != 'true'">runtimes/$(NuGetRuntimeIdentifier)/lib/$(PackageTargetFramework)</TargetPath>
        <TargetPath Condition="'%(FilesToPackage.IsNative)' == 'true'">runtimes/$(NuGetRuntimeIdentifier)/native</TargetPath>
      </FilesToPackage>

[pjanotti]

I think I will take the change to remove the pdb from the IL from /native/ and rename S.P.CoreLib.ni.pdb to S.P.CoreLib.pdb so by default it matches the native image of the runtime.

[jkotas]

The managed symbols have .pdb suffix, the native symbols have to have .ni.pdb suffix. Multiple things depend on it. Please do not change that.

[pjanotti]

Thanks for the heads up @jkotas I will keep the PDBs where they are right now.

[pjanotti]

Combining these latest two PRs and a small change to netci.groovy we get "ready" for coverage numbers again for CoreLib, however, I'm seeing relative frequent crashes from OpenCover.Profiler.dll in such scenario (similar to dotnet/corefx#29661). Besides that I didn't look at what can be done for source mapping yet.

[pjanotti]

Since there is some discussion on top with some outdated/incorrect information it seems useful to record the history of the issues affecting coverage and for System.Private.CoreLib in particular:

1. When S.P.C.ni.dll was renamed to S.P.C.dll coverage was broken due to two issues:

   a. At that time the Profiler API had a bug that prevented OpenCover from instrumenting any crossgen assembly. That's why Stephen's attempts here failed.

   b. Once the Profiler API was fixed OpenCover behavior regarding symbols, prevented it from instrumenting the methods in S.P.C.dll. OpenCover (current sources and version 4.6.519) bails on any assembly if it finds a pdb with same name and it fails to load it, i.e.: on firt mismatch it doesn't look in extra search paths (not typical symbol search algo). In this case S.P.C.pdb was for the IL version, so it didn't match S.P.C.dll.

2. Around 10/2017 pdbs became portable by default and since version 4.6.519 of OpenCover doesn't support portable PDBs a workaround was put in place to generate respective WindowsPDB. However, the workaround just target AssemblyBeingTestedName not any additional assemblies, like S.P.C.dll (this is a simple fix on CodeCoverage.targets).

One related issue that tripped me in the beginning of my investigation was the fact that current master sources of OpenCover w/ latest Mono.Cecil (that supports portable PDBs) failed with an unchanged CoreFx repo due to a bug (already fixed) in Mono.Cecil that prevented it from working with WindowsPDBs.

In summary: at the present, to make CoreLib work with OpenCover we can use either the native or IL S.P.C.dll, but, the S.P.C.pdb first found by OpenCover must match the version being loaded.

[stephentoub]

Great summary, thanks.

[pjanotti]

In my aspiration to avoid replacing native image CoreLib dll (more stable and less risky for dev boxes). I didn't notice something very important: using native image and fixing the PDB (needed due to OpenCover issue) doesn't get source information on the report for CoreLib. Worst, the way that I changed CodeCoverage.targets requires hacks to get source information for CoreLib on dev boxes. I expected source information for CoreLib to be broken on CI at least for the moment, but the intention was to have it working easily on devbox with CoreClrOverridePath... back to fixing this...

[pjanotti]

I'm working on having a dedicated folder for coverage with only IL to get source/line report for CoreLib. In the short run when running against CoreClr from a package (e.g.: CI) report will generate some messages like:

  File 'E:\A\_work\104\s\src\System.Private.CoreLib\src\System\Runtime\InteropServices\WindowsRuntime\IteratorToEnumeratorAdapter.cs' does not exist (any more).
   Creating report 770/1105 (Assembly: System.Private.CoreLib, Class: System.Runtime.InteropServices.WindowsRuntime.IteratorToEnumeratorAdapter`1)
...
   Creating report 1104/1105 (Assembly: System.Private.CoreLib, Class: System.WeakReference`1)
   File 'E:\A\_work\104\s\src\System.Private.CoreLib\src\System\WeakReferenceOfT.cs' does not exist (any more).
   File 'E:\A\_work\104\s\src\System.Private.CoreLib\src\System\WeakReferenceOfT.cs' does not exist (any more).
   Creating report 1105/1105 (Assembly: System.Private.CoreLib, Class: Windows.Foundation.Diagnostics.TracingStatusChangedEventArgs)

but since it doesn't fail the build I'm treating those as acceptable for now (one can even properly map the sources and get source coverage info for corelib).

[pjanotti]

Right now CoreLib is back in CI but as mentioned above just the percentage numbers for Line and Branch coverage, that will be fixed via https://github.com/dotnet/buildtools/pull/2077 that runs against the IL version of CoreLib.

[pjanotti]

Daily coverage run has numbers for CoreLib, however, due to crashes it doesn't appear as the last coverage run. Closing this issue and following up on crashes and others in separate issues:

• Coverage crashes at https://github.com/dotnet/corefx/issues/29661
• Jump to source for CoreLib daily coverage runs is broken at https://github.com/dotnet/corefx/issues/30974

Some other related PRs:
https://github.com/dotnet/corefx/pull/30972
https://github.com/dotnet/corefx/pull/30967
https://github.com/dotnet/corefx/pull/30966
https://github.com/dotnet/buildtools/pull/2094

[ViktorHofer]

Reopening as System.Private.CoreLib numbers are busted again. With my recent work I overlooked some details and we use the native System.Private.CoreLib.dll again. I'll try to solve this.