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.

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

src: introduce inspect-brk-node #20819

Closed
wants to merge 2 commits into from
Closed

Conversation

danbev
Copy link
Contributor

@danbev danbev commented May 18, 2018

This commit is a suggestion to add a new option to the node executable
to allow for breaking in node's javascript bootstrapper code.

Previously I've been able to set breakpoints in node bootstrapper code
and then restart the debugging session and those breakpoints would be
hit, but I don't seem to be able to do so anymore. Having this option
would allow me to use this option and then step through or add more
break points as needed.

Currently test are missing as I wanted to see if this is worth
pursuing first.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels May 18, 2018
@danbev
Copy link
Contributor Author

danbev commented May 18, 2018

@bnoordhuis
Copy link
Member

Is there another way to accomplish this? I don't really like the idea of adding a new option (certainly not a documented option) for functionality that is probably only relevant to core developers.

@danbev
Copy link
Contributor Author

danbev commented May 18, 2018

Is there another way to accomplish this?

I'm not sure and part of the reason for opening this pr. Hopefully someone more familiar can chime in if there are other ways to achieve this.

@eugeneo
Copy link
Contributor

eugeneo commented May 21, 2018

Your original approach (setting a breakpoint) should still work. We need to look into why it does not. Another option is to add a "debugger" statement into the bootstrap code.

@danbev
Copy link
Contributor Author

danbev commented May 22, 2018

Your original approach (setting a breakpoint) should still work.

Would you be able to try this out just to make sure this is not something local to my environment?

Another option is to add a "debugger" statement into the bootstrap code.

That works for sure, the only downside is that it requires a recompilation when added to any of the core javascript sources.

@BridgeAR
Copy link
Member

Adding the @nodejs/tsc review label to get some more opinions.

src/node.cc Outdated
@@ -3517,6 +3523,8 @@ static void PrintHelp() {
" --inspect-brk[=[host:]port]\n"
" activate inspector on host:port\n"
" and break at start of user script\n"
" --inspect-brk-node will break in node's bootstrap code\n"
" (default: false)\n"
Copy link
Member

Choose a reason for hiding this comment

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

If it's only relevant to Node core developers, I do not think we should really document this.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can document this somewhere in doc/guide?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this seems to fall along the same lines as --expose-internals... we don't document that one for similar reasons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove the info from PrintHelp and take a look at adding this to doc/guide. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've not really been able to find a good place for this in any of the docs in doc/guide. @joyeecheung Do you have any suggestions where this might go?

Would it be alright if it is not documented as that seems to be the case for --expose-internals?

@danbev danbev added wip Issues and PRs that are still a work in progress. and removed wip Issues and PRs that are still a work in progress. labels May 30, 2018
@danbev
Copy link
Contributor Author

danbev commented Jun 5, 2018

node-test-commit-freebsd failure looks unrelated

console output:

HTTP ERROR 404
Problem accessing /job/node-test-commit-freebsd/nodes=freebsd11-x64/17862/. Reason:

    Not Found

danbev added 2 commits June 7, 2018 13:10
This commit is a suggestion to add a new option to the node executable
to allow for breaking in node's javascript bootstrapper code.

Previously I've been able to set breakpoints in node bootstrapper code
and then restart the debugging session and those breakpoints would be
hit, but I don't seem to be able to do so anymore. Having this option
would allow me to use this option and then step through or add more
break points as needed.

$ ./node --help
Usage: node [options] [ -e script | script.js | - ] [arguments]
       node inspect script.js [arguments]

Options:
...
  --inspect-brk-node         will break in node's bootstrap code
                             (default: false)
...

Currently test are missing as I wanted to see if this is worth
pursuing first.
@danbev danbev force-pushed the inspect-brk-node branch from a4f0bbd to f367591 Compare June 7, 2018 11:30
@danbev
Copy link
Contributor Author

danbev commented Jun 22, 2018

Landed in 9ca41e9.

@danbev danbev closed this Jun 22, 2018
@danbev danbev deleted the inspect-brk-node branch June 22, 2018 03:38
danbev added a commit that referenced this pull request Jun 22, 2018
This commit is a suggestion to add a new option to the node executable
to allow for breaking in node's javascript bootstrapper code.

Previously I've been able to set breakpoints in node bootstrapper code
and then restart the debugging session and those breakpoints would be
hit, but I don't seem to be able to do so anymore. Having this option
would allow me to use this option and then step through or add more
break points as needed.

PR-URL: #20819
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jun 22, 2018
This commit is a suggestion to add a new option to the node executable
to allow for breaking in node's javascript bootstrapper code.

Previously I've been able to set breakpoints in node bootstrapper code
and then restart the debugging session and those breakpoints would be
hit, but I don't seem to be able to do so anymore. Having this option
would allow me to use this option and then step through or add more
break points as needed.

PR-URL: #20819
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Jul 3, 2018
@danbev danbev mentioned this pull request Mar 15, 2019
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants