Make FlushICache part of Assembler(Base) and take Isolate as parameter.

Make FlushICache part of Assembler(Base) and take Isolate as parameter.

BUG=chromium:524425
LOG=N

Committed: https://crrev.com/9fc4fc141fa1d1aa1a3ccb901518cdffa0d6d268
Cr-Commit-Position: refs/heads/master@{#30695}

[Michael Lippautz, 2015-09-11 11:08:32]

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

[commit-bot: I haz the power, 2015-09-11 11:08:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1332283002/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1332283002/210001

[Michael Lippautz, 2015-09-11 11:10:09]

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

[Michael Lippautz, 2015-09-11 11:10:10]

PTAL

https://codereview.chromium.org/1332283002/diff/210001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/1332283002/diff/210001/src/assembler.cc#newco...
src/assembler.cc:173: // out issues because we usually only need the isolate
when in the simulator.
I think this is unfortunate but apparently we already have calls that cannot use
Isolate::Current() when not executed on a simulator.

[commit-bot: I haz the power, 2015-09-11 11:12:26]

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

[commit-bot: I haz the power, 2015-09-11 11:12:26]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)

[Michael Starzinger, 2015-09-11 11:40:03]

Looking good, just a couple of comment. I only reviewed one architecture, but my
comments probably apply to all of them.

https://codereview.chromium.org/1332283002/diff/230001/src/arm/code-stubs-arm.h
File src/arm/code-stubs-arm.h (right):

https://codereview.chromium.org/1332283002/diff/230001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.h:132:
Assembler::FlushICacheWithoutIsolate(stub->instruction_start(),
This could be switched to the version taking an isolate by using
"stub->GetIsolate()" here.

https://codereview.chromium.org/1332283002/diff/230001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

https://codereview.chromium.org/1332283002/diff/230001/src/arm/codegen-arm.cc...
src/arm/codegen-arm.cc:937: Assembler::FlushICacheWithoutIsolate(sequence,
young_length);
The isolate is available here, let's use it.

https://codereview.chromium.org/1332283002/diff/230001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/1332283002/diff/230001/src/assembler.h#newcode64
src/assembler.h:64: class CpuFeatures : public AllStatic {
I don't see a particular need to move this up here, let's move it down again to
where it was before.

https://codereview.chromium.org/1332283002/diff/230001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/1332283002/diff/230001/src/deoptimizer.cc#new...
src/deoptimizer.cc:1855:
Assembler::FlushICacheWithoutIsolate(chunk->area_start(), desc.instr_size);
The isolate is available here, let's use it.

https://codereview.chromium.org/1332283002/diff/230001/src/snapshot/serialize.cc
File src/snapshot/serialize.cc (right):

https://codereview.chromium.org/1332283002/diff/230001/src/snapshot/serialize...
src/snapshot/serialize.cc:508:
Assembler::FlushICacheWithoutIsolate(p->area_start(),
The isolate is available as "isolate_" here, let's use it.

https://codereview.chromium.org/1332283002/diff/230001/src/snapshot/serialize...
src/snapshot/serialize.cc:517:
Assembler::FlushICacheWithoutIsolate(code->instruction_start(),
Likewise.

[Michael Lippautz, 2015-09-11 11:54:52]

Updated all callers to pass the isolate when available.

https://codereview.chromium.org/1332283002/diff/230001/src/arm/code-stubs-arm.h
File src/arm/code-stubs-arm.h (right):

https://codereview.chromium.org/1332283002/diff/230001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.h:132:
Assembler::FlushICacheWithoutIsolate(stub->instruction_start(),
On 2015/09/11 11:40:03, Michael Starzinger wrote:
> This could be switched to the version taking an isolate by using
> "stub->GetIsolate()" here.

Done.

https://codereview.chromium.org/1332283002/diff/230001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

https://codereview.chromium.org/1332283002/diff/230001/src/arm/codegen-arm.cc...
src/arm/codegen-arm.cc:937: Assembler::FlushICacheWithoutIsolate(sequence,
young_length);
On 2015/09/11 11:40:03, Michael Starzinger wrote:
> The isolate is available here, let's use it.

Done.

https://codereview.chromium.org/1332283002/diff/230001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/1332283002/diff/230001/src/assembler.h#newcode64
src/assembler.h:64: class CpuFeatures : public AllStatic {
On 2015/09/11 11:40:03, Michael Starzinger wrote:
> I don't see a particular need to move this up here, let's move it down again
to
> where it was before.

Done. This was needed when the methods were still defined in the .h file.

https://codereview.chromium.org/1332283002/diff/230001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/1332283002/diff/230001/src/deoptimizer.cc#new...
src/deoptimizer.cc:1855:
Assembler::FlushICacheWithoutIsolate(chunk->area_start(), desc.instr_size);
On 2015/09/11 11:40:03, Michael Starzinger wrote:
> The isolate is available here, let's use it.

Done.

https://codereview.chromium.org/1332283002/diff/230001/src/snapshot/serialize.cc
File src/snapshot/serialize.cc (right):

https://codereview.chromium.org/1332283002/diff/230001/src/snapshot/serialize...
src/snapshot/serialize.cc:508:
Assembler::FlushICacheWithoutIsolate(p->area_start(),
On 2015/09/11 11:40:03, Michael Starzinger wrote:
> The isolate is available as "isolate_" here, let's use it.

Done.

https://codereview.chromium.org/1332283002/diff/230001/src/snapshot/serialize...
src/snapshot/serialize.cc:517:
Assembler::FlushICacheWithoutIsolate(code->instruction_start(),
On 2015/09/11 11:40:03, Michael Starzinger wrote:
> Likewise.

Done.

[Michael Starzinger, 2015-09-11 11:58:40]

LGTM, just one more nit.

https://codereview.chromium.org/1332283002/diff/240001/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

https://codereview.chromium.org/1332283002/diff/240001/src/arm/assembler-arm....
src/arm/assembler-arm.h:663: using AssemblerBase::FlushICacheWithoutIsolate;
Now that the methods on AssemblerBase are public, we can drop these two using
directives again I think. Applies to all other architectures as well.

[Michael Lippautz, 2015-09-11 12:05:50]

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

[commit-bot: I haz the power, 2015-09-11 12:05:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1332283002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1332283002/300001

[Michael Lippautz, 2015-09-11 12:10:23]

mlippautz@chromium.org changed reviewers:
+ vogelheim@chromium.org

[Michael Lippautz, 2015-09-11 12:10:24]

+vogelheim

Please have a look at
  src/snapshot/serialize.cc

Thanks!

https://codereview.chromium.org/1332283002/diff/240001/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

https://codereview.chromium.org/1332283002/diff/240001/src/arm/assembler-arm....
src/arm/assembler-arm.h:663: using AssemblerBase::FlushICacheWithoutIsolate;
On 2015/09/11 11:58:40, Michael Starzinger wrote:
> Now that the methods on AssemblerBase are public, we can drop these two using
> directives again I think. Applies to all other architectures as well.

Done.

[Michael Starzinger, 2015-09-11 12:18:05]

https://codereview.chromium.org/1332283002/diff/300001/src/arm64/assembler-ar...
File src/arm64/assembler-arm64.h (right):

https://codereview.chromium.org/1332283002/diff/300001/src/arm64/assembler-ar...
src/arm64/assembler-arm64.h:2282: Assembler::FlushICache(isolate(), buffer_,
length);
This will crash, because the underlying assembler above is initialized wirh
isolate == NULL!

[Michael Lippautz, 2015-09-11 12:25:16]

https://codereview.chromium.org/1332283002/diff/300001/src/arm64/assembler-ar...
File src/arm64/assembler-arm64.h (right):

https://codereview.chromium.org/1332283002/diff/300001/src/arm64/assembler-ar...
src/arm64/assembler-arm64.h:2282: Assembler::FlushICache(isolate(), buffer_,
length);
On 2015/09/11 12:18:05, Michael Starzinger wrote:
> This will crash, because the underlying assembler above is initialized wirh
> isolate == NULL!

Thanks for catching this ... ;)

[Michael Lippautz, 2015-09-11 12:28:54]

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

[commit-bot: I haz the power, 2015-09-11 12:29:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1332283002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1332283002/320001

[vogelheim, 2015-09-11 12:51:19]

lgtm [for src/snapshot/serialize.cc]

[commit-bot: I haz the power, 2015-09-11 12:53:51]

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

[commit-bot: I haz the power, 2015-09-11 12:53:52]

Dry run: This issue passed the CQ dry run.

[Michael Lippautz, 2015-09-11 12:54:46]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2015-09-11 12:54:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1332283002/#ps320001
(title: "Fix arm64 PatchingAssembler that initialized its isolate with NULL :)")

[commit-bot: I haz the power, 2015-09-11 12:54:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1332283002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1332283002/320001

[commit-bot: I haz the power, 2015-09-11 12:57:32]

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

[commit-bot: I haz the power, 2015-09-11 12:57:33]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/5713)

[Michael Lippautz, 2015-09-11 12:57:54]

The CQ bit was checked by mlippautz@chromium.org

[commit-bot: I haz the power, 2015-09-11 12:58:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1332283002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1332283002/320001

[commit-bot: I haz the power, 2015-09-11 12:59:40]

Committed patchset #18 (id:320001)

[commit-bot: I haz the power, 2015-09-11 12:59:58]

Patchset 18 (id:??) landed as
https://crrev.com/9fc4fc141fa1d1aa1a3ccb901518cdffa0d6d268
Cr-Commit-Position: refs/heads/master@{#30695}