[GC] Fix code flushing to use bytecode if it exists.

[GC] Fix code flushing to use bytecode if it exists.

If code is flushed on a SFI, we can still use the bytecode if it was compiled,
since this never gets flushed.

This fixes a DCHECK where we were trying to compile the bytecode multiple
times after the baseline code was flushed.

BUG=chromium:668133
Committed: https://crrev.com/49ea60ef9d4a0659a3ff3ade356aacc29d65d586
Cr-Commit-Position: refs/heads/master@{#41274}

[rmcilroy, 2016-11-24 11:59:21]

rmcilroy@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[rmcilroy, 2016-11-24 11:59:21]

Michi, this fixes the clusterfuzz bug I had. Could you please take a look,
thanks.

[rmcilroy, 2016-11-24 11:59:58]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 12:00:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Benedikt Meurer, 2016-11-24 12:08:04]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2016-11-24 12:08:04]

Awesome, LGTM.

[Benedikt Meurer, 2016-11-24 12:08:48]

Description was changed from

==========
[GC] Fix code flushing to use bytecode if it exists.

If code is flushed on a SFI, we can still use the bytecode if it was compiled,
since this never gets flushed.

This fixes a DCHECK where we were trying to compile the bytecode multiple
times after the baseline code was flushed.

BUG=chromium:668133
==========

to

==========
[GC] Fix code flushing to use bytecode if it exists.

If code is flushed on a SFI, we can still use the bytecode if it was compiled,
since this never gets flushed.

This fixes a DCHECK where we were trying to compile the bytecode multiple
times after the baseline code was flushed.

BUG=chromium:668133
==========

[Benedikt Meurer, 2016-11-24 12:08:48]

bmeurer@chromium.org changed required reviewers:
+ mstarzinger@chromium.org

[Michael Starzinger, 2016-11-24 12:14:45]

https://codereview.chromium.org/2526243002/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/2526243002/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:931: if (shared->HasBytecodeArray()) {
I have the feeling we will have the same problem in {Runtime_InstantiateAsmJs}
as well. We also unconditionally reset the shared code to the CompileLazy
builtin there. Would it be possible to instead make compiler.cc resilient to
ensure we don't accidentally regenerate bytecode even if the CompileLazy stub is
installed as shared code?

[commit-bot: I haz the power, 2016-11-24 12:24:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 12:24:52]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-11-24 12:55:45]

