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

deps: cherry-pick 8ed65b97 from V8's upstream #8411

Closed
wants to merge 2 commits into
base: v6.x
from

Conversation

Projects
None yet
4 participants
@addaleax
Member

addaleax commented Sep 5, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

deps/v8

Description of change

Um, hope I’m doing this right by backporting v8/v8@8ed65b9 to v6.x – in master that should not be necessary as this will come with a regular V8 5.4 update before v7.x is cut?

The test needed a little fixup to work with V8 5.1.


Original commit message:

Make FieldType::None() non-nullptr value to avoid undefined behaviour

When FieldType::None() returns a cast Smi::FromInt(0), which translates
as nullptr, the FieldType::IsNone() check becomes equivalent to
`this == nullptr` which is not allowed by the standard and
therefore optimized away as a false constant by GCC 6.

This has lead to crashes when invoking methods on FieldType::None().

Using a different Smi constant for FieldType::None() makes the compiler
always include a comparison against that value. The choice of these
constants has no effect as they are effectively arbitrary.

BUG=https://github.com/nodejs/node/issues/8310

Review-Url: https://codereview.chromium.org/2292953002
Cr-Commit-Position: refs/heads/master@{#39023}

Fixes: #8310

CI:
https://ci.nodejs.org/job/node-test-commit/4923/
https://ci.nodejs.org/job/node-test-commit-v8-linux/305/

addaleax added some commits Sep 5, 2016

deps: cherry-pick 8ed65b97 from V8's upstream
Original commit message:

    Make FieldType::None() non-nullptr value to avoid undefined behaviour

    When FieldType::None() returns a cast Smi::FromInt(0), which translates
    as nullptr, the FieldType::IsNone() check becomes equivalent to
    `this == nullptr` which is not allowed by the standard and
    therefore optimized away as a false constant by GCC 6.

    This has lead to crashes when invoking methods on FieldType::None().

    Using a different Smi constant for FieldType::None() makes the compiler
    always include a comparison against that value. The choice of these
    constants has no effect as they are effectively arbitrary.

    BUG=#8310

    Review-Url: https://codereview.chromium.org/2292953002
    Cr-Commit-Position: refs/heads/master@{#39023}

Fixes: #8310
@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Sep 5, 2016

Member

LGTM. Still UB though, strictly speaking.

Member

bnoordhuis commented Sep 5, 2016

LGTM. Still UB though, strictly speaking.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 5, 2016

Member

Running CI again to make sure the test failures are unrelated: https://ci.nodejs.org/job/node-test-commit/4925/

Member

addaleax commented Sep 5, 2016

Running CI again to make sure the test failures are unrelated: https://ci.nodejs.org/job/node-test-commit/4925/

@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel Sep 6, 2016

Member

UB?

Do the tests pass on both commits separately? (I can never tell in which order the commits are)

Member

fhinkel commented Sep 6, 2016

UB?

Do the tests pass on both commits separately? (I can never tell in which order the commits are)

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Sep 6, 2016

Member

Undefined behavior. If you mean 'what kind of UB', it's not legal to construct pointers that don't point to a legal object or to legal_object+1 (the 'one element after' rule.)

Member

bnoordhuis commented Sep 6, 2016

Undefined behavior. If you mean 'what kind of UB', it's not legal to construct pointers that don't point to a legal object or to legal_object+1 (the 'one element after' rule.)

@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel Sep 6, 2016

Member

Thanks!

Member

fhinkel commented Sep 6, 2016

Thanks!

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 6, 2016

Member

Do the tests pass on both commits separately? (I can never tell in which order the commits are)

@fhinkel If you mean, do the tests work after the first commit here?, the answer is “no” – the second commit is needed to make them function properly. They should be landed as a single commit, I just wanted to clarify the distinction between what is in V8 and what is not. :)

Member

addaleax commented Sep 6, 2016

Do the tests pass on both commits separately? (I can never tell in which order the commits are)

@fhinkel If you mean, do the tests work after the first commit here?, the answer is “no” – the second commit is needed to make them function properly. They should be landed as a single commit, I just wanted to clarify the distinction between what is in V8 and what is not. :)

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 7, 2016

Member

@Fishrock123 … this one should be good to go. Since you’re putting a release together right now, I’m not sure if it’s easier if I land this in v6.x-staging (which is still up to date with v6.x) or if I’ll leave that to you?

Member

addaleax commented Sep 7, 2016

@Fishrock123 … this one should be good to go. Since you’re putting a release together right now, I’m not sure if it’s easier if I land this in v6.x-staging (which is still up to date with v6.x) or if I’ll leave that to you?

@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel Sep 7, 2016

Member

@addaleax Thanks for clarifying, that's what I meant.

Member

fhinkel commented Sep 7, 2016

@addaleax Thanks for clarifying, that's what I meant.

@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel Sep 7, 2016

Member

LGTM

Member

fhinkel commented Sep 7, 2016

LGTM

addaleax added a commit that referenced this pull request Sep 8, 2016

deps: cherry-pick 8ed65b97 from V8's upstream
Original commit message:

    Make FieldType::None() non-nullptr value to avoid undefined behaviour

    When FieldType::None() returns a cast Smi::FromInt(0), which translates
    as nullptr, the FieldType::IsNone() check becomes equivalent to
    `this == nullptr` which is not allowed by the standard and
    therefore optimized away as a false constant by GCC 6.

    This has lead to crashes when invoking methods on FieldType::None().

    Using a different Smi constant for FieldType::None() makes the compiler
    always include a comparison against that value. The choice of these
    constants has no effect as they are effectively arbitrary.

    BUG=#8310

    Review-Url: https://codereview.chromium.org/2292953002
    Cr-Commit-Position: refs/heads/master@{#39023}

Fixes: #8310
PR-URL: #8411
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 8, 2016

Member

Landed on v6.x-staging @ f829660

Member

addaleax commented Sep 8, 2016

Landed on v6.x-staging @ f829660

@addaleax addaleax closed this Sep 8, 2016

@addaleax addaleax deleted the addaleax:v8-fieldtype-v6.x branch Sep 8, 2016

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Oct 11, 2016

Member

@addaleax should this be backported?

Member

MylesBorins commented Oct 11, 2016

@addaleax should this be backported?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Oct 11, 2016

Member

This shouldn’t apply to v4.x, no.

Member

addaleax commented Oct 11, 2016

This shouldn’t apply to v4.x, no.

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