Skip to content
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

v8: fix stack overflow in recursive method (v4.x) #13080

Closed

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented May 17, 2017

HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes. Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
PR-URL: #12460
Reviewed-By: James M Snell [email protected]
Reviewed-By: Yang Guo [email protected]

CI: https://ci.nodejs.org/job/node-test-pull-request/8120/
CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/704/

HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: nodejs#11991
PR-URL: nodejs#12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
@nodejs-github-bot nodejs-github-bot added v4.x v8 engine Issues and PRs related to the V8 dependency. labels May 17, 2017
Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

Thanks Ben

@gibfahn
Copy link
Member

gibfahn commented May 17, 2017

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

MylesBorins pushed a commit that referenced this pull request Jul 10, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
Backport-PR-URL: #13080
PR-URL: #12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
@MylesBorins
Copy link
Contributor

landed in 161dce3

MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
Backport-PR-URL: #13080
PR-URL: #12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
Backport-PR-URL: #13080
PR-URL: #12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
gibfahn pushed a commit to ibmruntimes/node that referenced this pull request Nov 24, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: nodejs/node#11991
Backport-PR-URL: nodejs/node#13080
PR-URL: nodejs/node#12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants