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: always use V8_ENABLE_CHECKS in debug mode #12029

Closed
wants to merge 1 commit into
base: master
from

Conversation

@addaleax
Member

addaleax commented Mar 24, 2017

Define V8_ENABLE_CHECKS in common.gypi for the debug mode.
Without this, these checks would only be present in the object files
generated from the V8 build, and so for inline functions in v8.h
multiple different definitions could be generated, where one definition
includes the check and the other does not.

Refs: #11975 (comment)

/cc @jasongin @bnoordhuis @danbev

build: always use V8_ENABLE_CHECKS in debug mode
Define `V8_ENABLE_CHECKS` in `common.gypi` for the debug mode.
Without this, these checks would only be present in the object files
generated from the V8 build, and so for inline functions in v8.h
multiple different definitions could be generated, where one definition
includes the check and the other does not.

Refs: #11975 (comment)
@danbev

danbev approved these changes Mar 25, 2017

@bnoordhuis

LGTM. This doesn't change the API or ABI as far as I know so it could, in theory, land in the release branches.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 26, 2017

Member

I'm good with landing it in the release branches but let's wait just a bit with it sitting in master and in the 8.0.0 release before doing so, just to be on the safe side.

Member

jasnell commented Mar 26, 2017

I'm good with landing it in the release branches but let's wait just a bit with it sitting in master and in the 8.0.0 release before doing so, just to be on the safe side.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 26, 2017

Member

I don’t think anything in the CI is related to this PR, but here’s another run just to be sure:
CI: https://ci.nodejs.org/job/node-test-commit/8699/

@bnoordhuis @jasnell Do the common.gypi contents end up being included in addon configurations? I added the labels because I wasn’t sure. If they don’t, I agree, there’s no reason not to have this in the release branches.

Member

addaleax commented Mar 26, 2017

I don’t think anything in the CI is related to this PR, but here’s another run just to be sure:
CI: https://ci.nodejs.org/job/node-test-commit/8699/

@bnoordhuis @jasnell Do the common.gypi contents end up being included in addon configurations? I added the labels because I wasn’t sure. If they don’t, I agree, there’s no reason not to have this in the release branches.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 27, 2017

Member

I'm not sure actually.

Member

jasnell commented Mar 27, 2017

I'm not sure actually.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Mar 27, 2017

Member

Do the common.gypi contents end up being included in addon configurations?

It does (but see nodejs/node-gyp#1118.) It's still API/ABI-neutral though.

Member

bnoordhuis commented Mar 27, 2017

Do the common.gypi contents end up being included in addon configurations?

It does (but see nodejs/node-gyp#1118.) It's still API/ABI-neutral though.

@jasongin

This comment has been minimized.

Show comment
Hide comment
@jasongin

jasongin Mar 27, 2017

Contributor

If it is included by addons, then it could cause some addons to break in debug mode where they worked before.

Contributor

jasongin commented Mar 27, 2017

If it is included by addons, then it could cause some addons to break in debug mode where they worked before.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Mar 27, 2017

Member

Yes, but only if they were already broken, only silently.

Member

bnoordhuis commented Mar 27, 2017

Yes, but only if they were already broken, only silently.

@jasongin

This comment has been minimized.

Show comment
Hide comment
@jasongin

jasongin Mar 27, 2017

Contributor

Not necessarily. There are some operations that seem to behave perfectly fine without the checks enabled, but then will throw fatal errors with the checks enabled. For example, in the N-API PR we had code similar to this:

if (value->IsNumber()) {
  return value.As<v8::Int32>()->Value();
}

I was surprised when @addaleax pointed out that could crash in debug mode, because it was working fine in my release and debug builds on Windows, even when the number value was not an Int32. That's how I found this bug with V8_ENABLE_CHECKS.

Contributor

jasongin commented Mar 27, 2017

Not necessarily. There are some operations that seem to behave perfectly fine without the checks enabled, but then will throw fatal errors with the checks enabled. For example, in the N-API PR we had code similar to this:

if (value->IsNumber()) {
  return value.As<v8::Int32>()->Value();
}

I was surprised when @addaleax pointed out that could crash in debug mode, because it was working fine in my release and debug builds on Windows, even when the number value was not an Int32. That's how I found this bug with V8_ENABLE_CHECKS.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Mar 27, 2017

Member

I hope we can agree the code in your example is defective: not all numbers are int32s. V8_ENABLE_CHECKS is right to complain loudly.

v8::Int32::Value() is coded defensively, it doesn't crash when this is not a v8::Int32, but that doesn't mean it does the right thing for numbers that don't fit in an int32_t. In fact, the result is compiler-specific because casting a value from double to int32_t is not well-defined when it doesn't fit in an int32_t.

Hence my point: this only affects add-ons that are already broken - and only when rebuilding from source and in debug mode. I don't see a reason not to land this in the release branches.

Member

bnoordhuis commented Mar 27, 2017

I hope we can agree the code in your example is defective: not all numbers are int32s. V8_ENABLE_CHECKS is right to complain loudly.

v8::Int32::Value() is coded defensively, it doesn't crash when this is not a v8::Int32, but that doesn't mean it does the right thing for numbers that don't fit in an int32_t. In fact, the result is compiler-specific because casting a value from double to int32_t is not well-defined when it doesn't fit in an int32_t.

Hence my point: this only affects add-ons that are already broken - and only when rebuilding from source and in debug mode. I don't see a reason not to land this in the release branches.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 30, 2017

Member

Hence my point: this only affects add-ons that are already broken - and only when rebuilding from source and in debug mode.

It does turn a silent failure into a loud one, for JS exceptions we’d consider that semver-major. I’m also pretty sure there might be code that relies on such casts not crashing – for example, 0d290a2 fixed such a bug in our own codebase.

I agree that debug mode is going to make problems coming out of this exceptionally rare, but I’m still inclined to agree with @jasongin in that this maybe should not make it to other release branches (or maybe not LTS, at least).

Member

addaleax commented Mar 30, 2017

Hence my point: this only affects add-ons that are already broken - and only when rebuilding from source and in debug mode.

It does turn a silent failure into a loud one, for JS exceptions we’d consider that semver-major. I’m also pretty sure there might be code that relies on such casts not crashing – for example, 0d290a2 fixed such a bug in our own codebase.

I agree that debug mode is going to make problems coming out of this exceptionally rare, but I’m still inclined to agree with @jasongin in that this maybe should not make it to other release branches (or maybe not LTS, at least).

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 31, 2017

Member

Landed in c68da89. @bnoordhuis If you feel really strongly about this just pull the dont-land labels here. :)

Member

addaleax commented Mar 31, 2017

Landed in c68da89. @bnoordhuis If you feel really strongly about this just pull the dont-land labels here. :)

@addaleax addaleax closed this Mar 31, 2017

addaleax added a commit that referenced this pull request Mar 31, 2017

build: always use V8_ENABLE_CHECKS in debug mode
Define `V8_ENABLE_CHECKS` in `common.gypi` for the debug mode.
Without this, these checks would only be present in the object files
generated from the V8 build, and so for inline functions in v8.h
multiple different definitions could be generated, where one definition
includes the check and the other does not.

Refs: #11975 (comment)
PR-URL: #12029
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>

@addaleax addaleax deleted the addaleax:common-gypi-v8-checks branch Mar 31, 2017

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment