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

Expose _v8_internal_Print_Object symbol for debug addon and Node.js core easily #32402

Closed
gengjiawen opened this issue Mar 21, 2020 · 12 comments
Closed
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency.

Comments

@gengjiawen
Copy link
Member

gengjiawen commented Mar 21, 2020

Is your feature request related to a problem? Please describe.
Currently we can't debug v8::Value due to some symbol not exposed
After we import https://github.com/nodejs/node/blob/master/deps/v8/tools/lldb_commands.py in lldb, if we want to print a v8 value, it's not working

image

With the result, making hard to debug

Failed to evaluate command _v8_internal_Print_Object(*((v8::internal::Object**)((void*)(s).val_))) :
error: <user expression 0>:1:44: no member named 'Object' in namespace 'v8::internal'
_v8_internal_Print_Object(*((v8::internal::Object**)((void*)(s).val_)))
                             ~~~~~~~~~~~~~~^
error: <user expression 0>:1:52: expected expression
_v8_internal_Print_Object(*((v8::internal::Object**)((void*)(s).val_)))

This could be fixed by running a debug Node.js, but it's not very friendly for developers.

Source should here: https://github.com/nodejs/node/blob/2883c855e0105b51e5c8020d21458af109ffe3d4/deps/v8/src/diagnostics/objects-printer.cc#L2678-L2677

Describe the solution you'd like

Maybe expose it via node.h or declare this api public ?

cc @addaleax @bnoordhuis @danbev @joyeecheung @nodejs/n-api

@gengjiawen gengjiawen added the v8 engine Issues and PRs related to the V8 dependency. label Mar 21, 2020
@mmarchini
Copy link
Contributor

mmarchini commented Mar 21, 2020

Are you using a debug build? The _v8_internal_Print methods assume a Debug build*. Also, I'm pretty sure the lldb_commands is broken (I think there's an issue here about it, as well as an issue upstream), use gdb with gdbinit script instead (it should work, I used it a few weeks ago).

* Well, not quite, I think V8 has a flag to expose those methods without debug builds (with some performance overhead, but less than a full debug build)

Edit: here's the issue on how lldb_commands might be broken: #29667 (maybe it's fixed on 8.1)

@gengjiawen
Copy link
Member Author

Are you using a debug build?

debug build works fine. I want release also works.

I'm pretty sure the lldb_commands is broken

Yeap, but @danbev fixed again (should be bundled in v8 8.3, macOS still broken, but linux is fine). See danbev/learning-v8#10.

Well, not quite, I think V8 has a flag to expose those methods without debug builds

Maybe a solution. I am okay with any if we can debug v8::Value in addon api, n-api.

@mmarchini
Copy link
Contributor

debug build works fine. I want release also works.

AFAIK V8 recommends against using it in release builds. It's not just that these methods are not exposed on release builds, they are not compiled, as well as many code paths which are left out because of overhead. As for "Maybe expose it via node.h or declare this api public", this would be a huge no, we can't publicly expose a private API from V8.

If you still want to try, it's already possible (but not recommended and we should not document it) to make a release build with those print objects with ./configure -- -Dv8_enable_object_print=1. This should work, but I never tried it so I don't know for sure.

(if you want to see everything that changes on V8 when this flag is enabled: git grep OBJECT_PRINT).

@joyeecheung
Copy link
Member

Without OBJECT_PRINT you can only get as far as %DebugPrint in release build which only print some simplified information. But again, IMO a debug feature should be a debug feature, I don't think it's a good idea exposing something prefixed with _v8 in node.h, just like I don't think we'd want any embedder of Node.js expose our internal methods prefixed with underscores.

@mmarchini
Copy link
Contributor

mmarchini commented Mar 21, 2020

With that being said, V8 has a new API for debugging release builds called debug_helper. It is intended to replace the old approach used on llnode as well as provide an easier interface to write debugger tools in other platforms. Microsoft wrote a tool to debug V8 (WinDbg on the V8 folder I think), and it should be possible to write one for Node.js as well. Someone just needs to invest the time on implementing it (and figuring out how to link/embed/deliver it, which is the hardest part IMO).

