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

deps: cherry-pick 09de996 from V8 upstream #11905

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@fhinkel
Member

fhinkel commented Mar 17, 2017

Original commit message:

[debugger] fix switch block source positions.

The switch statement itself is part of the switch block.
However, the source position of the statement is outside of
the block. This leads to confusion for the debugger, if the
switch block pushes a block context: the current context is
a block context, but the scope analysis based on the current
source position tells the debugger that we should be outside
the scope, so we should have the function context.

R=marja@chromium.org
BUG=v8:6085
Review-Url: https://codereview.chromium.org/2744213003
Cr-Commit-Position: refs/heads/master@{#43744}
Committed: https://chromium.googlesource.com/v8/v8/+/09de9969ccb9bc3bbd667788afad665b309d02f5

Fixes: #11746

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, v8

/cc @nodejs/v8

The test in #11746
does not crash with this fix anymore. But since it's a V8 fix, I think we don't need an extra regression test here.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Mar 17, 2017

Contributor

@fhinkel @hashseed can this be backported to 5.7? Is the bug relevant to 5.1 and 4.5?

Contributor

ofrobots commented Mar 17, 2017

@fhinkel @hashseed can this be backported to 5.7? Is the bug relevant to 5.1 and 4.5?

@hashseed

This comment has been minimized.

Show comment
Hide comment
@hashseed

hashseed Mar 17, 2017

Member

I merged this upstream to 5.7 already. There is also another fix that I merged with it, which fixes a similar problem.

Member

hashseed commented Mar 17, 2017

I merged this upstream to 5.7 already. There is also another fix that I merged with it, which fixes a similar problem.

@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel Mar 17, 2017

Member

The bug doesn't occur in Node v6. It does occur in Node v7 (V8 5.4). If the patch applies cleanly we should backport to v7.

Member

fhinkel commented Mar 17, 2017

The bug doesn't occur in Node v6. It does occur in Node v7 (V8 5.4). If the patch applies cleanly we should backport to v7.

@ofrobots

LGTM once you include a bump to V8 version.

deps: cherry-pick 09de996 from V8 upstream
Original commit message:
  [debugger] fix switch block source positions.

  The switch statement itself is part of the switch block.
  However, the source position of the statement is outside of
  the block. This leads to confusion for the debugger, if the
  switch block pushes a block context: the current context is
  a block context, but the scope analysis based on the current
  source position tells the debugger that we should be outside
  the scope, so we should have the function context.

  R=marja@chromium.org
  BUG=v8:6085
  Review-Url: https://codereview.chromium.org/2744213003
  Cr-Commit-Position: refs/heads/master@{#43744}
  Committed: https://chromium.googlesource.com/v8/v8/+/09de9969ccb9bc3bbd667788afad665b309d02f5

Fixes: #11746
@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel Mar 17, 2017

Member

Good catch, thanks! Bumped the version.

Member

fhinkel commented Mar 17, 2017

Good catch, thanks! Bumped the version.

@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel Mar 19, 2017

Member

Landed in dd8982d

Member

fhinkel commented Mar 19, 2017

Landed in dd8982d

@fhinkel fhinkel closed this Mar 19, 2017

fhinkel added a commit that referenced this pull request Mar 19, 2017

deps: cherry-pick 09de996 from V8 upstream
Original commit message:
  [debugger] fix switch block source positions.

  The switch statement itself is part of the switch block.
  However, the source position of the statement is outside of
  the block. This leads to confusion for the debugger, if the
  switch block pushes a block context: the current context is
  a block context, but the scope analysis based on the current
  source position tells the debugger that we should be outside
  the scope, so we should have the function context.

  R=marja@chromium.org
  BUG=v8:6085
  Review-Url: https://codereview.chromium.org/2744213003
  Cr-Commit-Position: refs/heads/master@{#43744}
  Committed: https://chromium.googlesource.com/v8/v8/+/09de9969ccb9bc3bbd667788afad665b309d02f5

Fixes: #11746
PR-URL: #11905
Fixes: #11746
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017

deps: cherry-pick 09de996 from V8 upstream
Original commit message:
  [debugger] fix switch block source positions.

  The switch statement itself is part of the switch block.
  However, the source position of the statement is outside of
  the block. This leads to confusion for the debugger, if the
  switch block pushes a block context: the current context is
  a block context, but the scope analysis based on the current
  source position tells the debugger that we should be outside
  the scope, so we should have the function context.

  R=marja@chromium.org
  BUG=v8:6085
  Review-Url: https://codereview.chromium.org/2744213003
  Cr-Commit-Position: refs/heads/master@{#43744}
  Committed: https://chromium.googlesource.com/v8/v8/+/09de9969ccb9bc3bbd667788afad665b309d02f5

Fixes: nodejs#11746
PR-URL: nodejs#11905
Fixes: nodejs#11746
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@italoacasas

This comment has been minimized.

Show comment
Hide comment
@italoacasas

italoacasas Mar 21, 2017

Member

This is not landing clearly in v7, git conflicts.

@fhinkel be aware, the V8 version in Node 7 is 5.5 not 5.4.

Member

italoacasas commented Mar 21, 2017

This is not landing clearly in v7, git conflicts.

@fhinkel be aware, the V8 version in Node 7 is 5.5 not 5.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment