New issue
Advanced search Search tips

Issue 709753 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

V8 correctness failure in configs: x64,ignition:x64,ignition_turbo

Project Member Reported by ClusterFuzz, Apr 8 2017

Issue description

Cc: bmeu...@chromium.org jarin@chromium.org mstarzinger@chromium.org
Components: -Blink>JavaScript Blink>JavaScript>Compiler
Status: Available (was: Untriaged)
// PTAL, repro:

function foo(v, i) {
 v[i].x;
}

var v=[, 0.1];
foo(v, 1);
foo(v, 1);
%OptimizeFunctionOnNextCall(foo);
foo(v, 0);

// Output:
# Compared x64,ignition with x64,ignition_turbo
#
# Flags of x64,ignition:
--abort_on_stack_overflow --expose-gc --allow-natives-syntax --invoke-weak-callbacks --omit-quit --es-staging --random-seed 732681078 --ignition --turbo-filter=~ --hydrogen-filter=~ --nocrankshaft
# Flags of x64,ignition_turbo:
--abort_on_stack_overflow --expose-gc --allow-natives-syntax --invoke-weak-callbacks --omit-quit --es-staging --random-seed 732681078 --ignition --turbo
#
# Difference:
- ./repro.js:2: TypeError: Cannot read property 'x' of undefined
#
### Start of configuration x64,ignition:
./repro.js:2: TypeError: Cannot read property 'x' of undefined
 v[i].x;
     ^



### End of configuration x64,ignition
#
### Start of configuration x64,ignition_turbo:

### End of configuration x64,ignition_turbo

Cc: danno@chromium.org
Owner: bmeu...@chromium.org
Status: Assigned (was: Available)
SimplifiedLowering eliminates a CheckFloat64Hole because the (unused) CheckNumber passes a truncation to it. We really need to represent the float64 hole NaN appropriately to ensure that this doesn't happen anymore.
Labels: Merge-Request-58
Project Member

Comment 5 by sheriffbot@chromium.org, Apr 11 2017

Labels: -Merge-Request-58 Merge-Review-58 Hotlist-Merge-Review
This bug requires manual review: We are only 13 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 6 by gov...@chromium.org, Apr 11 2017

+hablich@ for M58 merge review

Comment 7 by gov...@chromium.org, Apr 11 2017

Cc: hablich@chromium.org
Patch is not yet in. I just added the label to ensure we don't forget the back-merge. Sorry for the inconvenience.
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/8c0c5e8117a0c935d6f2e5f6e540674d46753a87

commit 8c0c5e8117a0c935d6f2e5f6e540674d46753a87
Author: bmeurer <bmeurer@chromium.org>
Date: Wed Apr 12 10:10:48 2017

[turbofan] Properly represent the float64 hole.

The hole NaN should also have proper Type::Hole, and not silently hide
in the Type::Number. This way we can remove all the special casing for
the hole NaN, and we also finally get the CheckNumber right.

This also allows us to remove some ducktape from the Deoptimizer, as for
escape analyzed FixedDoubleArrays we always pass the hole value now to
represent the actual holes.

Also-By: jarin@chromium.org
BUG= chromium:684208 , chromium:709753 ,v8:5267
R=jarin@chromium.org

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

[modify] https://crrev.com/8c0c5e8117a0c935d6f2e5f6e540674d46753a87/src/compiler/js-create-lowering.cc
[modify] https://crrev.com/8c0c5e8117a0c935d6f2e5f6e540674d46753a87/src/compiler/js-native-context-specialization.cc
[modify] https://crrev.com/8c0c5e8117a0c935d6f2e5f6e540674d46753a87/src/compiler/operation-typer.cc
[modify] https://crrev.com/8c0c5e8117a0c935d6f2e5f6e540674d46753a87/src/compiler/operation-typer.h
[modify] https://crrev.com/8c0c5e8117a0c935d6f2e5f6e540674d46753a87/src/compiler/simplified-lowering.cc
[modify] https://crrev.com/8c0c5e8117a0c935d6f2e5f6e540674d46753a87/src/compiler/typed-optimization.cc
[modify] https://crrev.com/8c0c5e8117a0c935d6f2e5f6e540674d46753a87/src/compiler/typed-optimization.h
[modify] https://crrev.com/8c0c5e8117a0c935d6f2e5f6e540674d46753a87/src/compiler/typer.cc
[modify] https://crrev.com/8c0c5e8117a0c935d6f2e5f6e540674d46753a87/src/compiler/types.h
[modify] https://crrev.com/8c0c5e8117a0c935d6f2e5f6e540674d46753a87/src/compiler/verifier.cc
[modify] https://crrev.com/8c0c5e8117a0c935d6f2e5f6e540674d46753a87/src/deoptimizer.cc
[modify] https://crrev.com/8c0c5e8117a0c935d6f2e5f6e540674d46753a87/src/objects-inl.h
[modify] https://crrev.com/8c0c5e8117a0c935d6f2e5f6e540674d46753a87/src/objects.cc
[modify] https://crrev.com/8c0c5e8117a0c935d6f2e5f6e540674d46753a87/src/objects.h
[add] https://crrev.com/8c0c5e8117a0c935d6f2e5f6e540674d46753a87/test/mjsunit/regress/regress-crbug-709753.js

Status: Fixed (was: Assigned)
Project Member

Comment 11 by bugdroid1@chromium.org, Apr 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/2eeb085427ccc7fbb58c7c8a8c5301f5f2b9d29f

commit 2eeb085427ccc7fbb58c7c8a8c5301f5f2b9d29f
Author: bmeurer <bmeurer@chromium.org>
Date: Wed Apr 12 11:27:21 2017

[turbofan] Remove unused word32 truncation case for CheckFloat64Hole.

BUG= chromium:684208 , chromium:709753 ,v8:5267
R=jarin@chromium.org

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

[modify] https://crrev.com/2eeb085427ccc7fbb58c7c8a8c5301f5f2b9d29f/src/compiler/simplified-lowering.cc

Project Member

Comment 12 by ClusterFuzz, Apr 13 2017

ClusterFuzz has detected this issue as fixed in range 44602:44603.

Detailed report: https://clusterfuzz.com/testcase?key=4973823146590208

Fuzzer: foozzie_js_mutation
Job Type: v8_foozzie
Platform Id: linux

Crash Type: V8 correctness failure
Crash Address: 
Crash State:
  configs: x64,ignition:x64,ignition_turbo
  sources: 8fa
  
Sanitizer: address (ASAN)

Regressed: V8: 43348:43349
Fixed: V8: 44602:44603

Reproducer Testcase: https://clusterfuzz.com/download/AMIfv94aB7IHTIi_r3qXjeYbY5v1YE7hPkPeKj7qu1BfttFjFf0DjjG53d1eRlz20cAwMyp02F0SD4yPvVV4RHGbPMLcENhHcgJTvBeHYiIipQmLJhtS8P4W0wK0nGZ_yRVrSSwVxT4rXk-4nu7qiErBKIlP5HQjAxoxfwJziajX3EaE9foQ1qLn1epP_BXmvUvgIkn7cdNWTDAt0hR2--dzkDJM8SYpuzVrIBI-is4lCM1La8AjPWxgte2mHIiccyzgWW5hrIt1P_cp9RoToZmWuSgPHlH66HzPGX8rGwr7Q0QkM4paVhySi44siSkMTHYvLK_Ay0ZjEw2oCcHWLkgjWMG28lEsfobBHTX1J5y0_dkmaSKiWNI?testcase_id=4973823146590208


See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 13 by bugdroid1@chromium.org, Apr 13 2017

Labels: merge-merged-5.9
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/9372e6d03ddf825a9128cbccaf36e5430b0b9bb2

commit 9372e6d03ddf825a9128cbccaf36e5430b0b9bb2
Author: Michael Hablich <hablich@chromium.org>
Date: Thu Apr 13 12:29:58 2017

Merged: [turbofan] Properly represent the float64 hole.

Revision: 8c0c5e8117a0c935d6f2e5f6e540674d46753a87

BUG= chromium:684208 , chromium:709753 ,v8:5267
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
TBR=bmeurer@chromium.org

Change-Id: I98d2a7e39892cd186bb8c9a43c6cf87fc6067d85
Reviewed-on: https://chromium-review.googlesource.com/476671
Reviewed-by: Michael Hablich <hablich@chromium.org>
Cr-Commit-Position: refs/branch-heads/5.9@{#6}
Cr-Branched-From: fe9bb7e6e251159852770160cfb21dad3cf03523-refs/heads/5.9.211@{#1}
Cr-Branched-From: 70ad23791a21c0dd7ecef8d4d8dd30ff6fc291f6-refs/heads/master@{#44591}
[modify] https://crrev.com/9372e6d03ddf825a9128cbccaf36e5430b0b9bb2/src/compiler/js-create-lowering.cc
[modify] https://crrev.com/9372e6d03ddf825a9128cbccaf36e5430b0b9bb2/src/compiler/js-native-context-specialization.cc
[modify] https://crrev.com/9372e6d03ddf825a9128cbccaf36e5430b0b9bb2/src/compiler/operation-typer.cc
[modify] https://crrev.com/9372e6d03ddf825a9128cbccaf36e5430b0b9bb2/src/compiler/operation-typer.h
[modify] https://crrev.com/9372e6d03ddf825a9128cbccaf36e5430b0b9bb2/src/compiler/simplified-lowering.cc
[modify] https://crrev.com/9372e6d03ddf825a9128cbccaf36e5430b0b9bb2/src/compiler/typed-optimization.cc
[modify] https://crrev.com/9372e6d03ddf825a9128cbccaf36e5430b0b9bb2/src/compiler/typed-optimization.h
[modify] https://crrev.com/9372e6d03ddf825a9128cbccaf36e5430b0b9bb2/src/compiler/typer.cc
[modify] https://crrev.com/9372e6d03ddf825a9128cbccaf36e5430b0b9bb2/src/compiler/types.h
[modify] https://crrev.com/9372e6d03ddf825a9128cbccaf36e5430b0b9bb2/src/compiler/verifier.cc
[modify] https://crrev.com/9372e6d03ddf825a9128cbccaf36e5430b0b9bb2/src/deoptimizer.cc
[modify] https://crrev.com/9372e6d03ddf825a9128cbccaf36e5430b0b9bb2/src/objects-inl.h
[modify] https://crrev.com/9372e6d03ddf825a9128cbccaf36e5430b0b9bb2/src/objects.cc
[modify] https://crrev.com/9372e6d03ddf825a9128cbccaf36e5430b0b9bb2/src/objects.h
[add] https://crrev.com/9372e6d03ddf825a9128cbccaf36e5430b0b9bb2/test/mjsunit/regress/regress-crbug-709753.js

Labels: -Merge-Review-58 Merge-Approved-58
Would be nice to have this but not ultra-critical because I+TF is not going to be rolled out to 100 %
Project Member

Comment 15 by bugdroid1@chromium.org, Apr 13 2017

Labels: merge-merged-5.8
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/46013d6901425e3820a5f6378c5d1770f536dfb5

commit 46013d6901425e3820a5f6378c5d1770f536dfb5
Author: Michael Hablich <hablich@chromium.org>
Date: Thu Apr 13 21:18:10 2017

Merged: Squashed multiple commits.

Merged: [turbofan] Properly represent the float64 hole.
Revision: 8c0c5e8117a0c935d6f2e5f6e540674d46753a87

Merged: [turbofan] Remove unused word32 truncation case for CheckFloat64Hole.
Revision: 2eeb085427ccc7fbb58c7c8a8c5301f5f2b9d29f

BUG= chromium:684208 , chromium:684208 , chromium:709753 , chromium:709753 ,v8:5267,v8:5267
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
TBR=bmeurer@chromium.org

Review-Url: https://codereview.chromium.org/2819653002 .
Cr-Commit-Position: refs/branch-heads/5.8@{#64}
Cr-Branched-From: eda659cc5e307f20ac1ad542ba12ab32eaf4c7ef-refs/heads/5.8.283@{#1}
Cr-Branched-From: 4310cd02d2160b1457baed81a2f40063eb264a21-refs/heads/master@{#43429}

[modify] https://crrev.com/46013d6901425e3820a5f6378c5d1770f536dfb5/src/compiler/js-create-lowering.cc
[modify] https://crrev.com/46013d6901425e3820a5f6378c5d1770f536dfb5/src/compiler/js-native-context-specialization.cc
[modify] https://crrev.com/46013d6901425e3820a5f6378c5d1770f536dfb5/src/compiler/operation-typer.cc
[modify] https://crrev.com/46013d6901425e3820a5f6378c5d1770f536dfb5/src/compiler/operation-typer.h
[modify] https://crrev.com/46013d6901425e3820a5f6378c5d1770f536dfb5/src/compiler/simplified-lowering.cc
[modify] https://crrev.com/46013d6901425e3820a5f6378c5d1770f536dfb5/src/compiler/typed-optimization.cc
[modify] https://crrev.com/46013d6901425e3820a5f6378c5d1770f536dfb5/src/compiler/typed-optimization.h
[modify] https://crrev.com/46013d6901425e3820a5f6378c5d1770f536dfb5/src/compiler/typer.cc
[modify] https://crrev.com/46013d6901425e3820a5f6378c5d1770f536dfb5/src/compiler/types.h
[modify] https://crrev.com/46013d6901425e3820a5f6378c5d1770f536dfb5/src/compiler/verifier.cc
[modify] https://crrev.com/46013d6901425e3820a5f6378c5d1770f536dfb5/src/deoptimizer.cc
[modify] https://crrev.com/46013d6901425e3820a5f6378c5d1770f536dfb5/src/objects-inl.h
[modify] https://crrev.com/46013d6901425e3820a5f6378c5d1770f536dfb5/src/objects.cc
[modify] https://crrev.com/46013d6901425e3820a5f6378c5d1770f536dfb5/src/objects.h
[add] https://crrev.com/46013d6901425e3820a5f6378c5d1770f536dfb5/test/mjsunit/regress/regress-crbug-709753.js

Labels: -merge-merged-5.8
Breaks the build in 5.8. Seems like this depends on something else in 5.9? Is this even broken in 5.8?
Project Member

Comment 17 by bugdroid1@chromium.org, Apr 13 2017

Labels: merge-merged-5.8
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/3e5ace888b36d218a9868fb1f42950778d860d0d

commit 3e5ace888b36d218a9868fb1f42950778d860d0d
Author: hablich <hablich@chromium.org>
Date: Thu Apr 13 21:25:28 2017

Revert of Merged: Squashed multiple commits. (patchset #1 id:1 of https://codereview.chromium.org/2819653002/ )

Reason for revert:
Breaks the build

Original issue's description:
> Merged: Squashed multiple commits.
>
> Merged: [turbofan] Properly represent the float64 hole.
> Revision: 8c0c5e8117a0c935d6f2e5f6e540674d46753a87
>
> Merged: [turbofan] Remove unused word32 truncation case for CheckFloat64Hole.
> Revision: 2eeb085427ccc7fbb58c7c8a8c5301f5f2b9d29f
>
> BUG= chromium:684208 , chromium:684208 , chromium:709753 , chromium:709753 ,v8:5267,v8:5267
> LOG=N
> NOTRY=true
> NOPRESUBMIT=true
> NOTREECHECKS=true
> TBR=bmeurer@chromium.org
>
> Review-Url: https://codereview.chromium.org/2819653002 .
> Cr-Commit-Position: refs/branch-heads/5.8@{#64}
> Cr-Branched-From: eda659cc5e307f20ac1ad542ba12ab32eaf4c7ef-refs/heads/5.8.283@{#1}
> Cr-Branched-From: 4310cd02d2160b1457baed81a2f40063eb264a21-refs/heads/master@{#43429}
> Committed: https://chromium.googlesource.com/v8/v8/+/46013d6901425e3820a5f6378c5d1770f536dfb5

TBR=bmeurer@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= chromium:684208 , chromium:684208 , chromium:709753 , chromium:709753 ,v8:5267,v8:5267

Review-Url: https://codereview.chromium.org/2820603002
Cr-Commit-Position: refs/branch-heads/5.8@{#65}
Cr-Branched-From: eda659cc5e307f20ac1ad542ba12ab32eaf4c7ef-refs/heads/5.8.283@{#1}
Cr-Branched-From: 4310cd02d2160b1457baed81a2f40063eb264a21-refs/heads/master@{#43429}

[modify] https://crrev.com/3e5ace888b36d218a9868fb1f42950778d860d0d/src/compiler/js-create-lowering.cc
[modify] https://crrev.com/3e5ace888b36d218a9868fb1f42950778d860d0d/src/compiler/js-native-context-specialization.cc
[modify] https://crrev.com/3e5ace888b36d218a9868fb1f42950778d860d0d/src/compiler/operation-typer.cc
[modify] https://crrev.com/3e5ace888b36d218a9868fb1f42950778d860d0d/src/compiler/operation-typer.h
[modify] https://crrev.com/3e5ace888b36d218a9868fb1f42950778d860d0d/src/compiler/simplified-lowering.cc
[modify] https://crrev.com/3e5ace888b36d218a9868fb1f42950778d860d0d/src/compiler/typed-optimization.cc
[modify] https://crrev.com/3e5ace888b36d218a9868fb1f42950778d860d0d/src/compiler/typed-optimization.h
[modify] https://crrev.com/3e5ace888b36d218a9868fb1f42950778d860d0d/src/compiler/typer.cc
[modify] https://crrev.com/3e5ace888b36d218a9868fb1f42950778d860d0d/src/compiler/types.h
[modify] https://crrev.com/3e5ace888b36d218a9868fb1f42950778d860d0d/src/compiler/verifier.cc
[modify] https://crrev.com/3e5ace888b36d218a9868fb1f42950778d860d0d/src/deoptimizer.cc
[modify] https://crrev.com/3e5ace888b36d218a9868fb1f42950778d860d0d/src/objects-inl.h
[modify] https://crrev.com/3e5ace888b36d218a9868fb1f42950778d860d0d/src/objects.cc
[modify] https://crrev.com/3e5ace888b36d218a9868fb1f42950778d860d0d/src/objects.h
[delete] https://crrev.com/46013d6901425e3820a5f6378c5d1770f536dfb5/test/mjsunit/regress/regress-crbug-709753.js

Project Member

Comment 18 by sheriffbot@chromium.org, Apr 17 2017

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
If there is no pending work in M58, please remove Merge-Approved-58 label. 
Owner: hablich@chromium.org
Labels: -Merge-Approved-58 merge-merged-58
Labels: NodeJS-Backport-Rejected
As far as I can tell, this is not needed for Node 6.x (V8 5.1).

Sign in to add a comment