src, test: node internals' postmortem metadata #14901

mmarchini

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

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

bnoordhuis

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

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.

rnchamberlain

@bnoordhuis @mmarchini the isolate is a v8 class, the env is in node, and it looks to me that you could get the isolate from the env, but I don't see how you can go the other way?

mmarchini

@rnchamberlain the Environment is stored in a fixed position inside the Context, and the Context is reachable from the Isolate, but the offsets to get the Context from the Isolate and the position inside the Context where the Environment is stored are currently not available in the symbols table of V8. I'm writing a PR to add those symbols to V8, let's see how it goes.

Another option would be to get the environment from uv_key_t thread_local_env, but it seems way harder to do, and any solution would be platform dependent (for example, on Linux the environment is stored in a pthread's thread-specific store).

mmarchini

Pull Request updated:

Remove currentEnvironment symbol
Add environment_context_idx_embedder_data symbol (required to get the Environment from node_isolate)

Please let me know if there's any other changes needed.

BridgeAR

@nodejs/build @nodejs/post-mortem PTAL

BridgeAR

Again @nodejs/build @nodejs/post-mortem PTAL
Maybe @addaleax again as well?

addaleax

Maybe @addaleax again as well?

The code LGTM but I don’t think I have a sufficiently good feeling for the usefulness of this feature.

BridgeAR

Ping @mmarchini

mmarchini

Sorry for the late response. I've just updated the PR with changes requested by @bnoordhuis. I've also rebased it to match the current master.

@addaleax for now, it can be used to inspect active handles and requests on debuggers like LLDB (nodejs/llnode#122) and MDB (not implemented yet, but certainly will be if this is accepted). As we were discussing in nodejs/post-mortem#46, this is a first step to increase the power of debuggers to do post-mortem analysis of Node applications.

cjihrig

LGTM once @bnoordhuis is good with it.

bnoordhuis

LGTM if the last nits are addressed. Thanks for the PR.

mmarchini

Pull Request updated with those last changes requested

BridgeAR

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

BridgeAR

@mmarchini the commits should at least partially quashed and it is somewhat difficult for me to describe your changes in a single commit as I do not know what of the commit messages is still relevant and what not. Would you mind to either provide a single commit messages for all commits together or rebase on your own so this can be landed? Thanks a lot in advance!

mmarchini

I've squashed all commits into one and updated the commit message to best reflect these changes. If you need anything else please let me know :)

BridgeAR

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

BridgeAR

Rerun the CI as it is very red https://ci.nodejs.org/job/node-test-commit/12650/

BridgeAR

@mmarchini it seems like there are related failures on the CI. Please have a look. E.g. https://ci.nodejs.org/job/node-test-commit-linux/12698/nodes=centos5-64/console

joyeecheung

Another one...:https://ci.nodejs.org/job/node-test-commit-arm/13371/

EDIT: no luck, stopped.

joyeecheung

CI before landing: https://ci.nodejs.org/job/node-test-pull-request/12738/

joyeecheung

Landed in 756a34e, thanks for being so patient and following along! @mmarchini

Before these changes, only V8 added postmortem metadata to Node's binary, limiting the possibilities for debugger's developers to add some features that rely on investigating Node's internal structures. These changes are first steps towards empowering debug tools to navigate Node's internal structures. One example of what can be achieved with this is shown at nodejs/llnode#122 (a command which prints information about handles and requests on the queue for a core dump file). Node postmortem metadata are prefixed with nodedbg_. This also adds tests to validate if all postmortem metadata are calculated correctly, plus some documentation on what is postmortem metadata and a few care to be taken to avoid breaking it. Ref: nodejs/llnode#122 Ref: nodejs/post-mortem#46 PR-URL: #14901 Refs: nodejs/post-mortem#46 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

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

MSLaguana

I just ran into an issue caused by the redefinition of private in this change: it means that any attempt at private inheritance (in the included files) are broken and made invalid. Is this an intentional trade-off, or something that was unanticipated?

joyeecheung

@MSLaguana How would they be broken? Do you mean that their members are now all accessible to GenDebugSymbols()?

joyeecheung

@MSLaguana Ah, right, I realized that you are talking about that the inheritance statement will be broken...that is unintentional I believe.

bnoordhuis

It's of no concern since we don't use private inheritance. I don't foresee that happening either.

joyeecheung

@bnoordhuis Just curious, is that a convention? There is class StringSearch : private StringSearchBase and class Reference : private Finalizer in our codebase. If that needs to be enforced, we should put that in the C++ style guide.

bnoordhuis

I saw that by pure coincidence ten minutes after I posted that... the reason we don't use non-public inheritance is that there is never a reason to. The two files you mention both came from outside.

Yes, a note in the style guide would be good.

MylesBorins

How did this end up with the dont-land-on-9.x label?

That is in conflict with the open backport PR #18550

It is also worth mentioning that the original commit 756a34e lands cleanly on v9.x-staging, so unsure why we need to PR to begin with

MylesBorins

One more thing... should this be semver-minor?

mmarchini

How did this end up with the dont-land-on-9.x label?

AFAIK it's because there was one reverted commit here, see #14901 (comment) (this comment also explains why I opened a backport PR).

mmarchini

One more thing... should this be semver-minor?

Yep, since it's a new feature.

MylesBorins

@mmarchini thanks for pointing it out! We'll get it in the next release!

mmarchini

Yes. There's already a backport PR open (#19176), I just need to rebase it to resolve conflicts. I'll do it today.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. Issues and PRs related to general changes in the lib or src directory. labels Aug 17, 2017

addaleax reviewed Aug 17, 2017

View reviewed changes

mmarchini mentioned this pull request Aug 17, 2017

src: add commands to inspect the workqueue nodejs/llnode#122

Closed

mmarchini commented Aug 18, 2017

View reviewed changes

mmarchini mentioned this pull request Aug 18, 2017

Proposal: add more debugging symbols related to Node internal structures nodejs/post-mortem#46

Closed

bnoordhuis requested changes Sep 15, 2017

View reviewed changes

mmarchini force-pushed the debugging_symbols branch from 8416bc4 to dc9633f Compare September 20, 2017 00:34

cjihrig previously approved these changes Sep 20, 2017

View reviewed changes

bnoordhuis previously approved these changes Sep 20, 2017

View reviewed changes

jasnell previously approved these changes Sep 20, 2017

View reviewed changes

mmarchini force-pushed the debugging_symbols branch from 28136f3 to a7dfd9a Compare September 25, 2017 00:47

joyeecheung closed this Jan 26, 2018

mmarchini mentioned this pull request Feb 3, 2018

[v9.x backport] node internals' postmortem metadata (#14901, #18530, #18653, #18576) #18550

Closed

4 tasks

joyeecheung mentioned this pull request Feb 8, 2018

src: do not redefine private for GenDebugSymbols #18653

Closed

2 tasks

addaleax mentioned this pull request Feb 27, 2018

v9.7.0 proposal #19040

Closed

This was referenced Mar 6, 2018

[v8.x backport] node internals' postmortem metadata (#14901, #18530, #18653) #19176

Closed

[v6.x backport] node internals' postmortem metadata (#14901, #18530, #18653) #19323

Closed

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

Conversation

mmarchini commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

bnoordhuis commented Aug 18, 2017

Uh oh!

mmarchini commented Aug 18, 2017

Uh oh!

rnchamberlain commented Aug 22, 2017

Uh oh!

mmarchini commented Aug 22, 2017

Uh oh!

mmarchini commented Aug 29, 2017

Uh oh!

BridgeAR commented Sep 3, 2017

Uh oh!

BridgeAR commented Sep 13, 2017

Uh oh!

addaleax commented Sep 14, 2017

Uh oh!

BridgeAR commented Sep 19, 2017

Uh oh!

mmarchini commented Sep 20, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

mmarchini commented Sep 20, 2017

Uh oh!

BridgeAR commented Sep 22, 2017

Uh oh!

BridgeAR commented Sep 22, 2017

Uh oh!

mmarchini commented Sep 25, 2017

Uh oh!

BridgeAR commented Sep 27, 2017

Uh oh!

BridgeAR commented Sep 28, 2017

Uh oh!

BridgeAR commented Sep 28, 2017

Uh oh!

joyeecheung commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Jan 25, 2018

Uh oh!

joyeecheung commented Jan 26, 2018

Uh oh!

MSLaguana commented Feb 8, 2018

Uh oh!

joyeecheung commented Feb 8, 2018

Uh oh!

joyeecheung commented Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Feb 8, 2018

Uh oh!

joyeecheung commented Feb 8, 2018

Uh oh!

bnoordhuis commented Feb 8, 2018

Uh oh!

MylesBorins commented Feb 20, 2018

Uh oh!

MylesBorins commented Feb 20, 2018

Uh oh!

mmarchini commented Feb 20, 2018

Uh oh!

mmarchini commented Feb 20, 2018

Uh oh!

MylesBorins commented Feb 21, 2018

Uh oh!

mmarchini commented May 22, 2018

Uh oh!

Reviewers

Assignees

mmarchini commented Aug 17, 2017 •

edited

Loading

joyeecheung commented Jan 25, 2018 •

edited

Loading

joyeecheung commented Feb 8, 2018 •

edited

Loading