[wasm] OOB traps: build protected instruction list during codegen

[wasm] OOB traps: build protected instruction list during codegen

During codegen, we build a list mapping protected instructions to their
associated landing pads. This will ultimately by used by the signal handler to
recover from out of bounds faults and throw a JS exception.

This is mostly pulled from my larger in-progress CL at
https://codereview.chromium.org/2371833007/.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=5277

Committed: https://crrev.com/bf35d15e5265f2d6ac74a594876d73ced535d8d1
Cr-Commit-Position: refs/heads/master@{#41400}

[Eric Holk, 2016-11-11 21:55:04]

eholk@chromium.org changed reviewers:
+ bradnelson@chromium.org, mtrofin@google.com, titzer@chromium.org

[Eric Holk, 2016-11-11 21:59:22]

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

[commit-bot: I haz the power, 2016-11-11 21:59:32]

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-11 22:06:18]

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

[commit-bot: I haz the power, 2016-11-11 22:06:19]

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

[Eric Holk, 2016-11-11 22:37:34]

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

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

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-11 22:42:24]

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

[commit-bot: I haz the power, 2016-11-11 22:42:25]

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

[Eric Holk, 2016-11-11 22:58:35]

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

[commit-bot: I haz the power, 2016-11-11 22:58:45]

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-11 23:24:56]

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

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

Dry run: This issue passed the CQ dry run.

[Eric Holk, 2016-11-16 17:19:01]

Has anyone had a chance to take a look at this yet?

[titzer, 2016-11-16 18:06:47]

Generally the right direction, but I think we should move the protected
instruction list data structures into the compiler.