On 2016/11/24 12:14:45, Michael Starzinger wrote:
> https://codereview.chromium.org/2526243002/diff/1/src/heap/mark-compact.cc
> File src/heap/mark-compact.cc (right):
> 
>
https://codereview.chromium.org/2526243002/diff/1/src/heap/mark-compact.cc#ne...
> src/heap/mark-compact.cc:931: if (shared->HasBytecodeArray()) {
> I have the feeling we will have the same problem in {Runtime_InstantiateAsmJs}
> as well. We also unconditionally reset the shared code to the CompileLazy
> builtin there. Would it be possible to instead make compiler.cc resilient to
> ensure we don't accidentally regenerate bytecode even if the CompileLazy stub
is
> installed as shared code?

Hmm, in InstantiateAsmJs we should never have generated bytecode before this is
called, should we? So I don't think we should end up in a situation where we set
it to CompileLazy there but have a bytecode?

We could make CompileLazy in compiler.cc not regenerate bytecode if it already
exists, but I would prefer to keep the predicate that if is_compiled is false,
there is no bytecode. Maybe we could add a DCHECK in set_code to check that if
we are setting it to CompileLazy, there isn't bytecode, WDYT?

[Michael Starzinger, 2016-11-24 13:40:01]

On 2016/11/24 12:55:45, rmcilroy wrote:
> On 2016/11/24 12:14:45, Michael Starzinger wrote:
> > https://codereview.chromium.org/2526243002/diff/1/src/heap/mark-compact.cc
> > File src/heap/mark-compact.cc (right):
> > 
> >
>
https://codereview.chromium.org/2526243002/diff/1/src/heap/mark-compact.cc#ne...
> > src/heap/mark-compact.cc:931: if (shared->HasBytecodeArray()) {
> > I have the feeling we will have the same problem in
{Runtime_InstantiateAsmJs}
> > as well. We also unconditionally reset the shared code to the CompileLazy
> > builtin there. Would it be possible to instead make compiler.cc resilient to
> > ensure we don't accidentally regenerate bytecode even if the CompileLazy
stub
> is
> > installed as shared code?
> 
> Hmm, in InstantiateAsmJs we should never have generated bytecode before this
is
> called, should we? So I don't think we should end up in a situation where we
set
> it to CompileLazy there but have a bytecode?

Here is a repro that examplifies the issue. If one were to put a
DCHECK(!function->shared()->HasBytecodeArray()) into {Runtime_InstantiateAsmJs}
it would fire with the following repro:

function MkModule() {
  function Module(stdlib) {
    "use asm";
    var StdlibNaN = stdlib.NaN;
    function f() { return +StdlibNaN; }
    return { f:f };
  }
  return Module;
}
var ModuleA = MkModule();
var ModuleB = MkModule();
var ModuleC = MkModule();
var module1 = ModuleA(this);
var module2 = ModuleB(this);
var module3 = ModuleC(this);
var module4 = ModuleB(23);
var module5 = ModuleC(42);

> 
> We could make CompileLazy in compiler.cc not regenerate bytecode if it already
> exists, but I would prefer to keep the predicate that if is_compiled is false,
> there is no bytecode. Maybe we could add a DCHECK in set_code to check that if
> we are setting it to CompileLazy, there isn't bytecode, WDYT?

We already do have such a check in {GetLazyCode}, which is actually what safes
the above example from causing further havoc. This is the check I am referring
to:
https://cs.chromium.org/chromium/src/v8/src/compiler.cc?q=HasBytecodeArray&sq...

[rmcilroy, 2016-11-24 14:24:34]

On 2016/11/24 13:40:01, Michael Starzinger wrote:
> On 2016/11/24 12:55:45, rmcilroy wrote:
> > On 2016/11/24 12:14:45, Michael Starzinger wrote:
> > > https://codereview.chromium.org/2526243002/diff/1/src/heap/mark-compact.cc
> > > File src/heap/mark-compact.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2526243002/diff/1/src/heap/mark-compact.cc#ne...
> > > src/heap/mark-compact.cc:931: if (shared->HasBytecodeArray()) {
> > > I have the feeling we will have the same problem in
> {Runtime_InstantiateAsmJs}
> > > as well. We also unconditionally reset the shared code to the CompileLazy
> > > builtin there. Would it be possible to instead make compiler.cc resilient
to
> > > ensure we don't accidentally regenerate bytecode even if the CompileLazy
> stub
> > is
> > > installed as shared code?
> > 
> > Hmm, in InstantiateAsmJs we should never have generated bytecode before this
> is
> > called, should we? So I don't think we should end up in a situation where we
> set
> > it to CompileLazy there but have a bytecode?
> 
> Here is a repro that examplifies the issue. If one were to put a
> DCHECK(!function->shared()->HasBytecodeArray()) into
{Runtime_InstantiateAsmJs}
> it would fire with the following repro:
> 
> function MkModule() {
>   function Module(stdlib) {
>     "use asm";
>     var StdlibNaN = stdlib.NaN;
>     function f() { return +StdlibNaN; }
>     return { f:f };
>   }
>   return Module;
> }
> var ModuleA = MkModule();
> var ModuleB = MkModule();
> var ModuleC = MkModule();
> var module1 = ModuleA(this);
> var module2 = ModuleB(this);
> var module3 = ModuleC(this);
> var module4 = ModuleB(23);
> var module5 = ModuleC(42);
> 
> > 
> > We could make CompileLazy in compiler.cc not regenerate bytecode if it
already
> > exists, but I would prefer to keep the predicate that if is_compiled is
false,
> > there is no bytecode. Maybe we could add a DCHECK in set_code to check that
if
> > we are setting it to CompileLazy, there isn't bytecode, WDYT?
> 
> We already do have such a check in {GetLazyCode}, which is actually what safes
> the above example from causing further havoc. This is the check I am referring
> to:
>
https://cs.chromium.org/chromium/src/v8/src/compiler.cc?q=HasBytecodeArray&sq...

I tried this. You are right that we enter Runtime_InstansiateAsmJS with a
bytecode array on the execution of module5, however there is a check in
Runtime_InstansiateAsmJS which only sets the SFIs code to CompileLazy if it is
Builtin::kInstansiateAsmJs.  In this case the SFI code now points to the
interpreter entry routine, so we don't see this.

I tried adding the DCHECK I was suggesting to SFI::set_code, namely:
DCHECK(value != GetIsolate()->builtins()->builtin(Builtins::kCompileLazy) ||
       !HasBytecodeArray());
And this doesn't trip for the example above. It also seems to pass on all our
mjsunit tests. I uploaded the check in the latest patch set, WDYT?

[rmcilroy, 2016-11-24 14:25:15]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 14:25:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 14:44:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 14:44:14]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/13167)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[rmcilroy, 2016-11-24 14:56:56]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 14:57:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Michael Starzinger, 2016-11-24 15:33:53]

