Bytecode cache: write every value explicitly - #518
Conversation
The payload is now a byte stream: every value is written by a primitive (u8/u16/u32/u64 little-endian, LEB128 varints, byte runs) and read back by the matching one. No C++ object is placed in or read out of the payload, so the format no longer depends on the ABI or compiler that produced it, and every byte is a function of what was encoded. Each Cached* type is now the statement of one value's encoding (an encode and a decode function over Writer/Reader); the decode-side CachedCodeBlock and CachedFunctionExecutable are parsed records on the stack. References are absolute payload offsets: varuints to things written earlier (string records, shared TDZ environments), u32s patched later (child executable records, lazily decoded bodies, expression info). Strings are a tagged varuint: 1-3 Latin-1 characters in line, an external-table ordinal, or the offset of a string record. Hash tables are written in key order. Regions, breadth-first bodies, cold expression info, content sharing of arrays/expression info/private-name environments, checksums, the updatable records the jsc shell patches, and in-place use of instruction bytes, metadata steps, expression-info words and long 8-bit strings from a persistent payload are as before. The metadata table is stored as per-opcode entry counts and laid out by the decoder (UnlinkedMetadataTable::expandSteps). A function-update payload is encoded knowing where it will be appended, so its offsets are absolute in the combined payload.
A value a stream refers to that does not decode (a string record, a shared environment, an external-table ordinal with no table), a count larger than the bytes left, a reference that does not point backwards, a shared offset that decoded as another type, or a null where the format has none marks the reader overran; a code block or executable whose reader overran is rejected and generated from source instead. Found by flipping random bytes in cache files.
|
Warning Review limit reached
On-demand reviews are free for the next 26 days. After that, they cost $0.25 per reviewed file. Or wait 21 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 77 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 |
|
Closing: too large a change for what it buys. Portability will be #389 reduced to the metadata-table fix, with the cross-platform golden-hash test in Bun as the guard against layout drift. |
Preview Builds
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| symbolTable->m_maxScopeOffset = ScopeOffset(reader.varuint()); | ||
| uint8_t flags = reader.u8(); | ||
| symbolTable->m_usesSloppyEval = flags & UsesSloppyEval; | ||
| symbolTable->m_nestedLexicalScope = flags & NestedLexicalScope; |
There was a problem hiding this comment.
🔴 CachedSymbolTable::decode assigns flags & NestedLexicalScope (value 0 or 2) to SymbolTable::m_nestedLexicalScope, which is an unsigned : 1 bit-field — 2 truncates to 0, so the flag is always lost on decode
Extended reasoning...
Any cached code block whose constant pool contains a SymbolTable for a nested lexical scope (a { let x; ... } block, for (let ...) head, catch binding, etc.) round-trips through the bytecode cache with m_nestedLexicalScope cleared to 0. On the base branch the field was stored as its own 1-bit member and restored correctly; after this change flags & (1 << 1) yields 2, and assigning 2 to an unsigned : 1 bit-field stores 2 mod 2 == 0. isNestedLexicalScope() is consulted by BytecodeGenerator/scope resolution (e.g. Annex B sloppy-mode function hoisting decisions), so cached code can behave differently from freshly-generated code. Fix: symbolTable->m_nestedLexicalScope = !!(flags & NestedLexicalScope); (the sibling UsesSloppyEval line is fine only because its mask happens to be 1).
Verification: normal — regression introduced by this change. At Source/JavaScriptCore/runtime/CachedTypes.cpp:1514 the flag is NestedLexicalScope = 1 << 1 (value 2), and at :1555 decode does symbolTable->m_nestedLexicalScope = flags & NestedLexicalScope;. The RHS is an int with value 0 or 2. The LHS is declared in Source/JavaScriptCore/runtime/SymbolTable.h:724 as unsigned m_nestedLexicalScope : 1;. Per
| static Vector<char16_t, 32> read16(Reader& reader, unsigned length) | ||
| { | ||
| Vector<char16_t, 32> characters; | ||
| characters.grow(length); |
There was a problem hiding this comment.
🔴 CachedString::read16 grows a Vector<char16_t> to length before any bounds check, unlike every other counted decode which guards with checkCount()/bytes()
Extended reasoning...
A single bit-flip in a 16-bit string record's length varuint (string records for function names sit outside any checksummed region) yields a huge head.length; characters.grow(length) at line 1131 then attempts a multi-GB allocation and crashes the process, whereas the PR's stated robustness contract (and every sibling path such as the 8-bit branch via reader.bytes()) is that an oversized count marks the record damaged so the function is regenerated from source. Fix: check reader.has(2 * static_cast<size_t>(length)) (or reader.checkCount(length)) and return an empty vector before growing.
Verification: normal — CachedTypes.cpp:1128-1137: read16 does characters.grow(length) at line 1131 with length taken straight from reader.varuint() (readHead, line 1122) before any bounds check; WTF::Vector::grow uses FastMalloc which crashes on OOM, so a varuint decoding to ~UINT32_MAX attempts an ~8 GB allocation and terminates the process. This is inconsistent with every sibling path in the file: - T
| template<typename CodeBlockType> | ||
| UnlinkedCodeBlock::RareData* CachedCodeBlock<CodeBlockType>::rareData(Decoder& decoder) const | ||
| { | ||
| if (!(m_layout.flags & LayoutHasRareData)) | ||
| return nullptr; | ||
| Reader reader(decoder.payload(), m_regionAt + m_layout.rareDataAt); | ||
| return CachedCodeBlockRareData::decode(reader, decoder); | ||
| } |
There was a problem hiding this comment.
🔴 CachedCodeBlock::rareData() discards its Reader's overran flag, so a damaged rare-data stream never marks the code block damaged — unlike constants/identifiers/jumpTargets/ownMembers, whose readers all feed intact &= !reader.overran() in decodeArrays()/decode().
Extended reasoning...
A code block's rare data contains a StringJumpTable or constantIdentifierSet whose StringRef points (by checksummed offset) at a string record written by an earlier block, and that record is corrupted on disk (the PR notes string records live outside the reading block's checksummed region). CachedString::decodeNonNull() substitutes emptyAtom() and calls reader.setOverran() on the rare-data Reader; every subsequent varuint on that reader yields 0, so the remaining rare-data sections (later switch tables, bit vectors, needsClassFieldInitializer, privateBrandRequirement) are silently zeroed. rareData() then returns this half-decoded RareData without checking overran(), the UnlinkedCodeBlock constructor installs it, and decode() returns the block as valid because decodeArrays()/decodeOwnMembers() use fresh Readers that never see the flag. Relative to the base (which had no overran-propagation at all) the new invariant "a damaged record is regenerated from source instead" is violated only for rare data: a switch(str) will match "" for the corrupted case and later switch tables vanish
Verification: nit — The core observation is correct: CachedCodeBlock::rareData() drops the reader's overran flag. At Source/JavaScriptCore/runtime/CachedTypes.cpp:3289-3295: ```cpp UnlinkedCodeBlock::RareData* CachedCodeBlock::rareData(Decoder& decoder) const { if (!(m_layout.flags & LayoutHasRareData)) return nullptr; Reader reader(decoder.payload(), m_regionAt + m_layout.rareD
1 participant
Rewrites the bytecode cache encoder/decoder so that the payload is a plain byte stream: every value is written by a primitive (
u8/u16/u32/u64little-endian, LEB128 varints, byte runs) and read back by the matching one. No C++ object is ever placed in the payload or read out of it by casting, so the format no longer depends on the compiler, ABI or CPU that produced it — which is whatbun build --compile --bytecode --target=<other platform>needs — and every byte is a function of what was encoded.The format is described at the top of
CachedTypes.cpp, and eachCached*type is now just the statement of one value's encoding: anencode(Writer&, Encoder&, const T&)and adecode(Reader&, Decoder&), with the layout spelled out in a comment above it. The decode-sideCachedCodeBlock/CachedFunctionExecutableare parsed records on the stack rather than views over payload bytes.VariableLengthObject,CachedPtr,CachedVector,CachedObjectand the in-place machinery are gone; the file is ~1,000 lines shorter.What stays the same (this keeps #494/#515's design, just written explicitly):
CachedBytecode::commitUpdatespatches them as little-endian fields; a function-update payload is encoded knowing where it will be appended so its offsets are absolute in the combined payload.What changes in the bytes:
u32s patched later (child executable records, lazily decoded bodies, expression info).StringRefis a tagged varuint: inline characters / external ordinal / offset of a string record (u8 flags, varuint length, u32 hash, varuint ordinal, characters). Latin-1 contents are always stored 8-bit.u8 kind+ an inline encoding per kind (varint, f64, StringRef, symbol table, butterfly, regexp, template object, bigint digits).sizeof(Op::Metadata)(UnlinkedMetadataTable::expandSteps, shared withfinalize()).CodeBlock::llintBaselineCalleeSaveSpaceAsVirtualRegisters()and rejects a payload where it differs (the one CPU property generated bytecode depends on; equal on x64 and arm64).Robustness: every read is bounds-checked; a count larger than the bytes left, a reference that does not point backwards, a shared offset that decoded as another type, a string that does not decode, or a null where the format has none marks the record damaged, and a damaged code block / executable is generated from source instead. Found by flipping random bytes in cache files; what remains uncaught is well-formed-but-wrong content in string records outside a checksummed region, as before.
Numbers (Linux x64 release, vs.
main):--forceDiskCache), the same with--diskCachePayloadIsPersistentForTesting, and plain: identical failure sets tomain(35/35/36, all pre-existing).Supersedes #389 (the layout rule is unnecessary once nothing is laid out by the compiler) and the
CachedTypes.cpppart of #505.