Skip to content

[JSC] A failed check_tdz takes the name of its binding from the instruction - #1014

Open
robobun wants to merge 1 commit into
mainfrom
robobun/c3953d8f/check-tdz-names-its-binding
Open

robobun wants to merge 1 commit into
mainfrom
robobun/c3953d8f/check-tdz-names-its-binding

Conversation

@robobun

@robobun robobun commented Nov 8, 2026 •

Copy link
Copy Markdown
Collaborator

Part of oven-sh/bun#25569. Bun's half is oven-sh/bun#48285.

Problem

  • A read of a binding before its declaration names wrong text, or crashes. class X { name = foo } throws Cannot access '' before initialization. setTimeout(async (cfg = cfg) => { await null }, 0) ends in Segmentation fault at address 0x38.
  • slow_path_check_tdz (runtime/CommonSlowPaths.cpp:322) cuts the name out of the source, at the expression info of a frame from a stack walk. The walk skips hidden frames, and can find none.
  • In a visible frame that range is often not the name: x++ gives ''.

Fix

  • op_check_tdz, the instruction in front of each such read, carries the identifier-table index of its binding. slow_path_check_tdz builds the error from it: no stack walk, no source text.
  • Verified: JSTests/stress/bun-tdz-error-names-the-binding.js and JSTests/modules/bun-tdz-error-names-the-import.js fail on main. CI: no failure on seven Test lanes.
  • Self-reviewed: 9 concerns raised, 8 addressed. Not taken: a change of Bun's debug default (Notes).

Background

  • Hidden frames (ImplementationVisibility::Private) run class field initializers, and the parameter defaults of an async function that awaits.
  • Add variable name to ReferenceError #56 put the name on the instruction as a string constant. A merge of upstream replaced its reader (Bun 1.2.23). The index replaces that constant.
  • Considered: the executing frame's expression info. x++ stays wrong.

Downsides

  • Serialized bytecode changes: cachedTypesFormatRevision goes up, Bun's pinned hashes move.
  • Names of checked bindings in registers join the identifier table. ts.transpileModule: +36 entries of 8 bytes, 485 fewer constants, no Wide16 check in 1,230.
  • jsc text: 1,312 bytes smaller with LTO, 3,200 bytes larger without.
Notes

Reproduce

function field() { class X { name = foo } new X; const foo = 1; }
function derived() { class B { } class X extends B { name = foo } new X; const foo = 1; }
function increment() { x++; let x; }
function compound(log) { log("start"); retries -= 1; let retries; }
function pattern() { [x] = [1]; let x; }
function escaped() { a\u0062c; let abc; }
async function parameter(a = b, b) { await null; }
for (const f of [field, derived, increment, () => compound(() => { }), pattern, escaped]) {
    try { f(); } catch (e) { print(e.message); }
}
parameter().catch(e => print(e.message));
drainMicrotasks();
setTimeout(async (cfg = cfg) => { await null; }, 0);
row jsc of main (5a61071, release, from CI) jsc of this branch
field Cannot access '' before initialization. 'foo'
derived 'ss X { name = ' 'foo'
increment '' 'x'
compound 'log("start")' 'retries'
pattern '[x]' 'x'
escaped 'a\u' 'abc'
parameter 'parameter()' 'b'
the timer Segmentation fault, exit 139 the promise rejects, exit 0

Node v26.3.0 names the binding in every row. V8 has the name on its check too: ThrowReferenceErrorIfHole(variable->raw_name()) (deps/v8/src/interpreter/bytecode-generator.cc:4908).

