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

Jump to bottom

build: add configure flag to build V8 with DCHECKs #32787

Closed
wants to merge 1 commit into from
Closed

build: add configure flag to build V8 with DCHECKs #32787

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

@addaleax
build: add configure flag to build V8 with DCHECKs
f6f3d80 
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

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

@nodejs-github-bot
Copy link
Collaborator

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

@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 •
edited

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
@addaleax
build: add configure flag to build V8 with DCHECKs
d1587dc 
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
@addaleax addaleax mentioned this pull request Apr 14, 2020
Closed
4 tasks
BethGriggs pushed a commit that referenced this pull request Apr 14, 2020
@addaleax @BethGriggs
build: add configure flag to build V8 with DCHECKs
c0f43bf 
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>
@BethGriggs BethGriggs mentioned this pull request Apr 14, 2020
Merged
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@addaleax @targos
build: add configure flag to build V8 with DCHECKs
4e9a3ea 
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
@addaleax @BridgeAR
build: add configure flag to build V8 with DCHECKs
e1809c8 
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
Merged
targos pushed a commit that referenced this pull request Apr 28, 2020
@addaleax @targos
build: add configure flag to build V8 with DCHECKs
c0805f0 
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 targos mentioned this pull request May 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gengjiawen gengjiawen gengjiawen approved these changes

@mmarchini mmarchini mmarchini approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@addaleax @mmarchini @gireeshpunathil @nodejs-github-bot @gengjiawen @targos @cjihrig