Make Buffer read*/write* native functions with a DFG/FTL intrinsic - #35343
Make Buffer read*/write* native functions with a DFG/FTL intrinsic#35343Jarred-Sumner wants to merge 26 commits into
Conversation
The fixed-width accessors (readInt8 ... readDoubleBE, the BigInt64 reads, writeInt8 ... writeDoubleBE) were JS builtins that lazily created a hidden DataView on each Buffer (`this.$dataView ||= new DataView(...)`) and validated through $checkBufferRead / internal/buffer.js. They are now C++ host functions in JSBuffer.cpp with the same semantics (error codes, messages and the per-function argument-validation order of lib/internal/buffer.js), registered with JSC's BufferAccessorRegistry and carrying BufferAccessorIntrinsic so the DFG/FTL compile call sites into bounds-checked loads/stores on the receiver's storage and OSR-exit back to the host function for anything they don't speculate. The existing writeBigInt64* host functions get the intrinsic too. The variable-width accessors (readIntLE etc.) are unchanged; the now-unused $checkBufferRead global is removed. Adds tier-up coverage to test/js/node/buffer.test.js and a bench/snippets/buffer-read-write.mjs benchmark.
|
Updated 2:59 AM PT - Jul 26th, 2026
✅ @Jarred-Sumner, your commit 2a8c8377e3d4ef3abc06510f295146321554a26f passed in 🧪 To try this PR locally: bunx bun-pr 35343That installs a local version of the PR into your bun-35343 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesBuffer fixed-width and variable-width numeric accessors now use native JSC host functions and registered buffer accessor intrinsics. JavaScript implementations and obsolete builtin helpers are removed, with tests and benchmarks covering correctness, errors, receivers, JIT behavior, multiple buffers, and access patterns. Buffer accessor migration
WebKit build version
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
…on cleanups
- ERR_OUT_OF_RANGE("value") for write* now formats the coerced number (like Node
and the previous JS implementation), not the raw argument.
- Derive each accessor's registration descriptor from the same template arguments
the host function is instantiated with, instead of a hand-encoded flag list.
- Tighten the new tier-up tests: assert error codes, and count mismatches instead
of asserting per iteration (much faster under debug+ASAN).
- Trim over-long comments.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Both earlier nits are addressed (9bc68f3 passes jsNumber(number) to OUT_OF_RANGE with a test for the writeInt16LE("40000") message; 922d63b removes writeU_Int8 and the dead checkBounds export). No new issues found. Deferring to a human given the scope — 32 hot-path accessors reimplemented in C++ with a new JSC intrinsic and a companion WebKit change.
Also checked: the registerBufferAccessor<T, LE, isWrite> template args line up 1:1 with each DEFINE_BUFFER_READ/WRITE instantiation; bufferAccessLoad/Store byte-flip only on BE and use WTF::unalignedLoad/Store; the write .length change from 1→2 matches Node.
Extended reasoning...
Overview
Rewrites all 32 fixed-width Buffer.prototype.read*/write* accessors (plus the 4 BigInt64 reads) from JS builtins that lazily attached a per-Buffer DataView into C++ host functions in JSBuffer.cpp, each tagged BufferAccessorIntrinsic and registered with JSC's new BufferAccessorRegistry so DFG/FTL inline them as bounds-checked loads/stores. Removes the now-dead $checkBufferRead global, writeU_Int8, and the checkBounds export. Adds tier-up correctness/error tests and a benchmark.
Follow-up on prior review
My two earlier inline nits were both addressed:
- 9bc68f3:
throwValueOutOfRangenow reports the coercedjsNumber(number), not the raw argument; awriteInt16LE("40000")→"Received 40000"assertion covers it. - 922d63b:
writeU_Int8deleted andcheckBoundsdropped from the default export (kept as an internal callee ofcheckInt).
The one candidate raised and refuted this run — write* .length moving from 1 to 2 — is correct: Node's own write* have .length === 2 (the JS builtin table's 1 was the pre-existing divergence).
Security risks
None user-facing. The memory-safety surface is the unaligned load/store on view->vector() + offset: the fast path guards offset32 >= 0 && offset32 + byteSize <= byteLength (no overflow — offset32 is a non-negative int32 widened to size_t, byteSize ≤ 8), and the slow path re-derives offset from bufferAccessCheckOffsetBounds which enforces 0 ≤ offset ≤ byteLength - byteSize in doubles before the cast. Detached buffers have byteLength() == 0, so byteLength < byteSize throws ERR_BUFFER_OUT_OF_BOUNDS before any storage is touched.
Level of scrutiny
High. This is a hot-path Node-compat rewrite in C++ that reimplements bounds/range validation from scratch, introduces a new JSC intrinsic contract (host-function behavior must exactly match what the JIT'd form OSR-exits to), depends on a companion WebKit PR, and touches raw typed-array storage. That combination warrants a maintainer's eyes even though the implementation reads correctly and the 622 existing + 62 Node parallel buffer tests pass.
Other factors
WTF::flipBytesis only applied whensizeof(Storage) > 1and the accessor is BE — Bun targets are all little-endian, so LE accessors are a straightunalignedLoad/Store.readFloat*/readDouble*route throughJSC::purifyNaN, matching DataView semantics for NaN-encoded JSValues.registerBufferAccessorsWithJSC()runs understd::call_oncefromJSBufferPrototype::finishCreation, so per-worker prototype creation doesn't double-register.- The
bufferWriteslow path preserves Node's odd per-width validation order (offset-then-value for 1 byte, value-then-offset for 2/4 bytes, offset-only for floats), which the comment documents.
readIntLE/BE, readUIntLE/BE, writeIntLE/BE and writeUIntLE/BE (byteLength 1..6) become C++ host functions with the same lib/internal/buffer.js semantics, which removes the last users of the hidden `this.$dataView` and of internal/buffer.js (both deleted). They register with JSC as variable-width accessors, so a call site with a constant byteLength of 1, 2 or 4 compiles to the same bounds-checked load/store as the fixed-width methods; other widths stay on the host function.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build/deps/webkit.ts`:
- Line 13: Update the WEBKIT_VERSION constant to use the merged WebKit commit
SHA instead of the temporary autobuild-preview-pr-330-be77ad70 tag, ensuring
prebuilt downloads remain available after the pull request closes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4668b660-0b0a-47f9-8512-a03041a84680
📒 Files selected for processing (1)
scripts/build/deps/webkit.ts
| // unwind info (RtlAddGrowableFunctionTable) registered for the fixed JIT | ||
| // pool (LLInt pending offlineasm .seh_* emission). | ||
| export const WEBKIT_VERSION = "a40d462206e1caf8388062120acde61e37a4ae7d"; | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-330-be77ad70"; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant version pin and URL construction.
git ls-files 'scripts/build/deps/webkit.ts' 'scripts/build/download.ts'
printf '\n--- scripts/build/deps/webkit.ts ---\n'
cat -n scripts/build/deps/webkit.ts
printf '\n--- scripts/build/download.ts (matching lines) ---\n'
rg -n -C 3 'autobuild-preview|WEBKIT_VERSION|prebuiltUrl|webkit' scripts/build/download.ts scripts/build/deps/webkit.tsRepository: oven-sh/bun
Length of output: 35006
Do not pin WebKit to a preview release tag
scripts/build/deps/webkit.ts:13 uses autobuild-preview-pr-330-be77ad70, and autobuild-preview-pr-* releases are temporary. Once that PR closes, prebuilt WebKit downloads will 404; pin this to the merged WebKit commit SHA instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/build/deps/webkit.ts` at line 13, Update the WEBKIT_VERSION constant
to use the merged WebKit commit SHA instead of the temporary
autobuild-preview-pr-330-be77ad70 tag, ensuring prebuilt downloads remain
available after the pull request closes.
Source: MCP tools
| // unwind info (RtlAddGrowableFunctionTable) registered for the fixed JIT | ||
| // pool (LLInt pending offlineasm .seh_* emission). | ||
| export const WEBKIT_VERSION = "a40d462206e1caf8388062120acde61e37a4ae7d"; | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-330-be77ad70"; |
There was a problem hiding this comment.
🟡 Reminder: WEBKIT_VERSION is pinned to the ephemeral preview tag autobuild-preview-pr-330-be77ad70 for the not-yet-merged oven-sh/WebKit#330 (the PR description says the WebKit dependency is 'TBD', and commit 77ba531 confirms it's temporary). Once WebKit#330 merges, this needs to be swapped for the real merged commit hash before landing — autobuild-preview-* release artifacts are typically pruned when the underlying PR closes, at which point prebuiltUrl() will 404 and bun bd will fail for anyone without a cached extraction.
Extended reasoning...
What changed
-export const WEBKIT_VERSION = "a40d462206e1caf8388062120acde61e37a4ae7d";
+export const WEBKIT_VERSION = "autobuild-preview-pr-330-be77ad70";The stable 40-hex commit hash is replaced with a preview-PR autobuild tag. Commit 77ba531 ("Build against the WebKit preview build for oven-sh/WebKit#330") and the PR description ("depends on the WebKit PR: TBD") both confirm this is intentional temporary scaffolding while the companion WebKit PR (which introduces BufferAccessorRegistry / BufferAccessorIntrinsic) is still open.
How the value is consumed
In scripts/build/deps/webkit.ts:
function prebuiltUrl(cfg: Config): string {
...
const version = cfg.webkitVersion;
const tag = version.startsWith("autobuild-") ? version : `autobuild-${version}`;
return `https://github.com/oven-sh/WebKit/releases/download/${tag}/${name}.tar.gz`;
}and prebuiltDestDir() special-cases the autobuild- prefix for cache keying:
// For 40-hex shas, 16 chars is plenty. For autobuild-preview-* tags, the
// meaningful sha is at the end, so use the whole thing.
const version16 = v.startsWith("autobuild-") ? v.slice("autobuild-".length) : v.slice(0, 16);The file's own doc comment says the value is "From https://github.com/oven-sh/WebKit releases" — a stable release, which a preview PR tag is not.
Step-by-step: why this can't merge as-is
- This PR merges to
mainwithWEBKIT_VERSION = "autobuild-preview-pr-330-be77ad70". - Add DFG/FTL nodes for byte-offset scalar accessors on ArrayBufferView receivers WebKit#330 later merges (or closes); the oven-sh/WebKit release workflow prunes the
autobuild-preview-pr-330-*GitHub release, as it does for all preview builds once the PR is no longer open. - A contributor without a cached
~/.bun-cache/webkit-preview-pr-330-be77ad70*/extraction runsbun bd. prebuiltUrl(cfg)resolves tohttps://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-330-be77ad70/bun-webkit-<os>-<arch>....tar.gz.- GitHub returns 404; the WebKit dependency fetch fails; the build aborts.
CI and anyone who already downloaded the tarball keep working (the cache-dir extraction is keyed by the full tag), which is what makes this easy to accidentally merge — it only breaks fresh builds after the preview release is deleted.
Why existing code doesn't prevent it
Nothing in scripts/build/deps/webkit.ts distinguishes a stable autobuild-<sha> release from an autobuild-preview-pr-* release beyond cache-key formatting. There's no guard that refuses to pin main to a preview tag; the autobuild- special-casing exists precisely to support this workflow during PR development.
Impact
None until the WebKit preview release is pruned; then every uncached bun bd on main fails at the WebKit download step. This is a build-infrastructure availability issue, not a runtime bug — hence nit (a merge-checklist reminder, not a defect the author is unaware of).
Fix
Once oven-sh/WebKit#330 merges, replace this line with the resulting 40-hex commit hash on oven-sh/WebKit main:
export const WEBKIT_VERSION = "<merged-commit-sha>";(Per .claude/docs/landing-prs.md — Dependencies & vendoring.)
There was a problem hiding this comment.
Correct, and still the plan: the pin is currently the preview for WebKit#330's head (autobuild-preview-pr-330-6d8df126) so this PR's CI can build and test against the real dependency. It gets swapped for the merged commit hash before this lands — noted in the PR description too.
The bounds check used the receiver's byteLength; lib/internal/buffer.js (and the deleted builtins) bound by this.length -- the element count -- which only equals byteLength for byte-sized views. Buffer methods .call'd on a wider view now throw ERR_OUT_OF_RANGE where they used to, matching Node. Also cut the in-process tier-up loop counts in buffer.test.js.
| // Any ArrayBufferView receiver is accepted (byte-length semantics); anything else is | ||
| // ERR_INVALID_ARG_TYPE("buf"). |
There was a problem hiding this comment.
🟡 Two stale "byte-length" artifacts survived commit 6382eeb's switch from view->byteLength() to view->length() (element count): (1) this comment's parenthetical "(byte-length semantics)" — it's now element-count (this.length) semantics, matching lib/internal/buffer.js and the test this PR added; (2) size_t byteLengthOfView = view->length(); at :3379, whose name now lies — the sibling sites at :3171/:3250 were renamed to viewLength in the same commit but this one was missed. No functional effect; the parenthetical could be dropped or reworded, and the local renamed to viewLength.
Extended reasoning...
What went stale
Commit 6382eeb ("Bound the accessors by the receiver's element count, trim tier-up loops") changed every bounds check in the new accessor block from view->byteLength() to view->length() — the receiver's element count. Two artifacts from before that commit were not updated to match:
(1) The block comment at JSBuffer.cpp:3081:
// Any ArrayBufferView receiver is accepted (byte-length semantics); anything else is
// ERR_INVALID_ARG_TYPE("buf").The parenthetical "(byte-length semantics)" is now false. Every call site in the block passes view->length(): bufferRead at :3171 and :3188, bufferWrite at :3250 and :3281, bufferReadVarWidthOffset at :3379, bufferWriteVarWidth at :3493. And this PR's own test in buffer.test.js explicitly documents and asserts the opposite of the comment:
// The bound is the receiver's element count (this.length), as in lib/internal/buffer.js, not
// its byteLength: on a wider-element view these throw even though the bytes would fit.
const u16 = new Uint16Array(4);
expect(codeOf(() => Buffer.prototype.readInt32LE.call(u16, 3))).toBe("ERR_OUT_OF_RANGE");(2) The local at JSBuffer.cpp:3379:
size_t byteLengthOfView = view->length();The variable is named byteLengthOfView but holds the element count. Commit 6382eeb renamed the identically-computed locals in bufferRead (:3171) and bufferWrite (:3250) to viewLength, and the parameter of bufferAccessCheckOffsetBounds (:3105) is also viewLength — so this one local is both factually misnamed and inconsistent with the file's own convention that the same commit established.
Step-by-step proof
Take Buffer.prototype.readInt32LE.call(new Uint16Array(4), 3):
- The receiver is a
Uint16Arrayof 4 elements —byteLength() == 8,length() == 4. bufferRead<int32_t, true>computesviewLength = view->length() = 4(:3171).- Fast-path check:
3 + 4 <= 4is false → falls to slow path. bufferAccessCheckOffsetBounds(..., viewLength=4, byteSize=4)computesmaxOffset = 0;3 <= 0is false → throwsERR_OUT_OF_RANGE("offset", ">= 0 and <= 0", 3).- Under the comment's claimed "byte-length semantics", step 2 would have used
byteLength() = 8,maxOffset = 4, and offset 3 would have been in bounds — no throw. The test atbuffer.test.jsasserts it does throw, so the code is right and the comment is wrong.
For the local: at :3379 with the same receiver, byteLengthOfView = view->length() = 4, not 8. The name says "byteLength"; the value is the element count.
Why nothing else catches it
Both are documentation-only. The comment isn't compiled; the variable's numeric value is correct (only its name lies), and only that value flows into the makeString(">= 0 and <= "_s, byteLengthOfView - byteLength) at :3390 — the identifier itself is invisible to users. So there is no test that could fail; this is purely REVIEW.md hygiene: "Comments carry only durable non-obvious content" and "Name things truthfully" / "Match the exact file's local conventions". A comment that contradicts the code it labels is worse than none.
Impact
None functional — nit. The runtime behavior, error messages, and JIT semantics are all correct as-is; this is a mechanical rename and a comment cleanup that the same commit (6382eeb) should have carried.
Fix
:3081: drop the parenthetical, or reword to e.g. "(bounded by the receiver'slength, i.e. element count — matching lib/internal/buffer.js)".:3379: renamebyteLengthOfView→viewLength(three uses at :3379/:3380/:3386/:3390) to match :3171/:3250 and thebufferAccessCheckOffsetBoundsparameter.
| template<typename Storage, bool isLittleEndian> | ||
| static ALWAYS_INLINE void bufferAccessStore(uint8_t* address, Storage value) | ||
| { | ||
| if constexpr (!isLittleEndian && sizeof(Storage) > 1) | ||
| value = WTF::flipBytes(value); | ||
| WTF::unalignedStore<Storage>(address, value); | ||
| } |
There was a problem hiding this comment.
🟡 The new bufferAccessStore<uint64_t, true/false> (:3146-3152) is functionally identical to the pre-existing write_int64_le / write_int64_be helpers just above at :3048-3074 — both do an unaligned 8-byte store, optionally byte-swapped, on a host the file already static_asserts is little-endian. The old helpers' only callers are the four writeBig{Int,UInt}64{LE,BE} bodies at :3565/:3595/:3624/:3653, which this PR already brings into the accessor system (adds BufferAccessorIntrinsic + registerBufferAccessor<int64_t/uint64_t, ..., true>); those bodies can call bufferAccessStore<uint64_t, ...> and the two old helpers can be deleted — same consolidation class as the writeU_Int8 / $dataView cleanups already applied in 922d63b and f289a67.
Extended reasoning...
What's duplicated
This PR introduces bufferAccessStore<Storage, isLittleEndian> at JSBuffer.cpp:3146-3152:
template<typename Storage, bool isLittleEndian>
static ALWAYS_INLINE void bufferAccessStore(uint8_t* address, Storage value)
{
if constexpr (!isLittleEndian && sizeof(Storage) > 1)
value = WTF::flipBytes(value);
WTF::unalignedStore<Storage>(address, value);
}For Storage = uint64_t, this is byte-for-byte equivalent to the pre-existing helpers immediately above at :3048-3074:
template<typename I> void write_int64_le(uint8_t* buffer, I value)
{
static_assert(std::endian::native == std::endian::little);
auto val = reinterpret_cast<uint8_t*>(&value);
buffer[0] = val[0]; ... buffer[7] = val[7];
}
template<typename I> void write_int64_be(uint8_t* buffer, I value)
{
static_assert(std::endian::native == std::endian::little);
auto val = reinterpret_cast<uint8_t*>(&value);
buffer[0] = val[7]; ... buffer[7] = val[0];
}Both do an unaligned 8-byte store, optionally byte-swapped. The file already static_asserts a little-endian host at :3050 and :3064, so WTF::flipBytes + WTF::unalignedStore<uint64_t> is the same operation as the manual byte-by-byte reversed copy.
Why this belongs in the PR
Grep confirms write_int64_le / write_int64_be have exactly four callers, all in this file: the four jsBufferPrototypeFunction_writeBig{Int,UInt}64{LE,BE} bodies at :3565, :3595, :3624, :3653. This PR does touch those four functions — it changes their HashTable entries from NoIntrinsic to BufferAccessorIntrinsic and registers them via registerBufferAccessor<int64_t/uint64_t, ..., true> in registerBufferAccessorsWithJSC(), bringing them into the same accessor system whose store primitive is bufferAccessStore.
REVIEW.md, Architecture & layering: "One implementation, in the right place. Never copy a helper … share or derive it … when your change supersedes a mechanism, delete the old path in the same PR." And Code style: "If your fix makes two functions byte-identical, delete one." This is the same cleanup class already applied three times in this PR: writeU_Int8 / checkBounds in 922d63b, and the bufferWriteVarWidthOffset inline + $dataView removal in f289a67.
Why nothing prevents it
bufferAccessStore lives in an anonymous namespace, so it has internal linkage but is visible throughout the translation unit — including at :3565-3653 where write_int64_le/be are called. The four call sites can invoke it directly (with a static_cast<uint64_t>(value) at the two int64_t sites, which preserves the bit pattern).
Step-by-step proof of equivalence
Take Buffer.alloc(8).writeBigInt64BE(-1n, 0):
jsBufferPrototypeFunction_writeBigInt64BE(:3569) validates the receiver, coerces-1n→int64_t value = -1(bit pattern0xFFFF_FFFF_FFFF_FFFF), validatesoffset = 0.- Old path (:3595):
write_int64_be(vector + 0, value)reinterprets&valueasuint8_t*, thenbuffer[0] = val[7], ..., buffer[7] = val[0]. On the LE host,val[0..7] = FF FF FF FF FF FF FF FF; the reversed copy writesFF FF FF FF FF FF FF FF. - New path:
bufferAccessStore<uint64_t, false>(vector + 0, static_cast<uint64_t>(value)).static_cast<uint64_t>(-1)=0xFFFF_FFFF_FFFF_FFFF;WTF::flipBytesreverses to the same value;WTF::unalignedStore<uint64_t>writesFF FF FF FF FF FF FF FF. - Same for a non-symmetric value like
0x0102030405060708: old writes01 02 03 04 05 06 07 08; new flips to0x0807060504030201and stores it LE-native → the same bytes01 02 03 04 05 06 07 08.
The LE case is trivially identical (unaligned host-order store either way). Same argument for the writeBigUInt64* pair with uint64_t.
Impact
None functional — the two implementations produce identical bytes. This is purely duplicate-implementation cleanup; hence nit.
Fix
Replace the four call sites, e.g. at :3565:
bufferAccessStore<uint64_t, true>(static_cast<uint8_t*>(castedThis->vector()) + offset, static_cast<uint64_t>(value));(and <uint64_t, false> for the BE pair), then delete write_int64_le / write_int64_be at :3048-3074.
| const counting = { valueOf() { coerced++; return 300; } }; | ||
| for (let i = 0; i < 1000; i++) { | ||
| try { scratch.writeInt8(counting, 0); } catch (e) { assert(e.code === "ERR_OUT_OF_RANGE", "range code"); } | ||
| try { scratch.writeInt8(counting, 100); } catch (e) { assert(e.code === "ERR_OUT_OF_RANGE", "value checked before offset for 1-byte writes? no: offset first"); } | ||
| } | ||
| assert(coerced === 2000, "value coerced exactly once per call, even when it then throws: " + coerced); |
There was a problem hiding this comment.
🟡 The two try { scratch.writeInt8(counting, …); } catch (e) { assert(e.code === "ERR_OUT_OF_RANGE", …); } lines have no assert(false, "should throw") after the writeInt8 call, so if the value range check were broken and writeInt8(300, 0) silently succeeded, valueOf still runs once per call (coercion precedes the range check), coerced still hits 2000, the catch never fires, and the block passes — even though the assertion message says it's testing behavior 'even when it then throws'. This is REVIEW.md's flagged 'expects inside catch blocks that may never fire' pattern; the same test uses the correct try { f(); assert(false, "should throw"); } catch convention at :142/:151/:152, so these two are the only lines that drifted. Nit only: the range-check-throws behavior is separately covered non-vacuously in this PR's buffer.test.js additions (ranges === 6000).
Extended reasoning...
What the block does
In the "host semantics survive every exit path" test (test/js/node/buffer-jit.test.ts:127-133):
let coerced = 0;
const counting = { valueOf() { coerced++; return 300; } };
for (let i = 0; i < 1000; i++) {
try { scratch.writeInt8(counting, 0); } catch (e) { assert(e.code === "ERR_OUT_OF_RANGE", "range code"); }
try { scratch.writeInt8(counting, 100); } catch (e) { assert(e.code === "ERR_OUT_OF_RANGE", "...offset first"); }
}
assert(coerced === 2000, "value coerced exactly once per call, even when it then throws: " + coerced);Neither try body has an assert(false, "should throw") after the writeInt8 call. The catch-block assertions are the only place that verifies a throw occurred — and if the call doesn't throw, the catch simply never runs.
Why this is the flagged pattern
REVIEW.md, Every assertion must be able to fail: "Hunt vacuous patterns: … expects inside catch blocks or callbacks that may never fire". These two lines are exactly that shape. The same test uses the correct file convention ~10 lines below:
try { f(); assert(false, what + " should throw"); } catch (e) { assert(e.code === "ERR_OUT_OF_RANGE", ...); } // :142
try { bb.writeBigInt64LE(2n ** 63n, 0); assert(false, "should throw"); } catch (e) { ... } // :151
try { bb.writeBigUInt64LE(-1n, 0); assert(false, "should throw"); } catch (e) { ... } // :152so lines 130-131 are the only two that drifted from the pattern the rest of the file follows.
Step-by-step: why the block passes even if the range check is broken
Suppose bufferWrite<int8_t>'s valueIsInRange() were deleted (i.e. writeInt8(300, 0) no longer throws). scratch = Buffer.alloc(8).
- Line 130 —
scratch.writeInt8(counting, 0): InbufferWrite<int8_t>(JSBuffer.cpp),valueValue.toNumber()runs first →counting.valueOf()fires →coerced++, returns 300. With the broken range check, no throw; 300 is truncated viaJSC::toInt32and stored at offset 0. Catch never fires. Nothing fails. - Line 131 —
scratch.writeInt8(counting, 100):valueOfruns again →coerced++. With a broken value range check, control still reachesbufferAccessCheckOffsetBounds(…, viewLength=8, byteSize=1), which throwsERR_OUT_OF_RANGE("offset")because 100 > maxOffset 7. Catch fires, ande.code === "ERR_OUT_OF_RANGE"passes — same code, wrong reason (offset, not value). - After 1000 iterations:
coerced === 2000. The final assertion at :133 passes.
Result: the whole block is green even though the int8 value range check is completely gone — yet the assertion message at :133 explicitly claims to be verifying behavior "even when it then throws".
Why nothing else in this block prevents it
Coercion order is what makes the coerced === 2000 assertion insensitive to the throw: bufferWrite calls toNumber() (which invokes valueOf) before valueIsInRange() and before the offset bounds check, so valueOf runs exactly once per call whether or not the call subsequently throws. The coerced === 2000 assertion IS still meaningful (it catches double-coercion or skipped-coercion bugs) — it just doesn't test what its own message says about throwing.
Impact
Nit. The value-range-throws property IS covered non-vacuously elsewhere in this PR: test/js/node/buffer.test.js's new "writes match a DataView…" test does if (codeOf(() => buf.writeInt8(128, 0)) === "ERR_OUT_OF_RANGE") ranges++; … expect(ranges).toBe(6000) across 2000 iterations, which fails if the range check is removed. So the suite as a whole still catches a regression; this block just doesn't test what its own message claims, and is inconsistent with the file's own convention.
Fix
Match the file's own pattern at :142/:151/:152:
try { scratch.writeInt8(counting, 0); assert(false, "should throw"); } catch (e) { assert(e.code === "ERR_OUT_OF_RANGE", "range code"); }
try { scratch.writeInt8(counting, 100); assert(false, "should throw"); } catch (e) { assert(e.code === "ERR_OUT_OF_RANGE", "..."); }(with the assert(false) guarded by e.message.includes("should throw") in the catch, or by re-throwing when e.code is undefined — the file's :142 pattern relies on the thrown Error having no .code, so e.code === "ERR_OUT_OF_RANGE" fails and the catch's own assert catches it.)
…ts, byteLength dispatch, and BigInt writers - A DataView receiver has no `length` in lib/internal/buffer.js, so every accessor now reports the `>= 0 and <= NaN` ERR_OUT_OF_RANGE that Node and the deleted builtins produce, instead of reading/writing through byteLength. - The variable-width readers validated offsets with a validateInteger() fallback that printed the safe-integer-range message; Node's readIntLE family dispatches to the width-specific readers, whose validation is validateNumber() + boundsError(). Reuse the shared type/bounds helpers, so out-of-range integral offsets (including |offset| > 2^53) get the bounds message. - The variable-width writers coerced the value before validating byteLength, invoking user valueOf() where Node never does; coerce after the dispatch. - The BigInt writers keep their value validation but now go through the shared receiver check and element-count bound, so writeBigInt64* and readBigInt64* agree on the receiver and throw coded errors like siblings. Also give the subprocess-based JIT tests an explicit per-test timeout.
The buffer accessor stress tests in the WebKit fork's JSTests never run in CI, since oven's WebKit CI only builds. Bring the coverage that has no Bun equivalent into buffer-jit.test.ts so it runs on every PR: - A differential fuzzer: one seeded operation stream over all read*/write* accessors runs in a JIT process and a BUN_JSC_useJIT=0 process, and the digests of every return value, error and post-write buffer byte must match. The receiver pool avoids SharedArrayBuffer, which the interpreter- only mode does not expose, so both arms build the identical stream. - A >2GB receiver that must stay optimized (bounded compiles) while out-of-bounds accesses keep throwing. - Views with 2GB and ~4GB byteOffsets, cross-checked against a DataView on the raw buffer. The run() helper takes per-test env. Verified the fuzzer detects an injected JIT-arm divergence (digest mismatch) before trusting a pass.
…mitted offset in errors
There was a problem hiding this comment.
No new issues found — the last two ordering nits (BigInt writer receiver-vs-value/offset, and the Received undefined on the DataView branch) are addressed in 2a8c837 and pinned by tests. Deferring to a human because of scope: ~600 lines of new hot-path C++ in JSBuffer.cpp backed by a new JSC intrinsic, and WEBKIT_VERSION is pinned to the oven-sh/WebKit#330 preview build, so merge order with that PR needs a human call.
What was reviewed:
- All four accessor families (
bufferRead<T>,bufferWrite<T>,bufferReadVarWidth,bufferWriteVarWidth, and the four BigInt writers) now share the same offset-type → receiver → bounds validation order; the BigInt writers' deferred-receiver shape matchesbufferWrite<T>. validateOffsetBigInt64's new!viewHasLengthbranch reportsjsNumber(offsetD)so an omitted offset formats as0.- The
byteLength()→length()switch in the BigInt writers is intentional (element-count semantics, matchinglib/internal/buffer.jsand the fixed-width family) and is covered by theUint16Arrayreceiver test. registerBufferAccessorsWithJSC()template args match each host function's<T, isLittleEndian>instantiation one-for-one.
Extended reasoning...
Overview
This PR replaces ~40 Buffer.prototype.read*/write* JS builtins with C++ host functions in JSBuffer.cpp, each carrying BufferAccessorIntrinsic and registered via a new BufferAccessorRegistry so DFG/FTL can compile call sites into bounds-checked loads/stores. It deletes the $dataView/$checkBufferRead private machinery and internal/buffer.ts, updates the prototype hash table, and adds ~800 lines of tests (a JIT-behavior suite with a differential fuzzer against useJIT=0, plus tier-up coverage in buffer.test.js). WEBKIT_VERSION is bumped to a preview build of oven-sh/WebKit#330, which supplies the intrinsic and registry.
Security risks
None identified. The new host functions bounds-check via view->length() before every view->vector() access, use WTF::unalignedLoad/Store with explicit byte-swap, and all size/offset arithmetic widens to size_t/double before comparison. No user-controlled data reaches allocation or the filesystem. The toNumber coercion (which can run user JS) happens before any pointer is dereferenced, and RETURN_IF_EXCEPTION follows it.
Level of scrutiny
High. This is hot-path native code on one of the most-called Node APIs, with a new JIT intrinsic whose correctness depends on the host function and the registered DataViewData descriptor agreeing exactly (a mismatch would produce wrong results only after tier-up). It also pins the build to an unmerged WebKit PR's preview artifact — merging this before WebKit#330 lands (or without swapping the pin to a real hash) would leave main on a preview tag. That's a merge-ordering decision a maintainer should make.
Other factors
The PR has been through six rounds of review on argument-validation ordering in error paths (all nit-level, all resolved and pinned by tests verified against Node v26.5.0). This run's bug-hunting pass found nothing new. Test coverage is unusually thorough — differential fuzzing of every accessor against a JIT-off reference, cross-view aliasing / CSE ordering, resizable/growable receivers, >2GB/>4GB-byteOffset views, and prototype/instance shadowing after tier-up — and the JSC side ran the stress suite under 20 fuzzing configurations. Nonetheless the combination of new C++ template machinery, a new JSC intrinsic, and a cross-repo dependency is well past the bar for auto-approval.
2 participants
Buffer.prototype.read*/write*become JIT-inlined native functionsAll of the accessors (
readInt8…readDoubleBE, the BigInt64 reads/writes, and the variable-widthreadIntLE/BE,readUIntLE/BE,writeIntLE/BE,writeUIntLE/BE) move from JS builtins that lazily created a hiddenDataViewon each Buffer to C++ host functions inJSBuffer.cpp. Each is registered with JSC's newBufferAccessorRegistryand carriesBufferAccessorIntrinsic, so DFG/FTL compile call sites into a bounds-checked load/store on the receiver's storage; for the variable-width family, a call site whosebyteLengthargument is a constant 1, 2 or 4 gets the same node. Depends on oven-sh/WebKit#330 (WEBKIT_VERSIONcurrently points at that PR's preview build).The host functions are the single source of truth for behavior: they mirror
lib/internal/buffer.js'scheckBounds()/checkInt()/boundsError()(error codes, messages, and the per-function argument-validation order), and the JIT exits back to them for anything it doesn't speculate. The now-unused$checkBufferReadglobal, the$dataViewprivate name, andinternal/buffer.jsare removed.Benchmark: CI build of this PR (
bunx bun-pr 35343) vs Bun 1.4.0 vs Node 26.5.0, macOS arm64, ns per operation (bench/snippets/buffer-read-write.mjs):Writes moved the most because the old builtins bounds-checked each store by loading
this[offset]/this[offset+1]and comparing toundefined, then loaded the hidden$dataViewfield, then calledDataView.setX(which re-checks bounds); now it is one range check + one bounds check + the store, all hoistable in loops (the 1- and 2-byte value range checks are graph nodes visible to integer range analysis, and theoffset + byteSizereturn value is dead unless the caller uses it).Tests:
test/js/node/buffer.test.js(tier-up coverage),test/js/node/buffer-jit.test.ts(compile counts converge, per-exit host fallback, load/store ordering and cross-view aliasing, prototype/instance replacement after tier-up, resizable/growable receivers), plus Node'stest-buffer-*parallel suite, all on a debug/ASAN build. On the JSC side, the fivebuffer-accessor-jit*.jsstress tests pass under 20 fuzzing / stress configurations (randomizing / double / narrowing / widening prediction agents at several seeds, OSR-exit fuzzing at every check, AI-state validation, eager compilation, andcollectContinuously/forceGCSlowPathsGC stress).