History

  • Add variable name to ReferenceError #56 (353aa20, June 2024) added the second operand of op_check_tdz, a string constant with the name, and a slow path that read it.
  • Upstream's 300119@main (412a6c0, September 2025) gave upstream the same message another way: expression info in front of the checks of five node kinds, and the slow path that this PR removes.
  • The merge fc230df (2025-09-20) took upstream's slow path. Its fork parent df8aa4c still reads the operand. From then on nothing read the operand, and every code block still made the constant.
  • The same merge dropped two more behaviours of the fork. SpeculativeJIT::compileCallDOM lost its typed array and Int52 cases (188 lines before, 85 after, DFG DOMJIT: fix attemptToMakeCallDOM predicates and restore typed-array/Int52 lowering #318 restores them). The RangeError of JSValue::toIndex lost the value at its end (" cannot be negative: " before, " cannot be negative" after). This PR changes neither.
  • Bun 1.2.22 prints 'foo' for the field row and 'x' for x++. Bun 1.2.23 prints '' for both.
  • The crash is in Bun 1.3.10 to 1.4.2. Bun 1.3.5 rejects with 'cfg'. The outer half of an async function that awaits is a hidden frame (parser/Parser.cpp:2409).
  • Upstream main has the same slow path and the same null read. Its op_check_tdz has one operand, so this change cannot go upstream as it is.

The operand

  • The operand is the index of the name in the identifier table plus one. Zero is a check without a name. Only the check of this in a derived constructor has none, and slow_path_check_tdz knows that check by its register, before it reads the operand.
  • Upstream's only emitter is emitTDZCheck(RegisterID*). A sync can merge a new call of it for a variable with no conflict. Such a check has operand zero and throws Cannot access uninitialized variable., as Add variable name to ReferenceError #56's slow path did for its undefined constant. With a plain index it would name identifier 0 of the code block, or read past an empty table. ASSERT(target == thisRegister()) in the emitter reports it in a build with assertions.
  • No tier reads the operand on the fast path. llint_op_check_tdz, its wide16 and wide32 forms and JIT::emit_op_check_tdz: 0 lines of disassembly differ.

Measurements

The two jsc programs that CI built on the bun-webkit-linux-amd64-lto-aot lane, from 5a61071 (main) and from this branch. Instructions are user-mode instructions from perf_event_open, least of 5 runs, slope between n and 2n, concurrent JIT off.

what main this branch
a check that passes, LLInt (--useJIT=0) 342.00 342.00
a check that passes, Baseline only 94.00 94.00
a check that passes, all tiers 6.50 6.50
a check that fails in a visible frame 3,919 3,495
a check that fails in a field initializer (with the class and the new) 11,039 10,426
size text of jsc 53,673,261 53,671,949
slow_path_check_tdz 581 bytes 596 bytes
  • The bun-webkit-linux-amd64 lane has no LTO. Its jsc has 46,425,967 and 46,429,167 bytes of text (3,200 more), and slow_path_check_tdz has 418 and 321 bytes. The growth is in 28 functions of the bytecode generator that emit the check, for example AssignBracketNode::emitBytecode (344 bytes more).
  • .data and .bss have the same size before and after, on both lanes.
  • Calls of getBytecodeIndex for 100 failed checks: 100 on main, 0 here. Counted with a breakpoint, in two builds with symbols made here.

Bytecode of ts.transpileModule on a 6 KB source in the same two jsc programs (--dumpGeneratedBytecodes=1, 1,655 code blocks):

main this branch
check_tdz 1,230, all Narrow 1,230, all Narrow
check_tdz behind a Narrow resolve_scope + get_from_scope (what the interpreter of a compiled program runs as one step) 844 844
identifier entries 25,474 25,510
constants 17,374 16,889
instruction bytes 987,599 987,605
  • The 36 new identifier entries are names of bindings that live in a register. A captured binding already has its name in the table, for resolve_scope.
  • The 6 bytes are one put_by_id whose identifier index moved from 255 to 256.
  • A constant removed is a slot of 8 bytes in the unlinked code block and in each linked one. 482 of the 485 also had a JSString cell of 16 bytes.
  • The narrow form holds an operand up to 255. With 246 other names in front, the checks of the stress test have operands 247 to 262: five Narrow, seven Wide16. Before, the narrow form held a constant index up to 111.
  • Bun's half has the same counts for four workloads in Bun (ts.transpileModule, prettier, hono, tsc --noEmit). No check changes its width in any of them.

Bytecode cache, jsc shell with JSC_diskCachePath: a payload of main is a cache miss for this branch, which compiles the source again and names the bindings. A payload of this branch is a cache hit on the next run, with the same names. The payload of a 3-line program is 1,800 bytes on main and 1,784 here.

That run isolates the revision. The first word of a payload is computeJSCBytecodeCacheVersion() xor cachedTypesFormatRevision * 0x9E3779B9 (currentCacheVersion()), and GenericCacheEntry::isUpToDate compares it before it decodes anything. The two jsc programs of that run were built here from one tree, and their words are 0x24beeb01 and 0xc6f662c8. Without the revision both are 0x83b6431f. So the payload of main is rejected for its revision alone: [Disk Cache] Cache miss for sourceCode.

In Bun the first part is a hash of WEBKIT_VERSION, so there a payload of another engine differs in both parts.

Tests

  • CI of this PR: the seven Test jobs pass (linux amd64 and arm64, lto, lto-aot, lto-hardened and lto-hardened-aot lanes). On bun-webkit-linux-amd64-lto-aot, run-javascriptcore-tests reports 0 failures in more than 125,000 runs of the stress tests.
  • JSTests/stress/bun-tdz-error-names-the-binding.js with the jsc that CI built on the lto-aot lane: it passes, alone and with --useAOT=1. With the jsc of main from that lane it reports 65 wrong messages and then ends with a segmentation fault, alone and with --useAOT=1.
  • The same test in the 17 modes of run-jsc-stress-tests, with a jsc built here (--without-aot): all pass. It also passes with --forceDebuggerBytecodeGeneration=1, --useTypeProfiler=1, --useControlFlowProfiler=1, --useBytecodeOptimizer=0 and --showPrivateScriptsInStackTraces=1.
  • JSTests/modules/bun-tdz-error-names-the-import.js: 11 modes, all pass. With the jsc of main the first wrong message is 'liasCounter)' for typeof (aliasCounter).
  • JSTests/stress --quick with a jsc built here: 22,619 runs. 7 fail, none from this change. 5 are BigInt tests that pass the 600 MB of --memory-limited in my build, with the jsc of main too. 2 depend on the load of the machine (auxiliary-evacuation-with-concurrent-compiles.js, regexp-bytecode-of-deeply-nested-fixed-count-groups.js) and pass when they run alone.
  • JSTests/ChakraCore.yaml, modules.yaml, es6.yaml and LayoutTests/jsc-layout-tests.yaml with --quick: 5,958 runs. 2 fail (JSON-parse-reviver.js), with the jsc of main too. No expectation of a dead zone message moved.

Order of merges

Not changed

…uction

op_check_tdz names its binding by an index into the identifier table of
its code block. The operand is the index plus one, and zero is a check
without a name. slow_path_check_tdz builds the ReferenceError from that
identifier. It no longer walks the stack for a frame and cuts the name out
of the source text at the expression info of that frame.

The walk skips the frames that stack traces hide: the function that runs
the field initializers of a class, and the outer half of an async function
that awaits. A check that failed in such a frame got the expression info of
a caller, and the message held '' or text of the caller. With no JavaScript
caller the walk found no frame, and the slow path read through a null
CodeBlock. In a frame that the walk does find, the expression info in front
of the check is not the name for x++, ++x, x += 1, x ||= 1, a tagged
template and the targets of a destructuring assignment.

The operand was a string constant since 353aa20, whose slow path read
it. The merge of upstream's 300119@main (fc230df) replaced that slow
path. The constant is gone: a code block no longer makes one JSString for
each name that it checks.

createTDZError(const Identifier&) goes through the StringView overload,
which falls back to the message without a name when the message cannot be
allocated. cachedTypesFormatRevision goes up, because the operand of a
serialized check_tdz means something else.

JSTests/stress/bun-tdz-error-names-the-binding.js and
JSTests/modules/bun-tdz-error-names-the-import.js hold the messages.
@robobun

robobun commented Nov 8, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

Reproduced on the jsc shell of main (5a61071) and on Bun 1.4.2 and 1.4.3-canary.1, linux x64. For Bun, write console.log in place of print:

function field() { class X { name = foo } new X; const foo = 1; }
try { field(); } catch (e) { print(e.message); }
setTimeout(async (cfg = cfg) => { await null; }, 0);
output
jsc of main, Bun 1.4.3-canary.1 Cannot access '' before initialization., then Segmentation fault at address 0x38
jsc of this branch Cannot access 'foo' before initialization., exit 0

The two new tests fail with the jsc of main and pass with the jsc of this branch, also with --useAOT=1. The numbers in the description come from the jsc programs that CI built from main and from this branch.

Bun's half is oven-sh/bun#48285. It pins the build of this PR, adds test/regression/issue/25569.test.ts and moves the pinned bytecode hashes.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and did not find any bugs. Because it changes the encoding of a bytecode instruction (with a cache-format revision bump that Bun's pinned bytecode hashes depend on) and the paths fall under the JavaScriptCore CODEOWNERS entry, a human look is still worthwhile.