https://codereview.chromium.org/2500443004/diff/40001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2500443004/diff/40001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:811: Handle<FixedArray>
WasmModule::PackProtectedInstructions(
I think this can also move into the compiler.

https://codereview.chromium.org/2500443004/diff/40001/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/2500443004/diff/40001/src/wasm/wasm-module.h#...
src/wasm/wasm-module.h:35: struct ProtectedInstructionData {
I think it'd be best to move this into the compiler directory, e.g. in its own
header/cc file, so that the compiler doesn't depend on WASM at all.

https://codereview.chromium.org/2500443004/diff/40001/src/wasm/wasm-module.h#...
src/wasm/wasm-module.h:300: std::vector<ProtectedInstructionList>
protected_instructions;  // Instructions
I think this should somehow be associated with the module rather than the
instance, since aren't these all relative to some base (i.e. they don't change
for different instances of the same module)?

[Eric Holk, 2016-11-18 02:19:47]

I was able to move pretty much everything requested into the compiler.

The protected instruction lists now hang off the code objects, which is really a
more logical place for them to be.

https://codereview.chromium.org/2500443004/diff/40001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2500443004/diff/40001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:811: Handle<FixedArray>
WasmModule::PackProtectedInstructions(
On 2016/11/16 18:06:47, titzer wrote:
> I think this can also move into the compiler.

Done.

https://codereview.chromium.org/2500443004/diff/40001/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/2500443004/diff/40001/src/wasm/wasm-module.h#...
src/wasm/wasm-module.h:35: struct ProtectedInstructionData {
On 2016/11/16 18:06:47, titzer wrote:
> I think it'd be best to move this into the compiler directory, e.g. in its own
> header/cc file, so that the compiler doesn't depend on WASM at all.

Done.

https://codereview.chromium.org/2500443004/diff/40001/src/wasm/wasm-module.h#...
src/wasm/wasm-module.h:300: std::vector<ProtectedInstructionList>
protected_instructions;  // Instructions
On 2016/11/16 18:06:47, titzer wrote:
> I think this should somehow be associated with the module rather than the
> instance, since aren't these all relative to some base (i.e. they don't change
> for different instances of the same module)?

Done.

[Eric Holk, 2016-11-18 02:19:56]

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

[commit-bot: I haz the power, 2016-11-18 02:19:57]

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-18 02:49:09]

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

[commit-bot: I haz the power, 2016-11-18 02:49:10]

Dry run: This issue passed the CQ dry run.

[Eric Holk, 2016-11-30 17:12:05]

ping

[titzer, 2016-11-30 17:31:13]

Sorry for the delay. Mostly looking good.

https://codereview.chromium.org/2500443004/diff/80001/src/compiler/code-gener...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/2500443004/diff/80001/src/compiler/code-gener...
src/compiler/code-generator.cc:80: trap_handler::ProtectedInstructionData data;
This can be a little shorter if you either introduce a constructor or just use
the inline {} struct value. Also I think it's more efficient to use
emplace_back().

https://codereview.chromium.org/2500443004/diff/80001/src/compiler/pipeline.h
File src/compiler/pipeline.h (right):

https://codereview.chromium.org/2500443004/diff/80001/src/compiler/pipeline.h...
src/compiler/pipeline.h:22: typedef std::vector<ProtectedInstructionData>
ProtectedInstructionList;
Do you want to use a ZoneVector?

https://codereview.chromium.org/2500443004/diff/80001/src/compiler/x64/code-g...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/2500443004/diff/80001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:282: void Generate() final {
We should probably refactor this method to take the code generator as a
parameter. Can you leave a TODO here?

https://codereview.chromium.org/2500443004/diff/80001/src/trap-handler/trap-h...
File src/trap-handler/trap-handler.h (right):

https://codereview.chromium.org/2500443004/diff/80001/src/trap-handler/trap-h...
src/trap-handler/trap-handler.h:13: /// The offset of this instruction from the
start of its code object.
Two slashes?

https://codereview.chromium.org/2500443004/diff/80001/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/2500443004/diff/80001/src/wasm/wasm-module.h#...
src/wasm/wasm-module.h:33: using trap_handler::ProtectedInstructionData;
Is it necessary to add these here?

https://codereview.chromium.org/2500443004/diff/80001/src/wasm/wasm-module.h#...
src/wasm/wasm-module.h:185: enum TrapFields { kTrapCodeOffset,
kTrapLandingOffset, kTrapDataSize };
Maybe this fits better in wasm-objects.h? As I think this refers to the entries
in the FixedArray, yes?

https://codereview.chromium.org/2500443004/diff/80001/src/wasm/wasm-module.h#...
src/wasm/wasm-module.h:185: enum TrapFields { kTrapCodeOffset,
kTrapLandingOffset, kTrapDataSize };
Should this move to wasm-objects.h? I think it refers to entries within the
FixedArray there.

[Eric Holk, 2016-11-30 20:16:20]

Thanks for the feedback. Please take another look.

https://codereview.chromium.org/2500443004/diff/80001/src/compiler/code-gener...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/2500443004/diff/80001/src/compiler/code-gener...
src/compiler/code-generator.cc:80: trap_handler::ProtectedInstructionData data;
On 2016/11/30 17:31:12, titzer wrote:
> This can be a little shorter if you either introduce a constructor or just use
> the inline {} struct value. Also I think it's more efficient to use
> emplace_back().

Done.

https://codereview.chromium.org/2500443004/diff/80001/src/compiler/pipeline.h
File src/compiler/pipeline.h (right):

https://codereview.chromium.org/2500443004/diff/80001/src/compiler/pipeline.h...
src/compiler/pipeline.h:22: typedef std::vector<ProtectedInstructionData>
ProtectedInstructionList;
On 2016/11/30 17:31:12, titzer wrote:
> Do you want to use a ZoneVector?

Yes.

Done.

https://codereview.chromium.org/2500443004/diff/80001/src/compiler/x64/code-g...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/2500443004/diff/80001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:282: void Generate() final {
On 2016/11/30 17:31:12, titzer wrote:
> We should probably refactor this method to take the code generator as a
> parameter. Can you leave a TODO here?

Done.

https://codereview.chromium.org/2500443004/diff/80001/src/trap-handler/trap-h...
File src/trap-handler/trap-handler.h (right):

https://codereview.chromium.org/2500443004/diff/80001/src/trap-handler/trap-h...
src/trap-handler/trap-handler.h:13: /// The offset of this instruction from the
start of its code object.
On 2016/11/30 17:31:12, titzer wrote:
> Two slashes?

Done.

https://codereview.chromium.org/2500443004/diff/80001/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/2500443004/diff/80001/src/wasm/wasm-module.h#...
src/wasm/wasm-module.h:33: using trap_handler::ProtectedInstructionData;
On 2016/11/30 17:31:12, titzer wrote:
> Is it necessary to add these here?

No. Done.

https://codereview.chromium.org/2500443004/diff/80001/src/wasm/wasm-module.h#...
src/wasm/wasm-module.h:185: enum TrapFields { kTrapCodeOffset,
kTrapLandingOffset, kTrapDataSize };
On 2016/11/30 17:31:12, titzer wrote:
> Should this move to wasm-objects.h? I think it refers to entries within the
> FixedArray there.

I moved it to objects.h, because the protected instruction array actually moved
into the Code object as well.

Done.

[titzer, 2016-11-30 21:19:07]

lgtm

https://codereview.chromium.org/2500443004/diff/120001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2500443004/diff/120001/src/objects.h#newcode5426
src/objects.h:5426: DECL_ACCESSORS(protected_instructions, FixedArray)
Can you double check that the addition of this field didn't push the Code header
over 64 bytes? I think we have space, but if it overflows it will be a
significant regression.

[Eric Holk, 2016-11-30 21:55:55]

Thanks!

https://codereview.chromium.org/2500443004/diff/120001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2500443004/diff/120001/src/objects.h#newcode5426
src/objects.h:5426: DECL_ACCESSORS(protected_instructions, FixedArray)
On 2016/11/30 21:19:07, titzer wrote:
> Can you double check that the addition of this field didn't push the Code
header
> over 64 bytes? I think we have space, but if it overflows it will be a
> significant regression.

I played around with some static asserts, and it looks like without this field
we were already above 64 bytes, but with this field we are still less than 128,
so it seems like this change makes no difference in terms of multiples of 64
bytes.

[Eric Holk, 2016-11-30 21:56:01]

The CQ bit was checked by eholk@chromium.org

[Eric Holk, 2016-11-30 21:56:02]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2500443004/#ps160001
(title: "Removing spurious changes")

[commit-bot: I haz the power, 2016-11-30 21:56:04]

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-30 22:31:38]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1480542961804550,
"parent_rev": "3115f1cdae5407e09b3336567ffd583d07657108", "commit_rev":
"a2ccaffee4b19f3271b63dc978217a424f83bf4e"}

[commit-bot: I haz the power, 2016-11-30 22:31:47]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-11-30 22:32:12]

Description was changed from

==========
[wasm] OOB traps: build protected instruction list during codegen

During codegen, we build a list mapping protected instructions to their
associated landing pads. This will ultimately by used by the signal handler to
recover from out of bounds faults and throw a JS exception.

This is mostly pulled from my larger in-progress CL at
https://codereview.chromium.org/2371833007/.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=5277
==========

to

==========
[wasm] OOB traps: build protected instruction list during codegen

During codegen, we build a list mapping protected instructions to their
associated landing pads. This will ultimately by used by the signal handler to
recover from out of bounds faults and throw a JS exception.

This is mostly pulled from my larger in-progress CL at
https://codereview.chromium.org/2371833007/.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=5277

Committed: https://crrev.com/bf35d15e5265f2d6ac74a594876d73ced535d8d1
Cr-Commit-Position: refs/heads/master@{#41400}
==========

[commit-bot: I haz the power, 2016-11-30 22:32:13]

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