New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 708050 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 4 2017

Issue description

Cc: jarin@chromium.org mstarzinger@chromium.org
Components: -Blink>JavaScript Blink>JavaScript>Compiler
Owner: bmeu...@chromium.org
Status: Assigned (was: Untriaged)
// PTAL. Points to:
https://chromium.googlesource.com/v8/v8/+/17d932c06b40d1e6617ebbd6cc8d1e8f411f9750


var v = {};
function foo() {
  v[0] = 5;
  v[-0] = 27;
  print(v[0]);
}

foo();
%OptimizeFunctionOnNextCall(foo);
foo();

// 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:
- 27
+ 5
#
### Start of configuration x64,ignition:
27
27

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

### End of configuration x64,ignition_turbo


Project Member

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

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

commit 483812d46cf96e26500b93b5b3d1cb9737236b2b
Author: bmeurer <bmeurer@chromium.org>
Date: Wed Apr 12 09:02:28 2017

[turbofan] Fix typing rule for CheckBounds.

As of crrev.com/2760213003, the CheckBounds operator passes a truncation
that identfies zero and minus zero. However that was not reflected in
the typing rule, and as such the type of CheckBounds(-0,length) was
always Type::None. That confused the typed alias analysis in the
LoadElimination and led to ignoring StoreElement nodes.

BUG= chromium:708050 
R=jarin@chromium.org

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

[modify] https://crrev.com/483812d46cf96e26500b93b5b3d1cb9737236b2b/src/compiler/typer.cc
[add] https://crrev.com/483812d46cf96e26500b93b5b3d1cb9737236b2b/test/mjsunit/regress/regress-crbug-708050-1.js
[add] https://crrev.com/483812d46cf96e26500b93b5b3d1cb9737236b2b/test/mjsunit/regress/regress-crbug-708050-2.js

Labels: Merge-Request-58
Status: Fixed (was: Assigned)
Project Member

Comment 4 by sheriffbot@chromium.org, Apr 12 2017

Labels: -Merge-Request-58 Merge-Review-58 Hotlist-Merge-Review
This bug requires manual review: We are only 12 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 5 by gov...@chromium.org, Apr 12 2017

Cc: abdulsyed@chromium.org hablich@chromium.org
+hablich@ for M58 merge review
Project Member

Comment 6 by ClusterFuzz, Apr 13 2017

ClusterFuzz has detected this issue as fixed in range 44597:44598.

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

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: a40
  
Sanitizer: address (ASAN)

Regressed: V8: 43968:43969
Fixed: V8: 44597:44598

Reproducer Testcase: https://clusterfuzz.com/download/AMIfv946TJNk4q2qiW1R5cvB0Bq9z2pxgCLMscUyLVsP6SOEi-g2xmG5c-Na4E3NYHI_vOXixYfw-t4k5Tr4K94FonqmqAoCrJh8G0g_XLIFoAJTBqYqpPLhd72kjx52ci2Nuy1FA_7Fj6xqgTWvh601lZ2jZ9pLglzK77e4qq9fInnYlO9CDHJNxapAFb_TE1OCYAzsSwBFQomBWajbRwlvwF3gq_DyzATsJ3eeIvNDqSEEsBKkkIeoEZtEumuhXV7o1ORbRVX2P8eE0AdP98c_FgpvUgJS7u-S5fEMXfW8fZdNSb8bNZ3gxgp2nOIfxUwADg3w4BoqHq-BCa-UsA75rpsduU0qTOScNi6UK42OlVXCn_0-qMBfOUZ4fo57SMwMRZHdQPObdCtEluMKDjiMgW1HHtYsBQ?testcase_id=6689781447917568


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 7 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/+/52f1895c9ed00c2a7d2c0094ec6787c4e6c28cf1

commit 52f1895c9ed00c2a7d2c0094ec6787c4e6c28cf1
Author: Michael Hablich <hablich@chromium.org>
Date: Thu Apr 13 12:31:19 2017

Merged: [turbofan] Fix typing rule for CheckBounds.

Revision: 483812d46cf96e26500b93b5b3d1cb9737236b2b

BUG= chromium:708050 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
TBR=bmeurer@chromium.org

Change-Id: Ib447c84f8ae37e6c16760a0ad716d59b08e55799
Reviewed-on: https://chromium-review.googlesource.com/476352
Reviewed-by: Michael Hablich <hablich@chromium.org>
Cr-Commit-Position: refs/branch-heads/5.9@{#7}
Cr-Branched-From: fe9bb7e6e251159852770160cfb21dad3cf03523-refs/heads/5.9.211@{#1}
Cr-Branched-From: 70ad23791a21c0dd7ecef8d4d8dd30ff6fc291f6-refs/heads/master@{#44591}
[modify] https://crrev.com/52f1895c9ed00c2a7d2c0094ec6787c4e6c28cf1/src/compiler/typer.cc
[add] https://crrev.com/52f1895c9ed00c2a7d2c0094ec6787c4e6c28cf1/test/mjsunit/regress/regress-crbug-708050-1.js
[add] https://crrev.com/52f1895c9ed00c2a7d2c0094ec6787c4e6c28cf1/test/mjsunit/regress/regress-crbug-708050-2.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 %
As crrev.com/2760213003 is not in M58, I don't think we even need to merge anything.
Project Member

Comment 10 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
As per #9, there is no merge required. Please remove Merge-Approved-58 label. 
Labels: -Merge-Approved-58

Sign in to add a comment