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 assigning owner based on suspected regression changelist https://chromium.googlesource.com/v8/v8/+/a67b7aa2cb773264324581eb302653698c29e19f ([turbofan] Only create Phis for live registers).

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

This is the most I've managed to minimize this test case so far:

function* gen() {
  for (let x of []) {
    try {
      var y;
      y.z = { 1: 1 };
      yield* [];
    } catch (e) {}
  }
}
gen();

Here's what I have so far:

  * The DCHECK is failing when merging the exceptional case of the "y.z = ..." StaNamedProperty
  * At this point, the register "r18" is live in the catch block, but dead in the existing catch environment
  * r18 is live in the catch block because it's live in the loop header, and it's live there because it is (unnecessarily) saved in the suspend of the yield*.
  * I can trigger the failure earlier by checking for optimized-out during the initial catch environment creation
  * The catch environment is created as the exceptional case of the "{ 1: 1 }; CreateObjectLiteral
  * That CreateObjectLiteral has "r18" as its RegOut, which means r18 is dead (and already set to optimized-out) coming into it.

Based on this, I think we have an error in calculating the liveness of exception-throwing bytecodes -- currently exception handler in-liveness is added to the out-liveness of a bytecode, but possibly it should rather be added to its in-liveness, as the exception happens before the bytecode has any output.

Jaro, does the above sound right?

I think that would not fix the problem:

We use the out-liveness to compute the lazy deoptimization frame state. When we lazy deopt into a catch handler, we use that frame state to compute the interpreter state at the start of the catch handler. If we wired the states the way you suggest, we would lose the values that are only used in the catch handler.

I am wondering whether the real problem is that suspend claims it uses r18. 

This looks somewhat related to  http://crbug.com/794822 , where the compiler is confused by the generator control flow in loop headers. There, the compiler thought that you can enter the loop and then fall through into the loop body, and then got confused. This is impossible - you can only fall through to loop body if you came into the header by the backedge. The compiler does not really know this because the impossibility is encoded in the generator state variable. The fix was some hacky relaxation of the DCHECK.

Here, the compiler thinks you can take the back edge and then take the dispatch into the yield*. This is also impossible - if you came through the backedge, you will fall through the switch. We could also relax the DCHECK, but at this point it really begins to feel tasteless. I am thinking we will have to find a way to teach the compiler about the generator control flow invariants. Not sure how, yet...

I'm not sure I understand your point about lazy deopts. The only way in-liveness could not produce a value that out-liveness does is if the bytecode kills a live variable, i.e. has that register as a RegOut parameter. But, if that's the case, then I would not expect that the catch handler could be expected to read that RegOut value.

I do see that this is a problem with over-estimating liveness, and I think it's reasonable to argue that the DCHECK is expecting perfect liveness rather than estimated liveness. I feel like it'd be possible to trigger a similar issue without using generators though, by using some other source of liveness inaccuracy (though nothing springs to mind immediately).

That said, we could get a better liveness estimate if we had a better encoding of possible generator behaviour in the bytecode. We could, for example, have a single generator state switch that can (irreducibly) jump into loops in the bytecode, and then only make loops reducible during graph building.

Either way, I don't think this is a security issue or a blocker, plz re-add labels if you disagree (I won't un-restrict view for now just in case).

Actually, thinking on it some more, even better switching might not help here, because r18 is live because of the suspend, not because of the resume.

A short amount of hacking later, I've got a better understanding of what's happening: it's not that the loop falsely inherits liveness from the resume, but rather that with nested loops, the parent loop's switch inherits liveness from the nested loop's switch fallthrough. In the above repro case, it's that the for-loop inherits the yield*'s resume_mode liveness (which happens to be on r18), jumping over the store which sets it (before the loop).

yield* desugars to:
  {
    ...
    let resumeMode = kNext;
    while (true) {
      ...
      yield;
      resumeMode = ...
    }
    USE(resumeMode) ...
  }

and the for-loop header switch jumps over the "let resumeMode = kNext" into the loop, where resumeMode is still live.

So, better switching may well help after all.

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

commit 41b80eeffdb63a8eebc8c65955bc7b678a7441be
Author: Leszek Swirski <leszeks@chromium.org>
Date: Mon Jan 22 13:15:21 2018

[compiler] Propagate liveness across suspends

Suspend points (inside generators and async functions) have slightly
funky semantics when it comes to liveness, as they save and restore a
chunk of the register file as-is. In particular, this means that
granular liveness information is lost, as it is assumed that all
registers in that chunk of the register file are live in a suspend.

Rather than marking that entire chunk of register as live/dead in
suspend/restore, we can instead pattern-match the set of bytecodes in a
suspend point, and propagate liveness across them. This tightens
liveness estimates, and could be used to optimize which values TurboFan
actually saves when suspending.

Bug:  chromium:798137 
Change-Id: I5840cdbfc2c6edb1d3a48cf025f52615b629cdfc
Reviewed-on: https://chromium-review.googlesource.com/848895
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50757}
[modify] https://crrev.com/41b80eeffdb63a8eebc8c65955bc7b678a7441be/src/compiler/bytecode-analysis.cc

ClusterFuzz has detected this issue as fixed in range 50803:50804.

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

Fuzzer: ochang_js_fuzzer
Job Type: linux_asan_d8_dbg
Platform Id: linux

Crash Type: DCHECK failure
Crash Address: 
Crash State:
  values_[index] != builder()->jsgraph()->OptimizedOutConstant() in bytecode-graph
  v8::internal::compiler::BytecodeGraphBuilder::Environment::Merge
  v8::internal::compiler::BytecodeGraphBuilder::MergeIntoSuccessorEnvironment
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_d8_dbg&range=47388:47389
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_d8_dbg&range=50803:50804

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

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 5918016612335616 is verified as fixed, so closing issue as verified.

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