debugger: check -e flag in debug break setup #8876

kjin · 2016-10-01T04:48:42Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

module

Description of change

When both --debug-brk and --eval are set, and a filename is
specified, its full path is not set correctly, causing an error
for relative filenames with './' omitted.

For example, 'node --debug-brk -e 0 hello.js' throws an error.

Since the script referenced by the filename is never run anyway,
this change skips resolving its full path if both --debug-brk and
--eval are set.

mscdex · 2016-10-01T05:31:19Z

lib/module.js

@@ -61,7 +61,6 @@ Module._debug = util.debuglog('module');
 // We use this alias for the preprocessor that filters it out
 const debug = Module._debug;

-


Unnecessary whitespace change

Whoops, updated!

When both --debug-brk and --eval are set, and a filename is specified, its full path is not set correctly, causing an error for relative filenames with './' omitted. For example, 'node --debug-brk -e 0 hello.js' throws an error. Since the script referenced by the filename is never run anyway, this change skips resolving its full path if both --debug-brk and --eval are set.

bnoordhuis

Can you add a check that verifies the 'foo' argument is not evaluated? Basically, that node --debug-brk -p process.argv[1] foo prints 'foo'.

kjin · 2016-10-02T19:16:49Z

@bnoordhuis I've modified test-debug-brk.js to match the contents of stdout - it checks that node --debug-brk -p process.argv[1] foo does indeed print 'foo'.

bnoordhuis · 2016-10-03T09:42:37Z

test/parallel/test-debug-brk.js