LGTM to unblock this CL. I still think we should follow-up and make sure the
compilation pipeline is resilient enough against other components resetting the
shared code to the CompileLazy stub.

[rmcilroy, 2016-11-24 15:44:21]

On 2016/11/24 15:33:53, Michael Starzinger wrote:
> LGTM to unblock this CL. I still think we should follow-up and make sure the
> compilation pipeline is resilient enough against other components resetting
the
> shared code to the CompileLazy stub.

Thanks. I'm happy to do this follow-up, but let's discuss offline exactly what
we want, since I would rather we didn't add resilience for things which we don't
think / want to happen - I'd rather add [D]CHECKs to make sure they never happen
instead.

[commit-bot: I haz the power, 2016-11-24 15:57:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 15:57:46]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-11-24 16:36:26]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2016-11-24 16:36:27]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/2526243002/#ps40001
(title: "Fix cctests")

[commit-bot: I haz the power, 2016-11-24 16:36:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 17:26:34]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480005386611010,
"parent_rev": "7b1ac2b736df5478f10e5123a7f8aeb47d4f5760", "commit_rev":
"de25d982d2696e450e6b6e20a6ca86b7b40fd3a5"}

[commit-bot: I haz the power, 2016-11-24 17:26:43]

Description was changed from

==========
[GC] Fix code flushing to use bytecode if it exists.

If code is flushed on a SFI, we can still use the bytecode if it was compiled,
since this never gets flushed.

This fixes a DCHECK where we were trying to compile the bytecode multiple
times after the baseline code was flushed.

BUG=chromium:668133
==========

to

==========
[GC] Fix code flushing to use bytecode if it exists.

If code is flushed on a SFI, we can still use the bytecode if it was compiled,
since this never gets flushed.

This fixes a DCHECK where we were trying to compile the bytecode multiple
times after the baseline code was flushed.

BUG=chromium:668133
==========

[commit-bot: I haz the power, 2016-11-24 17:26:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-24 17:27:13]

Description was changed from

==========
[GC] Fix code flushing to use bytecode if it exists.

If code is flushed on a SFI, we can still use the bytecode if it was compiled,
since this never gets flushed.

This fixes a DCHECK where we were trying to compile the bytecode multiple
times after the baseline code was flushed.

BUG=chromium:668133
==========

to

==========
[GC] Fix code flushing to use bytecode if it exists.

If code is flushed on a SFI, we can still use the bytecode if it was compiled,
since this never gets flushed.

This fixes a DCHECK where we were trying to compile the bytecode multiple
times after the baseline code was flushed.

BUG=chromium:668133

Committed: https://crrev.com/49ea60ef9d4a0659a3ff3ade356aacc29d65d586
Cr-Commit-Position: refs/heads/master@{#41274}
==========

[commit-bot: I haz the power, 2016-11-24 17:27:14]

Patchset 3 (id:??) landed as
https://crrev.com/49ea60ef9d4a0659a3ff3ade356aacc29d65d586
Cr-Commit-Position: refs/heads/master@{#41274}