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.

Sign up for GitHub

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

Jump to bottom

deps: backport 75f2d65f00 from upstream V8 #12535

Closed
wants to merge 1 commit into from
Closed

deps: backport 75f2d65f00 from upstream V8 #12535

wants to merge 1 commit into from

Conversation

hashseed
Copy link
Member

Original commit message:

Don't treat catch scopes as possibly-shadowing for sloppy eval

Scope analysis is over-conservative when treating variable
resolutions as possibly-shadowed by a sloppy eval. In the attached
bug, this comes into play since catch scopes have different behavior
with respect to the "calls eval" in eager vs lazy compilation (in
the latter, they are never marked as "calls eval" because
CatchContexts don't have an associated ScopeInfo).

This patch changes the scope-type check to also eliminate a few
other cases where shadowing isn't possible, such as non-declaration
block scopes.

BUG=chromium:608279
LOG=n

Committed:
https://crrev.com/75f2d65f003ebb22815489e9970913ba37234f1b
Cr-Commit-Position: refs/heads/master@{#36046}

Fixes: #12308

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

deps

@hashseed
deps: backport 75f2d65f00 from upstream V8
e15d3c0 
Original commit message:

    Don't treat catch scopes as possibly-shadowing for sloppy eval

    Scope analysis is over-conservative when treating variable
    resolutions as possibly-shadowed by a sloppy eval. In the attached
    bug, this comes into play since catch scopes have different behavior
    with respect to the "calls eval" in eager vs lazy compilation (in
    the latter, they are never marked as "calls eval" because
    CatchContexts don't have an associated ScopeInfo).

    This patch changes the scope-type check to also eliminate a few
    other cases where shadowing isn't possible, such as non-declaration
    block scopes.

    BUG=chromium:608279
    LOG=n

    Committed:
    https://crrev.com/75f2d65f003ebb22815489e9970913ba37234f1b
    Cr-Commit-Position: refs/heads/master@{nodejs#36046}

Fixes: nodejs#12308
@nodejs-github-bot nodejs-github-bot added v6.x v8 engine Issues and PRs related to the V8 dependency. labels Apr 20, 2017
mhdawson
mhdawson reviewed Apr 20, 2017
View reviewed changes
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

@mhdawson
Copy link
Member

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/661/

bnoordhuis
bnoordhuis approved these changes Apr 20, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM but FWIW, I can't get the test to fail with node.js v6.10.2.

@hashseed
Copy link
Member Author

Only fails on debug build.

@MylesBorins MylesBorins changed the base branch from v6.x to v6.x-staging April 24, 2017 01:23
MylesBorins
MylesBorins approved these changes Apr 24, 2017
View reviewed changes
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

Rubber Stamp LGTM

@MylesBorins
Copy link
Contributor

MylesBorins commented Apr 24, 2017 •
edited
Loading

landed in 404d1bc

MylesBorins pushed a commit that referenced this pull request Apr 24, 2017
@hashseed @MylesBorins
deps: backport 75f2d65f00 from upstream V8
404d1bc 
Original commit message:

    Don't treat catch scopes as possibly-shadowing for sloppy eval

    Scope analysis is over-conservative when treating variable
    resolutions as possibly-shadowed by a sloppy eval. In the attached
    bug, this comes into play since catch scopes have different behavior
    with respect to the "calls eval" in eager vs lazy compilation (in
    the latter, they are never marked as "calls eval" because
    CatchContexts don't have an associated ScopeInfo).

    This patch changes the scope-type check to also eliminate a few
    other cases where shadowing isn't possible, such as non-declaration
    block scopes.

    BUG=chromium:608279
    LOG=n

    Committed:
    https://crrev.com/75f2d65f003ebb22815489e9970913ba37234f1b
    Cr-Commit-Position: refs/heads/master@{#36046}

Fixes: #12308

PR-URL: #12535
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 24, 2017
@hashseed @MylesBorins
deps: backport 75f2d65f00 from upstream V8
cd78a2b 
Original commit message:

    Don't treat catch scopes as possibly-shadowing for sloppy eval

    Scope analysis is over-conservative when treating variable
    resolutions as possibly-shadowed by a sloppy eval. In the attached
    bug, this comes into play since catch scopes have different behavior
    with respect to the "calls eval" in eager vs lazy compilation (in
    the latter, they are never marked as "calls eval" because
    CatchContexts don't have an associated ScopeInfo).

    This patch changes the scope-type check to also eliminate a few
    other cases where shadowing isn't possible, such as non-declaration
    block scopes.

    BUG=chromium:608279
    LOG=n

    Committed:
    https://crrev.com/75f2d65f003ebb22815489e9970913ba37234f1b
    Cr-Commit-Position: refs/heads/master@{#36046}

Fixes: #12308

PR-URL: #12535
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 24, 2017
@hashseed @MylesBorins
deps: backport 75f2d65f00 from upstream V8
99dab5b 
Original commit message:

    Don't treat catch scopes as possibly-shadowing for sloppy eval

    Scope analysis is over-conservative when treating variable
    resolutions as possibly-shadowed by a sloppy eval. In the attached
    bug, this comes into play since catch scopes have different behavior
    with respect to the "calls eval" in eager vs lazy compilation (in
    the latter, they are never marked as "calls eval" because
    CatchContexts don't have an associated ScopeInfo).

    This patch changes the scope-type check to also eliminate a few
    other cases where shadowing isn't possible, such as non-declaration
    block scopes.

    BUG=chromium:608279
    LOG=n

    Committed:
    https://crrev.com/75f2d65f003ebb22815489e9970913ba37234f1b
    Cr-Commit-Position: refs/heads/master@{#36046}

Fixes: #12308

PR-URL: #12535
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@hashseed hashseed deleted the fix-12308 branch April 25, 2017 05:08
@MylesBorins MylesBorins mentioned this pull request Apr 29, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@hashseed @andrew749
deps: backport 75f2d65f00 from upstream V8
43d93fa 
Original commit message:

    Don't treat catch scopes as possibly-shadowing for sloppy eval

    Scope analysis is over-conservative when treating variable
    resolutions as possibly-shadowed by a sloppy eval. In the attached
    bug, this comes into play since catch scopes have different behavior
    with respect to the "calls eval" in eager vs lazy compilation (in
    the latter, they are never marked as "calls eval" because
    CatchContexts don't have an associated ScopeInfo).

    This patch changes the scope-type check to also eliminate a few
    other cases where shadowing isn't possible, such as non-declaration
    block scopes.

    BUG=chromium:608279
    LOG=n

    Committed:
    https://crrev.com/75f2d65f003ebb22815489e9970913ba37234f1b
    Cr-Commit-Position: refs/heads/master@{#36046}

Fixes: nodejs#12308

PR-URL: nodejs/node#12535
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hashseed @mhdawson @MylesBorins @bnoordhuis @cjihrig @nodejs-github-bot