+    proc.stderr.on('data', (chunk) => {
+      procStderr += chunk;
+      debuggerListening = debuggerListening ||
+        /Debugger listening on/.test(procStderr);


Tiny style nit: line continuations should have four spaces of indent. The linter is supposed to check that but it's possible it gets thrown off by the binary operator.

bnoordhuis · 2016-10-03T09:43:51Z

test/parallel/test-debug-brk.js

+
+test(['-e', '0']);
+test(['-e', '0', 'foo']);
+test(['-p', 'process.argv[1]', 'foo'], /foo/);


Can you make this a little stricter? E.g. /^\s*foo\s*$/.

kjin · 2016-10-03T17:53:46Z

@bnoordhuis Thanks for taking a look - I've made the requested changes, make lint is error-free.

jasnell

LGTM if @bnoordhuis and @mscdex are also good with it.

bnoordhuis

LGTM too.

jasnell · 2016-10-06T21:58:56Z

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

jasnell · 2016-10-07T17:47:29Z

CI failures appear to be unrelated flaky tests. @mscdex does this LGTY?

jasnell · 2016-10-26T20:15:22Z

ping @mscdex

mscdex · 2016-10-26T20:22:54Z

@jasnell I'm not familiar enough with the debugger to really sign off on the (non-test) change, but as far as the one whitespace change I pointed out is concerned, @kjin removed that.

kjin · 2016-11-14T19:11:44Z

@jasnell It seems like there are no significant build problems with this change, is it ready to be merged in?

matthewloring · 2016-11-15T21:46:30Z

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

matthewloring · 2016-11-15T21:56:22Z

Thanks! Landed in ebff29f.

When both --debug-brk and --eval are set, and a filename is specified, its full path is not set correctly, causing an error for relative filenames with './' omitted. For example, 'node --debug-brk -e 0 hello.js' throws an error. Since the script referenced by the filename is never run anyway, this change skips resolving its full path if both --debug-brk and --eval are set. PR-URL: #8876 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2016-12-13T20:50:37Z

this is not landing cleanly on v4.x. Can someone please do a manual backport?

When both --debug-brk and --eval are set, and a filename is specified, its full path is not set correctly, causing an error for relative filenames with './' omitted. For example, 'node --debug-brk -e 0 hello.js' throws an error. Since the script referenced by the filename is never run anyway, this change skips resolving its full path if both --debug-brk and --eval are set. PR-URL: #8876 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

This LTS release comes with 312 commits. This includes 229 that are test related, 62 that are docs related, 17 which are build / tools related, and 4 commits which are updates to dependencies. Notable Changes: * build: - shared library support is now working for AIX builds (Stewart Addison) #9675 * deps: - *npm*: upgrade npm to 3.10.10 (Rebecca Turner) #9847 - *V8*: Destructuring of arrow function arguments via computed property no longer throws (Michaël Zasso) #10386) * inspector: - /json/version returns object, not an object wrapped in an array (Ben Noordhuis) #9762 * module: - using --debug-brk and --eval together now works as expected (Kelvin Jin) #8876 * process: - improve performance of nextTick up to 20% (Evan Lucas) #8932 * repl: - the division operator will no longer be accidentally parsed as regex (Teddy Katz) #10103 - improved support for generator functions (Teddy Katz) #9852 * timers: - Re canceling a cancelled timers will no longer throw (Jeremiah Senkpiel) #9685 PR-URL: #10394

This LTS release comes with 312 commits. This includes 229 that are test related, 62 that are docs related, 17 which are build / tools related, and 4 commits which are updates to dependencies. Notable Changes: * build: - shared library support is now working for AIX builds (Stewart Addison) nodejs/node#9675 * deps: - *npm*: upgrade npm to 3.10.10 (Rebecca Turner) nodejs/node#9847 - *V8*: Destructuring of arrow function arguments via computed property no longer throws (Michaël Zasso) nodejs/node#10386) * inspector: - /json/version returns object, not an object wrapped in an array (Ben Noordhuis) nodejs/node#9762 * module: - using --debug-brk and --eval together now works as expected (Kelvin Jin) nodejs/node#8876 * process: - improve performance of nextTick up to 20% (Evan Lucas) nodejs/node#8932 * repl: - the division operator will no longer be accidentally parsed as regex (Teddy Katz) nodejs/node#10103 - improved support for generator functions (Teddy Katz) nodejs/node#9852 * timers: - Re canceling a cancelled timers will no longer throw (Jeremiah Senkpiel) nodejs/node#9685 PR-URL: nodejs/node#10394 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

nodejs-github-bot added the module Issues and PRs related to the module subsystem. label Oct 1, 2016

mscdex added the debugger label Oct 1, 2016

mscdex reviewed Oct 1, 2016

View reviewed changes

mscdex added process Issues and PRs related to the process subsystem. and removed module Issues and PRs related to the module subsystem. labels Oct 1, 2016

kjin force-pushed the dbg-brk-fix branch from 223d5f4 to 9f22b8c Compare October 1, 2016 06:04

bnoordhuis requested changes Oct 1, 2016

View reviewed changes

bnoordhuis requested changes Oct 3, 2016

View reviewed changes

kjin force-pushed the dbg-brk-fix branch from 0d71de0 to 37eabc4 Compare October 3, 2016 17:51

test checks output

54be1e4

kjin force-pushed the dbg-brk-fix branch from 37eabc4 to 54be1e4 Compare October 3, 2016 17:53

kjin changed the title ~~module: check -e flag in debug break setup~~ debugger: check -e flag in debug break setup Oct 3, 2016

jasnell approved these changes Oct 6, 2016

View reviewed changes

bnoordhuis approved these changes Oct 6, 2016

View reviewed changes

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

matthewloring closed this Nov 15, 2016

MylesBorins added land-on-v6.x labels Dec 13, 2016

MylesBorins mentioned this pull request Dec 21, 2016

V6.9.3 proposal #10394

Merged

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Mar 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

debugger: check -e flag in debug break setup #8876

debugger: check -e flag in debug break setup #8876

kjin commented Oct 1, 2016

mscdex Oct 1, 2016

kjin Oct 1, 2016

bnoordhuis left a comment

kjin commented Oct 2, 2016

bnoordhuis Oct 3, 2016

bnoordhuis Oct 3, 2016

kjin commented Oct 3, 2016

jasnell left a comment

bnoordhuis left a comment

jasnell commented Oct 6, 2016

jasnell commented Oct 7, 2016

jasnell commented Oct 26, 2016

mscdex commented Oct 26, 2016 •

edited

Loading

kjin commented Nov 14, 2016

matthewloring commented Nov 15, 2016

matthewloring commented Nov 15, 2016

MylesBorins commented Dec 13, 2016

		@@ -61,7 +61,6 @@ Module._debug = util.debuglog('module');
		// We use this alias for the preprocessor that filters it out
		const debug = Module._debug;

debugger: check -e flag in debug break setup #8876

debugger: check -e flag in debug break setup #8876

Conversation

kjin commented Oct 1, 2016

Checklist

Affected core subsystem(s)

Description of change

mscdex Oct 1, 2016

Choose a reason for hiding this comment

kjin Oct 1, 2016

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

kjin commented Oct 2, 2016

bnoordhuis Oct 3, 2016

Choose a reason for hiding this comment

bnoordhuis Oct 3, 2016

Choose a reason for hiding this comment

kjin commented Oct 3, 2016

jasnell left a comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

jasnell commented Oct 6, 2016

jasnell commented Oct 7, 2016

jasnell commented Oct 26, 2016

mscdex commented Oct 26, 2016 • edited Loading

kjin commented Nov 14, 2016

matthewloring commented Nov 15, 2016

matthewloring commented Nov 15, 2016

MylesBorins commented Dec 13, 2016

mscdex commented Oct 26, 2016 •

edited

Loading