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

build: add flag to build V8 with OBJECT_PRINT #32834

Closed
wants to merge 1 commit into from

Conversation

mmarchini
Copy link
Contributor

@mmarchini mmarchini commented Apr 14, 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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Apr 14, 2020
@addaleax
Copy link
Member

@mmarchini Just landed #32787 so you can avoid conflicts with it :)

configure.py Show resolved Hide resolved
@mmarchini
Copy link
Contributor Author

So it works on gdb, but not on lldb (all commands give empty results on lldb). One caveat though: any command that requires v8:: will not work, which includes jlh, one of the most useful commands. That's not a concern of this PR, but something we should be aware of (and it means this PR doesn't fix #32402).

@addaleax
Copy link
Member

@mmarchini jlh works when you replace v8::internal::Object with void in its definition 🤷‍♀️

@bnoordhuis
Copy link
Member

I'd be okay with turning it on unconditionally. We already do that for the builtin disassembler and that pulls in a lot more code than does object printing.

diff --git a/common.gypi b/common.gypi
index d39668b100..7026f41565 100644
--- a/common.gypi
+++ b/common.gypi
@@ -53,6 +53,9 @@
     # Enable disassembler for `--print-code` v8 options
     'v8_enable_disassembler': 1,
 
+    # Sets -dOBJECT_PRINT.
+    'v8_enable_object_print%': 1,
+
     # https://github.com/nodejs/node/pull/22920/files#r222779926
     'v8_enable_handle_zapping': 0,
 

@gengjiawen
Copy link
Member

I'd be okay with turning it on unconditionally. We already do that for the builtin disassembler and that pulls in a lot more code than does object printing.

The binary maybe much bigger. If we make it enable by default, maybe need to check binary size first. If the size turns out okay, I think we can close #32402.

@mmarchini
Copy link
Contributor Author

mmarchini commented Apr 14, 2020

The binary maybe much bigger. If we make it enable by default, maybe need to check binary size first. If the size turns out okay, I think we can close #32402.

Shouldn't be that much, but I'll double check.

cc @nodejs/v8 any performance or stability concerns if we enable OBJECT_PRINT by default (considering we already have ENABLE_DISASSEMBLER enabled by default)?

@mmarchini
Copy link
Contributor Author

@gengjiawen 72724 Kb (current build) vs 72972 Kb (with OBJECT_PRINT), so 248 Kb increase, which is not concerning IMO.

@verwaest
Copy link
Contributor

This shouldn't affect anything beyond binary size and more verbose printing. It makes sense for debugging.

@mmarchini
Copy link
Contributor Author

The idea is to set it by default on release builds as well, so native module developers can use those debugging features as well.

@verwaest
Copy link
Contributor

If you can live with the binary size increase that sounds fine. This code isn't battle tested, but it seems somewhat unlikely to cause unexpected issues; especially if you don't accept random untrusted input as JS.

Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

Thanks

@mmarchini mmarchini marked this pull request as ready for review April 20, 2020 21:05
@mmarchini
Copy link
Contributor Author

I'll follow up with a PR to set this as default (as suggested by @bnoordhuis).

@gengjiawen
Copy link
Member

@mmarchini This needs rebase.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
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>
@mmarchini
Copy link
Contributor Author

Landed in dd0c522

@mmarchini mmarchini closed this Aug 10, 2020
mmarchini added a commit that referenced this pull request 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: #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>
mmarchini added a commit to mmarchini/node that referenced this pull request Aug 10, 2020
The flag improves the experience of debugging V8 with native debuggers.
It doens't incur performance penality, the only downside is an increase
in binary size by approximately 248 Kb.

Ref: nodejs#32834
mmarchini added a commit that referenced this pull request Aug 12, 2020
The flag improves the experience of debugging V8 with native debuggers.
It doens't incur performance penality, the only downside is an increase
in binary size by approximately 248 Kb.

Ref: #32834

PR-URL: #34705
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
MylesBorins pushed a commit that referenced this pull request 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>
MylesBorins pushed a commit that referenced this pull request Aug 17, 2020
The flag improves the experience of debugging V8 with native debuggers.
It doens't incur performance penality, the only downside is an increase
in binary size by approximately 248 Kb.

Ref: #32834

PR-URL: #34705
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@danielleadams danielleadams mentioned this pull request Aug 20, 2020
BethGriggs pushed a commit that referenced this pull request 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>
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
The flag improves the experience of debugging V8 with native debuggers.
It doens't incur performance penality, the only downside is an increase
in binary size by approximately 248 Kb.

Ref: #32834

PR-URL: #34705
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
addaleax pushed a commit that referenced this pull request 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 pull request Sep 22, 2020
The flag improves the experience of debugging V8 with native debuggers.
It doens't incur performance penality, the only downside is an increase
in binary size by approximately 248 Kb.

Ref: #32834

PR-URL: #34705
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
addaleax pushed a commit that referenced this pull request 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 pull request Sep 22, 2020
The flag improves the experience of debugging V8 with native debuggers.
It doens't incur performance penality, the only downside is an increase
in binary size by approximately 248 Kb.

Ref: #32834

PR-URL: #34705
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose _v8_internal_Print_Object symbol for debug addon and Node.js core easily
9 participants