What was reviewed:

  • The new operand end to end: addConstant(ident) + 1 in emitTDZCheck, the 0 sentinel for the this check, and codeBlock->identifier(m_identifier - 1) in slow_path_check_tdz all index the same code block's identifier table, so the read cannot go past it from the generator's own emits.
  • Every reader of op_check_tdz (LLInt asm, Baseline emit_op_check_tdz, DFG parser, BytecodeUseDef, BytecodeOptimizer's operand visitor, the AOT closure-read fusion): none decodes operand two as a register, so retyping it to unsigned only drops a phantom constant-register use from the optimizer's view.
  • identifier?: unsigned already has precedent in BytecodeList.rb, and the stress test covers operands on both sides of 0xff so Narrow and Wide16 decoding are both exercised.
  • Not done in this review: running the new JSTests or the stress suite (no build or test runner in this environment); the PR's pass claims are unverified here.
Extended reasoning...

The change replaces the second operand of op_check_tdz (a string constant VirtualRegister nothing read) with an identifier-table index plus one, and rewrites slow_path_check_tdz to build the ReferenceError from it instead of walking the stack and slicing source text, which fixed a null-frame crash and wrong names; it also bumps cachedTypesFormatRevision to 15, simplifies createTDZError(Identifier) to call the StringView overload, and adds two tests plus a BUN-TEST-DIFFERENCES.md row. No security-sensitive surface is touched. Deferring rather than approving because Source/JavaScriptCore and JSTests are covered by the repository's CODEOWNERS file, and because the serialized-bytecode format change requires coordination with Bun's pinned cache hashes, which is a cross-repo decision for a human; the five ruled-out candidates (format revision, external decoders, release-build ASSERT, out-of-table index, optimizer retargeting to the this register) all check out as non-issues on reading.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026 •

Copy link
Copy Markdown

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The check_tdz bytecode now carries an identifier-table index for named bindings. The runtime uses that identifier to construct TDZ error messages. New tests cover local and imported bindings across expression types, async contexts, module cycles, and execution tiers.

Changes

TDZ Binding Names

Layer / File(s) Summary
Encode TDZ binding identifiers
Source/JavaScriptCore/bytecode/BytecodeList.rb, Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp, Source/JavaScriptCore/runtime/CachedTypes.cpp
The check_tdz operand changes to an unsigned value. Named checks encode an identifier-table index plus one, and unnamed checks use zero. The cached-types format revision increases to 15.
Resolve TDZ error names
Source/JavaScriptCore/runtime/CommonSlowPaths.cpp, Source/JavaScriptCore/runtime/ExceptionHelpers.cpp
The slow path uses the encoded identifier for TDZ errors and retains the special derived-constructor this error. Ordinary identifier message construction delegates to the StringView overload.
Test binding names across execution contexts
JSTests/stress/bun-tdz-error-names-the-binding.js, JSTests/modules/bun-tdz-error-names-the-import.js, JSTests/modules/bun-tdz-error-names-the-import/*, JSTests/BUN-TEST-DIFFERENCES.md
New tests check binding names across expression forms, async contexts, module cycles, and warmed execution tiers. The test differences table records the covered cases.
Merge Risk: Minimal · up to bab54

This change makes the ReferenceError for a failed temporal-dead-zone check name the right binding, and it adds stress and module tests for that. No concrete merge-blocking defect was found. The ahead-of-time compiler stress run was not executed, so CI should confirm it before merge.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title "[JSC] A failed check_tdz takes the name of its binding from the instruction" matches the main change. The PR changes op_check_tdz to carry the binding's identifier-table index, and slow_pat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description follows the WebKit commit-message style and addresses the core elements a reviewer needs. It states the problem (wrong or crashing TDZ binding names in class fields, async parameter de…
Full details: Title check

Explanation

The title "[JSC] A failed check_tdz takes the name of its binding from the instruction" matches the main change. The PR changes op_check_tdz to carry the binding's identifier-table index, and slow_path_check_tdz builds the ReferenceError from that index instead of walking the stack and cutting text out of the source. The title is short, specific, and names the subsystem (JSC) and the instruction (check_tdz).

Full details: Description check

Explanation

The description follows the WebKit commit-message style and addresses the core elements a reviewer needs. It states the problem (wrong or crashing TDZ binding names in class fields, async parameter defaults, and visible frames such as x++), the fix (op_check_tdz carries the identifier-table index, and slow_path_check_tdz builds the error from it without a stack walk), and the verification (two new tests that fail on main and pass on the branch, CI results, and stress-test runs). It also lists downsides (cache revision bump, identifier-table growth, binary size changes), measurements, and merge-order notes. The template's required elements are a Bugzilla bug link, a 'Reviewed by' line, a explanation, and a list of changed files with function names. The description links to an upstream issue (oven-sh/bun#25569), but it has no WebKit Bugzilla bug URL or bug title in the template format. It has no 'Reviewed by' line and no changed-file list in the ChangeLog-style format. The core explanation is present and thorough, so the description is mostly complete, and the missing template items are non-critical to the review.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use path_filters to narrow the review scope.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

Recent review info

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 9eb85411-f51a-4c24-b14e-c0d23933adc9
Commits

Reviewing files that changed from the base of the PR and between 32dc1b0 and bab547d.

Files selected for processing: 9

  • JSTests/BUN-TEST-DIFFERENCES.md
  • JSTests/modules/bun-tdz-error-names-the-import.js
  • JSTests/modules/bun-tdz-error-names-the-import/early.js
  • JSTests/stress/bun-tdz-error-names-the-binding.js
  • Source/JavaScriptCore/bytecode/BytecodeList.rb
  • Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
  • Source/JavaScriptCore/runtime/CachedTypes.cpp
  • Source/JavaScriptCore/runtime/CommonSlowPaths.cpp
  • Source/JavaScriptCore/runtime/ExceptionHelpers.cpp

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

@github-actions

github-actions Bot commented Nov 8, 2026

Copy link
Copy Markdown

Preview build of bab547d: autobuild-preview-pr-1014-bab547d2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant