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

Fix debugger and profiler functionality #4298

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@cjihrig
Contributor

cjihrig commented Dec 15, 2015

This PR contains two commits. The first is a fix for #4297, which restores the old module wrapping behavior. It is my hope that this commit can be reverted in the not so distant future. The second commit is a regression test for #4297.

module,src: do not wrap modules with -1 lineOffset
In b799a74 and
dfee4e3 the module wrapping
mechanism was changed for better error reporting. However,
the changes causes issues with debuggers and profilers. This
commit reverts the wrapping changes.

Fixes: #4297

@silverwind silverwind added the debugger label Dec 15, 2015

@bnoordhuis

View changes

test/parallel/test-vm-debug-context.js Outdated
const Debug = vm.runInDebugContext('Debug');
const fn = require(common.fixturesDir + '/exports-function-with-param');
Debug.setListener(common.mustCall(function(e, s, d) {}));

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 15, 2015

Member

Is the common.mustCall(...) reliable here? In my experience the listener frequently gets called for more than just breakpoint events. I'd check manually that it gets called at least once with d.constructor.name === 'BreakEvent'. Maybe give it a more descriptive name than just d. :-)

EDIT: Or check that e === Debug.DebugEvent.Break.

test: add test for debugging one line files
This commit adds a regression test for debugging of
single line files.

Refs: #4297
@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Dec 16, 2015

@bnoordhuis updated based on your comment.

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Dec 16, 2015

LGTM if CI etc.

@silverwind silverwind added the relnotes label Dec 16, 2015

@ofrobots

This comment has been minimized.

Contributor

ofrobots commented Dec 16, 2015

cjihrig added a commit that referenced this pull request Dec 16, 2015

module,src: do not wrap modules with -1 lineOffset
In b799a74 and
dfee4e3 the module wrapping
mechanism was changed for better error reporting. However,
the changes causes issues with debuggers and profilers. This
commit reverts the wrapping changes.

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

cjihrig added a commit that referenced this pull request Dec 16, 2015

test: add test for debugging one line files
This commit adds a regression test for debugging of
single line files.

Refs: #4297
PR-URL: #4298
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Dec 16, 2015

Landed in 2a60e2a and cb0b4a6.

@cjihrig cjihrig closed this Dec 16, 2015

@cjihrig cjihrig deleted the cjihrig:4297 branch Dec 16, 2015

cjihrig added a commit that referenced this pull request Dec 16, 2015

module,src: do not wrap modules with -1 lineOffset
In b799a74 and
dfee4e3 the module wrapping
mechanism was changed for better error reporting. However,
the changes causes issues with debuggers and profilers. This
commit reverts the wrapping changes.

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

cjihrig added a commit that referenced this pull request Dec 16, 2015

test: add test for debugging one line files
This commit adds a regression test for debugging of
single line files.

Refs: #4297
PR-URL: #4298
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

@ofrobots ofrobots referenced this pull request Dec 17, 2015

Closed

Release a v5.2.1? #4316

@rvagg rvagg referenced this pull request Dec 17, 2015

Closed

Release 4.2.4 Planning #4321

@jasnell

This comment has been minimized.

Member

jasnell commented Dec 17, 2015

@cjihrig ... does this need to go into v4?

@jasnell

This comment has been minimized.

Member

jasnell commented Dec 17, 2015

(I'm assuming not)

@ofrobots

This comment has been minimized.

Contributor

ofrobots commented Dec 17, 2015

This is a fix for a regression in 5.2.0. This does not need to go into v4.

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Dec 17, 2015

If you are backporting #4254 and/or #2867, then this needs to be backported as well.

@rvagg

This comment has been minimized.

Member

rvagg commented Jan 15, 2016

If you are backporting #4254 and/or #2867, then this needs to be backported as well.

It looks like #4254 got in to v4.x already and #2867 has an lts-watch label on it. Can we have some clarification on whether this needs to go back to v4.x or not? @cjihrig, @ofrobots, @jasnell

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Jan 15, 2016

TL;DR yes, this should get backported.

#4254 and #2867 fixed a slight issue with line numbers on error messages. However, it exposed what appears to be a bug in V8 (https://bugs.chromium.org/p/v8/issues/detail?id=4620). This PR is a partial revert of #4254 and #2867.

@ofrobots

This comment has been minimized.

Contributor

ofrobots commented Jan 15, 2016

I agree with @cjihrig.

MylesBorins added a commit that referenced this pull request Jan 21, 2016

module,src: do not wrap modules with -1 lineOffset
In b799a74 and
dfee4e3 the module wrapping
mechanism was changed for better error reporting. However,
the changes causes issues with debuggers and profilers. This
commit reverts the wrapping changes.

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

MylesBorins added a commit that referenced this pull request Jan 21, 2016

test: add test for debugging one line files
This commit adds a regression test for debugging of
single line files.

Refs: #4297
PR-URL: #4298
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

MylesBorins added a commit that referenced this pull request Jan 21, 2016

module,src: do not wrap modules with -1 lineOffset
In b799a74 and
dfee4e3 the module wrapping
mechanism was changed for better error reporting. However,
the changes causes issues with debuggers and profilers. This
commit reverts the wrapping changes.

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

MylesBorins added a commit that referenced this pull request Jan 21, 2016

test: add test for debugging one line files
This commit adds a regression test for debugging of
single line files.

Refs: #4297
PR-URL: #4298
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

This was referenced Jan 21, 2016

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

module,src: do not wrap modules with -1 lineOffset
In b799a74 and
dfee4e3 the module wrapping
mechanism was changed for better error reporting. However,
the changes causes issues with debuggers and profilers. This
commit reverts the wrapping changes.

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

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

test: add test for debugging one line files
This commit adds a regression test for debugging of
single line files.

Refs: nodejs#4297
PR-URL: nodejs#4298
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment