Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1238)

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

Created:
4 years, 3 months ago by Anna Henningsen
Modified:
4 years, 3 months ago
Reviewers:
Benedikt Meurer
CC:
v8-reviews_googlegroups.com
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : Make FieldType::None() non-nullptr value to avoid undefined behaviour #

Patch Set 3 : (fix build by removing unused var in test) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -1 line) Patch
M src/field-type.cc View 1 chunk +3 lines, -1 line 0 comments Download
M test/cctest/test-field-type-tracking.cc View 1 2 2 chunks +11 lines, -0 lines 0 comments Download

Messages

Total messages: 17 (8 generated)
Anna Henningsen
Hey, this is my attempt at fixing a segfault that occurs when building with GCC ...
4 years, 3 months ago (2016-08-30 12:17:05 UTC) #2
Benedikt Meurer
Thanks for the bugfix. LGTM.
4 years, 3 months ago (2016-08-30 17:19:46 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2292953002/20001
4 years, 3 months ago (2016-08-30 17:19:55 UTC) #6
commit-bot: I haz the power
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/builds/3286) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, ...
4 years, 3 months ago (2016-08-30 17:22:25 UTC) #8
Anna Henningsen
Sorry for the mixup in the test file, I removed the unused line (was copied ...
4 years, 3 months ago (2016-08-30 17:28:47 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2292953002/40001
4 years, 3 months ago (2016-08-30 17:33:45 UTC) #12
Benedikt Meurer
Sure, thanks. Although I think you should be able to also click the CQ button.
4 years, 3 months ago (2016-08-30 17:34:04 UTC) #13
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 3 months ago (2016-08-30 17:55:29 UTC) #15
commit-bot: I haz the power
4 years, 3 months ago (2016-08-30 17:56:16 UTC) #17
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/8ed65b97a3307e4d603e567ae29020dd74baf7c0
Cr-Commit-Position: refs/heads/master@{#39023}

Powered by Google App Engine
This is Rietveld 408576698