Repository navigation
[CI Visibility] Fix coverage rewriter and counter lifetime defects - #9663
Open
tonyredondo wants to merge 6 commits into
Open
tonyredondo wants to merge 6 commits into
tonyredondo wants to merge 6 commits into
Conversation
Contributor
Snapshots difference summaryThe following differences have been observed in committed snapshots. It is meant to help the reviewer. |
BenchmarksBenchmark execution time: 2026-11-12 11:52:31 Comparing candidate commit c061244 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 72 metrics, 0 unstable metrics, 6 known flaky benchmarks, 119 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9663) and master. ✅ No regressions detected |
tonyredondo
force-pushed
the
tony/coverage-rewriter-fixes
branch
from
November 10, 2026 14:43
470f9ad to
49727d7
Compare
tonyredondo
marked this pull request as ready for review
November 10, 2026 15:59
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49727d7553
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![datadog-prod-us1-3[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
More details
Safety of retained counter buffers and rewritten IL was assessed through static inspection only; runtime tests were not executed.
🤖 Bits Code Review · Commit 49727d7
Contributor
There was a problem hiding this comment.
More details
Static review found no reportable defects in the probe-placement, buffer-lifetime, or bitmap changes. Buffer reuse deliberately permits over-coverage from late writes while keeping those writes in coverage-owned memory.
🤖 Bits Code Review · Commit a8f1020
Instrument each sequence point against its own document, keep lines whose earlier sequence points can be bypassed, use the counters local when a branch targets the first instruction, ignore line 0 and method-level [ExcludeFromCodeCoverage] methods, recycle context buffers instead of freeing them, keep probe failures out of customer code, and guard AVX2 and out-of-range bitmap access. Add a C#/VB corpus test that rewrites compiler output and checks JIT acceptance and reported lines.
…discard counters lazily
tonyredondo
force-pushed
the
tony/coverage-rewriter-fixes
branch
from
November 10, 2026 17:41
a8f1020 to
c9ed430
Compare
Contributor
There was a problem hiding this comment.
Cross-module buffer reuse can invalidate an entire global coverage artifact after a late write. Probe-acquisition failures can also leave partial per-test dependencies eligible for publication, risking incorrect test skipping.
🤖 Bits Code Review · Commit c9ed430
…ge after probe failures
Contributor
There was a problem hiding this comment.
More details
The PR repairs coverage rewriting, bitmap bounds, AVX2 handling, and counter-buffer lifetime. Both earlier findings are resolved: capture filters non-executable late writes, preventing global artifact rejection, and probe failures—including cached reporter-initialization failures—suppress incomplete per-test payloads, protecting test dependencies; no findings remain open.
🤖 Bits Code Review · Commit c061244
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Fix eight problems in CI Visibility code coverage found while reviewing #9658/#9661, and add a corpus test that rewrites real compiler output so this class of bug is caught in CI. Follows #9661, which is already merged.
Rewriter (
AssemblyProcessor):let rec f () = f ()) no longer produces invalid IL (InvalidProgramException).if (x == null) throw ...;,if (a) return 1; return 2;in Release, F# one-lineif/else). A sequence point is skipped only when every path to it goes through the previous one on the same line.partialfile,#linein Razor or SpecFlow) report each line in its own document, and source-file exclusions apply per document.#ExternalSource(..., 0)) are ignored instead of making the rewriter write outside the line bitmap (AccessViolationException).[ExcludeFromCodeCoverage]excludes the method again.Runtime (
Datadog.Trace):ExecutionContext.Runwith another context could keep writing after the session freed its buffer (heap corruption). A late write now lands in coverage-owned memory, at worst marking a line in a later test that reuses the buffer.GetFileCounterno longer throws into customer code (finalizers, type initializers) when the counters cannot be acquired. The global coverage and the current test's coverage are marked incomplete, the test reports no per-test coverage (as an aborted session does), and the probe writes into a per-module discard buffer.FileBitmapchecksAvx2.IsSupportedbefore using AVX2 integer operations, and ignores lines outside the bitmap. The second fix also coversgit diff -U0hunks+0,0, which madeGitCommandHelpercallSet(0).Reason for change
All of these produce invalid IL, crash the process or silently report wrong coverage. Missing lines and files reach per-test coverage, which Test Impact Analysis relies on.
Implementation details
NativeBufferPool.ModuleValuekeeps one instance for byte counters (line execution) and one for int counters (call counts), so a buffer is only reused for the same counter layout: a late write can then only add to a later owner's counters, apart from the wraparound 32-bit call counts already have. Each pool holds at most the peak number of concurrently live buffers of each size. Buffers are zeroed on reuse and never returned to the OS. Returning a buffer never throws: if the bookkeeping cannot be allocated, the block is left allocated. Global fallback buffers keep the existing lifetime.GetFileCounterrethrows as before. The module's coverage layout is initialized on first use insideGetFileCounter, so even a cached initialization failure marks every affected test incomplete. Invalid generated metadata still fails the reporter's type initializer, asInvalidReporterMetadataSuppressesGlobalOutputexpects.CoverageRewriteTestssnapshots gain one line. Thei++of aforheader is now also instrumented because its previous sequence point is on another line. Line counters are only consumed as executed or not executed, so results don't change.Test coverage
CoverageRewriterCorpusTestscompiles a C# and a Visual Basic corpus with Roslyn at test time (throughTestAssemblyCompiler, shared with the C# finalizer fixture from [CI Visibility] Fix coverage instrumentation of expression-bodied finalizers #9661), in Debug and Release, and rewrites it in both coverage modes. It then checks that the JIT accepts every rewritten method, that lines marked// executed:are reported, that lines marked// not-covered:are not, and that every reported line belongs to the document it is reported in. New constructs can be added to the corpus.BackwardBranchCoverageTests: the F# loop shape (one visible sequence point that a branch jumps back to), produced from C# in an optimized build with#line hidden. The test checks that the compiled fixture still has that shape before rewriting it.NativeBufferPoolTests: each test uses its own pool. They cover zeroed allocation, reuse of a returned buffer of the same size (zeroed again), separation by size, the counters, rejection of negative sizes, concurrent renters never sharing a buffer, andReturnkeeping the buffer allocated when the pool cannot record it.ModuleValueTests: context buffers go back to the pool onDisposeand from the finalizer, are not reused across counter widths, global fallback buffers are not pooled, and new buffers start zeroed.CoverageEventHandlerTests: a buffer released while a frame is inside anotherExecutionContextstays owned by coverage. A failed counter acquisition does not throw and allocates the discard buffer, later failures reuse it, and successful probes never allocate it. A session with a failed probe, including a cached reporter initialization failure, reports no coverage, and the next session reports normally.ModuleCoverageDataTests: counters on non-executable lines are not reported, in both coverage modes.FileBitmapBoundsTests: out-of-range lines are neither written nor read.Each new test fails without its fix, and each deliberate break of
NativeBufferPool(no zeroing, freeing instead of pooling, a throwingReturn, handing out a buffer without removing it, missing counter updates) makes at least one test fail. Corpus probes miss or misplace their lines, the VB corpus crashes the test host withAccessViolationException, the backward-branch case throwsInvalidProgramException, and the pool test aborts the test host withmalloc(): unaligned tcache chunk detected.Locally, on Linux x64 with SDK 11.0.100-rc.2,
Datadog.Trace.Tools.Runner.Testspasses on netcoreapp2.1 through net11.0, with older runtimes in the official SDK containers. The coverage tests inDatadog.Trace.Testspass on netcoreapp2.1, netcoreapp3.1 and net8.0, and the existingFileBitmapTestspass with and withoutDOTNET_EnableAVX2=0. The AVX2 guard has no automated test because CI machines support AVX2; it was verified withDOTNET_EnableAVX2=0. Windows, .NET Framework, macOS and arm64 rely on CI.Other details
Adds
Microsoft.CodeAnalysis.VisualBasic4.13.0 toDatadog.Trace.Tools.Runner.Tests, next to theMicrosoft.CodeAnalysis.CSharpreference added in #9661. No public API or configuration changes.