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 configure flag to build V8 with DCHECKs #32787

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

Add a configure flag that enables building V8 with -DDEBUG, in
particular with debug checks enabled and with runtime debugging
features, e.g. inspecting JS objects from debuggers, enabled.

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

Add a configure flag that enables building V8 with `-DDEBUG`, in
particular with debug checks enabled and with runtime debugging
features, e.g. inspecting JS objects from debuggers, enabled.
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Apr 11, 2020
@addaleax
Copy link
Member Author

/cc @mmarchini, I think this came up in conversation somewhere recently

@mmarchini
Copy link
Contributor

I think the discussion was about building with V8 print objects on.

@gireeshpunathil
Copy link
Member

runtime debugging features, e.g. inspecting JS objects from debuggers, enabled.

@addaleax - can you pls expand this a little bit, for my understanding - what kind of inspection? isn't the debuggers currently capable of inspecting JS objects?

@addaleax
Copy link
Member Author

@gireeshpunathil I thinkt that would be mostly the _v8_internal_... functions used in https://github.com/v8/v8/blob/master/tools/gdbinit.

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 12, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@gengjiawen
Copy link
Member

Will the binary size will be much bigger like debug one ?

@gengjiawen
Copy link
Member

/cc @mmarchini, I think this came up in conversation somewhere recently

Should be this one: #32402.

@addaleax
Copy link
Member Author

Will the binary size will be much bigger like debug one ?

No, only slightly larger, I think.

/cc @mmarchini, I think this came up in conversation somewhere recently

Should be this one: #32402.

Right – if you’re okay with having this PR be the solution, I can add a Fixes: tag.

@mmarchini
Copy link
Contributor

Why do we need dchecks to expose v8_internal* functions? I thought v8_enable_print_objects was enough.

@addaleax
Copy link
Member Author

@mmarchini Hm yeah, looks like that’s really the flag you’d want to toggle for that. Do you know if there would be any downside to us always enabling it? And this PR would be independent from that, right? (I’m personally mostly opening it for the extra DCHECKs.)

@mmarchini
Copy link
Contributor

mmarchini commented Apr 14, 2020

And this PR would be independent from that, right? (I’m personally mostly opening it for the extra DCHECKs.)

Yes, and I think it's a great addition :D

Do you know if there would be any downside to us always enabling it?

I thought I saw some slow paths with this flag enabled, but I might be wrong as I can't find those anymore. We can always run all benchmarks and see what happens (or provide an unofficial build so users can try it and report back).

addaleax added a commit that referenced this pull request Apr 14, 2020
Add a configure flag that enables building V8 with `-DDEBUG`, in
particular with debug checks enabled and with runtime debugging
features, e.g. inspecting JS objects from debuggers, enabled.

PR-URL: #32787
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@addaleax
Copy link
Member Author

Landed in d1587dc

@addaleax addaleax closed this Apr 14, 2020
@addaleax addaleax deleted the configure-v8-dcheck branch April 14, 2020 00:56
BethGriggs pushed a commit that referenced this pull request Apr 14, 2020
Add a configure flag that enables building V8 with `-DDEBUG`, in
particular with debug checks enabled and with runtime debugging
features, e.g. inspecting JS objects from debuggers, enabled.

PR-URL: #32787
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
Add a configure flag that enables building V8 with `-DDEBUG`, in
particular with debug checks enabled and with runtime debugging
features, e.g. inspecting JS objects from debuggers, enabled.

PR-URL: nodejs#32787
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
Add a configure flag that enables building V8 with `-DDEBUG`, in
particular with debug checks enabled and with runtime debugging
features, e.g. inspecting JS objects from debuggers, enabled.

PR-URL: #32787
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Apr 28, 2020
targos pushed a commit that referenced this pull request Apr 28, 2020
Add a configure flag that enables building V8 with `-DDEBUG`, in
particular with debug checks enabled and with runtime debugging
features, e.g. inspecting JS objects from debuggers, enabled.

PR-URL: #32787
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
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
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants