Skip to content

src, test: node internals' postmortem metadata #14901

Closed
mmarchini wants to merge 2 commits into
nodejs:masterfrom
mmarchini:debugging_symbols
Closed

src, test: node internals' postmortem metadata #14901
mmarchini wants to merge 2 commits into
nodejs:masterfrom
mmarchini:debugging_symbols

Conversation

@mmarchini
Copy link
Copy Markdown
Contributor

@mmarchini mmarchini commented Aug 17, 2017 •
edited
Loading

Those changes are the first steps towards allowing debug tools to
navigate some of Node's internals strucutres, giving more possibilities
to developers doing post-mortem debugging. One example of what can be
achieved with the symbols added is a new command being developed for
llnode, which prints information about handles and requests on the
queue for a core dump file.

Ref: nodejs/post-mortem#46

Obs.: To add good tests for those changes, something like nodejs/build#777 is needed.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
  • Building system
  • Tools

@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 Aug 17, 2017
addaleax
addaleax reviewed Aug 17, 2017
View reviewed changes
Comment thread src/env.cc Outdated
Copy link
Copy Markdown
Member

@addaleax addaleax Aug 17, 2017

Choose a reason for hiding this comment

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

style nit: align the * to the left (i.e. Environment* Environment::currentEnvironment)

Comment thread src/env.h Outdated
Copy link
Copy Markdown
Member

@addaleax addaleax Aug 17, 2017

Choose a reason for hiding this comment

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

(ditto)

Comment thread tools/gen-postmortem-metadata.py Outdated
Copy link
Copy Markdown
Member

@addaleax addaleax Aug 17, 2017

Choose a reason for hiding this comment

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

uintptr_t?

Comment thread tools/gen-postmortem-metadata.py Outdated
Copy link
Copy Markdown
Member

@addaleax addaleax Aug 17, 2017

Choose a reason for hiding this comment

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

typo: persistant

Comment thread common.gypi Outdated
Copy link
Copy Markdown
Member

@addaleax addaleax Aug 17, 2017

Choose a reason for hiding this comment

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

What’s the motivation for making this optional?

Comment thread src/req-wrap.h Outdated
Copy link
Copy Markdown
Member

@addaleax addaleax Aug 17, 2017

Choose a reason for hiding this comment

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

I don’t think these guards are necessary either way.

Comment thread tools/gen-postmortem-metadata.py Outdated
Copy link
Copy Markdown
Member

@addaleax addaleax Aug 17, 2017

Choose a reason for hiding this comment

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

Tbh, I’d just put #define private public at the top of the generated file and use offsetof.

mmarchini pushed a commit to mmarchini/llnode that referenced this pull request Aug 17, 2017
src: add commands to inspect the workqueue
7d9d1e1 
Added two new commands (getactivehandles and getactiverequests)
which prints all pending handles and requests (same return
given by process._getActiveHandles() and process._getActiveRequests()).
Those changes were built upon the symbols added on nodejs/node#14901,
which means it's currently not working with node's latest build.

Fixes: nodejs#100
Ref: nodejs/node#14901
@mmarchini mmarchini mentioned this pull request Aug 17, 2017
Closed
@bnoordhuis
Copy link
Copy Markdown
Member

bnoordhuis commented Aug 18, 2017

The changes to src/ seem rather ad hoc. I'm also not a fan of adding global variables. You don't need to because the env can be found by following node_isolate.

@mmarchini
Copy link
Copy Markdown
Contributor Author

mmarchini commented Aug 18, 2017

Thanks for the feedbacks, I just updated the PR with the suggestions made by @addaleax. I'm just not sure if #define private public is safe enough to be used here (seems rather extreme to me), but it worked like a charm.

All changes previously made on src/ were removed (with the exception of Environment::currentEnvironment). Now I'll take a look into node_isolate to see how to access the environment from there (it's a little challenging because it's not possible to call functions or methods when debugging a core file, but I think it can be done). I'll update this PR after finding a better way to access the current environment.

mmarchini
mmarchini commented Aug 18, 2017
View reviewed changes
Comment thread node.gyp Outdated
Copy link
Copy Markdown
Contributor Author

@mmarchini mmarchini Aug 18, 2017

Choose a reason for hiding this comment

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

Is this the right place to include node-postmortem-metadata as a dependency?

Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Sep 15, 2017

Choose a reason for hiding this comment

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

Seems fine to me but you should add a #host at the end. For consistency, I'd name it node_postmortem_metadata, i.e., underscores instead of dashes.

Copy link
Copy Markdown
Contributor Author

@mmarchini mmarchini Sep 18, 2017

Choose a reason for hiding this comment

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

I'll make those changes. Should I also rename gen-postmortem-metadata.py to gen_postmortem_metadata.py (most files on tools/ are using dashes, but there are some files using underscores)?

Comment thread src/env.cc Outdated
Copy link
Copy Markdown
Contributor Author

@mmarchini mmarchini Aug 18, 2017

Choose a reason for hiding this comment

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

This will be changed in the next update to this PR

Comment thread tools/gen-postmortem-metadata.py Outdated
Copy link
Copy Markdown
Contributor Author

@mmarchini mmarchini Aug 18, 2017

Choose a reason for hiding this comment

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

Is it safe to be used here?

Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Sep 15, 2017

Choose a reason for hiding this comment

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

Seems fine to me.

@mmarchini mmarchini mentioned this pull request Aug 18, 2017
Closed