diff --git a/src/jsc/bindings/ZigSourceProvider.cpp b/src/jsc/bindings/ZigSourceProvider.cpp index fa3778b481d1..315e44ef6f6f 100644 --- a/src/jsc/bindings/ZigSourceProvider.cpp +++ b/src/jsc/bindings/ZigSourceProvider.cpp @@ -48,7 +48,6 @@ SourceOrigin toSourceOrigin(const String& sourceURL, bool isBuiltin) return SourceOrigin(WTF::URL::fileURLWithFileSystemPath(sourceURL)); } -extern "C" int ByteRangeMapping__getSourceID(void* mappings, BunString sourceURL); extern "C" void* ByteRangeMapping__find(BunString sourceURL); void* sourceMappingForSourceURL(const WTF::String& sourceURL) { @@ -57,16 +56,6 @@ void* sourceMappingForSourceURL(const WTF::String& sourceURL) extern "C" void ByteRangeMapping__generate(BunString sourceURL, BunString code, int sourceID); -JSC::SourceID sourceIDForSourceURL(const WTF::String& sourceURL) -{ - void* mappings = ByteRangeMapping__find(Bun::toString(sourceURL)); - if (!mappings) { - return 0; - } - - return ByteRangeMapping__getSourceID(mappings, Bun::toString(sourceURL)); -} - extern "C" bool BunTest__shouldGenerateCodeCoverage(BunString sourceURL); extern "C" void Bun__addSourceProviderSourceMap(void* bun_vm, SourceProvider* opaque_source_provider, BunString* specifier); extern "C" void Bun__removeSourceProviderSourceMap(void* bun_vm, SourceProvider* opaque_source_provider, BunString* specifier); diff --git a/src/jsc/bindings/ZigSourceProvider.h b/src/jsc/bindings/ZigSourceProvider.h index ccbb659c734e..b08795dc5542 100644 --- a/src/jsc/bindings/ZigSourceProvider.h +++ b/src/jsc/bindings/ZigSourceProvider.h @@ -21,7 +21,6 @@ namespace Zig { class GlobalObject; void forEachSourceProvider(WTF::Function); -JSC::SourceID sourceIDForSourceURL(const WTF::String& sourceURL); void* sourceMappingForSourceURL(const WTF::String& sourceURL); JSC::SourceOrigin toSourceOrigin(const String& sourceURL, bool isBuiltin); class SourceProvider final : public JSC::SourceProvider { diff --git a/src/jsc/modules/BunJSCModule.h b/src/jsc/modules/BunJSCModule.h index 816f6efe2a3e..647942a434cb 100644 --- a/src/jsc/modules/BunJSCModule.h +++ b/src/jsc/modules/BunJSCModule.h @@ -49,8 +49,6 @@ extern "C" char* mi_stats_get_json(size_t, char*); extern "C" char* mi_heap_dump_json(bool include_blocks, bool hash_addresses); -#include - #if OS(DARWIN) #if ASSERT_ENABLED #if !__has_feature(address_sanitizer) @@ -878,8 +876,7 @@ JSC_DEFINE_HOST_FUNCTION(functionDeserialize, (JSGlobalObject * globalObject, Ca } extern "C" JSC::EncodedJSValue ByteRangeMapping__findExecutedLines( - JSC::JSGlobalObject*, BunString sourceURL, BasicBlockRange* ranges, - size_t len, size_t functionOffset, bool ignoreSourceMap); + JSC::JSGlobalObject*, BunString sourceURL, bool ignoreSourceMap); JSC_DEFINE_HOST_FUNCTION(functionCodeCoverageForFile, (JSGlobalObject * globalObject, @@ -892,41 +889,16 @@ JSC_DEFINE_HOST_FUNCTION(functionCodeCoverageForFile, RETURN_IF_EXCEPTION(throwScope, {}); bool ignoreSourceMap = callFrame->argument(1).toBoolean(globalObject); - auto sourceID = Zig::sourceIDForSourceURL(fileName); - if (!sourceID) { + JSValue result = JSValue::decode(ByteRangeMapping__findExecutedLines( + globalObject, Bun::toString(fileName), ignoreSourceMap)); + RETURN_IF_EXCEPTION(throwScope, {}); + if (result.isNull()) { throwException(globalObject, throwScope, createError(globalObject, "No source for file"_s)); return {}; } - auto basicBlocks = vm.controlFlowProfiler()->getBasicBlocksForSourceIDWithoutFunctionRange( - sourceID, vm); - - if (basicBlocks.isEmpty()) { - return JSC::JSValue::encode( - JSC::constructEmptyArray(globalObject, nullptr, 0)); - } - - size_t functionStartOffset = basicBlocks.size(); - - const Vector>& functionRanges = vm.functionHasExecutedCache()->getFunctionRanges(sourceID); - - basicBlocks.reserveCapacity(functionRanges.size() + basicBlocks.size()); - - for (const auto& functionRange : functionRanges) { - BasicBlockRange range; - range.m_hasExecuted = std::get<0>(functionRange); - range.m_startOffset = static_cast(std::get<1>(functionRange)); - range.m_endOffset = static_cast(std::get<2>(functionRange)); - range.m_executionCount = range.m_hasExecuted - ? 1 - : 0; // This is a hack. We don't actually count this. - basicBlocks.append(range); - } - - return ByteRangeMapping__findExecutedLines( - globalObject, Bun::toString(fileName), basicBlocks.begin(), - basicBlocks.size(), functionStartOffset, ignoreSourceMap); + return JSValue::encode(result); } JSC_DEFINE_HOST_FUNCTION(functionEstimateDirectMemoryUsageOf, (JSGlobalObject * globalObject, CallFrame* callFrame)) diff --git a/src/sourcemap_jsc/CodeCoverage.rs b/src/sourcemap_jsc/CodeCoverage.rs index 0aca9bcbc11d..97b55e165ae7 100644 --- a/src/sourcemap_jsc/CodeCoverage.rs +++ b/src/sourcemap_jsc/CodeCoverage.rs @@ -87,30 +87,24 @@ impl Report { // functionHasExecutedCache), so we must preserve write provenance. let vm = global_this.vm_ptr(); - let mut result: Option = None; - - let mut generator = Generator { - result: &mut result, - byte_range_mapping, - }; - - // SAFETY: `vm` is the live `*mut VM` owning `global_this`; Generator and the - // callback are kept alive for the duration of the FFI call; - // CodeCoverage__withBlocksAndFunctions invokes the callback synchronously. - let ok = unsafe { - CodeCoverage__withBlocksAndFunctions( - vm, - generator.byte_range_mapping.source_id, - (&raw mut generator).cast::(), - ignore_sourcemap_, - Generator::do_, - ) - }; - if !ok { + let collector = byte_range_mapping.collect_blocks(vm, ignore_sourcemap_)?; + if collector.blocks.is_empty() { return None; } - result + // No ownership transfer here: + // `from_utf8_never_free` already detaches the lifetime by design, and + // `generate_report_from_blocks` only borrows `&self`, so no &/&mut overlap. + let source_url = + ZigStringSlice::from_utf8_never_free(byte_range_mapping.source_url.slice()); + byte_range_mapping + .generate_report_from_blocks( + source_url, + &collector.blocks, + &collector.function_blocks, + ignore_sourcemap_, + ) + .ok() } } @@ -360,25 +354,26 @@ unsafe extern "C" { source_id: i32, ctx: *mut c_void, ignore_sourcemap: bool, - cb: extern "C" fn(*mut Generator, *const BasicBlockRange, usize, usize, bool), + cb: extern "C" fn(*mut BlockCollector, *const BasicBlockRange, usize, usize, bool), ) -> bool; } -struct Generator<'a> { - byte_range_mapping: &'a mut ByteRangeMapping, - result: &'a mut Option, +struct BlockCollector { + blocks: Vec, + function_blocks: Vec, } -impl<'a> Generator<'a> { - extern "C" fn do_( - this: *mut Generator, +impl BlockCollector { + extern "C" fn collect( + this: *mut BlockCollector, blocks_ptr: *const BasicBlockRange, blocks_len: usize, function_start_offset: usize, - ignore_sourcemap: bool, + _ignore_sourcemap: bool, ) { - // SAFETY: `this` was passed as &mut Generator to CodeCoverage__withBlocksAndFunctions - // and is valid for the duration of this synchronous callback. + // SAFETY: `this` was passed as &mut BlockCollector to + // CodeCoverage__withBlocksAndFunctions and is valid for the duration of this + // synchronous callback. let this = unsafe { &mut *this }; // The C++ side (CodeCoverage.cpp) invokes this callback with `(nullptr, 0, 0)` when // basicBlocks is empty. `core::slice::from_raw_parts` requires a non-null, aligned @@ -395,20 +390,34 @@ impl<'a> Generator<'a> { function_blocks = &function_blocks[1..]; } - if blocks.is_empty() { - return; - } + this.blocks.extend_from_slice(blocks); + this.function_blocks.extend_from_slice(function_blocks); + } +} - // No ownership transfer here: - // `from_utf8_never_free` already detaches the lifetime by design, and - // `generate_report_from_blocks` only borrows `&self`, so no &/&mut overlap. - let source_url = - ZigStringSlice::from_utf8_never_free(this.byte_range_mapping.source_url.slice()); - *this.result = this - .byte_range_mapping - .generate_report_from_blocks(source_url, blocks, function_blocks, ignore_sourcemap) - .ok(); +/// Union coverage data for identical byte ranges collected from multiple +/// instances of the same source: a range counts as executed if it executed in +/// any instance, and hit counts are summed. +fn merge_duplicate_ranges(ranges: &mut Vec) { + if ranges.len() < 2 { + return; } + ranges.sort_unstable_by_key(|r| (r.start_offset, r.end_offset)); + let mut out: usize = 0; + for i in 1..ranges.len() { + if ranges[i].start_offset == ranges[out].start_offset + && ranges[i].end_offset == ranges[out].end_offset + { + ranges[out].has_executed |= ranges[i].has_executed; + ranges[out].execution_count = ranges[out] + .execution_count + .saturating_add(ranges[i].execution_count); + } else { + out += 1; + ranges[out] = ranges[i]; + } + } + ranges.truncate(out + 1); } #[repr(C)] @@ -422,7 +431,12 @@ pub struct BasicBlockRange { pub struct ByteRangeMapping { pub line_offset_table: line_offset_table::List, + /// The most recent JSC source ID for this source URL. pub source_id: i32, + /// Source IDs of earlier instances of the same URL (one per extra + /// evaluation, e.g. cache-busting query-string re-imports). Coverage is + /// unioned across all of them. + pub prior_source_ids: Vec, pub source_url: ZigStringSlice, } @@ -480,6 +494,50 @@ impl ByteRangeMapping { thread_map_opt() } + /// Collect the profiler's basic-block and function ranges for every + /// instance of this source URL. A URL has multiple JSC source IDs when the + /// same file is evaluated more than once (e.g. cache-busting + /// `import("./mod?v=" + n)`); the per-instance data is unioned so earlier + /// instances' hits are not lost. + fn collect_blocks(&self, vm: *mut VM, ignore_sourcemap: bool) -> Option { + let mut collector = BlockCollector { + blocks: Vec::new(), + function_blocks: Vec::new(), + }; + + for source_id in self + .prior_source_ids + .iter() + .copied() + .chain(core::iter::once(self.source_id)) + { + // SAFETY: `vm` is the caller's live `*mut VM`; the collector and the + // callback are kept alive for the duration of the FFI call; + // CodeCoverage__withBlocksAndFunctions invokes the callback synchronously. + let ok = unsafe { + CodeCoverage__withBlocksAndFunctions( + vm, + source_id, + (&raw mut collector).cast::(), + ignore_sourcemap, + BlockCollector::collect, + ) + }; + if !ok { + return None; + } + } + + if !self.prior_source_ids.is_empty() { + // The source text is identical across instances, so ranges line up + // byte-for-byte; union the per-instance data per range. + merge_duplicate_ranges(&mut collector.blocks); + merge_duplicate_ranges(&mut collector.function_blocks); + } + + Some(collector) + } + pub fn generate_report_from_blocks( &self, source_url: ZigStringSlice, @@ -829,6 +887,7 @@ impl ByteRangeMapping { line_offset_table: LineOffsetTable::generate(source_contents, 0) .unwrap_or_else(|_| bun_alloc::out_of_memory()), source_id, + prior_source_ids: Vec::new(), source_url, } } @@ -852,17 +911,21 @@ pub(crate) extern "C" fn ByteRangeMapping__generate( let hash = bun_wyhash::hash(slice.slice()); let source_contents = source_contents_str.to_utf8(); - let new_value = ByteRangeMapping::compute(source_contents.slice(), source_id, slice); + let mut new_value = ByteRangeMapping::compute(source_contents.slice(), source_id, slice); + // Re-evaluating the same URL (e.g. `import("./mod?v=" + n)`) creates a new + // JSC source ID; keep the old ones so report generation can union coverage + // across every instance instead of only the latest. + if let Some(old) = map.get_mut(&hash) { + new_value.prior_source_ids = core::mem::take(&mut old.prior_source_ids); + if old.source_id != source_id { + new_value.prior_source_ids.push(old.source_id); + } + } map.insert(hash, new_value); // `source_contents` drops here (matches `defer source_contents.deinit()`). // Note: `slice` ownership transferred into the new ByteRangeMapping.source_url. } -#[unsafe(no_mangle)] -pub(crate) extern "C" fn ByteRangeMapping__getSourceID(this: &ByteRangeMapping) -> i32 { - this.source_id -} - #[unsafe(no_mangle)] pub(crate) extern "C" fn ByteRangeMapping__find( path: bun_core::String, @@ -881,9 +944,6 @@ pub(crate) extern "C" fn ByteRangeMapping__find( pub(crate) extern "C" fn ByteRangeMapping__findExecutedLines( global_this: &JSGlobalObject, source_url: bun_core::String, - blocks_ptr: NonNull, - blocks_len: usize, - function_start_offset: usize, ignore_sourcemap: bool, ) -> JSValue { let Some(this_ptr) = ByteRangeMapping__find(source_url.clone()) else { @@ -892,18 +952,21 @@ pub(crate) extern "C" fn ByteRangeMapping__findExecutedLines( // SAFETY: pointer into the thread-local map, valid for this call. let this = unsafe { &*this_ptr.as_ptr() }; - // SAFETY: blocks_ptr[0..blocks_len] is a valid contiguous C array from JSC. - let all = unsafe { core::slice::from_raw_parts(blocks_ptr.as_ptr(), blocks_len) }; - let blocks: &[BasicBlockRange] = &all[0..function_start_offset]; - let mut function_blocks: &[BasicBlockRange] = &all[function_start_offset..blocks_len]; - if function_blocks.len() > 1 { - function_blocks = &function_blocks[1..]; + let Some(collector) = this.collect_blocks(global_this.vm_ptr(), ignore_sourcemap) else { + return JSValue::NULL; + }; + if collector.blocks.is_empty() { + return match JSValue::create_empty_array(global_this, 0) { + Ok(v) => v, + Err(_) => JSValue::ZERO, + }; } + let url_slice = source_url.to_utf8(); let report = match this.generate_report_from_blocks( url_slice, - blocks, - function_blocks, + &collector.blocks, + &collector.function_blocks, ignore_sourcemap, ) { Ok(r) => r, diff --git a/test/cli/test/coverage.test.ts b/test/cli/test/coverage.test.ts index 26a1ed10b320..edb3d71b531d 100644 --- a/test/cli/test/coverage.test.ts +++ b/test/cli/test/coverage.test.ts @@ -589,3 +589,72 @@ Ran 1 test across 1 file." `); expect(result.exitCode).toBe(0); }); + +// https://github.com/oven-sh/bun/issues/35345 +test("coverage is unioned across re-imported instances of the same module", () => { + const dir = tempDirWithFiles("cov", { + "qs-target.ts": ` +export function fnA(x: number): number { + const a = x + 1; + return a * 2; +} + +export function fnB(x: number): number { + const b = x + 10; + return b * 3; +} +`, + "qs-repro.test.ts": ` +import { expect, test } from "bun:test"; + +let n = 0; +async function load() { + return import(\`./qs-target?bun-test=\${++n}\`); +} + +test("instance 1 calls only fnA", async () => { + const { fnA } = await load(); + expect(fnA(1)).toBe(4); +}); + +test("instance 2 calls only fnB", async () => { + const { fnB } = await load(); + expect(fnB(1)).toBe(33); +}); + +test("bun:jsc codeCoverageForFile sees both instances", () => { + const { codeCoverageForFile } = require("bun:jsc"); + const report: string = codeCoverageForFile(require("path").join(import.meta.dir, "qs-target.ts")); + const m = report.match(/\\|\\s*([\\d.]+)\\s*\\|\\s*([\\d.]+)\\s*\\|(.*)$/); + expect(m).not.toBeNull(); + expect([m![1], m![2], m![3].trim()]).toEqual(["100.00", "100.00", ""]); +}); +`, + }); + + const result = Bun.spawnSync([bunExe(), "test", "--coverage", "--coverage-reporter", "lcov", "./qs-repro.test.ts"], { + cwd: dir, + env: { + ...bunEnv, + }, + stdio: ["inherit", "inherit", "inherit"], + }); + expect(result.exitCode).toBe(0); + + const lcov = readFileSync(path.join(dir, "coverage", "lcov.info"), "utf-8"); + const record = lcov.split("end_of_record").find(r => r.includes("qs-target.ts")); + expect(record).toBeDefined(); + + // Both functions executed, each in a different instance of the module. + expect(record).toContain("FNF:2"); + expect(record).toContain("FNH:2"); + + // Every executable line was hit in one of the two instances; no DA line + // may report 0 hits. + const zeroHitLines = [...record!.matchAll(/^DA:(\d+),0$/gm)].map(m => m[1]); + expect(zeroHitLines).toEqual([]); + + const lf = Number(record!.match(/^LF:(\d+)$/m)![1]); + const lh = Number(record!.match(/^LH:(\d+)$/m)![1]); + expect(lh).toBe(lf); +});