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

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

Committed: https://crrev.com/8ed65b97a3307e4d603e567ae29020dd74baf7c0
Cr-Commit-Position: refs/heads/master@{#39023}

[Anna Henningsen, 2016-08-30 12:17:05]

addaleax@gmail.com changed reviewers:
+ bmeurer@chromium.org

[Anna Henningsen, 2016-08-30 12:17:05]

Hey, this is my attempt at fixing a segfault that occurs when building with GCC
6, reported at https://github.com/nodejs/node/issues/8310.

This would be my second contribution here, but I’m definitely not sure if I’m
doing this right (especially wrt reviewers – I’ve chosen one based on recent
activity in src/types.cc where src/field-type.cc was extracted from).

[Benedikt Meurer, 2016-08-30 17:17:53]

Description was changed from

==========
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=

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

to

==========
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
==========

[Benedikt Meurer, 2016-08-30 17:19:46]

Thanks for the bugfix. LGTM.

[Benedikt Meurer, 2016-08-30 17:19:52]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2016-08-30 17:19:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 17:22:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 17:22:25]

Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)

[Anna Henningsen, 2016-08-30 17:28:47]

Sorry for the mixup in the test file, I removed the unused line (was copied from
another test), could you run the build bots again?

[Benedikt Meurer, 2016-08-30 17:33:41]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2016-08-30 17:33:42]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/2292953002/#ps40001
(title: "(fix build by removing unused var in test)")

[commit-bot: I haz the power, 2016-08-30 17:33:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Benedikt Meurer, 2016-08-30 17:34:04]

Sure, thanks. Although I think you should be able to also click the CQ button.

[commit-bot: I haz the power, 2016-08-30 17:55:29]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-08-30 17:55:29]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-30 17:56:15]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/8ed65b97a3307e4d603e567ae29020dd74baf7c0
Cr-Commit-Position: refs/heads/master@{#39023}
==========

[commit-bot: I haz the power, 2016-08-30 17:56:16]

Patchset 3 (id:??) landed as
https://crrev.com/8ed65b97a3307e4d603e567ae29020dd74baf7c0
Cr-Commit-Position: refs/heads/master@{#39023}