deps: cherry-pick 8ed65b97 from V8's upstream#8411
deps: cherry-pick 8ed65b97 from V8's upstream#8411addaleax wants to merge 2 commits intonodejs:v6.xfrom
Conversation
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=nodejs#8310
Review-Url: https://codereview.chromium.org/2292953002
Cr-Commit-Position: refs/heads/master@{nodejs#39023}
Fixes: nodejs#8310
|
LGTM. Still UB though, strictly speaking. |
|
Running CI again to make sure the test failures are unrelated: https://ci.nodejs.org/job/node-test-commit/4925/ |
|
UB? Do the tests pass on both commits separately? (I can never tell in which order the commits are) |
|
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.) |
|
Thanks! |
@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. :) |
|
@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 |
|
@addaleax Thanks for clarifying, that's what I meant. |
|
LGTM |
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>
|
Landed on |
|
@addaleax should this be backported? |
|
This shouldn’t apply to v4.x, no. |
4 participants
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
deps/v8
Description of change
Um, hope I’m doing this right by backporting v8/v8@8ed65b97 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:
Fixes: #8310
CI:
https://ci.nodejs.org/job/node-test-commit/4923/
https://ci.nodejs.org/job/node-test-commit-v8-linux/305/