@gengjiawen gengjiawen added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 21, 2020
@gengjiawen
Copy link
Member Author

gengjiawen commented Mar 24, 2020

Maybe publish a debug version when making a new release, only for debug purpose.
Like node_pdb for windows IIUC (https://nodejs.org/download/nightly/v14.0.0-nightly20200322ecfb7b0988/SHASUMS256.txt)

PS: If you want to check the repo in description, see https://github.com/gengjiawen/node-debug.

@mmarchini
Copy link
Contributor

mmarchini commented Mar 24, 2020

node_pdb is equivalent to Linux DWARF metadata AFAIK (but someone experienced on Windows will be able to answer that better than I). It's still a Release build though. I also don't want to add that burden to Build or Release teams.

@richardlau
Copy link
Member

I suggested in nodejs/Release#555 (comment) but if you want a debug build perhaps add it to https://unofficial-builds.nodejs.org/? That would avoid placing an extra burden on the release team. You can PR your dockerfile and scripts to https://github.com/nodejs/unofficial-builds/.

@mmarchini
Copy link
Contributor

@gengjiawen the biggest request here is an improved experience for debugging addons, and not for debugging core itself (since core developers will build Node.js on their own), correct? If that's the case, this seems like something worth discussing on nodejs/diagnostics, and we probably need to think broader than "publish a Debug build of Node.js" (as an addon developer, I don't want to rely on Node.js Debug build to debug my addon).

(it also seems to fall under the scope of llnode, but I'm well aware of the UX problems there, so I'm not going to advocate much here :) )

@gengjiawen
Copy link
Member Author

You can PR your dockerfile and scripts to nodejs/unofficial-builds.

Looks like a solution, I will take a look.

it also seems to fall under the scope of llnode

I am thinking bundled it with the default docker image, but it will help debugging with this case for now IIUC.

@gengjiawen
Copy link
Member Author

The debug version Node.js solution works (Though you have to see the output in terminal window, not sure it's a bug or something else).
image
image

@mmarchini
Copy link
Contributor

Not a bug, since lldb_comamnds are aliases to call V8 functions. You'd need to write a VSCode extension to use the output from those commands in a more integrated way (which is not recommended, since there's no format guarantees for the output of those commands from V8).

mmarchini added a commit to mmarchini/node that referenced this issue Aug 10, 2020
Add a configure flag to build V8 with `-DOBJECT_PRINT`, which will
expose auxiliar functions to inspect heap objects using native
debuggers.

Fixes: nodejs#32402
Signed-off-by: Mary Marchini <mmarchini@netflix.com>
MylesBorins pushed a commit that referenced this issue Aug 17, 2020
Add a configure flag to build V8 with `-DOBJECT_PRINT`, which will
expose auxiliar functions to inspect heap objects using native
debuggers.

Fixes: #32402
Signed-off-by: Mary Marchini <mmarchini@netflix.com>

PR-URL: #32834
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
BethGriggs pushed a commit that referenced this issue Aug 20, 2020
Add a configure flag to build V8 with `-DOBJECT_PRINT`, which will
expose auxiliar functions to inspect heap objects using native
debuggers.

Fixes: #32402
Signed-off-by: Mary Marchini <mmarchini@netflix.com>

PR-URL: #32834
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Add a configure flag to build V8 with `-DOBJECT_PRINT`, which will
expose auxiliar functions to inspect heap objects using native
debuggers.

Fixes: #32402
Signed-off-by: Mary Marchini <mmarchini@netflix.com>

PR-URL: #32834
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Add a configure flag to build V8 with `-DOBJECT_PRINT`, which will
expose auxiliar functions to inspect heap objects using native
debuggers.

Fixes: #32402
Signed-off-by: Mary Marchini <mmarchini@netflix.com>

PR-URL: #32834
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
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++. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants