[wasm] Initial signal handler

[wasm] Initial signal handler

This is basically the minimum viable signal handler for Wasm bounds checks.
It includes the TLS check and the fine grained instructions checks. These
two checks provide most of the safety for the signal handler. Future CLs will
add code range and data range checks for more robustness.

The trap handling code and data structures are all in src/trap-handler, with
the code that actually runs in the signal handler confined to
src/trap-handler/signal-handler.cc.

This changes adds a new V8 API that the embedder should call from a signal 
handler that will give V8 the chance to handle the fault first. For hosts that
do not want to implement their own signal handler, we include the option to
install a simple one. This simple handler is also used for the tests.

When a Wasm module is instantiated, information about each function is passed
to the trap handler, which is used to classify faults. These are removed during
the instance finalizer.

Several future enhancements are planned before turning this on by default.
Obviously, the additional checks will be added to MaybeHandleFault. We are
also planning to add a two-level CodeObjectData table that is grouped by
isolates to make cleanup easier and also reduce potential for contending on
a single data structure.

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

Review-Url: https://codereview.chromium.org/2371833007
Cr-Original-Original-Commit-Position: refs/heads/master@{#43523}
Committed: https://chromium.googlesource.com/v8/v8/+/a5af7fe9ee388a636675f4a6872b1d34fa7d1a7a
Review-Url: https://codereview.chromium.org/2371833007
Cr-Original-Commit-Position: refs/heads/master@{#43755}
Committed: https://chromium.googlesource.com/v8/v8/+/338622d7cae787a63cece1f2e79a8b030023940b
Review-Url: https://codereview.chromium.org/2371833007
Cr-Commit-Position: refs/heads/master@{#43759}
Committed: https://chromium.googlesource.com/v8/v8/+/118c376fcbe483b155eec26f8d9192f8cb4b4721

[Eric Holk, 2016-12-28 22:19:29]

Description was changed from

==========
WIP: pull most signal handler stuff into its own file.
==========

to

==========
[wasm] Initial signal handler

This is basically the minimum viable signal handler for Wasm bounds checks.
It includes the TLS check and the fine grained instructions checks. These
two checks provide most of the safety for the signal handler. Future CLs will
add code range and data range checks for more robustness.

The trap handling code and data structures are all in src/trap-handler, with
the code that actually runs in the signal handler confined to
src/trap-handler/signal-handler.cc.

This changes adds a new V8 API that the embedder should call from a signal 
handler that will give V8 the chance to handle the fault first. For hosts that
do not want to implement their own signal handler, we include the option to
install a simple one. This simple handler is also used for the tests.

When a Wasm module is instantiated, information about each function is passed
to the trap handler, which is used to classify faults. These are removed during
the instance finalizer.

Several future enhancements are planned before turning this on by default.
Obviously, the additional checks will be added to MaybeHandleFault. We are
also planning to add a two-level CodeObjectData table that is grouped by
isolates to make cleanup easier and also reduce potential for contending on
a single data structure.

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

[Eric Holk, 2016-12-28 22:23:20]

eholk@chromium.org changed reviewers:
+ bradnelson@google.com, hpayer@chromium.org, jochen@chromium.org,
mark@chromium.org, mseaborn@chromium.org, titzer@chromium.org

[Eric Holk, 2016-12-28 22:23:21]

This change adds an initial signal handler and the plumbing needed to make it
up. A few checks are still not implemented, but those will follow shortly in
followup CLs. It touches quite a few parts of the V8 code, so I've added a
number of reviewers. Here are some areas to focus on:

Ben and Brad: The changes in src/compiler and src/wasm

Mark and Mark: The changes in src/trap-handler

Jochen: The new APIs that I added in v8.h

Hannes: The code relocation changes in objects.cc

Thanks!
-Eric

[jochen (gone - plz use gerrit), 2017-01-02 07:35:09]

some general comments.

I guess we should wait for Mark's feedback before going into too much depth here

https://codereview.chromium.org/2371833007/diff/500001/src/trap-handler/OWNERS
File src/trap-handler/OWNERS (right):

https://codereview.chromium.org/2371833007/diff/500001/src/trap-handler/OWNER...
src/trap-handler/OWNERS:5: mark@chromium.org
only v8 committers can be owners

https://codereview.chromium.org/2371833007/diff/500001/src/trap-handler/signa...
File src/trap-handler/signal-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/500001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:42: extern __thread bool gThreadInWasmCode;
please use g_thread_in_wasm_code

https://codereview.chromium.org/2371833007/diff/500001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:79: };
disallow copy& assign

https://codereview.chromium.org/2371833007/diff/500001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:117: MetadataLock _;
please use a proper variable name

https://codereview.chromium.org/2371833007/diff/500001/src/trap-handler/v8-ap...
File src/trap-handler/v8-api.cc (right):

https://codereview.chromium.org/2371833007/diff/500001/src/trap-handler/v8-ap...
src/trap-handler/v8-api.cc:9: #include
"src/trap-handler/trap-handler-internal.h"
this should either go into src/api.cc or src/api/trap-handler.cc

[jochen (gone - plz use gerrit), 2017-01-02 07:36:29]

https://codereview.chromium.org/2371833007/diff/500001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2371833007/diff/500001/include/v8.h#newcode7584
include/v8.h:7584: static bool MaybeHandleFault(int signum, void* info, void*
context);
also, what is signum, info, and context supposed to be? Please add comments
explaining what the embedder is supposed to pass here.

Please make sure that the embedder can pass this information an OS/arch
combinations we support.

[titzer, 2017-01-09 09:26:56]

https://codereview.chromium.org/2371833007/diff/540001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2371833007/diff/540001/include/v8.h#newcode7584
include/v8.h:7584: static bool MaybeHandleFault(int signum, void* info, void*
context);
Trying to think of a better name here...MaybeHandleFault reads a little funny. I
thought of "TryHandleFault", but that might be confusing as well. Other ideas?

https://codereview.chromium.org/2371833007/diff/540001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2371833007/diff/540001/src/isolate.cc#newcode...
src/isolate.cc:1206: if (frame->is_wasm() && trap_handler::EnableTrapHandler())
{
I think this logic is correct, but maybe it would be more straightforward to
just unconditionally set the ThreadInWasm flag to be true if the handler frame
is wasm, and false otherwise?

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/OWNERS
File src/trap-handler/OWNERS (right):

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/OWNER...
src/trap-handler/OWNERS:5: mark@chromium.org
Does it make sense to have a MUC owner for this as well?

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/signa...
File src/trap-handler/signal-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:36: // #define TRACE(...) fprintf(stderr,
__VA_ARGS__)
Can you use the PrintF function here?

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:105: // This isn't Wasm code anymore. At this
point we were either crash or throw
Not sure I understand this comment. Does this protect against nested faults?

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/trap-...
File src/trap-handler/trap-handler-internal.h (right):

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/trap-...
src/trap-handler/trap-handler-internal.h:21: struct CodeObjectData {
Can we make this name a little more descriptive?

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/trap-...
File src/trap-handler/trap-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/trap-...
src/trap-handler/trap-handler.cc:87: MetadataLock _;
Can we name this little temporary locks "lock"?

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

https://codereview.chromium.org/2371833007/diff/540001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1351: trap_handler::CodeObjectData* handler_data =
Is it possible to hide this data structure altogether by making a single API
function that creates and registers the data?

https://codereview.chromium.org/2371833007/diff/540001/test/cctest/wasm/test-...
File test/cctest/wasm/test-run-wasm.cc (right):

https://codereview.chromium.org/2371833007/diff/540001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm.cc:1087: if (trap_handler::EnableTrapHandler()) {
I think this is because we'd need to put the trap handler data registration into
either compiler/wasm-compiler.cc or into compiler/pipeline.cc. In addition to
that, we'd have to make the code generated for the trap handler landing pads
work in cctests as well.

Recommend changing the definition of WASM_EXEC_TEST_WITH_TRAP to do this check.

[Eric Holk, 2017-01-10 23:10:48]

https://codereview.chromium.org/2371833007/diff/500001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2371833007/diff/500001/include/v8.h#newcode7584
include/v8.h:7584: static bool MaybeHandleFault(int signum, void* info, void*
context);
On 2017/01/02 07:36:28, jochen wrote:
> also, what is signum, info, and context supposed to be? Please add comments
> explaining what the embedder is supposed to pass here.
> 
> Please make sure that the embedder can pass this information an OS/arch
> combinations we support.

This version here is overly specific to Posix systems. I will rewrite it to have
the specific bits of information we need, which are available on the platforms
we support. Ideally we would pass all of the platform-specific information as
well. Would passing in a void* that points to platform specific context
information be okay, with documentation saying what should be there on each
system?

https://codereview.chromium.org/2371833007/diff/500001/src/trap-handler/OWNERS
File src/trap-handler/OWNERS (right):

https://codereview.chromium.org/2371833007/diff/500001/src/trap-handler/OWNER...
src/trap-handler/OWNERS:5: mark@chromium.org
On 2017/01/02 07:35:08, jochen wrote:
> only v8 committers can be owners

I changed this to list you and bradnelson as owners, with a comment that changes
to this directory should also be reviewed by these three people.

https://codereview.chromium.org/2371833007/diff/500001/src/trap-handler/signa...
File src/trap-handler/signal-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/500001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:42: extern __thread bool gThreadInWasmCode;
On 2017/01/02 07:35:08, jochen wrote:
> please use g_thread_in_wasm_code

Done.

https://codereview.chromium.org/2371833007/diff/500001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:79: };
On 2017/01/02 07:35:08, jochen wrote:
> disallow copy& assign

Done.

https://codereview.chromium.org/2371833007/diff/500001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:117: MetadataLock _;
On 2017/01/02 07:35:08, jochen wrote:
> please use a proper variable name

Done.

https://codereview.chromium.org/2371833007/diff/500001/src/trap-handler/v8-ap...
File src/trap-handler/v8-api.cc (right):

https://codereview.chromium.org/2371833007/diff/500001/src/trap-handler/v8-ap...
src/trap-handler/v8-api.cc:9: #include
"src/trap-handler/trap-handler-internal.h"
On 2017/01/02 07:35:08, jochen wrote:
> this should either go into src/api.cc or src/api/trap-handler.cc

I'd like to keep the signal handling code all under src/trap-handler to
facilitate easy auditing. I added stubs to src/api.cc that call into
src/trap-handler though. Does that work?

https://codereview.chromium.org/2371833007/diff/540001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2371833007/diff/540001/include/v8.h#newcode7584
include/v8.h:7584: static bool MaybeHandleFault(int signum, void* info, void*
context);
On 2017/01/09 09:26:56, titzer wrote:
> Trying to think of a better name here...MaybeHandleFault reads a little funny.
I
> thought of "TryHandleFault", but that might be confusing as well. Other ideas?

Yeah, Maybe seems a little noncommittal. I feel like what we really want is
HandleFaultIfYouWantTo, but that's far worse. I'll go with Try for now, because
that at least has some precedent.

https://codereview.chromium.org/2371833007/diff/540001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2371833007/diff/540001/src/isolate.cc#newcode...
src/isolate.cc:1206: if (frame->is_wasm() && trap_handler::EnableTrapHandler())
{
On 2017/01/09 09:26:56, titzer wrote:
> I think this logic is correct, but maybe it would be more straightforward to
> just unconditionally set the ThreadInWasm flag to be true if the handler frame
> is wasm, and false otherwise?

Was the concern about the extra call to EnableTrapHandler()? If so, I went ahead
and removed that.

Otherwise, we want to clear the ThreadInWasm flag when we unwind out of a Wasm
frame, which is what's happening here.

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/OWNERS
File src/trap-handler/OWNERS (right):

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/OWNER...
src/trap-handler/OWNERS:5: mark@chromium.org
On 2017/01/09 09:26:56, titzer wrote:
> Does it make sense to have a MUC owner for this as well?

The OWNERS file is turning out to be a little tricky, since Jochen said we can
only have V8 committers here. The goal was to enforce that any changes to this
directory are also reviewed by someone focused on security and crash handling.
It seems like OWNERS isn't actually the best way to do this.

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/signa...
File src/trap-handler/signal-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:36: // #define TRACE(...) fprintf(stderr,
__VA_ARGS__)
On 2017/01/09 09:26:56, titzer wrote:
> Can you use the PrintF function here?

We'd have to include more v8 headers, which increases the surface of things we'd
have to audit for the trap handler.

In reality, we shouldn't even call fprintf from a signal handler, but it seems
to work well enough for debugging. What about just removing the commented out
line, and if I need it then I can add it in while debugging but not commit it?

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:105: // This isn't Wasm code anymore. At this
point we were either crash or throw
On 2017/01/09 09:26:56, titzer wrote:
> Not sure I understand this comment. Does this protect against nested faults? 

Yes. I changed the comment to reflect that.

I guess I was thinking that once we're in the signal handler we're no longer in
Wasm-generated code, and we won't be again because we either throw a JS
exception or crash. Of course, the real reason we clear the flag is to protect
against nested faults, so I removed the other irrelevant details.

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/trap-...
File src/trap-handler/trap-handler-internal.h (right):

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/trap-...
src/trap-handler/trap-handler-internal.h:21: struct CodeObjectData {
On 2017/01/09 09:26:56, titzer wrote:
> Can we make this name a little more descriptive?

How's CodeProtectionInfo?

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/trap-...
File src/trap-handler/trap-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/trap-...
src/trap-handler/trap-handler.cc:87: MetadataLock _;
On 2017/01/09 09:26:56, titzer wrote:
> Can we name this little temporary locks "lock"?

Done.

I thought I had fixed this already but I guess the change didn't stick.

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

https://codereview.chromium.org/2371833007/diff/540001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1351: trap_handler::CodeObjectData* handler_data =
On 2017/01/09 09:26:56, titzer wrote:
> Is it possible to hide this data structure altogether by making a single API
> function that creates and registers the data?

Yes. Originally I was thinking keeping them separate would let us share the same
data between different instances, but I don't think this plays nicely with JS
objects and the GC.

Done.

https://codereview.chromium.org/2371833007/diff/540001/test/cctest/wasm/test-...
File test/cctest/wasm/test-run-wasm.cc (right):

https://codereview.chromium.org/2371833007/diff/540001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm.cc:1087: if (trap_handler::EnableTrapHandler()) {
On 2017/01/09 09:26:56, titzer wrote:
> I think this is because we'd need to put the trap handler data registration
into
> either compiler/wasm-compiler.cc or into compiler/pipeline.cc. In addition to
> that, we'd have to make the code generated for the trap handler landing pads
> work in cctests as well.
> 
> Recommend changing the definition of WASM_EXEC_TEST_WITH_TRAP to do this
check.

Done.

[Mark Mentovai, 2017-01-10 23:11:53]

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/OWNERS
File src/trap-handler/OWNERS (right):

https://codereview.chromium.org/2371833007/diff/540001/src/trap-handler/OWNER...
src/trap-handler/OWNERS:5: mark@chromium.org
Eric Holk wrote:
> On 2017/01/09 09:26:56, titzer wrote:
> > Does it make sense to have a MUC owner for this as well?
> 
> The OWNERS file is turning out to be a little tricky, since Jochen said we can
> only have V8 committers here. The goal was to enforce that any changes to this
> directory are also reviewed by someone focused on security and crash handling.
> It seems like OWNERS isn't actually the best way to do this.

I believe I’m a v8 committer if that helps. Or at least I was once upon a time.

[Eric Holk, 2017-01-12 00:25:46]

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

[commit-bot: I haz the power, 2017-01-12 00:25:56]

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, 2017-01-12 00:27:26]

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

[commit-bot: I haz the power, 2017-01-12 00:27:26]

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

[Eric Holk, 2017-01-12 00:57:49]

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

[commit-bot: I haz the power, 2017-01-12 00:57:56]

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, 2017-01-12 01:06:09]

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

[commit-bot: I haz the power, 2017-01-12 01:06:10]

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

[Eric Holk, 2017-01-12 02:13:29]

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

[commit-bot: I haz the power, 2017-01-12 02:13:31]

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, 2017-01-12 02:22:00]

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

[commit-bot: I haz the power, 2017-01-12 02:22:01]

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

[Eric Holk, 2017-01-12 02:33:00]

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

[commit-bot: I haz the power, 2017-01-12 02:33:09]

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, 2017-01-12 02:59:39]

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

[commit-bot: I haz the power, 2017-01-12 02:59:40]

Dry run: This issue passed the CQ dry run.

[Eric Holk, 2017-01-17 23:25:25]

@Mark Seaborn and @Mark Mentovai - Could you take a look at the signal handler
code under src/trap-handler?

Thanks,
Eric

[Mark Seaborn, 2017-01-24 07:14:40]

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
File src/trap-handler/signal-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:41: extern THREAD_LOCAL bool
g_thread_in_wasm_code;
Can you put these extern declarations in a header, please?  Presumably
trap-handler-internal.h.  Then the compiler will check they match the
definitions.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:44: extern CodeProtectionInfo **gCodeObjects;
Nit: Presumably this should follow the Chromium style and use "* " spacing?

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:69: pthread_sigmask(SIG_UNBLOCK, &sigs,
&old_mask_);
Can you add a TODO to consider using linux-syscall-support for calling this
syscall, please?

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:95: // Unmask the signal in case we fault in
the signal hander.
This could be more explicit.  How about: "This means that if this signal handler
crashes, the crash will be handled by the crash reporter.  Otherwise, the
process might be killed with the crash going unreported."

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:97: sigemptyset(&sigs);
Happily, sigemptyset() and sigaddset() are declared as async-signal-safe by
POSIX.  That might be worth noting in a comment.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:115: const intptr_t base =
reinterpret_cast<intptr_t>(data->base);
Can you use uintptr_t for this, rather than intptr_t?

Firstly, that avoids having to worry about any undefined behaviour from C signed
overflow.  (I generally prefer unsigned types over signed types for that
reason.)

Secondly, "fault_addr >= base" on signed addresses could do the wrong thing if
used on 32-bit systems, in rare cases. :-)

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:143: // If we get here, it's not a wasm
fault, so we go to the next handler.
To be more precise, "If we get here, it's not a recoverable wasm fault".

