Skip to content

fix(jsc): initialize new VM-entry scratch storage - #795

Open
steipete wants to merge 1 commit into
oven-sh:mainfrom
steipete:upstream/vm-entry-record-initialization
Open

steipete wants to merge 1 commit into
oven-sh:mainfrom
steipete:upstream/vm-entry-record-initialization

Conversation

@steipete

Copy link
Copy Markdown

What does this PR do?

An otherwise dead JavaScript value can remain alive when a fresh VM-entry frame reuses native stack bytes containing its address. LLInt writes the entry record's header but leaves its callee-save scratch and trailing alignment storage untouched; conservative stack scanning can treat those stale words as roots.

Clear that storage when creating the 64-bit generic and fixed-arity entry records. The address lookup helper stays side-effect free, and later unwind/OSR writers retain ownership of active records. The native regression poisons the future record immediately before calling the real entry stub and captures its contents at the callee's first instruction. It covers all seven fixed arities, generic JavaScript and native entry, header preservation, pure lookup, nested/outer-record preservation and an ordinary JavaScript exception.

Ported from the entry-record portion of openclaw/WebKit#24 and #26. Related #694 covers deferred VMEntryScope optional storage, a separate retaining owner; this PR makes no change to that representation. The same entry allocation is present in canonical WebKit's JavaScriptCore.

How did you verify your code works?

Built the same native regression on upstream main at 5d2ceba2921d29312827883b1fafa5c7cd5cba7e, then rebuilt with only the two LLInt assembly files changed. The Linux x86-64 build used LLVM 23.1.2, Release (-O2 -g1), static JSC, Bun additions/event loop, FTL and external mimalloc.

cmake --build build --target testVMEntryRecord
build/bin/testVMEntryRecord
Check Upstream main with regression With this fix
Seven fixed-arity entries, generic JavaScript entry, generic native entry All nine retain all six poisoned scratch/alignment words All nine clear all six words
Nested fresh entry Retains poison Clears poison
Headers, address lookup, outer active record Pass Pass
Ordinary JavaScript exception handling Pass Pass
Process exit 1 0

The control and candidate each ran once; a completed failing test was not retried. check-webkit-style reports zero errors in the five changed files, and git diff --check passes.

The standalone testVMEntryRecord target uses Linux x86-64 SysV assembly and the native LLInt. Runtime coverage of other architectures is not claimed. Historical downstream catalog rates involved both entry-record and deferred-scope changes and are not used as proof for this isolated change.

Clear callee-save scratch and trailing alignment words when creating a 64-bit VM entry record, preventing stale pointers from becoming conservative GC roots. Cover generic and fixed-arity entry with native poison, nested-record, and unwind checks.

@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.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai

coderabbitai Bot commented Oct 12, 2026

Copy link
Copy Markdown

Warning

Review limit reached

  • Run on-demand review

This review includes 5 billable files and costs up to $1.25.

Or wait 47 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 206c4aee-a414-4478-8388-0a40a081954f

📥 Commits

Reviewing files that changed from the base of the PR and between 5d2ceba and 4161f90.


📒 Files selected for processing (5)
  • Source/JavaScriptCore/API/tests/VMEntryRecordProbeX86_64.S
  • Source/JavaScriptCore/API/tests/testVMEntryRecord.cpp
  • Source/JavaScriptCore/llint/LowLevelInterpreter.asm
  • Source/JavaScriptCore/llint/LowLevelInterpreter64.asm
  • Source/JavaScriptCore/shell/CMakeLists.txt

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

Comment @coderabbitai help to get the list of available commands.

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