[wasm] Trap handling: ProtectedLoad instruction

[wasm] Trap handling: ProtectedLoad instruction

This CL introduces a ProtectedLoad instruction with is needed for out
of bounds trap handling. ProtectedLoad behaves like a regular load,
but it takes a context and source position parameter as well. These
are used by an out of line code fragment to generate code to throw a
JS exception for an out of bounds memory reference in Wasm.

These changes a cleaned up subset of
https://codereview.chromium.org/2148743004/

The rest of this feature will follow in future CLs. This includes a
table mapping memory instructions to landing pads as well as the
actual signal handler.

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

Committed: https://crrev.com/0027218e1a0021af8729ed0d6c169d45b548f853
Cr-Commit-Position: refs/heads/master@{#39318}

[Eric Holk, 2016-09-01 19:27:24]

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

[Mircea Trofin, 2016-09-02 04:05:45]

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/machine-op...
File src/compiler/machine-operator.h (right):

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/machine-op...
src/compiler/machine-operator.h:44: typedef LoadRepresentation
ProtectedLoadRepresentation;
What's the value of the typedef - just shorter? If so, I'd prefer the long name,
it improves readability.

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (left):

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2754: 
We want to keep this space here. Did "git cl upload" not complain?

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2774: auto* context =
HeapConstant(module_->instance->context);
HeapConstant* instead of auto. AFAIK, auto would have been fine if this were an
instantiation. It's a function call.

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2776: load =
graph()->NewNode(jsgraph()->machine()->ProtectedLoad(memtype),
What's position_node's type? (i.e. please no auto)

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

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:277: (void)pc_;
you could do USE(pc_);

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:283: auto trap_id =
wasm::kTrapMemOutOfBounds;
please no auto. Same below.

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:1896: auto pc = __ GetPC();
no auto.

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:1900: const auto needs_frame =
!frame_access_state()->has_frame();
no auto

https://codereview.chromium.org/2301833004/diff/20001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2301833004/diff/20001/src/flag-definitions.h#...
src/flag-definitions.h:531: "use signal handlers to catch out of bounds memory
access in wasm")
do you want to call this flag wasm_trap_handler_x64, and/or specify in the
comment that it's just on x64?

https://codereview.chromium.org/2301833004/diff/20001/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

https://codereview.chromium.org/2301833004/diff/20001/src/x64/assembler-x64.h...
src/x64/assembler-x64.h:1855: Address GetPC() const { return pc_; }
for simple accessors, the naming pattern is just the field name, so pc() in this
case.

[Eric Holk, 2016-09-02 20:05:32]

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/machine-op...
File src/compiler/machine-operator.h (right):

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/machine-op...
src/compiler/machine-operator.h:44: typedef LoadRepresentation
ProtectedLoadRepresentation;
On 2016/09/02 04:05:45, Mircea Trofin wrote:
> What's the value of the typedef - just shorter? If so, I'd prefer the long
name,
> it improves readability.

This is actually defining ProtectedLoadRepresentation as an alias of
LoadRepresentation. Just using LoadRepresentation is probably fine, but I did it
this way for consistency with how The other loads like CheckedLoad work. See
machine-operator.cc around line 505.

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (left):

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2754: 
On 2016/09/02 04:05:45, Mircea Trofin wrote:
> We want to keep this space here. Did "git cl upload" not complain?

Done.

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2774: auto* context =
HeapConstant(module_->instance->context);
On 2016/09/02 04:05:45, Mircea Trofin wrote:
> HeapConstant* instead of auto. AFAIK, auto would have been fine if this were
an
> instantiation. It's a function call.

Done. It was actually Node* instead of HeapConstant*.

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2776: load =
graph()->NewNode(jsgraph()->machine()->ProtectedLoad(memtype),
On 2016/09/02 04:05:45, Mircea Trofin wrote:
> What's position_node's type? (i.e. please no auto)

Done. This was also Node*.

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

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:277: (void)pc_;
On 2016/09/02 04:05:45, Mircea Trofin wrote:
> you could do USE(pc_);

Done.

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:283: auto trap_id =
wasm::kTrapMemOutOfBounds;
On 2016/09/02 04:05:45, Mircea Trofin wrote:
> please no auto. Same below.

Done.

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:1896: auto pc = __ GetPC();
On 2016/09/02 04:05:45, Mircea Trofin wrote:
> no auto.

Done.

https://codereview.chromium.org/2301833004/diff/20001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:1900: const auto needs_frame =
!frame_access_state()->has_frame();
On 2016/09/02 04:05:45, Mircea Trofin wrote:
> no auto

Done.

https://codereview.chromium.org/2301833004/diff/20001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2301833004/diff/20001/src/flag-definitions.h#...
src/flag-definitions.h:531: "use signal handlers to catch out of bounds memory
access in wasm")
On 2016/09/02 04:05:45, Mircea Trofin wrote:
> do you want to call this flag wasm_trap_handler_x64, and/or specify in the
> comment that it's just on x64?

I added a note in the comment. We could also make it an error to use this on an
unsupported platform.

https://codereview.chromium.org/2301833004/diff/20001/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

https://codereview.chromium.org/2301833004/diff/20001/src/x64/assembler-x64.h...
src/x64/assembler-x64.h:1855: Address GetPC() const { return pc_; }
On 2016/09/02 04:05:45, Mircea Trofin wrote:
> for simple accessors, the naming pattern is just the field name, so pc() in
this
> case.

Done.

[titzer, 2016-09-08 17:06:36]

lgtm other than nits

https://codereview.chromium.org/2301833004/diff/40001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2301833004/diff/40001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2771: if (!FLAG_wasm_trap_handler) {
Flip sense of if?

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

https://codereview.chromium.org/2301833004/diff/40001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:1900: bool needs_frame =
!frame_access_state()->has_frame();
Can we invert the sense of this and just call it frame_elided? That way it just
records the state at this load and the out of line code can handle that (by
emitting the prologue).

https://codereview.chromium.org/2301833004/diff/40001/src/compiler/x64/instru...
File src/compiler/x64/instruction-selector-x64.cc (right):

https://codereview.chromium.org/2301833004/diff/40001/src/compiler/x64/instru...
src/compiler/x64/instruction-selector-x64.cc:203: void
InstructionSelector::VisitProtectedLoad(Node* node) {
Can we factor out a helper routine here?

https://codereview.chromium.org/2301833004/diff/40001/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (left):

https://codereview.chromium.org/2301833004/diff/40001/src/x64/assembler-x64.c...
src/x64/assembler-x64.cc:4611: 
Spurious whitespace change.

[Eric Holk, 2016-09-08 21:11:56]

https://codereview.chromium.org/2301833004/diff/40001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2301833004/diff/40001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2771: if (!FLAG_wasm_trap_handler) {
On 2016/09/08 17:06:35, titzer wrote:
> Flip sense of if?

Done.

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

https://codereview.chromium.org/2301833004/diff/40001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:1900: bool needs_frame =
!frame_access_state()->has_frame();
On 2016/09/08 17:06:35, titzer wrote:
> Can we invert the sense of this and just call it frame_elided? That way it
just
> records the state at this load and the out of line code can handle that (by
> emitting the prologue).

Isn't inverting the sense at odds with calling it frame_elided? Right now, if
needs_frame is true then the out of line code will emit the prologue. I'm fine
with calling it frame_elided, but it seems like we'll have a ! either here or on
line 279 where the value is used.

https://codereview.chromium.org/2301833004/diff/40001/src/compiler/x64/instru...
File src/compiler/x64/instruction-selector-x64.cc (right):

https://codereview.chromium.org/2301833004/diff/40001/src/compiler/x64/instru...
src/compiler/x64/instruction-selector-x64.cc:203: void
InstructionSelector::VisitProtectedLoad(Node* node) {
On 2016/09/08 17:06:36, titzer wrote:
> Can we factor out a helper routine here?

I factored out the big switch statement. I think much more than that will
complicate things more than help.

https://codereview.chromium.org/2301833004/diff/40001/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (left):

https://codereview.chromium.org/2301833004/diff/40001/src/x64/assembler-x64.c...
src/x64/assembler-x64.cc:4611: 
On 2016/09/08 17:06:36, titzer wrote:
> Spurious whitespace change.

Done.

[Eric Holk, 2016-09-08 21:18:34]

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

[commit-bot: I haz the power, 2016-09-08 21:18:41]

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-09-08 21:24:52]

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

[commit-bot: I haz the power, 2016-09-08 21:24:53]

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/8518)

[Eric Holk, 2016-09-08 22:30:42]

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

[commit-bot: I haz the power, 2016-09-08 22:30:48]

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-09-08 22:58:41]

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

[commit-bot: I haz the power, 2016-09-08 22:58:42]

Dry run: This issue passed the CQ dry run.

[Eric Holk, 2016-09-08 23:13:14]

ptal

I got a bunch of trybot failures for the non-x64 architectures. I fixed this by
adding an UNIMPLEMENTED() stub. Everything seems fine now.

[titzer, 2016-09-09 15:36:56]

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

https://codereview.chromium.org/2301833004/diff/40001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:1900: bool needs_frame =
!frame_access_state()->has_frame();
On 2016/09/08 21:11:56, Eric Holk wrote:
> On 2016/09/08 17:06:35, titzer wrote:
> > Can we invert the sense of this and just call it frame_elided? That way it
> just
> > records the state at this load and the out of line code can handle that (by
> > emitting the prologue).
> 
> Isn't inverting the sense at odds with calling it frame_elided? Right now, if
> needs_frame is true then the out of line code will emit the prologue. I'm fine
> with calling it frame_elided, but it seems like we'll have a ! either here or
on
> line 279 where the value is used.

Yes, you are correct, sir!

[Eric Holk, 2016-09-09 17:33:08]

The CQ bit was checked by eholk@chromium.org

[Eric Holk, 2016-09-09 17:33:09]

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/2301833004/#ps80001
(title: "Add VisitProtectedLoad stubs for other architectures")

[commit-bot: I haz the power, 2016-09-09 17:33:18]

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-09-09 17:40:44]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-09 17:41:18]

