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

inspector: break in eval script #14581

Merged
merged 1 commit into from
Sep 20, 2017
Merged

inspector: break in eval script #14581

merged 1 commit into from
Sep 20, 2017

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Aug 1, 2017

Fixes: #14577

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)

eval scripts are now wrapped into inspector call to break on a first line.

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 1, 2017
@eugeneo
Copy link
Contributor Author

eugeneo commented Aug 1, 2017

What I don't like about this solution is that the wrapper is reported to frontend. E.g. running console.log(3) shows the following source:

process.binding('inspector').callAndPauseOnStart(function() {

console.log(3);

}, {})

Alternative would be to modify the node_contextify.cc to add a parameter to break on the first line of the code ran in the context. I am not sure this is something that should be done for just debugging the eval scripts.

@eugeneo eugeneo added the inspector Issues and PRs related to the V8 inspector protocol label Aug 1, 2017
addaleax
addaleax reviewed Aug 1, 2017
View reviewed changes
lib/internal/bootstrap_node.js
@@ -420,7 +427,7 @@
const module = new Module(name);
module.filename = path.join(cwd, name);
module.paths = Module._nodeModulePaths(cwd);
const body = process._eval;
const body = wrapForBreakOnFirstLine(process._eval);
const script = `global.__filename = ${JSON.stringify(name)};\n` +
Copy link
Member

Choose a reason for hiding this comment

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

would it work better if you applied wrapForBreakOnFirstLine to script, not to body?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then the entire block will be reported as source and actual user code will be a string - e.g. it will not be possible to set a breakpoint there. Stepping into the vm module will be required in order to step into the user code.

@eugeneo eugeneo changed the title inspector: break in eval scripto inspector: break in eval script Aug 1, 2017
@BridgeAR
Copy link
Member

BridgeAR commented Sep 2, 2017

PTAL @nodejs/v8-inspector

@BridgeAR
Copy link
Member

@nodejs/collaborators PTAL

@addaleax
Copy link
Member

LGTM. I feel like there could be a few weird edge cases with this approach, but if so I guess just seeing when they show up is fine.

CI: https://ci.nodejs.org/job/node-test-commit/12414/

@addaleax
Copy link
Member

@eugeneo I think this might need to be updated

@eugeneo eugeneo added the wip Issues and PRs that are still a work in progress. label Sep 18, 2017
@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 18, 2017

I added "in progress" label as this PR needs some rework due to changes elsewhere.

@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 18, 2017

As it turns out, a number of tests were relying on this bug. I updated them to expect the new breakpoint and adjusted the line numbers.

CI: https://ci.nodejs.org/job/node-test-pull-request/10127/

@eugeneo eugeneo removed the wip Issues and PRs that are still a work in progress. label Sep 18, 2017
@addaleax
Copy link
Member

@eugeneo by the way, the typo is still in the commit message … ;)

addaleax
addaleax approved these changes Sep 18, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Still LGTM

@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 19, 2017

Another CI attempt - https://ci.nodejs.org/job/node-test-pull-request/10146/

@eugeneo eugeneo added the wip Issues and PRs that are still a work in progress. label Sep 19, 2017
@joaocgreis
Copy link
Member

Windows CI failed because of an issue that's already fixed. Here is another run with the same parameters: https://ci.nodejs.org/job/node-test-commit-windows-fanned/11803/

inspector: break in eval script
75606c4 
Fixes: #14577
PR-URL: #14581
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 20, 2017

This CI does not seems to indicate any regressions caused by this PR: https://ci.nodejs.org/job/node-test-pull-request/10147/

@eugeneo eugeneo merged commit 75606c4 into nodejs:master Sep 20, 2017
@eugeneo eugeneo deleted the break_in_eval branch September 20, 2017 16:23
@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 20, 2017

Landed as 75606c4

jasnell pushed a commit that referenced this pull request Sep 20, 2017
@jasnell
inspector: break in eval script
25692a9 
Fixes: #14577
PR-URL: #14581
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell mentioned this pull request Sep 20, 2017
Merged
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
inspector: break in eval script
ec44d2c 
Fixes: nodejs/node#14577
PR-URL: nodejs/node#14581
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
inspector: break in eval script
0d206fd 
Fixes: nodejs/node#14577
PR-URL: nodejs/node#14581
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@joaocgreis joaocgreis mentioned this pull request Jul 31, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@benjamingr benjamingr benjamingr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
inspector Issues and PRs related to the V8 inspector protocol lib / src Issues and PRs related to general changes in the lib or src directory. wip Issues and PRs that are still a work in progress.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

--inspector-brk does not stop in code passed with the -e flag
8 participants
@eugeneo @BridgeAR @addaleax @joaocgreis @jasnell @benjamingr @MylesBorins @nodejs-github-bot