If we get here, it *should* be a fault in wasm-generated code, unless
g_thread_in_wasm_code was set wrongly.

Shouldn't we restore g_thread_in_wasm_code to true here for completeness?

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:155: if (signum != SIGSEGV && signum !=
SIGBUS) {
RegisterDefaultSignalHandler() only registers for SIGSEGV.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:160: if (info->si_code <= 0 || info->si_code
== SI_USER ||
I think these checks should be done by TryHandleFault(), since that's meant to
be the API that the embedder (Chromium) uses.

TryHandleFault() should take the 'siginfo_t*' and 'void* context', so that the
embedder doesn't have to unpack things from those data structures to pass to
TryHandleFault().

That will be a POSIX-specific interface, but that's OK.  This is all
POSIX-specific.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:170: void *target_data =
reinterpret_cast<void *>(info->si_addr);
I notice this isn't used yet, so you could remove it.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/trap-...
File src/trap-handler/trap-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/trap-...
src/trap-handler/trap-handler.cc:52: return sizeof(CodeProtectionInfo) +
I think this should be offsetof(CodeProtectionInfo, instructions).  Otherwise,
isn't it larger than necessary by one element?

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/trap-...
src/trap-handler/trap-handler.cc:108: if (gCodeObjects == nullptr) {
If this happens, then we've just set gCodeObjects to nullptr but gNumCodeObjects
may still be non-zero. :-)

Does V8 follow Chromium's policy to abort on the failure of small allocations?

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/trap-...
src/trap-handler/trap-handler.cc:119: return static_cast<int>(i);
It's probably safer to make the return type a size_t, in case an attacker can
allocate 2^31 code blocks.

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

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/trap-...
src/trap-handler/trap-handler.h:40: int RegisterHandlerData(void* base, size_t
size,
Can you add a comment saying what this returns?

[Eric Holk, 2017-01-26 01:33:37]

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
File src/trap-handler/signal-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:41: extern THREAD_LOCAL bool
g_thread_in_wasm_code;
On 2017/01/24 07:14:39, Mark Seaborn wrote:
> Can you put these extern declarations in a header, please?  Presumably
> trap-handler-internal.h.  Then the compiler will check they match the
> definitions.

Done.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:44: extern CodeProtectionInfo **gCodeObjects;
On 2017/01/24 07:14:39, Mark Seaborn wrote:
> Nit: Presumably this should follow the Chromium style and use "* " spacing?

Done.

I was fighting with clangfmt because it was trying to infer what I should use
from the rest of the file. V8 clangfmt is set to always use the Chromium style
spacing now.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:69: pthread_sigmask(SIG_UNBLOCK, &sigs,
&old_mask_);
On 2017/01/24 07:14:39, Mark Seaborn wrote:
> Can you add a TODO to consider using linux-syscall-support for calling this
> syscall, please?

Done.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:95: // Unmask the signal in case we fault in
the signal hander.
On 2017/01/24 07:14:39, Mark Seaborn wrote:
> This could be more explicit.  How about: "This means that if this signal
handler
> crashes, the crash will be handled by the crash reporter.  Otherwise, the
> process might be killed with the crash going unreported."

Done.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:97: sigemptyset(&sigs);
On 2017/01/24 07:14:39, Mark Seaborn wrote:
> Happily, sigemptyset() and sigaddset() are declared as async-signal-safe by
> POSIX.  That might be worth noting in a comment.

Done.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:115: const intptr_t base =
reinterpret_cast<intptr_t>(data->base);
On 2017/01/24 07:14:39, Mark Seaborn wrote:
> Can you use uintptr_t for this, rather than intptr_t?
> 
> Firstly, that avoids having to worry about any undefined behaviour from C
signed
> overflow.  (I generally prefer unsigned types over signed types for that
> reason.)
> 
> Secondly, "fault_addr >= base" on signed addresses could do the wrong thing if
> used on 32-bit systems, in rare cases. :-)

You're right, there's no reason for these to be signed.

Done.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:143: // If we get here, it's not a wasm
fault, so we go to the next handler.
On 2017/01/24 07:14:39, Mark Seaborn wrote:
> To be more precise, "If we get here, it's not a recoverable wasm fault".
> 
> If we get here, it *should* be a fault in wasm-generated code, unless
> g_thread_in_wasm_code was set wrongly.
> 
> Shouldn't we restore g_thread_in_wasm_code to true here for completeness?

Done.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:155: if (signum != SIGSEGV && signum !=
SIGBUS) {
On 2017/01/24 07:14:39, Mark Seaborn wrote:
> RegisterDefaultSignalHandler() only registers for SIGSEGV.

I changed it so that RegisterDefaultSignalHandler registers for SIGBUS now too.

Done.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:160: if (info->si_code <= 0 || info->si_code
== SI_USER ||
On 2017/01/24 07:14:39, Mark Seaborn wrote:
> I think these checks should be done by TryHandleFault(), since that's meant to
> be the API that the embedder (Chromium) uses.
> 
> TryHandleFault() should take the 'siginfo_t*' and 'void* context', so that the
> embedder doesn't have to unpack things from those data structures to pass to
> TryHandleFault().
> 
> That will be a POSIX-specific interface, but that's OK.  This is all
> POSIX-specific.

Doing it this way is the result of some changes Jochen requested, so there's
probably a bigger design question here. Because TryHandleFault() is expected to
be called by the host-supplied signal handler, it is part of the V8 public API.
As such, we need it to be platform independent.

I decided to break out the pieces of information we need now or will need in the
near future (i.e. before we turn this feature on by default), since there should
be some way to get this on any platform we plan to support.

I'll follow up with Jochen to see what precedent there is in V8 for having
platform specificity in the API.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:170: void *target_data =
reinterpret_cast<void *>(info->si_addr);
On 2017/01/24 07:14:39, Mark Seaborn wrote:
> I notice this isn't used yet, so you could remove it.

Assuming the signature for TryHandleFault stay as is, it seems like we might as
well keep it here. I've included it as a parameter to TryHandleFault because
it's needed for one of the planned checks and I want to avoid making a breaking
change to the API in the future.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/trap-...
File src/trap-handler/trap-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/trap-...
src/trap-handler/trap-handler.cc:52: return sizeof(CodeProtectionInfo) +
On 2017/01/24 07:14:39, Mark Seaborn wrote:
> I think this should be offsetof(CodeProtectionInfo, instructions).  Otherwise,
> isn't it larger than necessary by one element?

Done.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/trap-...
src/trap-handler/trap-handler.cc:108: if (gCodeObjects == nullptr) {
On 2017/01/24 07:14:40, Mark Seaborn wrote:
> If this happens, then we've just set gCodeObjects to nullptr but
gNumCodeObjects
> may still be non-zero. :-)

Yikes! Good catch.

> Does V8 follow Chromium's policy to abort on the failure of small allocations?

Good question. I think the sensible thing to do here is to abort because
whatever we previously there is now gone, so there is no way to recover from
Wasm faults. They're should be still safe to run, but now V8 will crash on an
out of bounds access in Wasm instead of recovering from it.

Done.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/trap-...
src/trap-handler/trap-handler.cc:119: return static_cast<int>(i);
On 2017/01/24 07:14:39, Mark Seaborn wrote:
> It's probably safer to make the return type a size_t, in case an attacker can
> allocate 2^31 code blocks.

Good point.

At the moment, this immediately gets stored in a smaller integer (Smi), which is
limited to 31 bits.

What if instead we find an open slot and fail (return -1) if its index is more
than 2^31?

For what it's worth, I'm considering changing this protocol because right now
the index is stored on a field in the Code object, which means we increase the
memory usage measurably on even non-wasm Code objects.

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

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/trap-...
src/trap-handler/trap-handler.h:40: int RegisterHandlerData(void* base, size_t
size,
On 2017/01/24 07:14:40, Mark Seaborn wrote:
> Can you add a comment saying what this returns?

Done.

[Mark Seaborn, 2017-01-27 20:17:30]

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
File src/trap-handler/signal-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:155: if (signum != SIGSEGV && signum !=
SIGBUS) {
On 2017/01/26 01:33:36, Eric Holk wrote:
> On 2017/01/24 07:14:39, Mark Seaborn wrote:
> > RegisterDefaultSignalHandler() only registers for SIGSEGV.
> 
> I changed it so that RegisterDefaultSignalHandler registers for SIGBUS now
too.
> 
> Done.

I'd recommend only doing SIGSEGV, because I don't think SIGBUS is needed for
Wasm on x86-64 Linux, is it?  It's simpler to keep the set smaller until we know
we need to change it.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:160: if (info->si_code <= 0 || info->si_code
== SI_USER ||
On 2017/01/26 01:33:36, Eric Holk wrote:
> On 2017/01/24 07:14:39, Mark Seaborn wrote:
> > I think these checks should be done by TryHandleFault(), since that's meant
to
> > be the API that the embedder (Chromium) uses.
> > 
> > TryHandleFault() should take the 'siginfo_t*' and 'void* context', so that
the
> > embedder doesn't have to unpack things from those data structures to pass to
> > TryHandleFault().
> > 
> > That will be a POSIX-specific interface, but that's OK.  This is all
> > POSIX-specific.
> 
> Doing it this way is the result of some changes Jochen requested, so there's
> probably a bigger design question here. Because TryHandleFault() is expected
to
> be called by the host-supplied signal handler, it is part of the V8 public
API.
> As such, we need it to be platform independent.
> 
> I decided to break out the pieces of information we need now or will need in
the
> near future (i.e. before we turn this feature on by default), since there
should
> be some way to get this on any platform we plan to support.
> 
> I'll follow up with Jochen to see what precedent there is in V8 for having
> platform specificity in the API.

In the interests of doing things incrementally, you can leave it as-is for now. 
We can revisit this when it comes to hooking this up to crash reporting in
Chrome.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:170: void *target_data =
reinterpret_cast<void *>(info->si_addr);
On 2017/01/26 01:33:36, Eric Holk wrote:
> On 2017/01/24 07:14:39, Mark Seaborn wrote:
> > I notice this isn't used yet, so you could remove it.
> 
> Assuming the signature for TryHandleFault stay as is, it seems like we might
as
> well keep it here. I've included it as a parameter to TryHandleFault because
> it's needed for one of the planned checks and I want to avoid making a
breaking
> change to the API in the future.

OK, though I'd prefer to remove this variable if it's not used, if you want to
leave it in, can you add a TODO to make use of it?

https://codereview.chromium.org/2371833007/diff/720001/src/trap-handler/signa...
File src/trap-handler/signal-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/720001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:68: // TODO(eholk): consider using
linux-syscall-support instead of
Nit: This comment should probably go on the first call to pthread_sigmask().

https://codereview.chromium.org/2371833007/diff/720001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:199: // Undo registering for SIGSEGV so that
this function has no effect if
The caller doesn't actually check for an error.  Can you either make this abort,
or have the caller check?

[Mark Mentovai, 2017-01-27 20:34:09]

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
File src/trap-handler/signal-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:155: if (signum != SIGSEGV && signum !=
SIGBUS) {
Mark Seaborn wrote:
> I'd recommend only doing SIGSEGV, because I don't think SIGBUS is needed for
> Wasm on x86-64 Linux, is it?  It's simpler to keep the set smaller until we
know
> we need to change it.

I have seen signal mappings move with kernel versions, and although I can offer
no directly applicable evidence, I feel like the difference between SIGSEGV and
SIGBUS can be tickled by things like which instruction tried to access memory,
whether the access was aligned, whether alignment checks were enabled, etc. It’s
certainly more portable to lump SIGSEGV and SIGBUS into one category, although I
concede that we’re only concerned with Linux x86_64 here at the moment. But
still, I’m of the mind that this should consider both, although I wouldn’t force
the issue if otherMark disagrees.

[Mark Seaborn, 2017-01-27 20:50:51]

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
File src/trap-handler/signal-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:155: if (signum != SIGSEGV && signum !=
SIGBUS) {
On 2017/01/27 20:34:09, Mark Mentovai wrote:
> Mark Seaborn wrote:
> > I'd recommend only doing SIGSEGV, because I don't think SIGBUS is needed for
> > Wasm on x86-64 Linux, is it?  It's simpler to keep the set smaller until we
> > know we need to change it.
> 
> I have seen signal mappings move with kernel versions, and although I can
offer
> no directly applicable evidence, I feel like the difference between SIGSEGV
and
> SIGBUS can be tickled by things like which instruction tried to access memory,
> whether the access was aligned, whether alignment checks were enabled, etc.

Those are the kind of things that it would be good to be strict about.  e.g. If
SIGBUS were generated for an unaligned access, then only handle it once we have
test coverage for that case.

However, I can live with handling both if this has changed between Linux kernel
versions.

I just didn't want the reason for having SIGBUS here be "Mac OS X generates
SIGBUS for the Wasm case", because we're pretty sure we don't want to use POSIX
signal handling for this on Mac.

[Mark Mentovai, 2017-01-27 20:55:24]

Mark Seaborn wrote:
>
https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
> File src/trap-handler/signal-handler.cc (right):
> 
>
https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
> src/trap-handler/signal-handler.cc:155: if (signum != SIGSEGV && signum !=
> SIGBUS) {
> On 2017/01/27 20:34:09, Mark Mentovai wrote:
> > Mark Seaborn wrote:
> > > I'd recommend only doing SIGSEGV, because I don't think SIGBUS is needed
for
> > > Wasm on x86-64 Linux, is it?  It's simpler to keep the set smaller until
we
> > > know we need to change it.
> > 
> > I have seen signal mappings move with kernel versions, and although I can
> offer
> > no directly applicable evidence, I feel like the difference between SIGSEGV
> and
> > SIGBUS can be tickled by things like which instruction tried to access
memory,
> > whether the access was aligned, whether alignment checks were enabled, etc.
> 
> Those are the kind of things that it would be good to be strict about.  e.g.
If
> SIGBUS were generated for an unaligned access, then only handle it once we
have
> test coverage for that case.

I suppose that’s reasonable.

> However, I can live with handling both if this has changed between Linux
kernel
> versions.
> 
> I just didn't want the reason for having SIGBUS here be "Mac OS X generates
> SIGBUS for the Wasm case", because we're pretty sure we don't want to use
POSIX
> signal handling for this on Mac.

That’s certainly true, and the code as-is wouldn’t work on Mac anyway even if we
did want to use a POSIX signal handler here.

[Eric Holk, 2017-02-02 18:43:30]

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
File src/trap-handler/signal-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:155: if (signum != SIGSEGV && signum !=
SIGBUS) {
On 2017/01/27 20:50:51, Mark Seaborn wrote:
> On 2017/01/27 20:34:09, Mark Mentovai wrote:
> > Mark Seaborn wrote:
> > > I'd recommend only doing SIGSEGV, because I don't think SIGBUS is needed
for
> > > Wasm on x86-64 Linux, is it?  It's simpler to keep the set smaller until
we
> > > know we need to change it.
> > 
> > I have seen signal mappings move with kernel versions, and although I can
> offer
> > no directly applicable evidence, I feel like the difference between SIGSEGV
> and
> > SIGBUS can be tickled by things like which instruction tried to access
memory,
> > whether the access was aligned, whether alignment checks were enabled, etc.
> 
> Those are the kind of things that it would be good to be strict about.  e.g.
If
> SIGBUS were generated for an unaligned access, then only handle it once we
have
> test coverage for that case.
> 
> However, I can live with handling both if this has changed between Linux
kernel
> versions.
> 
> I just didn't want the reason for having SIGBUS here be "Mac OS X generates
> SIGBUS for the Wasm case", because we're pretty sure we don't want to use
POSIX
> signal handling for this on Mac.

I will go with handling just SIGSEGV until we have a specific and concrete
reason to do otherwise. It's easy to fix if we find a case where different
kernel versions are behaving differently.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:160: if (info->si_code <= 0 || info->si_code
== SI_USER ||
On 2017/01/27 20:17:30, Mark Seaborn wrote:
> On 2017/01/26 01:33:36, Eric Holk wrote:
> > On 2017/01/24 07:14:39, Mark Seaborn wrote:
> > > I think these checks should be done by TryHandleFault(), since that's
meant
> to
> > > be the API that the embedder (Chromium) uses.
> > > 
> > > TryHandleFault() should take the 'siginfo_t*' and 'void* context', so that
> the
> > > embedder doesn't have to unpack things from those data structures to pass
to
> > > TryHandleFault().
> > > 
> > > That will be a POSIX-specific interface, but that's OK.  This is all
> > > POSIX-specific.
> > 
> > Doing it this way is the result of some changes Jochen requested, so there's
> > probably a bigger design question here. Because TryHandleFault() is expected
> to
> > be called by the host-supplied signal handler, it is part of the V8 public
> API.
> > As such, we need it to be platform independent.
> > 
> > I decided to break out the pieces of information we need now or will need in
> the
> > near future (i.e. before we turn this feature on by default), since there
> should
> > be some way to get this on any platform we plan to support.
> > 
> > I'll follow up with Jochen to see what precedent there is in V8 for having
> > platform specificity in the API.
> 
> In the interests of doing things incrementally, you can leave it as-is for
now. 
> We can revisit this when it comes to hooking this up to crash reporting in
> Chrome.

Jochen suggested that he was okay with having #ifdef'd APIs to do this on a
per-platform basis, so I renamed the public API function TryHandleSignal and
changed it back so that it takes the Linux signal handler parameters.

https://codereview.chromium.org/2371833007/diff/680001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:170: void *target_data =
reinterpret_cast<void *>(info->si_addr);
On 2017/01/27 20:17:30, Mark Seaborn wrote:
> On 2017/01/26 01:33:36, Eric Holk wrote:
> > On 2017/01/24 07:14:39, Mark Seaborn wrote:
> > > I notice this isn't used yet, so you could remove it.
> > 
> > Assuming the signature for TryHandleFault stay as is, it seems like we might
> as
> > well keep it here. I've included it as a parameter to TryHandleFault because
> > it's needed for one of the planned checks and I want to avoid making a
> breaking
> > change to the API in the future.
> 
> OK, though I'd prefer to remove this variable if it's not used, if you want to
> leave it in, can you add a TODO to make use of it?

Since this has moved back into TryHandleSignal, I will remove it.

https://codereview.chromium.org/2371833007/diff/720001/src/trap-handler/signa...
File src/trap-handler/signal-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/720001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:68: // TODO(eholk): consider using
linux-syscall-support instead of
On 2017/01/27 20:17:30, Mark Seaborn wrote:
> Nit: This comment should probably go on the first call to pthread_sigmask().

Done.

https://codereview.chromium.org/2371833007/diff/720001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:199: // Undo registering for SIGSEGV so that
this function has no effect if
On 2017/01/27 20:17:30, Mark Seaborn wrote:
> The caller doesn't actually check for an error.  Can you either make this
abort,
> or have the caller check?

I made the caller check.

Done.

[Mark Seaborn, 2017-02-09 16:44:36]

Just a few more comments, then this is good to go...

https://codereview.chromium.org/2371833007/diff/720001/src/trap-handler/signa...
File src/trap-handler/signal-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/720001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:25: #include <stdio.h>
If we're following our rules about minimising dependencies, we should remove
string.h (unless we need memcpy()) and stdio.h.

https://codereview.chromium.org/2371833007/diff/720001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:31: #include "src/flags.h"
We shouldn't have a dependency on that, right?  It's for command line flags.

https://codereview.chromium.org/2371833007/diff/720001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:35: #define TRACE(...)
What is V8's policy on leaving debug logging in code?

https://codereview.chromium.org/2371833007/diff/720001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:76: SigUnmaskStack(const SigUnmaskStack&) =
delete;
You could comment this with: We'd usually use DISALLOW_COPY_AND_ASSIGN, but we
are avoiding a dependency on base/macros.h.

https://codereview.chromium.org/2371833007/diff/720001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:187: bool RegisterDefaultSignalHandler() {
If we're restricting this file to code that runs inside the signal handler, then
this ought to go in the other .cc file.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
File src/trap-handler/signal-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:8: // WebAssembly. Signal handlers are
notoriously difficult to get
Linking to the design doc (the Google Doc) could be useful.  Does V8 have a
place for collecting design docs?

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:41: MetadataLock::MetadataLock() {
MetadataLock is the only code that runs both inside and outside the trap
handler.  If we use the naming "{inside,outside}-handler.cc", maybe this ought
to go in a separate file -- "shared.cc" perhaps?

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
File src/trap-handler/trap-handler-internal.h (right):

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
src/trap-handler/trap-handler-internal.h:28: struct CodeProtectionInfo {
Can you add a comment, e.g. This describes a chunk of code that the trap handler
will handle traps in.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
File src/trap-handler/trap-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
src/trap-handler/trap-handler.cc:4: 
The naming of the files as "trap-handler.cc" and "signal-handler.cc" is a bit
odd, since a signal handler is a type of trap handler.

How about something like "inside-handler.cc" and "outside-handler.cc" for code
that runs inside and outside the trap handler?  That is the distinction we
really have between these two files.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
src/trap-handler/trap-handler.cc:7: // This file implements the out of bounds
signal handler for
Can you drop this sentence?  *This* file doesn't implement the signal handler,
the other file does.  This file contains code that runs outside the trap
handler, and it maintains the data structures that the trap handler reads.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
src/trap-handler/trap-handler.cc:14: //    to be self contained so it is easy to
audit everything that a
This reason doesn't apply for the outside-the-trap-handler code.

External dependencies are more OK for this case.  Probably the only issue is
lock ordering, but the TLS check should mean we avoid most lock ordering
problems.

[Mark Mentovai, 2017-02-09 17:39:42]

I agree, this is generally in good shape.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/OWNERS
File src/trap-handler/OWNERS (right):

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/OWNER...
src/trap-handler/OWNERS:8: #   eholk@chromium.org
Why are these in comments?

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
File src/trap-handler/signal-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:18: //    or security team.
Point to the this directory’s OWNERS file as a reminder, especially if you have
to keep certain things commented out in there. Same in the other .cc.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:84: if (signum != SIGSEGV && signum !=
SIGBUS) {
We decided on just SIGSEGV here and on line 112.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:121: MetadataLock lock_holder;
Add a comment here explaining how you know that this lock won’t already be held
by the same thread (g_thread_in_wasm_code). The assertion in MetadataLock’s
constructor is good, but make it more obvious by putting a comment right here,
front and center.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:159: abort();
I don’t see how this #else abort() is ever reachable, but if it is, the next
comment applies here too.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:171: abort();
Is this really how you want things to go down, as opposed to preserving the
actual signal and context?

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:185: if (sigaction(SIGSEGV, &action,
&old_action) != 0) {
You can give nullptr instead of old_action, since you don’t care.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
File src/trap-handler/trap-handler-internal.h (right):

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
src/trap-handler/trap-handler-internal.h:35: class MetadataLock {
Same question about the header file used for this one.

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

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
src/trap-handler/trap-handler.h:57: bool EnableTrapHandler();
It sounds like this should be called ShouldEnableTrapHandler() or
IsTrapHandlerSupported(). EnableTrapHandler() sounds like it does the enabling,
but that’s not what this does.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
src/trap-handler/trap-handler.h:58: bool RegisterDefaultSignalHandler();
I find it a little weird that this function and the next one are declared here
instead of having a signal-handler.h, when they’re defined in signal-handler.cc.
Was there a particular rationale for that?

[Eric Holk, 2017-02-15 02:02:45]

In addition to addressing the review feedback, I also realized you can use DEPS
to enforce include rules. I've added some rules to help force us to be careful
about pulling external dependencies into src/trap-handler.

https://codereview.chromium.org/2371833007/diff/720001/src/trap-handler/signa...
File src/trap-handler/signal-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/720001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:25: #include <stdio.h>
On 2017/02/09 16:44:36, Mark Seaborn wrote:
> If we're following our rules about minimising dependencies, we should remove
> string.h (unless we need memcpy()) and stdio.h.

Done.

https://codereview.chromium.org/2371833007/diff/720001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:31: #include "src/flags.h"
On 2017/02/09 16:44:35, Mark Seaborn wrote:
> We shouldn't have a dependency on that, right?  It's for command line flags.

Done.

https://codereview.chromium.org/2371833007/diff/720001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:35: #define TRACE(...)
On 2017/02/09 16:44:36, Mark Seaborn wrote:
> What is V8's policy on leaving debug logging in code?

I'm not sure about V8's in specific, but the chromium policy is to not do it, so
I'll remove it.

Done.

https://codereview.chromium.org/2371833007/diff/720001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:76: SigUnmaskStack(const SigUnmaskStack&) =
delete;
On 2017/02/09 16:44:36, Mark Seaborn wrote:
> You could comment this with: We'd usually use DISALLOW_COPY_AND_ASSIGN, but we
> are avoiding a dependency on base/macros.h.

Done.

https://codereview.chromium.org/2371833007/diff/720001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:187: bool RegisterDefaultSignalHandler() {
On 2017/02/09 16:44:36, Mark Seaborn wrote:
> If we're restricting this file to code that runs inside the signal handler,
then
> this ought to go in the other .cc file.

Done.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/OWNERS
File src/trap-handler/OWNERS (right):

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/OWNER...
src/trap-handler/OWNERS:8: #   eholk@chromium.org
On 2017/02/09 17:39:41, Mark Mentovai wrote:
> Why are these in comments?

Apparently only v8-committers can be listed here, and based on moma badges,
these three aren't committers. Instead, I added jochen@ and bradnelson@ and
asked them to make sure we have signoff from the rest of us when reviewing
changes in this directory.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
File src/trap-handler/signal-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:8: // WebAssembly. Signal handlers are
notoriously difficult to get
On 2017/02/09 16:44:36, Mark Seaborn wrote:
> Linking to the design doc (the Google Doc) could be useful.  Does V8 have a
> place for collecting design docs?

I don't know if we have an official place, but I made the doc public from my
Chromium account and added a link.

Done.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:18: //    or security team.
On 2017/02/09 17:39:41, Mark Mentovai wrote:
> Point to the this directory’s OWNERS file as a reminder, especially if you
have
> to keep certain things commented out in there. Same in the other .cc.

Done.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:41: MetadataLock::MetadataLock() {
On 2017/02/09 16:44:36, Mark Seaborn wrote:
> MetadataLock is the only code that runs both inside and outside the trap
> handler.  If we use the naming "{inside,outside}-handler.cc", maybe this ought
> to go in a separate file -- "shared.cc" perhaps?

How does handler-{inside,outside,shared}.cc sound?

I moved the shared variable definitions to handler-shared.cc as well.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:84: if (signum != SIGSEGV && signum !=
SIGBUS) {
On 2017/02/09 17:39:41, Mark Mentovai wrote:
> We decided on just SIGSEGV here and on line 112.

Done.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:121: MetadataLock lock_holder;
On 2017/02/09 17:39:41, Mark Mentovai wrote:
> Add a comment here explaining how you know that this lock won’t already be
held
> by the same thread (g_thread_in_wasm_code). The assertion in MetadataLock’s
> constructor is good, but make it more obvious by putting a comment right here,
> front and center.

Done.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:159: abort();
On 2017/02/09 17:39:41, Mark Mentovai wrote:
> I don’t see how this #else abort() is ever reachable, but if it is, the next
> comment applies here too.

It shouldn't ever be reachable. I'll see if I can tighten up the #ifdefs to
eliminate it and if not I will add a comment and change it to NOTREACHED().

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:171: abort();
On 2017/02/09 17:39:41, Mark Mentovai wrote:
> Is this really how you want things to go down, as opposed to preserving the
> actual signal and context?

We want to preserve the signal and context.

Done.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:185: if (sigaction(SIGSEGV, &action,
&old_action) != 0) {
On 2017/02/09 17:39:41, Mark Mentovai wrote:
> You can give nullptr instead of old_action, since you don’t care.

Done.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
File src/trap-handler/trap-handler-internal.h (right):

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
src/trap-handler/trap-handler-internal.h:28: struct CodeProtectionInfo {
On 2017/02/09 16:44:36, Mark Seaborn wrote:
> Can you add a comment, e.g. This describes a chunk of code that the trap
handler
> will handle traps in.

Done.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
src/trap-handler/trap-handler-internal.h:35: class MetadataLock {
On 2017/02/09 17:39:41, Mark Mentovai wrote:
> Same question about the header file used for this one.

Hopefully my other comment answers this one too.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
File src/trap-handler/trap-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
src/trap-handler/trap-handler.cc:4: 
On 2017/02/09 16:44:36, Mark Seaborn wrote:
> The naming of the files as "trap-handler.cc" and "signal-handler.cc" is a bit
> odd, since a signal handler is a type of trap handler.
> 
> How about something like "inside-handler.cc" and "outside-handler.cc" for code
> that runs inside and outside the trap handler?  That is the distinction we
> really have between these two files.

Done.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
src/trap-handler/trap-handler.cc:7: // This file implements the out of bounds
signal handler for
On 2017/02/09 16:44:36, Mark Seaborn wrote:
> Can you drop this sentence?  *This* file doesn't implement the signal handler,
> the other file does.  This file contains code that runs outside the trap
> handler, and it maintains the data structures that the trap handler reads.

Done.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
src/trap-handler/trap-handler.cc:14: //    to be self contained so it is easy to
audit everything that a
On 2017/02/09 16:44:36, Mark Seaborn wrote:
> This reason doesn't apply for the outside-the-trap-handler code.
> 
> External dependencies are more OK for this case.  Probably the only issue is
> lock ordering, but the TLS check should mean we avoid most lock ordering
> problems.

I softened the wording a bit to say "avoid adding new external dependencies." I
think we still want to be judicious here, since this file maintains data
structures that affect the signal handler and deserves close scrutiny.

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

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
src/trap-handler/trap-handler.h:57: bool EnableTrapHandler();
On 2017/02/09 17:39:41, Mark Mentovai wrote:
> It sounds like this should be called ShouldEnableTrapHandler() or
> IsTrapHandlerSupported(). EnableTrapHandler() sounds like it does the
enabling,
> but that’s not what this does.

Agreed. I'll go with ShouldEnableTrapHandler.

Done.

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/trap-...
src/trap-handler/trap-handler.h:58: bool RegisterDefaultSignalHandler();
On 2017/02/09 17:39:41, Mark Mentovai wrote:
> I find it a little weird that this function and the next one are declared here
> instead of having a signal-handler.h, when they’re defined in
signal-handler.cc.
> Was there a particular rationale for that?

The original idea was that trap-handler.h provided the external interface that
V8 would use to communicate with the trap handling facilities, trap-handler.cc
provided the implementation of these, and signal-handler.cc was for the
Linux-specific signal handling code. As this CL has evolved, this naming doesn't
make as much sense anymore.

Here's the new naming:

* trap-handler.h - Trap handler interface for V8.
* trap-handler-internal.h - Declarations that are shared between the various
files in src/trap-handler/
* handler-outside.cc - Trap handler code that does not run in the signal
handler, such as data structure maintenance. This is code that's called by the
rest of V8.
* handler-inside.cc - Trap handler code that runs from within the signal
handler.
* handler-shared.cc - Code that is used by both handler-inside.cc and
handler-outside.cc. Because it includes code that is called from the signal
handler, it needs to be subject to the same dependency rules as
handler-inside.cc.

In general, I've tried to use the phrase "trap handling" to refer to the overall
OS/hardware-based bounds check feature, and "signal handling" to refer to the
aspects of trap handling that are specific to Linux signals.

[Eric Holk, 2017-02-15 17:26:11]

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

[commit-bot: I haz the power, 2017-02-15 17:26:19]

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, 2017-02-15 17:29:28]

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

[commit-bot: I haz the power, 2017-02-15 17:29:29]

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

[Eric Holk, 2017-02-15 18:33:03]

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

[commit-bot: I haz the power, 2017-02-15 18:33:07]

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, 2017-02-15 18:57:17]

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

[commit-bot: I haz the power, 2017-02-15 18:57:19]

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

[Eric Holk, 2017-02-15 19:07:56]

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

[commit-bot: I haz the power, 2017-02-15 19:08:08]

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, 2017-02-15 19:15:37]

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

[commit-bot: I haz the power, 2017-02-15 19:15:39]

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

[Eric Holk, 2017-02-15 20:12:56]

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

[commit-bot: I haz the power, 2017-02-15 20:13:07]

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, 2017-02-15 21:02:32]

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

[commit-bot: I haz the power, 2017-02-15 21:02:34]

Dry run: This issue passed the CQ dry run.

[Mark Mentovai, 2017-02-15 21:17:02]

https://codereview.chromium.org/2371833007/diff/900001/src/trap-handler/handl...
File src/trap-handler/handler-inside.cc (right):

https://codereview.chromium.org/2371833007/diff/900001/src/trap-handler/handl...
src/trap-handler/handler-inside.cc:134: g_thread_in_wasm_code = true;
This should go to true *after* ~SigUnmaskStack runs.

[Eric Holk, 2017-02-16 22:15:57]

https://codereview.chromium.org/2371833007/diff/900001/src/trap-handler/handl...
File src/trap-handler/handler-inside.cc (right):

https://codereview.chromium.org/2371833007/diff/900001/src/trap-handler/handl...
src/trap-handler/handler-inside.cc:134: g_thread_in_wasm_code = true;
On 2017/02/15 21:17:02, Mark Mentovai wrote:
> This should go to true *after* ~SigUnmaskStack runs.

I surrounded the region between here and SigUnmaskStack with a new scope so that
the destructor will run first.

Done.

[Eric Holk, 2017-02-16 22:16:32]

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

[commit-bot: I haz the power, 2017-02-16 22:16:42]

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, 2017-02-16 22:49:28]

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

[commit-bot: I haz the power, 2017-02-16 22:49:30]

Dry run: This issue passed the CQ dry run.

[Eric Holk, 2017-02-17 17:23:12]

eholk@chromium.org changed reviewers:
+ ahaas@chromium.org

[Mark Seaborn, 2017-02-17 21:41:13]

LGTM for src/trap-handler/

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
File src/trap-handler/signal-handler.cc (right):

https://codereview.chromium.org/2371833007/diff/780001/src/trap-handler/signa...
src/trap-handler/signal-handler.cc:41: MetadataLock::MetadataLock() {
On 2017/02/15 02:02:45, Eric Holk wrote:
> On 2017/02/09 16:44:36, Mark Seaborn wrote:
> > MetadataLock is the only code that runs both inside and outside the trap
> > handler.  If we use the naming "{inside,outside}-handler.cc", maybe this
ought
> > to go in a separate file -- "shared.cc" perhaps?
> 
> How does handler-{inside,outside,shared}.cc sound?
> 
> I moved the shared variable definitions to handler-shared.cc as well.

Sounds good.

https://codereview.chromium.org/2371833007/diff/920001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/isolate.cc#newcode...
src/isolate.cc:1205: trap_handler::ClearThreadInWasm();
The other calls are guarded by "if (trap_handler::ShouldEnableTrapHandler())"

Maybe that guard should go into trap_handler::Set/ClearThreadInWasm()?

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
File src/trap-handler/handler-inside.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-inside.cc:49: // TODO(eholk): consider using
linux-syscall-support instead of
Nit: "instead of pthread_sigmask" -> "for calling this syscall"

With LSS, you'd still be calling pthread_sigmask(), just not via glibc's
wrapper.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-inside.cc:59: // We'd normally use
DISALLOW_COPY_AND_ASSIGN, but we're  avoiding a
Nit: collapse 2 spaces into 1 after "we're"

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-inside.cc:87: // begin signal mask scope. We need to be
sure to restore the signal mask
"begin" -> "Begin"

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-inside.cc:91: // be
Fix comment wrapping.  (Does clang-format try to wrap comments?)

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-inside.cc:113: // to take the lock if
g_thread_in_wasm_code is not set.
Fix comment wrapping

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
File src/trap-handler/handler-outside.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-outside.cc:34: #define TRACE(...)
I think you said you were removing the TRACE() logging.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-outside.cc:121: return static_cast<int>(i);
Please check that this doesn't overflow/wrap, as we talked about earlier.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-outside.cc:136: free(data);
You could do this outside of the lock.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-outside.cc:141: g_thread_in_wasm_code = true;
This could first assert that g_thread_in_wasm_code is false, and the opposite
for ClearThreadInWasm().

That would be instead of the checks in src/runtime/runtime-wasm.cc.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/trap-...
File src/trap-handler/trap-handler-internal.h (right):

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/trap-...
src/trap-handler/trap-handler-internal.h:47: MetadataLock(const MetadataLock&) =
delete;
You could put the same comment about base/macros.h here.

https://codereview.chromium.org/2371833007/diff/920001/test/cctest/wasm/test-...
File test/cctest/wasm/test-wasm-trap-position.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/test/cctest/wasm/test-...
test/cctest/wasm/test-wasm-trap-position.cc:103: // TODO(eholk): find out why
and fix it.
Does this need to get fixed?

https://codereview.chromium.org/2371833007/diff/920001/test/mjsunit/wasm/grow...
File test/mjsunit/wasm/grow-memory.js (right):

https://codereview.chromium.org/2371833007/diff/920001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:45: print('testGrowMemoryReadWrite32');
Is this debugging that should be removed?

[Mark Mentovai, 2017-02-17 22:06:43]

And an LGTM from me too.

[jochen (gone - plz use gerrit), 2017-02-20 09:23:04]

lgtm

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
File src/trap-handler/handler-outside.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-outside.cc:1: // Copyright 2016 the V8 project authors.
All rights reserved.
nit. 2017

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-outside.cc:5: // PLEASE READ BEFORE CHANGING THIS FILE!
maybe move that to a README.md?

[ahaas, 2017-02-20 09:27:24]

https://codereview.chromium.org/2371833007/diff/920001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2371833007/diff/920001/include/v8.h#newcode7654
include/v8.h:7654: * Given the V8 signal handler a chance to handle a fault.
"Give" I guess

https://codereview.chromium.org/2371833007/diff/920001/include/v8.h#newcode7659
include/v8.h:7659: * return true.
return false;

https://codereview.chromium.org/2371833007/diff/920001/include/v8.h#newcode7664
include/v8.h:7664: * \param signum The signal number.
I would still prefer the name signal_number

https://codereview.chromium.org/2371833007/diff/920001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:2812:
BuildCallToRuntime(Runtime::kSetThreadInWasm, jsgraph(),
I wonder if we could set this flag without a runtime call. I think we should
make calls from JavaScript to WebAssembly as fast as possible. I think this is
critical to support the use case of providing WebAssembly libraries for
JavaScript. Could you add a TODO for this?

https://codereview.chromium.org/2371833007/diff/920001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:2829:
BuildCallToRuntime(Runtime::kClearThreadInWasm, jsgraph(),
Is this runtime function called when WebAssembly throws a trap, e.g. division by
zero?

https://codereview.chromium.org/2371833007/diff/920001/src/compiler/x64/code-...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/compiler/x64/code-...
src/compiler/x64/code-generator-x64.cc:291: __ Push(Smi::FromInt(position_));
I think it would be better to pass in the instruction and determine the source
position from the stack walk by calling here
CodeGenerator::AssembleSourcePosition(instr_). Passing the source position as a
parameter requires patching the stack trace later, which we should better avoid.
Take a look at AssemblyArchTrap, it uses AssembleSourcePosition already.

https://codereview.chromium.org/2371833007/diff/920001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/isolate.cc#newcode...
src/isolate.cc:1205: trap_handler::ClearThreadInWasm();
Is it intentional that ClearThreadInWasm is called multiple times here?

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/OWNERS
File src/trap-handler/OWNERS (right):

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/OWNER...
src/trap-handler/OWNERS:8: #   eholk@chromium.org
Why are you not an owner of your own code?

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
File src/trap-handler/handler-inside.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-inside.cc:1: // Copyright 2016 the V8 project authors.
All rights reserved.
2017

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-inside.cc:80: if (!g_thread_in_wasm_code) {
Use IsThreadInWasm() instead?

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
File src/trap-handler/handler-outside.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-outside.cc:161: if (sigaction(SIGSEGV, &action,
nullptr) != 0) {
Could you add a comment for this code?

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
File src/trap-handler/handler-shared.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-shared.cc:34: if (g_thread_in_wasm_code) {
CHECK(!g_thread_in_wasm_code)?

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-shared.cc:43: if (g_thread_in_wasm_code) {
same here

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/trap-...
File src/trap-handler/trap-handler-internal.h (right):

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/trap-...
src/trap-handler/trap-handler-internal.h:29: // handler faults in. base points
to the beginning of the chunk, and size is the
handle

{base} points to ...

{size} is the ...

https://codereview.chromium.org/2371833007/diff/920001/test/cctest/wasm/test-...
File test/cctest/wasm/test-wasm-trap-position.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/test/cctest/wasm/test-...
test/cctest/wasm/test-wasm-trap-position.cc:102: // cctests don't register the
handler data, so this test won't work
Runtime calls don't work in cctests, so I guess you never set the flag that wasm
code is executed.

https://codereview.chromium.org/2371833007/diff/920001/test/cctest/wasm/wasm-...
File test/cctest/wasm/wasm-run-utils.h (right):

https://codereview.chromium.org/2371833007/diff/920001/test/cctest/wasm/wasm-...
test/cctest/wasm/wasm-run-utils.h:824: TEST(RunWasmCompiledWithTrapIf_##name) { 
             \
Why do you change RunWasmCompiledWithoutTrapIf_ to RunWasmCompiledWithTrapIf_?

[titzer, 2017-02-20 09:50:09]

https://codereview.chromium.org/2371833007/diff/920001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:2812:
BuildCallToRuntime(Runtime::kSetThreadInWasm, jsgraph(),
On 2017/02/20 09:27:23, ahaas wrote:
> I wonder if we could set this flag without a runtime call. I think we should
> make calls from JavaScript to WebAssembly as fast as possible. I think this is
> critical to support the use case of providing WebAssembly libraries for
> JavaScript. Could you add a TODO for this?

+1. I think you can embed a store directly to a location in the isolate (later
TLS) that sets the ThreadInWasm flag.

https://codereview.chromium.org/2371833007/diff/920001/src/compiler/x64/code-...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/compiler/x64/code-...
src/compiler/x64/code-generator-x64.cc:295: ReferenceMap* reference_map =
I don't think we always want to generate a reference map (especially for all
loads/stores), because they consume metadata.

https://codereview.chromium.org/2371833007/diff/920001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/isolate.cc#newcode...
src/isolate.cc:1205: trap_handler::ClearThreadInWasm();
On 2017/02/17 21:41:12, Mark Seaborn wrote:
> The other calls are guarded by "if (trap_handler::ShouldEnableTrapHandler())"
> 
> Maybe that guard should go into trap_handler::Set/ClearThreadInWasm()?

+1 and make the method inline

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
File src/trap-handler/handler-outside.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-outside.cc:96: for (i = 0; i < gNumCodeObjects; ++i) {
Oh, this is kinda bad. It's gonna be linear time to insert a code object into
this list. I think we should introduce a free list to make it constant time.

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

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/trap-...
src/trap-handler/trap-handler.h:26: const bool kTrapHandlerSupported = true;
why not just move this down to:

const bool kTrapHandlerSupported = V8_TRAP_HANDLER_SUPPORTED?

Or better yet, just always use the #define instead of the constant?

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/trap-...
src/trap-handler/trap-handler.h:43: void UpdateCodePointer(int index, void*
base);
We could use a more specific name here, because it's not clear what code pointer
this refers to.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/trap-...
src/trap-handler/trap-handler.h:61: V8_EXPORT_PRIVATE bool
ShouldEnableTrapHandler();
Uses of this method read kinda funny. Maybe

UseTrapHandler?
TrapHandlerEnabled?

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

https://codereview.chromium.org/2371833007/diff/920001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:662: if (trap_handler::ShouldEnableTrapHandler()) {
Heads up that this code is going to move (see
https://codereview.chromium.org/2696143006/)

[Eric Holk, 2017-02-23 02:16:58]

PTAL

There were a lot of comments on that last round. I did by best to respond to
them all, but some may have slipped through the cracks.

https://codereview.chromium.org/2371833007/diff/920001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2371833007/diff/920001/include/v8.h#newcode7654
include/v8.h:7654: * Given the V8 signal handler a chance to handle a fault.
On 2017/02/20 09:27:23, ahaas wrote:
> "Give" I guess

Done.

https://codereview.chromium.org/2371833007/diff/920001/include/v8.h#newcode7659
include/v8.h:7659: * return true.
On 2017/02/20 09:27:23, ahaas wrote:
> return false;

Done.

https://codereview.chromium.org/2371833007/diff/920001/include/v8.h#newcode7664
include/v8.h:7664: * \param signum The signal number.
On 2017/02/20 09:27:23, ahaas wrote:
> I would still prefer the name signal_number

Done.

https://codereview.chromium.org/2371833007/diff/920001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:2812:
BuildCallToRuntime(Runtime::kSetThreadInWasm, jsgraph(),
On 2017/02/20 09:27:23, ahaas wrote:
> I wonder if we could set this flag without a runtime call. I think we should
> make calls from JavaScript to WebAssembly as fast as possible. I think this is
> critical to support the use case of providing WebAssembly libraries for
> JavaScript. Could you add a TODO for this?

TODO is added.

https://codereview.chromium.org/2371833007/diff/920001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:2829:
BuildCallToRuntime(Runtime::kClearThreadInWasm, jsgraph(),
On 2017/02/20 09:27:24, ahaas wrote:
> Is this runtime function called when WebAssembly throws a trap, e.g. division
by
> zero?

The changes in isolate.cc will clear the flag when unwinding out of a Wasm
frame, which should handle this case.

https://codereview.chromium.org/2371833007/diff/920001/src/compiler/x64/code-...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/compiler/x64/code-...
src/compiler/x64/code-generator-x64.cc:291: __ Push(Smi::FromInt(position_));
On 2017/02/20 09:27:24, ahaas wrote:
> I think it would be better to pass in the instruction and determine the source
> position from the stack walk by calling here
> CodeGenerator::AssembleSourcePosition(instr_). Passing the source position as
a
> parameter requires patching the stack trace later, which we should better
avoid.
> Take a look at AssemblyArchTrap, it uses AssembleSourcePosition already.

I added a TODO. It looks like we can and should reuse most of the code from
AssembleArchTrap here too.

https://codereview.chromium.org/2371833007/diff/920001/src/compiler/x64/code-...
src/compiler/x64/code-generator-x64.cc:295: ReferenceMap* reference_map =
On 2017/02/20 09:50:08, titzer wrote:
> I don't think we always want to generate a reference map (especially for all
> loads/stores), because they consume metadata.

Is there a way to call RecordSafepoint without a reference map? AssembleArchTrap
does the same thing as here.

https://codereview.chromium.org/2371833007/diff/920001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/isolate.cc#newcode...
src/isolate.cc:1205: trap_handler::ClearThreadInWasm();
On 2017/02/17 21:41:12, Mark Seaborn wrote:
> The other calls are guarded by "if (trap_handler::ShouldEnableTrapHandler())"
> 
> Maybe that guard should go into trap_handler::Set/ClearThreadInWasm()?

Done.

https://codereview.chromium.org/2371833007/diff/920001/src/isolate.cc#newcode...
src/isolate.cc:1205: trap_handler::ClearThreadInWasm();
On 2017/02/20 09:50:08, titzer wrote:
> On 2017/02/17 21:41:12, Mark Seaborn wrote:
> > The other calls are guarded by "if
(trap_handler::ShouldEnableTrapHandler())"
> > 
> > Maybe that guard should go into trap_handler::Set/ClearThreadInWasm()?
> 
> +1 and make the method inline

Done.

https://codereview.chromium.org/2371833007/diff/920001/src/isolate.cc#newcode...
src/isolate.cc:1205: trap_handler::ClearThreadInWasm();
On 2017/02/20 09:27:24, ahaas wrote:
> Is it intentional that ClearThreadInWasm is called multiple times here?

Originally, it didn't really matter either way since ClearThreadInWasm was
idempotent. Just calling ClearThreadInWasm is probably cheaper than determining
whether to call it.

However, I made Set/ClearThreadInWasm DCHECK the correct state first, so now we
can't call them multiple times. I've added IsThreadInWasm() to the if condition
here.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/OWNERS
File src/trap-handler/OWNERS (right):

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/OWNER...
src/trap-handler/OWNERS:8: #   eholk@chromium.org
On 2017/02/20 09:27:24, ahaas wrote:
> Why are you not an owner of your own code?

Because I'm not a v8 committer yet :(

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
File src/trap-handler/handler-inside.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-inside.cc:1: // Copyright 2016 the V8 project authors.
All rights reserved.
On 2017/02/20 09:27:24, ahaas wrote:
> 2017

Done.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-inside.cc:49: // TODO(eholk): consider using
linux-syscall-support instead of
On 2017/02/17 21:41:12, Mark Seaborn wrote:
> Nit: "instead of pthread_sigmask" -> "for calling this syscall"
> 
> With LSS, you'd still be calling pthread_sigmask(), just not via glibc's
> wrapper.

Done.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-inside.cc:59: // We'd normally use
DISALLOW_COPY_AND_ASSIGN, but we're  avoiding a
On 2017/02/17 21:41:12, Mark Seaborn wrote:
> Nit: collapse 2 spaces into 1 after "we're"

Done.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-inside.cc:80: if (!g_thread_in_wasm_code) {
On 2017/02/20 09:27:24, ahaas wrote:
> Use IsThreadInWasm() instead?

Done.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-inside.cc:87: // begin signal mask scope. We need to be
sure to restore the signal mask
On 2017/02/17 21:41:12, Mark Seaborn wrote:
> "begin" -> "Begin"

Done.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-inside.cc:91: // be
On 2017/02/17 21:41:12, Mark Seaborn wrote:
> Fix comment wrapping.  (Does clang-format try to wrap comments?)

Done. (Apparently not... Maybe it's cautious about potential ASCII art)

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-inside.cc:113: // to take the lock if
g_thread_in_wasm_code is not set.
On 2017/02/17 21:41:12, Mark Seaborn wrote:
> Fix comment wrapping

Done.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
File src/trap-handler/handler-outside.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-outside.cc:1: // Copyright 2016 the V8 project authors.
All rights reserved.
On 2017/02/20 09:23:04, jochen wrote:
> nit. 2017

Done.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-outside.cc:5: // PLEASE READ BEFORE CHANGING THIS FILE!
On 2017/02/20 09:23:04, jochen wrote:
> maybe move that to a README.md?

I'd rather leave it here so that people are more likely to see it before
changing this file.

I'd be happy to add a README.md with more information about the layout of this
directory, if you'd like.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-outside.cc:34: #define TRACE(...)
On 2017/02/17 21:41:12, Mark Seaborn wrote:
> I think you said you were removing the TRACE() logging.

I must have missed some. This is removed now too.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-outside.cc:96: for (i = 0; i < gNumCodeObjects; ++i) {
On 2017/02/20 09:50:08, titzer wrote:
> Oh, this is kinda bad. It's gonna be linear time to insert a code object into
> this list. I think we should introduce a free list to make it constant time.

Good call. This basically makes instantiation quadratic in the number of
functions a wasm module has.

I reworked this to maintain a lazily-initialized free list. Fortunately, it only
required a trivial one line change in handler-inside.cc.

Done.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-outside.cc:121: return static_cast<int>(i);
On 2017/02/17 21:41:12, Mark Seaborn wrote:
> Please check that this doesn't overflow/wrap, as we talked about earlier.

Done.

I also changed the new_size calculation above to clamp it at
std::numeric_limits<int>::max() so we don't allocate memory that can never be
used.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-outside.cc:136: free(data);
On 2017/02/17 21:41:12, Mark Seaborn wrote:
> You could do this outside of the lock.

Done.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-outside.cc:141: g_thread_in_wasm_code = true;
On 2017/02/17 21:41:12, Mark Seaborn wrote:
> This could first assert that g_thread_in_wasm_code is false, and the opposite
> for ClearThreadInWasm().
> 
> That would be instead of the checks in src/runtime/runtime-wasm.cc.

Done.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-outside.cc:161: if (sigaction(SIGSEGV, &action,
nullptr) != 0) {
On 2017/02/20 09:27:24, ahaas wrote:
> Could you add a comment for this code?

Done.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
File src/trap-handler/handler-shared.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-shared.cc:34: if (g_thread_in_wasm_code) {
On 2017/02/20 09:27:24, ahaas wrote:
> CHECK(!g_thread_in_wasm_code)?

This code can run within a signal handler, which means there are only a handful
of functions that are safe to call. CHECK potentially pulls in a bunch of unseen
dependencies that are not safe to call from signal handlers, so it's safer not
to use it.

https://www.securecoding.cert.org/confluence/display/c/SIG30-C.+Call+only+asy...

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/handl...
src/trap-handler/handler-shared.cc:43: if (g_thread_in_wasm_code) {
On 2017/02/20 09:27:24, ahaas wrote:
> same here

See above comment.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/trap-...
File src/trap-handler/trap-handler-internal.h (right):

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/trap-...
src/trap-handler/trap-handler-internal.h:29: // handler faults in. base points
to the beginning of the chunk, and size is the
On 2017/02/20 09:27:24, ahaas wrote:
> handle
> 
> {base} points to ...
> 
> {size} is the ...

Done.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/trap-...
src/trap-handler/trap-handler-internal.h:47: MetadataLock(const MetadataLock&) =
delete;
On 2017/02/17 21:41:12, Mark Seaborn wrote:
> You could put the same comment about base/macros.h here.

Done.

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

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/trap-...
src/trap-handler/trap-handler.h:26: const bool kTrapHandlerSupported = true;
On 2017/02/20 09:50:08, titzer wrote:
> why not just move this down to:
> 
> const bool kTrapHandlerSupported = V8_TRAP_HANDLER_SUPPORTED?
> 
> Or better yet, just always use the #define instead of the constant?

Originally I thought there was some good reason for the constant, but I don't
think there is. I've removed it now.

Done.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/trap-...
src/trap-handler/trap-handler.h:43: void UpdateCodePointer(int index, void*
base);
On 2017/02/20 09:50:08, titzer wrote:
> We could use a more specific name here, because it's not clear what code
pointer
> this refers to.

How about UpdateHandlerDataCodePointer?

Done.

https://codereview.chromium.org/2371833007/diff/920001/src/trap-handler/trap-...
src/trap-handler/trap-handler.h:61: V8_EXPORT_PRIVATE bool
ShouldEnableTrapHandler();
On 2017/02/20 09:50:08, titzer wrote:
> Uses of this method read kinda funny. Maybe
> 
> UseTrapHandler?
> TrapHandlerEnabled?

I went with UseTrapHandler.

Done.

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

https://codereview.chromium.org/2371833007/diff/920001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:662: if (trap_handler::ShouldEnableTrapHandler()) {
On 2017/02/20 09:50:08, titzer wrote:
> Heads up that this code is going to move (see
> https://codereview.chromium.org/2696143006/)

Acknowledged. (Interestingly, I didn't get any significant merge conflicts
here...)

https://codereview.chromium.org/2371833007/diff/920001/test/cctest/wasm/test-...
File test/cctest/wasm/test-wasm-trap-position.cc (right):

https://codereview.chromium.org/2371833007/diff/920001/test/cctest/wasm/test-...
test/cctest/wasm/test-wasm-trap-position.cc:102: // cctests don't register the
handler data, so this test won't work
On 2017/02/20 09:27:24, ahaas wrote:
> Runtime calls don't work in cctests, so I guess you never set the flag that
wasm
> code is executed.

Acknowledged.

https://codereview.chromium.org/2371833007/diff/920001/test/cctest/wasm/test-...
test/cctest/wasm/test-wasm-trap-position.cc:103: // TODO(eholk): find out why
and fix it.
On 2017/02/17 21:41:12, Mark Seaborn wrote:
> Does this need to get fixed?

This test just does the basic out of bounds memory access test, which we have
covered in the mjsunit tests, and those do run. I think we should fix this, but
I'd rather do it in a followup CL.

I did realize the comment was a bit out of date. The immediate problem is that
r.module().AddMemory down below doesn't create the guard pages, so our signal
handler never runs. I've updated the comment here to reflect this.

https://codereview.chromium.org/2371833007/diff/920001/test/cctest/wasm/wasm-...
File test/cctest/wasm/wasm-run-utils.h (right):

https://codereview.chromium.org/2371833007/diff/920001/test/cctest/wasm/wasm-...
test/cctest/wasm/wasm-run-utils.h:824: TEST(RunWasmCompiledWithTrapIf_##name) { 
             \
On 2017/02/20 09:27:24, ahaas wrote:
> Why do you change RunWasmCompiledWithoutTrapIf_ to RunWasmCompiledWithTrapIf_?

Good question... I changed it back.

https://codereview.chromium.org/2371833007/diff/920001/test/mjsunit/wasm/grow...
File test/mjsunit/wasm/grow-memory.js (right):

https://codereview.chromium.org/2371833007/diff/920001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:45: print('testGrowMemoryReadWrite32');
On 2017/02/17 21:41:12, Mark Seaborn wrote:
> Is this debugging that should be removed?

We're inconsistent about this; some tests print the name of each subtest and
others don't. I prefer it when they do, but that is not really relevant to this
CL so I will change it back.

[Eric Holk, 2017-02-23 18:13:05]

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

[commit-bot: I haz the power, 2017-02-23 18:13:11]

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, 2017-02-23 18:14:40]

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

[commit-bot: I haz the power, 2017-02-23 18:14:41]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/17982)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/35391)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/23114)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/23199)

[Eric Holk, 2017-02-23 19:05:35]

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

[commit-bot: I haz the power, 2017-02-23 19:05:47]

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, 2017-02-23 19:10:37]

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

[commit-bot: I haz the power, 2017-02-23 19:10:39]

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

[Eric Holk, 2017-02-23 19:37:25]

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

[commit-bot: I haz the power, 2017-02-23 19:37:31]

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, 2017-02-23 20:05:47]

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

[commit-bot: I haz the power, 2017-02-23 20:05:49]

Dry run: This issue passed the CQ dry run.

[titzer, 2017-03-01 16:48:47]

lgtm with nit

https://codereview.chromium.org/2371833007/diff/1050001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2371833007/diff/1050001/src/isolate.cc#newcod...
src/isolate.cc:1243: if (trap_handler::UseTrapHandler()) {
Any harm in moving the if inside the SetThreadInWasm?

[Eric Holk, 2017-03-01 17:26:13]

https://codereview.chromium.org/2371833007/diff/1050001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2371833007/diff/1050001/src/isolate.cc#newcod...
src/isolate.cc:1243: if (trap_handler::UseTrapHandler()) {
On 2017/03/01 16:48:47, titzer wrote:
> Any harm in moving the if inside the SetThreadInWasm?

Nope, in fact it's already there. I'll remove it here.

Done.

[Eric Holk, 2017-03-01 17:34:08]

The CQ bit was checked by eholk@chromium.org

[Eric Holk, 2017-03-01 17:34:09]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mark@chromium.org, mseaborn@chromium.org, titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2371833007/#ps1090001
(title: "Fixing nit")

[commit-bot: I haz the power, 2017-03-01 17:34:19]

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, 2017-03-01 18:02:14]

CQ is committing da patch.

Bot data: {"patchset_id": 1090001, "attempt_start_ts": 1488389648325440,
"parent_rev": "d6808c0f9cdca496b11a397da91f2f5b360185e6", "commit_rev":
"a5af7fe9ee388a636675f4a6872b1d34fa7d1a7a"}

[commit-bot: I haz the power, 2017-03-01 18:02:20]

Description was changed from

==========
[wasm] Initial signal handler

This is basically the minimum viable signal handler for Wasm bounds checks.
It includes the TLS check and the fine grained instructions checks. These
two checks provide most of the safety for the signal handler. Future CLs will
add code range and data range checks for more robustness.

The trap handling code and data structures are all in src/trap-handler, with
the code that actually runs in the signal handler confined to
src/trap-handler/signal-handler.cc.

This changes adds a new V8 API that the embedder should call from a signal 
handler that will give V8 the chance to handle the fault first. For hosts that
do not want to implement their own signal handler, we include the option to
install a simple one. This simple handler is also used for the tests.

When a Wasm module is instantiated, information about each function is passed
to the trap handler, which is used to classify faults. These are removed during
the instance finalizer.

Several future enhancements are planned before turning this on by default.
Obviously, the additional checks will be added to MaybeHandleFault. We are
also planning to add a two-level CodeObjectData table that is grouped by
isolates to make cleanup easier and also reduce potential for contending on
a single data structure.

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

to

==========
[wasm] Initial signal handler

This is basically the minimum viable signal handler for Wasm bounds checks.
It includes the TLS check and the fine grained instructions checks. These
two checks provide most of the safety for the signal handler. Future CLs will
add code range and data range checks for more robustness.

The trap handling code and data structures are all in src/trap-handler, with
the code that actually runs in the signal handler confined to
src/trap-handler/signal-handler.cc.

This changes adds a new V8 API that the embedder should call from a signal 
handler that will give V8 the chance to handle the fault first. For hosts that
do not want to implement their own signal handler, we include the option to
install a simple one. This simple handler is also used for the tests.

When a Wasm module is instantiated, information about each function is passed
to the trap handler, which is used to classify faults. These are removed during
the instance finalizer.

Several future enhancements are planned before turning this on by default.
Obviously, the additional checks will be added to MaybeHandleFault. We are
also planning to add a two-level CodeObjectData table that is grouped by
isolates to make cleanup easier and also reduce potential for contending on
a single data structure.

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

Review-Url: https://codereview.chromium.org/2371833007
Cr-Commit-Position: refs/heads/master@{#43523}
Committed:
https://chromium.googlesource.com/v8/v8/+/a5af7fe9ee388a636675f4a6872b1d34fa7...
==========

[commit-bot: I haz the power, 2017-03-01 18:02:22]

Committed patchset #56 (id:1090001) as
https://chromium.googlesource.com/v8/v8/+/a5af7fe9ee388a636675f4a6872b1d34fa7...

[Benedikt Meurer, 2017-03-01 19:47:06]

A revert of this CL (patchset #56 id:1090001) has been created in
https://codereview.chromium.org/2723133003/ by bmeurer@chromium.org.

The reason for reverting is: Breaks tree, i.e.
https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20ASAN/builds/18....

[Eric Holk, 2017-03-03 23:15:14]

Description was changed from

==========
[wasm] Initial signal handler

This is basically the minimum viable signal handler for Wasm bounds checks.
It includes the TLS check and the fine grained instructions checks. These
two checks provide most of the safety for the signal handler. Future CLs will
add code range and data range checks for more robustness.

The trap handling code and data structures are all in src/trap-handler, with
the code that actually runs in the signal handler confined to
src/trap-handler/signal-handler.cc.

This changes adds a new V8 API that the embedder should call from a signal 
handler that will give V8 the chance to handle the fault first. For hosts that
do not want to implement their own signal handler, we include the option to
install a simple one. This simple handler is also used for the tests.

When a Wasm module is instantiated, information about each function is passed
to the trap handler, which is used to classify faults. These are removed during
the instance finalizer.

Several future enhancements are planned before turning this on by default.
Obviously, the additional checks will be added to MaybeHandleFault. We are
also planning to add a two-level CodeObjectData table that is grouped by
isolates to make cleanup easier and also reduce potential for contending on
a single data structure.

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

Review-Url: https://codereview.chromium.org/2371833007
Cr-Commit-Position: refs/heads/master@{#43523}
Committed:
https://chromium.googlesource.com/v8/v8/+/a5af7fe9ee388a636675f4a6872b1d34fa7...
==========

to

==========
[wasm] Initial signal handler

This is basically the minimum viable signal handler for Wasm bounds checks.
It includes the TLS check and the fine grained instructions checks. These
two checks provide most of the safety for the signal handler. Future CLs will
add code range and data range checks for more robustness.

The trap handling code and data structures are all in src/trap-handler, with
the code that actually runs in the signal handler confined to
src/trap-handler/signal-handler.cc.

This changes adds a new V8 API that the embedder should call from a signal 
handler that will give V8 the chance to handle the fault first. For hosts that
do not want to implement their own signal handler, we include the option to
install a simple one. This simple handler is also used for the tests.

When a Wasm module is instantiated, information about each function is passed
to the trap handler, which is used to classify faults. These are removed during
the instance finalizer.

Several future enhancements are planned before turning this on by default.
Obviously, the additional checks will be added to MaybeHandleFault. We are
also planning to add a two-level CodeObjectData table that is grouped by
isolates to make cleanup easier and also reduce potential for contending on
a single data structure.

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

Review-Url: https://codereview.chromium.org/2371833007
Cr-Commit-Position: refs/heads/master@{#43523}
Committed:
https://chromium.googlesource.com/v8/v8/+/a5af7fe9ee388a636675f4a6872b1d34fa7...
==========

[Eric Holk, 2017-03-09 17:53:36]

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

[commit-bot: I haz the power, 2017-03-09 17:53: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, 2017-03-09 18:23:07]

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

[commit-bot: I haz the power, 2017-03-09 18:23:09]

Dry run: This issue passed the CQ dry run.

[Eric Holk, 2017-03-13 18:42:18]

The CQ bit was checked by eholk@chromium.org

[Eric Holk, 2017-03-13 18:42:18]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mark@chromium.org, mseaborn@chromium.org, titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2371833007/#ps1170001
(title: "Merge branch 'master' of https://chromium.googlesource.com/v8/v8 into
wasm-trap")

[commit-bot: I haz the power, 2017-03-13 18:42:25]

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, 2017-03-13 19:14:36]

CQ is committing da patch.

Bot data: {"patchset_id": 1170001, "attempt_start_ts": 1489430538073120,
"parent_rev": "ae46d384cee0dcfd3d9361a53985815d48c88467", "commit_rev":
"338622d7cae787a63cece1f2e79a8b030023940b"}

[commit-bot: I haz the power, 2017-03-13 19:14:45]

Description was changed from

==========
[wasm] Initial signal handler

This is basically the minimum viable signal handler for Wasm bounds checks.
It includes the TLS check and the fine grained instructions checks. These
two checks provide most of the safety for the signal handler. Future CLs will
add code range and data range checks for more robustness.

The trap handling code and data structures are all in src/trap-handler, with
the code that actually runs in the signal handler confined to
src/trap-handler/signal-handler.cc.

This changes adds a new V8 API that the embedder should call from a signal 
handler that will give V8 the chance to handle the fault first. For hosts that
do not want to implement their own signal handler, we include the option to
install a simple one. This simple handler is also used for the tests.

When a Wasm module is instantiated, information about each function is passed
to the trap handler, which is used to classify faults. These are removed during
the instance finalizer.

Several future enhancements are planned before turning this on by default.
Obviously, the additional checks will be added to MaybeHandleFault. We are
also planning to add a two-level CodeObjectData table that is grouped by
isolates to make cleanup easier and also reduce potential for contending on
a single data structure.

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

Review-Url: https://codereview.chromium.org/2371833007
Cr-Commit-Position: refs/heads/master@{#43523}
Committed:
https://chromium.googlesource.com/v8/v8/+/a5af7fe9ee388a636675f4a6872b1d34fa7...
==========

to

==========
[wasm] Initial signal handler

This is basically the minimum viable signal handler for Wasm bounds checks.
It includes the TLS check and the fine grained instructions checks. These
two checks provide most of the safety for the signal handler. Future CLs will
add code range and data range checks for more robustness.

The trap handling code and data structures are all in src/trap-handler, with
the code that actually runs in the signal handler confined to
src/trap-handler/signal-handler.cc.

This changes adds a new V8 API that the embedder should call from a signal 
handler that will give V8 the chance to handle the fault first. For hosts that
do not want to implement their own signal handler, we include the option to
install a simple one. This simple handler is also used for the tests.

When a Wasm module is instantiated, information about each function is passed
to the trap handler, which is used to classify faults. These are removed during
the instance finalizer.

Several future enhancements are planned before turning this on by default.
Obviously, the additional checks will be added to MaybeHandleFault. We are
also planning to add a two-level CodeObjectData table that is grouped by
isolates to make cleanup easier and also reduce potential for contending on
a single data structure.

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

Review-Url: https://codereview.chromium.org/2371833007
Cr-Original-Commit-Position: refs/heads/master@{#43523}
Committed:
https://chromium.googlesource.com/v8/v8/+/a5af7fe9ee388a636675f4a6872b1d34fa7...
Review-Url: https://codereview.chromium.org/2371833007
Cr-Commit-Position: refs/heads/master@{#43755}
Committed:
https://chromium.googlesource.com/v8/v8/+/338622d7cae787a63cece1f2e79a8b03002...
==========

[commit-bot: I haz the power, 2017-03-13 19:14:49]

Committed patchset #60 (id:1170001) as
https://chromium.googlesource.com/v8/v8/+/338622d7cae787a63cece1f2e79a8b03002...

[Eric Holk, 2017-03-13 19:57:05]

A revert of this CL (patchset #60 id:1170001) has been created in
https://codereview.chromium.org/2744383002/ by eholk@chromium.org.

The reason for reverting is: ASAN breakage, such as
https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20ASAN/builds/19....

[Michael Achenbach, 2017-03-13 20:33:46]

On 2017/03/13 19:57:05, Eric Holk wrote:
> A revert of this CL (patchset #60 id:1170001) has been created in
> https://codereview.chromium.org/2744383002/ by mailto:eholk@chromium.org.
> 
> The reason for reverting is: ASAN breakage, such as
>
https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20ASAN/builds/19....

PLease let me know if you figure out why the asan CQ bot didn't break the same
way...

[Eric Holk, 2017-03-13 21:31:27]

eholk@chromium.org changed reviewers:
+ gdeepti@chromium.org

[Eric Holk, 2017-03-13 21:31:29]

I've added a fix for the asan failures. Deepti, can you take a look? It's in the
grow_memory code.

[gdeepti, 2017-03-13 21:39:18]

lgtm for grow_memory code.

[gdeepti, 2017-03-13 21:39:27]

lgtm for grow_memory code.

[Eric Holk, 2017-03-13 21:41:09]

Description was changed from

==========
[wasm] Initial signal handler

This is basically the minimum viable signal handler for Wasm bounds checks.
It includes the TLS check and the fine grained instructions checks. These
two checks provide most of the safety for the signal handler. Future CLs will
add code range and data range checks for more robustness.

The trap handling code and data structures are all in src/trap-handler, with
the code that actually runs in the signal handler confined to
src/trap-handler/signal-handler.cc.

This changes adds a new V8 API that the embedder should call from a signal 
handler that will give V8 the chance to handle the fault first. For hosts that
do not want to implement their own signal handler, we include the option to
install a simple one. This simple handler is also used for the tests.

When a Wasm module is instantiated, information about each function is passed
to the trap handler, which is used to classify faults. These are removed during
the instance finalizer.

Several future enhancements are planned before turning this on by default.
Obviously, the additional checks will be added to MaybeHandleFault. We are
also planning to add a two-level CodeObjectData table that is grouped by
isolates to make cleanup easier and also reduce potential for contending on
a single data structure.

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

Review-Url: https://codereview.chromium.org/2371833007
Cr-Original-Commit-Position: refs/heads/master@{#43523}
Committed:
https://chromium.googlesource.com/v8/v8/+/a5af7fe9ee388a636675f4a6872b1d34fa7...
Review-Url: https://codereview.chromium.org/2371833007
Cr-Commit-Position: refs/heads/master@{#43755}
Committed:
https://chromium.googlesource.com/v8/v8/+/338622d7cae787a63cece1f2e79a8b03002...
==========

to

==========
[wasm] Initial signal handler

This is basically the minimum viable signal handler for Wasm bounds checks.
It includes the TLS check and the fine grained instructions checks. These
two checks provide most of the safety for the signal handler. Future CLs will
add code range and data range checks for more robustness.

The trap handling code and data structures are all in src/trap-handler, with
the code that actually runs in the signal handler confined to
src/trap-handler/signal-handler.cc.

This changes adds a new V8 API that the embedder should call from a signal 
handler that will give V8 the chance to handle the fault first. For hosts that
do not want to implement their own signal handler, we include the option to
install a simple one. This simple handler is also used for the tests.

When a Wasm module is instantiated, information about each function is passed
to the trap handler, which is used to classify faults. These are removed during
the instance finalizer.

Several future enhancements are planned before turning this on by default.
Obviously, the additional checks will be added to MaybeHandleFault. We are
also planning to add a two-level CodeObjectData table that is grouped by
isolates to make cleanup easier and also reduce potential for contending on
a single data structure.

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

Review-Url: https://codereview.chromium.org/2371833007
Cr-Original-Commit-Position: refs/heads/master@{#43523}
Committed:
https://chromium.googlesource.com/v8/v8/+/a5af7fe9ee388a636675f4a6872b1d34fa7...
Review-Url: https://codereview.chromium.org/2371833007
Cr-Commit-Position: refs/heads/master@{#43755}
Committed:
https://chromium.googlesource.com/v8/v8/+/338622d7cae787a63cece1f2e79a8b03002...
==========

[Eric Holk, 2017-03-13 21:41:22]

The CQ bit was checked by eholk@chromium.org

[Eric Holk, 2017-03-13 21:41:23]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mark@chromium.org, mseaborn@chromium.org, titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2371833007/#ps1190001
(title: "Make sure guard pages get set up when resizing from 0 to more than 0")

[commit-bot: I haz the power, 2017-03-13 21:41:28]

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, 2017-03-13 22:12:25]

CQ is committing da patch.

Bot data: {"patchset_id": 1190001, "attempt_start_ts": 1489441282532470,
"parent_rev": "65200967b735be0cd91593ffb03fb96ee90df1a0", "commit_rev":
"118c376fcbe483b155eec26f8d9192f8cb4b4721"}

[commit-bot: I haz the power, 2017-03-13 22:12:34]

Description was changed from

==========
[wasm] Initial signal handler

This is basically the minimum viable signal handler for Wasm bounds checks.
It includes the TLS check and the fine grained instructions checks. These
two checks provide most of the safety for the signal handler. Future CLs will
add code range and data range checks for more robustness.

The trap handling code and data structures are all in src/trap-handler, with
the code that actually runs in the signal handler confined to
src/trap-handler/signal-handler.cc.

This changes adds a new V8 API that the embedder should call from a signal 
handler that will give V8 the chance to handle the fault first. For hosts that
do not want to implement their own signal handler, we include the option to
install a simple one. This simple handler is also used for the tests.

When a Wasm module is instantiated, information about each function is passed
to the trap handler, which is used to classify faults. These are removed during
the instance finalizer.

Several future enhancements are planned before turning this on by default.
Obviously, the additional checks will be added to MaybeHandleFault. We are
also planning to add a two-level CodeObjectData table that is grouped by
isolates to make cleanup easier and also reduce potential for contending on
a single data structure.

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

Review-Url: https://codereview.chromium.org/2371833007
Cr-Original-Commit-Position: refs/heads/master@{#43523}
Committed:
https://chromium.googlesource.com/v8/v8/+/a5af7fe9ee388a636675f4a6872b1d34fa7...
Review-Url: https://codereview.chromium.org/2371833007
Cr-Commit-Position: refs/heads/master@{#43755}
Committed:
https://chromium.googlesource.com/v8/v8/+/338622d7cae787a63cece1f2e79a8b03002...
==========

to

==========
[wasm] Initial signal handler

This is basically the minimum viable signal handler for Wasm bounds checks.
It includes the TLS check and the fine grained instructions checks. These
two checks provide most of the safety for the signal handler. Future CLs will
add code range and data range checks for more robustness.

The trap handling code and data structures are all in src/trap-handler, with
the code that actually runs in the signal handler confined to
src/trap-handler/signal-handler.cc.

This changes adds a new V8 API that the embedder should call from a signal 
handler that will give V8 the chance to handle the fault first. For hosts that
do not want to implement their own signal handler, we include the option to
install a simple one. This simple handler is also used for the tests.

When a Wasm module is instantiated, information about each function is passed
to the trap handler, which is used to classify faults. These are removed during
the instance finalizer.

Several future enhancements are planned before turning this on by default.
Obviously, the additional checks will be added to MaybeHandleFault. We are
also planning to add a two-level CodeObjectData table that is grouped by
isolates to make cleanup easier and also reduce potential for contending on
a single data structure.

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

Review-Url: https://codereview.chromium.org/2371833007
Cr-Original-Original-Commit-Position: refs/heads/master@{#43523}
Committed:
https://chromium.googlesource.com/v8/v8/+/a5af7fe9ee388a636675f4a6872b1d34fa7...
Review-Url: https://codereview.chromium.org/2371833007
Cr-Original-Commit-Position: refs/heads/master@{#43755}
Committed:
https://chromium.googlesource.com/v8/v8/+/338622d7cae787a63cece1f2e79a8b03002...
Review-Url: https://codereview.chromium.org/2371833007
Cr-Commit-Position: refs/heads/master@{#43759}
Committed:
https://chromium.googlesource.com/v8/v8/+/118c376fcbe483b155eec26f8d9192f8cb4...
==========

[commit-bot: I haz the power, 2017-03-13 22:12:36]

Committed patchset #61 (id:1190001) as
https://chromium.googlesource.com/v8/v8/+/118c376fcbe483b155eec26f8d9192f8cb4...

[Michael Achenbach, 2017-03-14 08:09:58]

Lack of trybot test coverage was my bad, sorry for the trouble. Fix in flight:
https://chromium-review.googlesource.com/c/453840/