Description was changed from

==========
[wasm] Trap handling: ProtectedLoad instruction

This CL introduces a ProtectedLoad instruction with is needed for out
of bounds trap handling. ProtectedLoad behaves like a regular load,
but it takes a context and source position parameter as well. These
are used by an out of line code fragment to generate code to throw a
JS exception for an out of bounds memory reference in Wasm.

These changes a cleaned up subset of
https://codereview.chromium.org/2148743004/

The rest of this feature will follow in future CLs. This includes a
table mapping memory instructions to landing pads as well as the
actual signal handler.

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

to

==========
[wasm] Trap handling: ProtectedLoad instruction

This CL introduces a ProtectedLoad instruction with is needed for out
of bounds trap handling. ProtectedLoad behaves like a regular load,
but it takes a context and source position parameter as well. These
are used by an out of line code fragment to generate code to throw a
JS exception for an out of bounds memory reference in Wasm.

These changes a cleaned up subset of
https://codereview.chromium.org/2148743004/

The rest of this feature will follow in future CLs. This includes a
table mapping memory instructions to landing pads as well as the
actual signal handler.

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

Committed: https://crrev.com/0027218e1a0021af8729ed0d6c169d45b548f853
Cr-Commit-Position: refs/heads/master@{#39318}
==========

[commit-bot: I haz the power, 2016-09-09 17:41:19]

Patchset 5 (id:??) landed as
https://crrev.com/0027218e1a0021af8729ed0d6c169d45b548f853
Cr-Commit-Position: refs/heads/master@{#39318}