Bytecode cache: compile-time layout asserts (replacing the layout dump); canonical NaN constants - #521
Conversation
…build layout artifacts Each Docker-built job uploads its bytecode-cache-layout.txt as a small layout-<label> artifact next to its tarball. A new bytecode-cache-layout job downloads only those, diffs every build against linux-amd64, writes a per-build table (and the diffs) to the job summary, uploads the diffs on failure, and fails on any difference or missing dump. The release job depends on it, so a layout mismatch blocks the release; it no longer re-reads every tarball to get at the dumps. The artifact list moves to workflow-level env so both jobs share it.
|
Warning Review limit reached
On-demand reviews are free for the next 24 days. After that, they cost $0.25 per reviewed file. Or wait 42 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 84 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. Since it introduces a new job that gates the release pipeline and can only really be validated by running it, a quick human look is still worthwhile.
What was reviewed:
ARTIFACTShoist to workflowenv— content is byte-identical to the removed step-level list, and the release job'sDownload artifactsstep still reads it via$ARTIFACTS.- Per-job layout upload —
cpruns aftercd ${{runner.temp}}, so the upload path matches;if-no-files-found: errorcovers a missing dump; the nativewindowsjob is correctly left untouched and its label is skipped in the compare loop. - New job —
needscovers exactly the six jobs that uploadlayout-*;gh run download -D layoutsproduceslayouts/layout-<label>/bytecode-cache-layout.txt, matching the reference/compare paths;set -uo pipefail(no-e) is intentional sodiffand the&& continuetest don't abort the loop, withexit $statuscarrying the result; release download uses-n <name>so the extralayout-*artifacts don't interfere.
Extended reasoning...
Overview
This PR touches only .github/workflows/build-reusable.yml. It hoists the ARTIFACTS list from a step-level env in the release job to workflow-level env, adds a cp bun-webkit/bytecode-cache-layout.txt . plus a second upload-artifact step (layout-<label>, if-no-files-found: error) to each of the six Docker-based build jobs (linux, linux-musl, windows-cross, macos-cross, freebsd, linux-android), introduces a new bytecode-cache-layout job that downloads all layout-* artifacts and diffs them against linux-amd64, and wires release to needs: bytecode-cache-layout. The old disabled tarball-extraction comparison step in release is removed.
Security risks
None identified. All uses: references remain SHA-pinned to the same commits already in use. The new job requests only permissions: actions: read and authenticates gh with github.token. There is no interpolation of untrusted PR-controlled input into shell — the only expansions are ${{ github.token }}, ${{ github.repository }}, $GITHUB_RUN_ID, and the static ARTIFACTS list defined in the workflow itself. The step-summary content is derived from diff output of build-produced text files, not attacker input.
Level of scrutiny
Moderate. It is CI-only and does not touch JSC/WTF/bmalloc source, but it changes release gating: a logic error could either spuriously block every release or silently pass mismatched layouts. I traced the artifact path shape (gh run download --pattern 'layout-*' -D layouts → layouts/layout-<label>/bytecode-cache-layout.txt), confirmed the needs list matches exactly the jobs that now upload layout artifacts, confirmed bun-webkit-windows-arm64-debug is the sole native-built label and is skipped, and confirmed the release job's per-name gh run download -n "$1" is unaffected by the new layout-* / bytecode-cache-layout-diffs artifacts. The compare step deliberately omits set -e so non-zero diff and the [ … ] && continue idiom don't abort the loop; failure is propagated via exit $status. The download retry loop falls through to ls layouts under set -e if all three attempts fail, which still fails the job.
Other factors
The ARTIFACTS list content is unchanged (verified line-for-line against the removed block), so the release job's existing grep -v '^ *#' <<< "$ARTIFACTS" | grep . filter behaves identically. The change is mechanical across the six build jobs (same three-line pattern each). Deferring rather than approving because this is a new ~50-line shell job on the release critical path whose behavior (artifact download layout, step-summary rendering, failure propagation) is best confirmed by an actual workflow run, which I cannot observe here.
…VC would pack differently fails the Itanium build
…icCacheEntry (a \w*CacheEntry pattern also matched JSC::SpecialPropertyCacheEntry, which clang-cl lays out in this TU)
…ique_object_representations) and has an asserted size A payload written under one C++ ABI is read in place under another. Those ABIs place bases and fields in declaration order and differ only in where they pad, so a record with no padding under each of them has the same layout under all of them; a per-record sizeof literal closes the remaining case (a leaf type whose size differs). Both are now static_asserts: PayloadType<T> at every typed allocation into the encoder, and one sizeof per record. The dozen records that had padding spell it out as m_unused (or, for the two 16-bit map entry types, are held in 32 bits via CachedEntryBits), so their bytes were already zero and now are fields.
…e host CPU's (x86 and ARM disagree on the sign of 0/0)
…ts in CachedTypes.cpp cover it at compile time
dylan-conway
changed the title
Preview Builds
|
There was a problem hiding this comment.
I reviewed the pushes since my last pass and didn't find any bugs; both earlier inline notes (the __has_warning guard and the ENABLE(WEBASSEMBLY) guard on the size assert) are now addressed. Given this reworks the on-disk bytecode-cache record layout and its cross-ABI guarantees, a human look is still worthwhile.
What was reviewed:
PayloadTypeconcept and its assertion sites — every typed placement into the encoder is covered; thedoubleexemption is scoped tostd::is_same_vso it does not leak into records containing doubles.CachedEntryBits<Entry>—VariableEnvironmentEntry/PrivateNameEntryare each a singleuint16_t m_bits, so thebit_cast<uint16_t>round-trip is sound.- NaN canonicalization in
addConstantValue— gated onisDouble()so it can't touch cell/int32 encodings, andjsNaN()is the existing canonical PNaN. CachedBitVector::m_numBitsnarrowing touint32_tusessafeCast; Dockerfile/CI removals match the deleted script.
Extended reasoning...
Overview
Since the previous review, the PR grew substantially: it now introduces the PayloadType concept (std::has_unique_object_representations_v<T> || double) asserted at every typed allocation into the encoder buffer, adds explicit m_unused padding/bit-field fill to eleven Cached* records so they satisfy that trait identically under Itanium and MSVC, wraps the two 16-bit map-entry types in a 32-bit CachedEntryBits<> holder, narrows CachedBitVector::m_numBits to uint32_t, pins every serialized record's size with a static_assert block, canonicalizes NaN constants in BytecodeGenerator::addConstantValue, and removes the now-superseded layout-dump script plus its Dockerfile/CI plumbing. The two issues I flagged inline on earlier pushes — __has_warning used without a defined() guard, and the unguarded CachedWebAssemblySourceProvider size assert — have both been fixed in the current diff.
Security risks
No new attack surface is introduced: this is compile-time layout enforcement plus deterministic constant emission. The bytecode cache itself is a binary format read in place across ABIs, so a layout mismatch would be a correctness/memory-safety hazard — but the direction of this change is to make such mismatches fail at compile time rather than at runtime, which reduces risk. The m_unused members are brace-initialized to zero, so no uninitialized bytes leak into the serialized payload. safeCast<uint32_t> on m_numBits will crash rather than truncate. The bit_cast in CachedEntryBits operates on trivially-copyable 2-byte structs and is size-checked by bit_cast itself.
Level of scrutiny
High. This changes the serialized shape of several cached records (payload bytes differ from before, per the PR description), and the correctness of the whole approach rests on a subtle argument about how has_unique_object_representations plus per-record sizeof asserts jointly imply identical field offsets across the Itanium and MSVC ABIs. The reasoning is well-documented and appears sound, but it deserves a human reviewer familiar with the cross-compile bytecode-cache use case. /Source/JavaScriptCore and /Tools/Scripts/ are also CODEOWNERS-covered paths.
Other factors
The multi-platform CI matrix will exercise the static_asserts under each target ABI, which is the intended verification mechanism. The NaN canonicalization is a small, well-contained runtime change with no observable JS-semantics effect (all NaNs compare unequal and stringify identically). No outstanding third-party CHANGES_REQUESTED reviews are visible in the timeline.
…table JSC bytecode cache) (#40270) ## What `bun build --compile --bytecode --target=<other OS>` produced executables that segfault on launch (#18416): the embedded JSC bytecode cache held a per-function metadata offset table computed from the *building* machine's `sizeof(Op::Metadata)`, and the target decoded it with its own. oven-sh/WebKit#505 and oven-sh/WebKit#521 (both merged) make the cache format independent of the encoding platform — metadata stored as entry counts and laid out by the decoder; every serialized record compile-time-asserted to be padding-free with a fixed size; encoder output a pure function of the source (hash-table order, address order, host page size, host FPU NaN, SourceProviderCache re-parses all removed as inputs). This PR bumps WebKit to that build, adds the test that keeps it true, and fixes the Bun-side places that fed the encoder something non-deterministic. ## Changes - **WebKit** → `7259739917cd`. - **`node:vm` cachedData** (`NodeVM.cpp`, `NodeVMScript.cpp`, `NodeVMSourceTextModule.cpp`): `produceCachedData` / `createCachedData()` now generate the complete code-block tree (`recursivelyGenerateUnlinkedCodeBlockFor{Program,ModuleProgram}`) instead of serializing whatever the VM's CodeCache happened to hold, so the data contains every nested function and no longer depends on what ran before or on an attached debugger; module cached data is produced and looked up with the key JSC uses for module code (strict, `ModuleType`), so `SourceTextModule` cached data round-trips. This makes `cachedData` larger (all functions instead of the top level only) and a little slower to produce. - **Standalone executables**: one new `Flags::CROSS_COMPILED_BYTECODE` bit, set when `--bytecode` output goes into an executable for a different os/arch/libc than the bun that built it; at startup it raises the `cross_compiled_bytecode` crash-report feature so such crashes are identifiable. - **Builtin private names** (`BunBuiltinNames.cpp`): Bun's `@name` symbols are now static `SymbolImpl`s with content-derived hashes, like JavaScriptCore's own, instead of per-VM `PrivateSymbolImpl`s hashed by the process-wide symbol counter — so the bytecode generated for a builtin module no longer depends on how many `Symbol`s the process created before (one was enough to reorder `node:http2`'s constants). Also ~600 fewer allocations per VM; startup time and RSS unchanged within noise. - **Exact-length bytecode embed**: `--compile` previously counted up to 128 never-written buffer bytes after each module's bytecode in its length; the executable now contains exactly the payload, so two builds of the same input are byte-identical. - `bun:internal-for-testing`: `internalModuleBytecode()` exposes the builtin-module bytecode + external string table `--compile --bytecode` embeds, for the test below. ## Test `test/bundler/bundler_bytecode_portable.test.ts` encodes a corpus that reaches every kind of object the cache serializes and 170/194 opcodes — `features.js`, `records.js` (one section per record type), a generated `big.js` (32-bit metadata tables, wide operands, multi-page payloads), `shapes.js`, `source-forms.js` (hashbang, CRLF, U+2028/9, `\u{}` escapes, legacy octal — via `vm.Script`), `module.js` / `esm/` (module code), a builtin-syntax module, 7 individual libraries and a 64-library bundle from `test/node_modules`, and `all.js` (everything in one payload) — through `bun build --bytecode` (± `--minify`), `vm.Script`, `vm.SourceTextModule` and the builtin encoder, and compares one inline snapshot of the SHA-256s on every CI platform. Because the encoder's output depends only on the source, equal hashes on Linux/macOS/Windows × x64/arm64 (+ musl) mean every platform writes — and therefore reads — the same bytes. If the snapshot ever fails on only *some* platforms, that is a portability regression in JSC, not a snapshot to update (it caught one during this PR: x86 and ARM folding `0/0` to differently-signed NaNs, fixed in oven-sh/WebKit#521). The same file checks that the output is independent of the encoding process (GC timing, SourceProviderCache, a busy VM, internal modules), that every corpus build loads from its cache with identical output, that `--compile --bytecode` executables (CJS and ESM, including a Worker entry) run from the embedded bytecode, and that an unusable `.jsc` is a cache miss rather than a crash. Also: `vm.test.ts` (SourceTextModule accepts its own cached data), `capture-stack-trace.test.js` (line numbers after a multi-line template literal, with and without the SourceProviderCache — one of the parser fixes in oven-sh/WebKit#505), `bun-build-compile.test.ts` (`cross_compiled_bytecode` set only for a foreign target). Fixes #18416 --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
1 participant
Follow-up to #505.
Compile-time layout guarantees instead of the layout dump. A bytecode cache payload written under one C++ ABI is read in place under another (
bun build --compilecross-compiles). #505 verified the records' layouts match by dumping them in every image and diffing in CI; this replaces that with checks the compiler makes on the platform that would misread:PayloadType<T>—std::has_unique_object_representations_v<T>: no padding bytes, no unused bit-field bits (doubleexempted; it only fails the trait because ±0/NaN have several representations) — asserted at each typed allocation into the encoder. The ABIs a payload moves between place bases and fields in declaration order and differ only in where they pad, so a type that is padding-free under each of them has the same field offsets under all of them.static_assert(sizeof(X) == N), which closes the remaining case (a leaf type whose size differs between ABIs) and makes any change to a record's serialized form an explicit edit.CachedTypes.cppturns clang's-Wms-bitfield-paddinginto an error, so a bit-field MSVC would pack differently fails the Linux/macOS build with a message naming the field.m_unusedbytes / bit-fields;CachedBitVector::m_numBitsisuint32_t; the two 16-bit map-entry types are held in 32 bits viaCachedEntryBits<>). Payload bytes change; semantics don't.Tools/Scripts/dump-bytecode-cache-layout.ts, the per-image dump steps, node in the images that only had it for the dump, and the release-job comparison are removed (Dockerfiles and workflow are back to their pre-Bytecode cache: portable across platforms and deterministic (same source ⇒ same bytes everywhere) #505 state).One more determinism fix, found by Bun's cross-platform golden test on its first linux-aarch64 run: the parser folds
0 / 0with the host's arithmetic, and x86 produces the negative default NaN while ARM produces the positive one, so lodash (var NAN = 0 / 0) serialized differently per CPU.BytecodeGenerator::addConstantValuenow stores any NaN constant as the canonicalPNaN. No runtime effect (both are NaN to every JS operation).What the three checks catch, concretely (each on whichever platform's compiler sees it):
PayloadType<T>(has_unique_object_representations) fails for:uint8_t a; uint32_t b;);uint32_t a; uint8_t b;→ 8 bytes, 3 unused);unsigned a : 1, b : 1;leaves 30), and therefore also any MSVC/Itanium disagreement about how bit-fields are grouped into units, since the two can only disagree by one of them leaving bits unused;doublearrays are exempted explicitly);bool/enum stored in a type wider than its value bits on an ABI that reports it so.static_assert(sizeof(X) == N)fails for:long,wchar_t,long double,size_ton a 32-bit target);-Wms-bitfield-padding(error) fires, on the Itanium build, for:unsigned a : 3; uint8_t b : 2;), which MSVC would start a new allocation unit for;Not caught by any of these (by construction — they are about layout, not values): anything the encoder writes that depends on the machine or the run, e.g. argument-evaluation order, hash-table iteration order, pointer-derived values, host page size, host
sizeofbaked into a value, the host FPU's NaN. Those are what Bun's cross-platform golden-hash test covers.