Repository navigation
Conversation
Each evaluation of the same source URL (e.g. cache-busting
import("./mod?v=" + n)) creates a new JSC source provider with a new
sourceID, but the coverage byte-range map is keyed by sourceURL and
overwrote the entry on every re-import. Report generation then queried
the control flow profiler for only the last instance's sourceID, so
line and function hits from earlier instances were silently dropped
(lcov showed FNH:1 and DA:n,0 for functions exercised by passing tests).
Keep every sourceID recorded for a URL and, at report time, query the
profiler for each instance and union the basic-block data: a range
counts as executed if it executed in any instance, and execution counts
are summed.
Fixes #35345
Collaborator
Author
|
Updated 9:41 PM PT - Jul 23rd, 2026
❌ @robobun, your commit 94d6a3a has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35346That installs a local version of the PR into your bun-35346 --bun |
Contributor
WalkthroughChangesCross-instance coverage aggregation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…e collector The bun:jsc entry point still queried the control flow profiler for only the latest sourceID, so it kept the last-instance-wins behavior fixed for the test reporters. Move the profiler collection loop into a shared ByteRangeMapping::collect_blocks and have both Report::generate and ByteRangeMapping__findExecutedLines use it; the C++ side now just passes the source URL. Removes the now-unused sourceIDForSourceURL and ByteRangeMapping__getSourceID.
Contributor
There was a problem hiding this comment.
LGTM — the follow-up commit routes both Report::generate and bun:jsc codeCoverageForFile through the shared collect_blocks helper, removes the now-dead sourceIDForSourceURL/ByteRangeMapping__getSourceID, and adds a codeCoverageForFile assertion to the fixture, so my earlier concern is fully addressed.
What was reviewed:
- Single-instance path:
merge_duplicate_rangesis gated on!prior_source_ids.is_empty(), so block order/content is unchanged and the existing lcov snapshot tests stay byte-identical. - Multi-instance merge: sort+dedup by
(start_offset, end_offset)withhas_executed |=and saturating count sum;generate_report_from_blocksis order-agnostic (bitset/count-based), andBasicBlockRangeisCopyso the in-place compaction is sound. BlockCollector::collectapplies the per-instancefunction_blocks[1..]drop before extending, matching the previous per-sourceID behavior; the empty-blocks → empty-array return inByteRangeMapping__findExecutedLinespreserves the old C++ contract.- No remaining callers of the removed C++/Rust symbols;
CodeCoverage__withBlocksAndFunctionsC ABI unchanged (callback ctx isvoid*).
Extended reasoning...
Overview
The PR fixes #35345: when a module is re-imported with a cache-busting query string, each evaluation gets a fresh JSC SourceID, but the coverage ByteRangeMapping map (keyed by source URL) previously overwrote the entry, so only the last instance's profiler data was read. The fix adds prior_source_ids: Vec<i32> to ByteRangeMapping, carries earlier IDs forward in ByteRangeMapping__generate, and introduces a shared collect_blocks helper that queries CodeCoverage__withBlocksAndFunctions for every instance and unions identical byte ranges via merge_duplicate_ranges. Both consumers — Report::generate (the bun test --coverage reporters) and ByteRangeMapping__findExecutedLines (bun:jsc codeCoverageForFile) — now route through it. The C++ functionCodeCoverageForFile shrinks to a thin call into Rust, and the dead sourceIDForSourceURL / ByteRangeMapping__getSourceID pair is deleted along with the now-unused ControlFlowProfiler.h include.
Security risks
None. This is coverage-report generation over profiler data from JSC; no untrusted input parsing, no network/auth/crypto surface. The one unsafe block added (from_raw_parts in BlockCollector::collect) is guarded by the pre-existing blocks_len == 0 early return and mirrors the code it replaces.
Level of scrutiny
Moderate. The change touches Rust↔C++ FFI signatures and a thread-local map, but the FFI ABI is unchanged (CodeCoverage__withBlocksAndFunctions still takes void* ctx and the same callback shape; the Rust-side type rename from Generator to BlockCollector is invisible to C++). The critical single-instance path is guaranteed unchanged because the merge/sort step is skipped when prior_source_ids is empty — confirmed by the existing lcov inline-snapshot tests still passing. The multi-instance path is new and was previously broken outright, so there is no regression surface there.
Other factors
- My earlier review flagged that
bun:jsccodeCoverageForFilestill read only the latestsource_id; commit 75900d4 addressed this exactly as suggested (shared helper, C++ passes only the URL, dead code deleted, test assertion added for the sibling path). That thread is resolved. - I checked
merge_duplicate_rangesagainstgenerate_report_from_blocks: the downstream code sizesstmts_which_have_executed/functions_which_have_executedfrom the (post-merge) slice length and only ever consumes them via.count(), so reordering/compaction is safe.BasicBlockRangederivesCopy, soranges[out] = ranges[i]is a plain copy. BlockCollector::collectdrops the leading function block per callback invocation (i.e. per instance), matching the previous per-sourceID slicing in both the oldGenerator::do_and the old C++/RustfindExecutedLinespaths.- Grep confirms no remaining references to
sourceIDForSourceURLorByteRangeMapping__getSourceIDanywhere insrc/. - The
map.insertoverwrite of the oldByteRangeMapping(and itssource_url) is pre-existing behavior; this PR only adds amem::takeof the oldprior_source_idsbefore the overwrite, which is leak-neutral. - Evidence in the PR body shows the new test failing on both ASAN-debug and release builds without the fix and passing with it, and the full 13-test
coverage.test.tssuite green.
Collaborator
Author
|
CI status: the diff itself is green. The remaining failures are unrelated to this change:
The coverage changes are covered by test/cli/test/coverage.test.ts, which passes on every lane. Ready for review. |
Jarred-Sumner
added a commit
that referenced
this pull request
Aug 15, 2026
…++ (#38943) ### Problem - Rust and C++ each spell out bun's internal C-ABI functions by hand, and the two copies only meet in the linker, which matches names, not signatures. 13 symbols are spelled differently on the two sides. Each works by accident of the x64/arm64 calling conventions (an extra argument lands in a register the callee never reads, a 32-bit declaration reads the low half of a 64-bit return) and is undefined behaviour on the Rust side. All of them predate the Rust port (checked against the removed Zig sources). - Parameter count: `Bun__JSWrappingFunction__create` (Rust passes a 5th `strong` argument, `JSWrappingFunction.cpp:57` takes 4), `ByteRangeMapping__getSourceID` (`ZigSourceProvider.cpp:43` passes a 2nd `BunString`, `CodeCoverage.rs:845` takes 1), `ffi_vfprintf` / `ffi_vprintf` / `ffi_vsscanf` (declared variadic in `ffi_body.rs`, defined with a `va_list` parameter in `c-bindings.cpp`). - Return width: `URL__originLength` (`url/lib.rs:74` says `u32`, `BunString.cpp:570` returns `size_t`); `Bun__setExitCode`, `Bun__closeChildIPC`, `Bun__ensureProcessIPCInitialized` (`BunProcess.cpp`), `Bun__setTLSRejectUnauthorizedValue`, `Bun__setVerboseFetchValue` (`JSEnvironmentVariableMap.cpp`) declared with a scalar return in C++ while the Rust definitions return nothing; `Bun__reportUnhandledError` returns a constant `undefined` that `ZigGlobalObject.h:90` declares as `void`; `WebCore__AbortSignal__signal` returns its argument, which the Rust declaration (void) never reads. ### Fix - Makes each pair agree, on whichever side carries information: - the phantom `strong` / `sourceURL` arguments are dropped (C++ never read `strong`; Rust never read `sourceURL`, and the `Bun::toString` that built it is a non-owning view, so nothing was leaked or needs releasing); - the three `ffi_v*` declarations get a `va_list` parameter, spelled as an opaque pointer (only their addresses are taken, for TinyCC; on every target bun builds for a `va_list` argument travels as one pointer-sized value); - `URL__originLength` becomes `usize`, and the `as usize` at its only call site goes away; - the five C++ declarations of void Rust functions become `void` (every C++ caller already discards the value); - `report_unhandled_error` stops returning its constant (no Rust callers; the C++ declaration and all eight C++ callers already treat it as void); - `WebCore__AbortSignal__signal` returns void in `bindings.cpp` and `headers.h` (no C++ callers; the Rust declaration was already void). - No behaviour changes: every call site either ignored the dropped value or never passed anything the callee read. Verified with `bun bd` (the regenerated `Bun__reportUnhandledError` thunk is now `-> ()`) and `bun bd test` on `test/js/bun/test/expect-extend*.test.*` and `jest-extended.test.js` (JSWrappingFunction), `test/cli/test/coverage.test.ts` plus a manual `bun:jsc` `codeCoverageForFile` run (`ByteRangeMapping__getSourceID`), `test/js/node/process/process.test.js`, `test/js/web/abort/abort.test.ts`, `test/js/bun/spawn/spawn.ipc.test.ts`, `test/js/node/child_process/child_process_ipc.test.js`, `test/js/node/events/event-emitter.test.ts`, `test/js/node/timers/node-timers.test.ts`, `test/js/web/fetch/fetch.tls.test.ts`. `cargo fmt --check` and clang-format are clean. - There is no test: these are declaration-only corrections with nothing observable at runtime. The source lint that found them was part of the first revision and was removed from the PR by @Jarred-Sumner (31377e1); its output is kept below for the record. - Related, not duplicated: #35346 removes `ByteRangeMapping__getSourceID` altogether as part of a larger coverage change; this PR only corrects its declaration. ### Background - `extern "C"` linkage: a Rust `extern "C" { fn X(..); }` item (or `#[unsafe(no_mangle)] extern "C" fn X` definition) and a C++ `extern "C"` declaration or definition are matched by the linker purely by the name `X`; each compiler generates its calls and prologues from its own local copy of the signature, so the copies can disagree without any diagnostic. - `HOST_EXPORT`: a `// HOST_EXPORT(Sym)` comment above a safe Rust fn makes `src/codegen/generate-host-exports.ts` emit the `#[unsafe(no_mangle)]` thunk for `Sym` with the impl's parameters and return type, which is why changing `report_unhandled_error`'s Rust signature is what changes the exported symbol's. - `headers.h` spells `extern "C"` as `CPP_DECL`; `bindings.cpp` includes it, so its `WebCore__AbortSignal__signal` line has to change together with the definition. <details> <summary>How the 13 were found (lint output from the first revision; the lint itself is no longer in this PR)</summary> ``` (fail) every extern "C" symbol is declared with the same parameter count at every site Bun__JSWrappingFunction__create rust src/runtime/test_runner/expect.rs:3253: 5 params, returns JSValue c++ src/jsc/bindings/JSWrappingFunction.cpp:57: 4 params, returns JSC::EncodedJSValue ByteRangeMapping__getSourceID rust src/sourcemap_jsc/CodeCoverage.rs:845: 1 params, returns i32 c++ src/jsc/bindings/ZigSourceProvider.cpp:43: 2 params, returns int ffi_vfprintf rust src/runtime/ffi/ffi_body.rs:322: 2+... params, returns c_int c++ src/jsc/bindings/c-bindings.cpp:822: 3 params, returns int ffi_vprintf rust src/runtime/ffi/ffi_body.rs:323: 1+... params, returns c_int c++ src/jsc/bindings/c-bindings.cpp:815: 2 params, returns int ffi_vsscanf rust src/runtime/ffi/ffi_body.rs:329: 2+... params, returns c_int c++ src/jsc/bindings/c-bindings.cpp:867: 3 params, returns int (fail) every extern "C" symbol is declared with the same return width at every site Bun__closeChildIPC rust src/runtime/hw_exports.rs:171: 1 params, returns void c++ src/jsc/bindings/BunProcess.cpp:175: 1 params, returns bool Bun__ensureProcessIPCInitialized rust src/runtime/ipc_host.rs:602: 1 params, returns void c++ src/jsc/bindings/BunProcess.cpp:179: 1 params, returns bool Bun__reportUnhandledError rust src/jsc/virtual_machine_exports.rs:75: 2 params, returns JSValue c++ src/jsc/bindings/ZigGlobalObject.h:90: 2 params, returns void Bun__setExitCode rust src/jsc/VirtualMachine.rs:592: 2 params, returns void c++ src/jsc/bindings/BunProcess.cpp:174: 2 params, returns uint8_t Bun__setTLSRejectUnauthorizedValue rust src/jsc/virtual_machine_exports.rs:196: 1 params, returns void c++ src/jsc/bindings/JSEnvironmentVariableMap.cpp:355: 1 params, returns int Bun__setVerboseFetchValue rust src/jsc/virtual_machine_exports.rs:241: 1 params, returns void c++ src/jsc/bindings/JSEnvironmentVariableMap.cpp:357: 1 params, returns int URL__originLength rust src/url/lib.rs:74: 2 params, returns u32 c++ src/jsc/bindings/BunString.cpp:570: 2 params, returns size_t WebCore__AbortSignal__signal rust src/jsc/AbortSignal.rs:57: 3 params, returns void c++ src/jsc/bindings/bindings.cpp:5969: 3 params, returns WebCore::AbortSignal* c++ src/jsc/bindings/headers.h:127: 3 params, returns WebCore::AbortSignal* ``` The lint compared parameter counts and return widths of every hand-written `extern "C"` site on both sides (1342 symbols declared in both languages); these 13 were the only disagreements in the tree. </details> --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
Collaborator
Author
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-24, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. The linked issue (#35345) stays open. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
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
1 participant
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.
Fixes #35345
Repro
A per-test fresh module instance via cache-busting query imports loses coverage for every instance except the last:
bun test --coverage --coverage-reporter=lcovreportedFNF:2 FNH:1andDA:n,0forfnA's body even though a passing test executed it. Swapping test order swapped which function read 0.Cause
Each evaluation of the same source URL creates a new JSC source provider with a new sourceID, but the coverage byte-range map (
src/sourcemap_jsc/CodeCoverage.rs) is keyed by sourceURL andByteRangeMapping__generateoverwrote the entry on every re-import.Report::generatethen queried the control flow profiler for only the surviving (last) sourceID, so basic-block hits from earlier instances were never read. This affected all reporters (lcov DA/FNH, the text table) identically.Fix
ByteRangeMappingkeeps the sourceIDs of earlier instances (prior_source_ids) when an entry for the same URL is regenerated;source_idstays the latest, preserving the existing lookup behavior ofByteRangeMapping__find/sourceIDForSourceURL.Report::generatequeries the profiler for every instance's sourceID, collects the basic-block and function ranges, and unions per byte range: a range counts as executed if it executed in any instance, and execution counts are summed. The source text is identical across instances, so ranges line up byte-for-byte and one line-offset table serves all of them.Verification
New test in
test/cli/test/coverage.test.ts(two instances, each exercising a different function; assertsFNH:2, noDA:n,0, andLH == LF). Fails on the current release withFNH:1and zeroed DA lines, passes with this change. The fullcoverage.test.tssuite (13 tests, including lcov snapshots) passes.[review] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
When the same source file was imported multiple times, such as through cache-busting query imports, each new instance registered a fresh JSC source ID that overwrote the existing byte range mapping for that URL, so hit counters from earlier instances were discarded and the lcov report reflected only the last instance. The fix makes the mapping retain all prior source IDs for a URL, then collects coverage and function blocks across every instance and merges duplicate ranges before generating the report. As a result, line and function hits are aggregated across all instances of a file, so cod…