Repository navigation
windows: commit one page of stack per thread and check the stack sizes of the linked exe #45094
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
robobun
wants to merge
6
commits into
main
Choose a base branch
from
robobun/ca5656d7/windows-stack-commit
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+173
−8
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
9a2483a
windows: commit one page of stack per thread and check the linked header
robobun f70fd68
temp: measure the stack commit change on the Windows test machines
robobun 6f54640
Remove the Windows measurement files, note what a larger commit does …
robobun b46f3de
Merge remote-tracking branch 'origin/main' into robobun/ca5656d7/wind…
robobun e083680
io_thread_pool: name the stack size instead of explaining it in a com…
robobun e6f89ab
Merge remote-tracking branch 'origin/main' into robobun/ca5656d7/wind…
robobun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Windows users whose script has a large constant regexp tested against a constant string get an access-violation crash (exit 0xC0000005) that the base build does not have. The
/STACK:...,0x1000operand at scripts/build/flags.ts:1044 leaves a JIT worklist thread with a 4 KB committed stack plus one guard page, and Yarr's JIT prologue moves the stack pointer by the whole frame without a probe when DFGStrengthReductionPhase folds the match on that thread. Fix: before shipping a one-page header, make every thread that runs JIT-generated Yarr code safe for frames above one page: give the JIT worklist threads a commit size of their own (WTF Thread::create) or add a stack probe to the Yarr prologue, so the guard page cannot be skipped.Why this was flagged
A script with a constant regexp of many capture groups, e.g.
/^(a?){N}b$/written out, tested in a hot loop against a constant subject, so the DFG constant-folds the match on a JIT worklist thread via RegExp::matchConcurrently. With scripts/build/flags.ts:1044 now emitting/STACK:0x1200000,0x1000(WINDOWS_STACK_COMMIT at flags.ts:53), that thread is created by WTF with size 0 and starts with 4096 bytes committed and a single guard page; nothing pre-commits a JIT thread's stack (preCommitStackMemory only runs on JS threads). The Yarr JIT prologue subtracts the full frame from the stack pointer without a probe, so a frame larger than about 16 KB lands past the guard page and the process dies with 0xC0000005. On the base the header commits 0x200000 bytes, so the same frame lands on committed pages and the script runs. The hardening/stack checks in scripts/build/verify-binary.ts:636-644 only confirm the header values and cannot catch this. The PR description measures 15.6 KB passing and 31.3 KB, 46.9 KB and 125 KB failing on both x64 and arm64.Verification: On Windows, a script whose hot function tests a large constant regexp against a constant string, so the DFG folds the match on a JIT worklist thread. scripts/build/flags.ts:1044 now emits
/STACK:${WINDOWS_STACK_RESERVE},${WINDOWS_STACK_COMMIT}withWINDOWS_STACK_COMMIT = "0x1000"(flags.ts:53), replacing/STACK:0x1200000,0x200000. JIT-generated Yarr code emits no__chkstk-style probe.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is correct. It is the second bullet under Downsides, and the numbers quoted here are the ones measured for this PR.
The gap is in JSC. For a match on a compiler thread, Yarr takes its stack limit from the reserve of the thread, and nothing commits or probes the frame. Upstream JSC has the same gap with its own one-page header. The 2 MB commit covered it for frames below about 1.9 MB.
Both fixes named here are changes in oven-sh/WebKit:
JITWorklist::wakeThreads).The order is a decision for the maintainer: