Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.

Automatically adding ccs based on suspected regression changelists:

[turbofan] Lower NumberConstant nodes to IntPtrConstant. by pierre.langlois@arm.com - https://chromium.googlesource.com/v8/v8/+/7ac10da795aa55eaf5217864972672e4d22f2709

If this is incorrect, please apply the Test-Predator-Wrong-CLs label.

Hello,

I'm afraid I do not have permissions to see the report. Do you have instructions to reproduce the issue?

Thanks,
Pierre

Local bisect confirmed your CL. Assigning to Jaro for now (owner must be a project member), CC'ing Benedikt.

Minimized Reproducer:

function assertEquals(expected, found) {
  found.length !== expected.length;
}
assertEquals([], [])
assertEquals("a", "a");
assertEquals([], []);
function f() {
  assertEquals(0, undefined);
}
try {
  f();
} catch (e) {
}
%OptimizeFunctionOnNextCall(f);
f();

Thanks for the reproducer, I can confirm the crash on x64 with "is_asan = true". Let's see if I can find what the problem is.

Hello,

I've been investigating this today so here is some analysis.

When f fails to compile, assertEquals is inlined and it appears that
its arguments, expected and found, were speculated to be tagged
pointers. As a result, we end up with a CheckHeapObject on a
NumberConstant(0), this is guaranteed to deopt.

Was this a scenario you were aware of?

Now to the actual crash. We have the following bits of schedule for f:

~~~
  // snip
  77: Int64Constant[0]  <--- This used to be NumberConstant[0] before this patch.
  // snip
--- BLOCK B3 <- B2, B1 ---
  // snip

  // Deopt if Int64Contant[0] is a Smi.
  99: Int64Constant[1]
  100: Word64And(77, 99)
  101: Word64Equal(100, 77)
  102: DeoptimizeIf[Eager:Smi](101, 42, 38, 38)

  // Up until now, all code is dead since we know 0 is a Smi.
  // Load map.
  137: Int64Constant[-1]
  104: HeapConstant[0x7ec12c702481 <Map(HOLEY_ELEMENTS)>]
  103: Load[kRepTaggedPointer|kTypeAny](77, 137, 102, 102)
  105: Word64Equal(103, 104)
  125: Int64Constant[32]
  107: Branch[None](105, 102) -> B5, B4
--- BLOCK B4 <- B3 ---
  109: IfFalse(107)

  // Deopt if the map does not match.
  122: HeapConstant[0x7ea59c002751 <Map(PACKED_SMI_ELEMENTS)>]
  121: Load[kRepTaggedPointer|kTypeAny](77, 137, 103, 109)
  123: Word64Equal(121, 122)
  124: DeoptimizeUnless[Eager:WrongMap](123, 42, 121, 109)

  // Load property and tag it.
  139: Int64Constant[23]
  63: Load[kRepTaggedSigned|kTypeInt32](77, 139, 124, 124) ;; Cannot compile this.
  126: Word64Sar(63, 125)                                  ;;
  127: TruncateInt64ToInt32(126)                           ;;
  Goto -> B6
--- BLOCK B5 <- B3 ---
~~~

The issue appears to be with compiling the last load+tag operation. We generate:

~~~
v10(R) = X64Movl : MR #27
~~~

instead of:

~~~
v9(R) = X64Movl : MRI v7(R) #27
~~~

And the former triggers an assertion because we expect to see a base
register and not just an immediate. Also, the address mode doesn't
look right either.

Looking through the source code led me to
https://chromium.googlesource.com/v8/v8/+/master/src/compiler/x64/instruction-selector-x64.cc#101
were we have a special handling of Int64Constant[0]. The patch which
triggers the issue replaced NumberConstant[0] with Int64Constant[0] so
this must be related. Note that this only happens when this is called
from VisitTruncateInt64ToInt32 where we try to load and tag at the
same time.

I hope all this makes sense and helps. I tried to see if I could fix
it myself but I'm afraid I'm a little lost in the x64 selector as I'm
not familiar with x64 architecture at all.

Thanks,
Pierre

This looks like some bug in the address mode matching. Danno, could you have a look, please?

This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

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

I was also able to reproduce on x64, but I am no longer sure that this is an instruction selection problem. Examining the schedule before the crash, I get the following:

  ...
  77: Int64Constant[0]
  ...
  137: Int64Constant[-1]
  ...
  103: Load[kRepTaggedPointer|kTypeAny](77, 137, 102, 102)
  ...

This is a load from the immediate address -1, which doesn't seem right both because the address is weird and because both parameters to the load are immediate values with the first one being zero (rather than the second).

Hi danno,

I was also unclear about this. It seems the bigger issue with this test case is that the function `f` is inlining `assertEqual` while passing it values that are at odds with the feedback collected for it. `assertEqual` expects tagged pointers but we are passing the Smi value 0. So as a result of using %OptimizeFunctionOnNextCall, we end up compiling `f` while knowing for sure that it will be deoptimized. And we generate dead code as a result:

~~~
if (IsTaggedPointer(0)) {
  map = Load(0, -1);
} else {
  Deopt(kIsSmi);
}
~~~

Technically, the problematic load is dominated by the tagged pointer check so we should be safe. Which then made me think maybe it's OK to compile this and then the problem would be with the x64 selector getting confused when it shouldn't.

Also, is it correct that this strange state we end up in here would not normally occur? Here we have use %OptimizedFunctionOnNextCall to force TurboFan to compile `f` when it shouldn't have.

Does this make sense?

Finally, I noticed cases similar to this in CSA code, where we pass NumberConstant(0) to a method that can handle both Smis and HeapNumbers (see https://chromium.googlesource.com/v8/v8/+/master/src/builtins/builtins-array-gen.cc#1397). This generates similar code where we statically know the result of a Smi check but still generate code for both outcomes. We generate "load at address 0 - offset" code in these cases too. We have an internal task to maybe look into optimizing those out one day but that's another topic :-).

It sometimes does happen that static type and type feedback disagree in the compiler. Typically, when we inline a function that has collected feedback somewhere else (from some other call site). If that happens then the compiler might see nonsensical operations, but that's fine because the code is unreachable. We just need to survive the compilation.

We already had this problem in other places (@tebbi has great experience with fixing such bugs).

See also V8 issue https://crbug.com/v8/7362. Maybe duplicate? It runs into the same dcheck with the gc fuzzer. Repros reliably just with optdebug.

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

commit 9ef2ed308522d86ea93cd5a39bd4e188a5ae4f02
Author: Daniel Clifford <danno@chromium.org>
Date: Thu Feb 01 14:44:19 2018

Fix bug in x64 immediate operand handling for smi-converting loads

Bug:  chromium:802060 
Change-Id: I032930af26f7eab8d5d3469ad273bdcdff85b045
Reviewed-on: https://chromium-review.googlesource.com/897723
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Daniel Clifford <danno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51035}
[modify] https://crrev.com/9ef2ed308522d86ea93cd5a39bd4e188a5ae4f02/src/compiler/x64/instruction-selector-x64.cc
[add] https://crrev.com/9ef2ed308522d86ea93cd5a39bd4e188a5ae4f02/test/mjsunit/regress/regress-802060.js

ClusterFuzz has detected this issue as fixed in range 51034:51035.

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

Fuzzer: ochang_js_fuzzer
Job Type: linux_asan_d8_dbg
Platform Id: linux

Crash Type: DCHECK failure
Crash Address: 
Crash State:
  op->IsAnyLocationOperand() in instruction.h
  cast
  ToRegister
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_d8_dbg&range=50568:50569
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_d8_dbg&range=51034:51035

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=4563349668102144

See https://github.com/google/clusterfuzz-tools for more information.

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

ClusterFuzz testcase 4563349668102144 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

This bug requires manual review: M65 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

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

[Bulk Edit]

+awhalley@ (Security TPM) for M65 merge review

govind@ - good for 65.

Approving merge to M65 branch 3325 based on comment #27. Please merge ASAP. Thank you.

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

commit cc2e87e4bb15bff9423336db29131ab7afa86e99
Author: Daniel Clifford <danno@chromium.org>
Date: Fri Feb 09 11:14:27 2018

Merged: Fix bug in x64 immediate operand handling for smi-converting loads

NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true

Bug:  chromium:802060 
Change-Id: I032930af26f7eab8d5d3469ad273bdcdff85b045
Reviewed-on: https://chromium-review.googlesource.com/897723
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Daniel Clifford <danno@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#51035}(cherry picked from commit 9ef2ed308522d86ea93cd5a39bd4e188a5ae4f02)
Reviewed-on: https://chromium-review.googlesource.com/910928
Cr-Commit-Position: refs/branch-heads/6.5@{#35}
Cr-Branched-From: 73c55f57fe8506011ff854b15026ca765b669700-refs/heads/6.5.254@{#1}
Cr-Branched-From: 594a1a0b6e551397cfdf50870f6230da34db2dc8-refs/heads/master@{#50664}
[modify] https://crrev.com/cc2e87e4bb15bff9423336db29131ab7afa86e99/src/compiler/x64/instruction-selector-x64.cc
[add] https://crrev.com/cc2e87e4bb15bff9423336db29131ab7afa86e99/test/mjsunit/regress/regress-802060.js

Per comment #29, this is already merged to M65.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

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