NonSFI NaCl: Plumb Exception IRT enough for breakpad.

NonSFI NaCl: Plumb Exception IRT enough for breakpad.

Implements irt_exception.

This is enough to get breakpad start reporting minidumps on crash.

Stack overflow case handling with sigaltstack will be handled in a followup change.

BUG=356925
NOTRY=True

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266968

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267120

[Junichi Uekawa, 2014-04-14 22:51:04]

Hi,

I have got an end-to-end demo that breakpad minidump would dump something in a
non-SFI NaCl.

I have a few questions on how things should be structured.

 - there's bunch of code duplications with 
native_client/src/trusted/service_runtime internals, especially about registers
and signal context. 
I was wondering whether to copy and maintain a fork of the code or add a new
dependency to it.

 - The timing of initialization might be tricky.



This is also based on a 10-day old CL, I'm going to try syncing to head and see
if anything breaks.

https://codereview.chromium.org/230413002/diff/60001/components/nacl/loader/n...
File components/nacl/loader/nonsfi/irt_exception_handling.cc (right):

https://codereview.chromium.org/230413002/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/irt_exception_handling.cc:43: 0 /*
context_user_addr, what is this? */);
SFI-NaCl converts ucontext -> NaClSignalContext -> NaClExceptionFrame, I'm
reusing the utilities here.

I was wondering if I should be reimplementing them here or move them around.

https://codereview.chromium.org/230413002/diff/60001/components/nacl/loader/n...
File components/nacl/loader/nonsfi/nonsfi_main.cc (right):

https://codereview.chromium.org/230413002/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_main.cc:184: InitializeSignalHandler();
I'm not too sure when I need to initialize the handler, it seems like
InitializeBPFSandbox happens early (and I'm not quite sure sigaction succeeds
here).

[hamaji, 2014-04-15 15:23:34]

https://codereview.chromium.org/230413002/diff/60001/components/nacl/loader/n...
File components/nacl/loader/nonsfi/nonsfi_main.cc (right):

https://codereview.chromium.org/230413002/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_main.cc:184: InitializeSignalHandler();
On 2014/04/14 22:51:04, uekawa wrote:
> I'm not too sure when I need to initialize the handler, it seems like
> InitializeBPFSandbox happens early (and I'm not quite sure sigaction succeeds
> here).

As chatted, this won't succeed after seccomp-bpf filter is landed. I think you
need call this in BecomeNaClLoader (nacl_helper_linux.cc).

[Junichi Uekawa, 2014-04-16 05:17:10]

On 2014/04/15 15:23:34, hamaji wrote:
>
https://codereview.chromium.org/230413002/diff/60001/components/nacl/loader/n...
> File components/nacl/loader/nonsfi/nonsfi_main.cc (right):
> 
>
https://codereview.chromium.org/230413002/diff/60001/components/nacl/loader/n...
> components/nacl/loader/nonsfi/nonsfi_main.cc:184: InitializeSignalHandler();
> On 2014/04/14 22:51:04, uekawa wrote:
> > I'm not too sure when I need to initialize the handler, it seems like
> > InitializeBPFSandbox happens early (and I'm not quite sure sigaction
succeeds
> > here).
> 
> As chatted, this won't succeed after seccomp-bpf filter is landed. I think you
> need call this in BecomeNaClLoader (nacl_helper_linux.cc).

now I'm trying to rebase to HEAD. 
#  but there's too much conflicts going on I haven't been able to resolve them
all.

[Junichi Uekawa, 2014-04-17 07:54:55]

https://codereview.chromium.org/230413002/diff/60001/components/nacl/loader/n...
File components/nacl/loader/nonsfi/nonsfi_main.cc (right):

https://codereview.chromium.org/230413002/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_main.cc:184: InitializeSignalHandler();
On 2014/04/15 15:23:35, hamaji wrote:
> On 2014/04/14 22:51:04, uekawa wrote:
> > I'm not too sure when I need to initialize the handler, it seems like
> > InitializeBPFSandbox happens early (and I'm not quite sure sigaction
succeeds
> > here).
> 
> As chatted, this won't succeed after seccomp-bpf filter is landed. I think you
> need call this in BecomeNaClLoader (nacl_helper_linux.cc).

I've moved it around.

https://codereview.chromium.org/230413002/diff/140001/chrome/test/data/nacl/n...
File chrome/test/data/nacl/nonsfi/irt_test.html (right):

https://codereview.chromium.org/230413002/diff/140001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nonsfi/irt_test.html:38: // TODO(uekawa): Do I want to
split this test out to a different location?
I see existing tests related to signal handling in 
ppapi/native_client/tests/
native_client/tests/signal_handler/
I am wondering if adapting them to work with NonSFI.

[Junichi Uekawa, 2014-04-21 05:51:29]

I think this is ready for review now that it has some tests.

Mark: I have several questions around how code should be shared / split between
NaCl and NonSFI NaCl, marked as TODO(uekawa) in the code.

Brad Nelson: FYI, I think I have followed your advise.

hamaji, hidehiko: FYI.

[hidehiko, 2014-04-22 00:22:37]

Just drive by.

https://codereview.chromium.org/230413002/diff/340001/components/nacl/loader/...
File components/nacl/loader/nacl_helper_linux.cc (right):

https://codereview.chromium.org/230413002/diff/340001/components/nacl/loader/...
components/nacl/loader/nacl_helper_linux.cc:127: if (uses_nonsfi_mode) {
Let's drop '{}' to follow the style around here.

Ditto for other files.

https://codereview.chromium.org/230413002/diff/340001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_exception_handling.cc (right):

https://codereview.chromium.org/230413002/diff/340001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:18: /* This is NonSFI
version of exception handling codebase, NaCl side of
nit: // comment style.

Ditto for below.

https://codereview.chromium.org/230413002/diff/340001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:107:
perror("sigaction");
PLOG(ERROR) ?

[Junichi Uekawa, 2014-04-22 04:57:00]

https://codereview.chromium.org/230413002/diff/340001/components/nacl/loader/...
File components/nacl/loader/nacl_helper_linux.cc (right):

https://codereview.chromium.org/230413002/diff/340001/components/nacl/loader/...
components/nacl/loader/nacl_helper_linux.cc:127: if (uses_nonsfi_mode) {
On 2014/04/22 00:22:37, hidehiko wrote:
> Let's drop '{}' to follow the style around here.
> 
> Ditto for other files.

Done.

https://codereview.chromium.org/230413002/diff/340001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_exception_handling.cc (right):

https://codereview.chromium.org/230413002/diff/340001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:18: /* This is NonSFI
version of exception handling codebase, NaCl side of
On 2014/04/22 00:22:37, hidehiko wrote:
> nit: // comment style.
> 
> Ditto for below.

Done.

https://codereview.chromium.org/230413002/diff/340001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:107:
perror("sigaction");
On 2014/04/22 00:22:37, hidehiko wrote:
> PLOG(ERROR) ?

Here base/logging.h conflicts with native_client/src/trusted/service_runtime/
headers, it seems like I need to get rid of them to use PLOG, or use NaClLog
instead.

[Mark Seaborn, 2014-04-23 03:49:49]

Apologies for being slow to review this...

https://codereview.chromium.org/230413002/diff/360001/chrome/test/data/nacl/c...
File chrome/test/data/nacl/crash/nonsfi_crash_via_signal_handler.cc (right):

https://codereview.chromium.org/230413002/diff/360001/chrome/test/data/nacl/c...
chrome/test/data/nacl/crash/nonsfi_crash_via_signal_handler.cc:4: 
Could you name this file after the interface it tests, i.e. irt_exception,
please?  (The "signal handler" part is a detail of how irt_exception is
implemented.)

Something like chrome/test/data/nacl/irt_exception_test.cc.

Could you enable this for SFI NaCl too, since it's not specific to Non-SFI NaCl?

https://codereview.chromium.org/230413002/diff/360001/chrome/test/data/nacl/c...
chrome/test/data/nacl/crash/nonsfi_crash_via_signal_handler.cc:9: #include
"native_client/src/shared/platform/nacl_check.h"
I don't think you use this?

https://codereview.chromium.org/230413002/diff/360001/chrome/test/data/nacl/c...
chrome/test/data/nacl/crash/nonsfi_crash_via_signal_handler.cc:18: // exit from
here we shall crash and it's hard to distinguish that
You could exit using setjmp()/longjmp() to exit from this fault handler.  That
might be a little cleaner than doing an infinite loop.

See exception_test.c in the NaCl tree, which does something similar.

https://codereview.chromium.org/230413002/diff/360001/chrome/test/data/nacl/c...
chrome/test/data/nacl/crash/nonsfi_crash_via_signal_handler.cc:35: CRASH;
I think it would be better to use __builtin_trap() -- that's a bit more direct
than reusing CRASH.

https://codereview.chromium.org/230413002/diff/360001/chrome/test/data/nacl/c...
chrome/test/data/nacl/crash/nonsfi_crash_via_signal_handler.cc:45: 
Nit: remove empty line at start of function

https://codereview.chromium.org/230413002/diff/360001/chrome/test/data/nacl/c...
chrome/test/data/nacl/crash/nonsfi_crash_via_signal_handler.cc:46: // none
Nit: maybe remove this?  Empty functions aren't usually commented.

https://codereview.chromium.org/230413002/diff/360001/chrome/test/nacl/nacl_b...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://codereview.chromium.org/230413002/diff/360001/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest.cc:373: #if defined(OS_LINUX) &&
defined(ARCH_CPU_X86)
Could you copy the MAYBE_NONSFI #define from ppapi_browsertest.cc?  That is:

// The NonSFI test is currently available only on linux-x86-32 architecture.
#if defined(OS_LINUX) && defined(ARCH_CPU_X86)
#define MAYBE_NONSFI(test_name) test_name
#else
#define MAYBE_NONSFI(test_name) DISABLED_##test_name
#endif

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_exception_handling.cc (right):

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:4: #include <errno.h>
Nit: add empty line between boilerplate and first #include

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:7: #include <map>
Not used?

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:23: // TODO(uekawa): The
list of signals to be handled might need updating.
It should be OK (so you can remove this TODO).  Any hypothetical omissions we
can find by improving the test coverage later.

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:24: // NonSFI NaCl does
not use NACL_THREAD_SUSPEND_SIGNAL (==SIGUSR1) (??check??)
Correct.  That's used for thread suspension, currently used for NaCl's GDB debug
stub, which we don't use in Non-SFI NaCl.

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:36: static void
SignalCatch(int sig, siginfo_t *info, void *uc) {
Nit: use Chromium-style "* " spacing

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:38: // TODO(uekawa):
Whether to add dependency or copy the implementation?
Adding the dependency isn't ideal but it's OK for now.  Eventually we'll want to
move the IRT interface implementations to the NaCl repo (where they will be more
easily testable).  Then maybe we can refactor this so that it's not depending on
part of service_runtime.

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:41: // Is this safe to
allocate this on stack ?
Yes

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:45: 0 /*
context_user_addr, what is this? */);
This parameter doesn't apply if we're calling user code's handler function from
C.

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:49: // TODO(uekawa):
Only exit on crash signals?
All the signals you're handling are crash signals

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:55: // TODO(uekawa): Do
I need to have a mutex lock?
You could use AtomicExchange from base/atomicops.h

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:69: return -EINVAL;
This could return 0, since it's mostly-OK as a no-op.  That would prevent crash
reporter libraries from failing on startup if this returns an error.

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:75: return -EINVAL;
ENOSYS would be more appropriate

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:86: void
InitializeSignalHandler(void) {
Nit: Should be "()" rather than "(void)" in C++.  (There's only a difference in
C.)

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:103:
perror("sigaction");
This should be a fatal error, otherwise this would be ignored too easily

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_thread.cc (right):

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_thread.cc:52:
NaClSignalStackRegister(context->signal_stack);
This probably doesn't work: the seccomp sandbox will disallow sigaltstack().

Can you run the linux_rel_precise32 trybot, please?  That covers x86-32 Linux,
which is the only Chromium bot that runs Non-SFI NaCl tests currently.

I'd just omit the changes to this file for now.  We can fix the handling of
stack overflow crashes later.  NaClSignalStackAllocate() likely allocates a
stack that's too small for user code (because it wasn't intended for that
purpose).

[Junichi Uekawa, 2014-04-23 07:56:15]

ptal

https://codereview.chromium.org/230413002/diff/360001/chrome/test/data/nacl/c...
File chrome/test/data/nacl/crash/nonsfi_crash_via_signal_handler.cc (right):

https://codereview.chromium.org/230413002/diff/360001/chrome/test/data/nacl/c...
chrome/test/data/nacl/crash/nonsfi_crash_via_signal_handler.cc:4: 
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> Could you name this file after the interface it tests, i.e. irt_exception,
> please?  (The "signal handler" part is a detail of how irt_exception is
> implemented.)
> 
> Something like chrome/test/data/nacl/irt_exception_test.cc.

Done.

> Could you enable this for SFI NaCl too, since it's not specific to Non-SFI
NaCl?

Done.

gyp looks ugly, I think we probably need to fix generate_nmf.

https://codereview.chromium.org/230413002/diff/360001/chrome/test/data/nacl/c...
chrome/test/data/nacl/crash/nonsfi_crash_via_signal_handler.cc:9: #include
"native_client/src/shared/platform/nacl_check.h"
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> I don't think you use this?

Done.

https://codereview.chromium.org/230413002/diff/360001/chrome/test/data/nacl/c...
chrome/test/data/nacl/crash/nonsfi_crash_via_signal_handler.cc:18: // exit from
here we shall crash and it's hard to distinguish that
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> You could exit using setjmp()/longjmp() to exit from this fault handler.  That
> might be a little cleaner than doing an infinite loop.
> 
> See exception_test.c in the NaCl tree, which does something similar.

Done.

https://codereview.chromium.org/230413002/diff/360001/chrome/test/data/nacl/c...
chrome/test/data/nacl/crash/nonsfi_crash_via_signal_handler.cc:35: CRASH;
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> I think it would be better to use __builtin_trap() -- that's a bit more direct
> than reusing CRASH.

It seemed to me that CRASH is more consistent with other tests in
//chrome/test/data/..., but either way is fine with me.

https://codereview.chromium.org/230413002/diff/360001/chrome/test/data/nacl/c...
chrome/test/data/nacl/crash/nonsfi_crash_via_signal_handler.cc:45: 
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> Nit: remove empty line at start of function

Done.

https://codereview.chromium.org/230413002/diff/360001/chrome/test/data/nacl/c...
chrome/test/data/nacl/crash/nonsfi_crash_via_signal_handler.cc:46: // none
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> Nit: maybe remove this?  Empty functions aren't usually commented.

Done.

https://codereview.chromium.org/230413002/diff/360001/chrome/test/nacl/nacl_b...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://codereview.chromium.org/230413002/diff/360001/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest.cc:373: #if defined(OS_LINUX) &&
defined(ARCH_CPU_X86)
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> Could you copy the MAYBE_NONSFI #define from ppapi_browsertest.cc?  That is:
> 
> // The NonSFI test is currently available only on linux-x86-32 architecture.
> #if defined(OS_LINUX) && defined(ARCH_CPU_X86)
> #define MAYBE_NONSFI(test_name) test_name
> #else
> #define MAYBE_NONSFI(test_name) DISABLED_##test_name
> #endif

Done.

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_exception_handling.cc (right):

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:4: #include <errno.h>
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> Nit: add empty line between boilerplate and first #include

Done.

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:7: #include <map>
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> Not used?

Done.

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:23: // TODO(uekawa): The
list of signals to be handled might need updating.
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> It should be OK (so you can remove this TODO).  Any hypothetical omissions we
> can find by improving the test coverage later.

Done.

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:24: // NonSFI NaCl does
not use NACL_THREAD_SUSPEND_SIGNAL (==SIGUSR1) (??check??)
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> Correct.  That's used for thread suspension, currently used for NaCl's GDB
debug
> stub, which we don't use in Non-SFI NaCl.

I've re-wrote the comment with the context.

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:36: static void
SignalCatch(int sig, siginfo_t *info, void *uc) {
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> Nit: use Chromium-style "* " spacing

Done.

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:38: // TODO(uekawa):
Whether to add dependency or copy the implementation?
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> Adding the dependency isn't ideal but it's OK for now.  Eventually we'll want
to
> move the IRT interface implementations to the NaCl repo (where they will be
more
> easily testable).  Then maybe we can refactor this so that it's not depending
on
> part of service_runtime.

I've pondered of not adding a dependency; it introduces yet another list of
registers for all architectures, so I would prefer doing a refactor after we are
inside native_client repo.

https://codereview.chromium.org/248463005/

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:41: // Is this safe to
allocate this on stack ?
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> Yes

thanks

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:45: 0 /*
context_user_addr, what is this? */);
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> This parameter doesn't apply if we're calling user code's handler function
from
> C.

I think we don't need this for nonsfi NaCl, but probably won't hurt to leave it
as it is either.

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:49: // TODO(uekawa):
Only exit on crash signals?
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> All the signals you're handling are crash signals

Removed the comment.

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:55: // TODO(uekawa): Do
I need to have a mutex lock?
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> You could use AtomicExchange from base/atomicops.h

after staring at AtomicExchange, I don't think this is worth it (unless we are
running on 386 which doesn't have atomic word writes).

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:69: return -EINVAL;
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> This could return 0, since it's mostly-OK as a no-op.  That would prevent
crash
> reporter libraries from failing on startup if this returns an error.

Done.

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:75: return -EINVAL;
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> ENOSYS would be more appropriate

Done.

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:86: void
InitializeSignalHandler(void) {
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> Nit: Should be "()" rather than "(void)" in C++.  (There's only a difference
in
> C.)

Done.

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:103:
perror("sigaction");
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> This should be a fatal error, otherwise this would be ignored too easily

Done.

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_thread.cc (right):

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_thread.cc:52:
NaClSignalStackRegister(context->signal_stack);
On 2014/04/23 03:49:50, Mark Seaborn wrote:
> This probably doesn't work: the seccomp sandbox will disallow sigaltstack().
> 
> Can you run the linux_rel_precise32 trybot, please?  That covers x86-32 Linux,
> which is the only Chromium bot that runs Non-SFI NaCl tests currently.
> 
> I'd just omit the changes to this file for now.  We can fix the handling of
> stack overflow crashes later.  NaClSignalStackAllocate() likely allocates a
> stack that's too small for user code (because it wasn't intended for that
> purpose).

I was wondering why my tests are succeeding and I guess of course I am not
testing thread creation path.

I'll revert changes to this file for now.

[Mark Seaborn, 2014-04-24 23:39:27]

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_exception_handling.cc (right):

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:55: // TODO(uekawa): Do
I need to have a mutex lock?
On 2014/04/23 07:56:15, uekawa wrote:
> On 2014/04/23 03:49:50, Mark Seaborn wrote:
> > You could use AtomicExchange from base/atomicops.h
> 
> after staring at AtomicExchange, I don't think this is worth it (unless
> we are running on 386 which doesn't have atomic word writes).

But setting the new handler and getting the old one might be expected to be
atomic overall, so you should either use a mutex (as your earlier comment
suggested) or use AtomicExchange.

https://codereview.chromium.org/230413002/diff/420001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_exception_handling.cc (right):

https://codereview.chromium.org/230413002/diff/420001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:59: // TODO(uekawa): I
think we shouldn't implement this until we really
Can you state the TODO a bit more formally, please?

i.e. "Implement this function so that the exception stack actually gets used for
running an exception handler.  Currently we don't switch stack, which means we
can't handle stack overflow exceptions."

https://codereview.chromium.org/230413002/diff/420001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:60: // need it. 
IrtThreadCreate allocates sigaltstack already, and
BTW, this part doesn't reflect the code currently.

https://codereview.chromium.org/230413002/diff/420001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:70: // TODO(uekawa): I
think we shouldn't implement this until we really
Ditto -- can you state it a bit more formally?  i.e. "Implement clear_flag() to
behave like SFI NaCl's implementation, so that a thread can handle a second
exception after handling a first exception"

https://codereview.chromium.org/230413002/diff/470001/chrome/test/data/nacl/i...
File chrome/test/data/nacl/irt_exception/irt_exception_test.cc (right):

https://codereview.chromium.org/230413002/diff/470001/chrome/test/data/nacl/i...
chrome/test/data/nacl/irt_exception/irt_exception_test.cc:6: #include <unistd.h>
Nit: I *think* you don't use anything from unistd.h here...

https://codereview.chromium.org/230413002/diff/470001/chrome/test/data/nacl/n...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/230413002/diff/470001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:406: # TODO(uekawa): Because of nmf
file, NonSFI and SFI gyp configs don't mix, or do they?
I think they can be mixed.

Have a look at Hidehiko's earlier change, which did mix them OK:
https://src.chromium.org/viewvc/chrome/trunk/src/ppapi/ppapi_nacl.gyp?r1=2613...

https://codereview.chromium.org/230413002/diff/470001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:416: 'enable_x86_32_nonsfi': 1,
This should ideally be conditionalised as in
https://src.chromium.org/viewvc/chrome/trunk/src/ppapi/ppapi_nacl.gyp?r1=2613...

https://codereview.chromium.org/230413002/diff/470001/chrome/test/data/nacl/n...
File chrome/test/data/nacl/nonsfi/irt_test.html (right):

https://codereview.chromium.org/230413002/diff/470001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nonsfi/irt_test.html:21: embed.basic_tests = '1';
It's not clear to me why you're changing this file?

https://codereview.chromium.org/230413002/diff/470001/chrome/test/nacl/nacl_b...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://codereview.chromium.org/230413002/diff/470001/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest.cc:392: MAYBE_NONSFI(IrtException)) {
Can you put the two IrtException test decls next to each other, for clarity,
please?

https://codereview.chromium.org/230413002/diff/470001/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest.cc:394: "irt_exception_test.html"));
Nit: fits on previous line?

https://codereview.chromium.org/230413002/diff/470001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_exception_handling.cc (right):

https://codereview.chromium.org/230413002/diff/470001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:33: volatile
NaClExceptionHandler signal_handler_function_pointer = NULL;
Doesn't need to be volatile

https://codereview.chromium.org/230413002/diff/470001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:35: // Signal handler,
responsible for calling the registered handlers.
Nit: "handler" singular

https://codereview.chromium.org/230413002/diff/470001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:100: NaClLog(LOG_FATAL,
"sigaction to register signals failed.");
Nit: needs a "\n" at the end (NaClLog doesn't add one)

[Junichi Uekawa, 2014-04-25 01:06:09]

ptal

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_exception_handling.cc (right):

https://codereview.chromium.org/230413002/diff/360001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:55: // TODO(uekawa): Do
I need to have a mutex lock?
On 2014/04/24 23:39:27, Mark Seaborn wrote:
> On 2014/04/23 07:56:15, uekawa wrote:
> > On 2014/04/23 03:49:50, Mark Seaborn wrote:
> > > You could use AtomicExchange from base/atomicops.h
> > 
> > after staring at AtomicExchange, I don't think this is worth it (unless
> > we are running on 386 which doesn't have atomic word writes).
> 
> But setting the new handler and getting the old one might be expected to be
> atomic overall, so you should either use a mutex (as your earlier comment
> suggested) or use AtomicExchange.

Done.

https://codereview.chromium.org/230413002/diff/420001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_exception_handling.cc (right):

https://codereview.chromium.org/230413002/diff/420001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:59: // TODO(uekawa): I
think we shouldn't implement this until we really
On 2014/04/24 23:39:27, Mark Seaborn wrote:
> Can you state the TODO a bit more formally, please?
> 
> i.e. "Implement this function so that the exception stack actually gets used
for
> running an exception handler.  Currently we don't switch stack, which means we
> can't handle stack overflow exceptions."

Done.

https://codereview.chromium.org/230413002/diff/420001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:60: // need it. 
IrtThreadCreate allocates sigaltstack already, and
On 2014/04/24 23:39:27, Mark Seaborn wrote:
> BTW, this part doesn't reflect the code currently.

Done.

https://codereview.chromium.org/230413002/diff/420001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:70: // TODO(uekawa): I
think we shouldn't implement this until we really
On 2014/04/24 23:39:27, Mark Seaborn wrote:
> Ditto -- can you state it a bit more formally?  i.e. "Implement clear_flag()
to
> behave like SFI NaCl's implementation, so that a thread can handle a second
> exception after handling a first exception"

Done.

https://codereview.chromium.org/230413002/diff/470001/chrome/test/data/nacl/i...
File chrome/test/data/nacl/irt_exception/irt_exception_test.cc (right):

https://codereview.chromium.org/230413002/diff/470001/chrome/test/data/nacl/i...
chrome/test/data/nacl/irt_exception/irt_exception_test.cc:6: #include <unistd.h>
On 2014/04/24 23:39:28, Mark Seaborn wrote:
> Nit: I *think* you don't use anything from unistd.h here...

I think we I should have included stdio.h instead although something else seems
to be pulling in that dependency.

https://codereview.chromium.org/230413002/diff/470001/chrome/test/data/nacl/n...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/230413002/diff/470001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:406: # TODO(uekawa): Because of nmf
file, NonSFI and SFI gyp configs don't mix, or do they?
On 2014/04/24 23:39:28, Mark Seaborn wrote:
> I think they can be mixed.
> 
> Have a look at Hidehiko's earlier change, which did mix them OK:
>
https://src.chromium.org/viewvc/chrome/trunk/src/ppapi/ppapi_nacl.gyp?r1=2613...

Done.

https://codereview.chromium.org/230413002/diff/470001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:416: 'enable_x86_32_nonsfi': 1,
On 2014/04/24 23:39:28, Mark Seaborn wrote:
> This should ideally be conditionalised as in
>
https://src.chromium.org/viewvc/chrome/trunk/src/ppapi/ppapi_nacl.gyp?r1=2613...

Done.

https://codereview.chromium.org/230413002/diff/470001/chrome/test/data/nacl/n...
File chrome/test/data/nacl/nonsfi/irt_test.html (right):

https://codereview.chromium.org/230413002/diff/470001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nonsfi/irt_test.html:21: embed.basic_tests = '1';
On 2014/04/24 23:39:28, Mark Seaborn wrote:
> It's not clear to me why you're changing this file?

It seems like I didn't revert this file cleanly. done.

https://codereview.chromium.org/230413002/diff/470001/chrome/test/nacl/nacl_b...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://codereview.chromium.org/230413002/diff/470001/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest.cc:392: MAYBE_NONSFI(IrtException)) {
On 2014/04/24 23:39:28, Mark Seaborn wrote:
> Can you put the two IrtException test decls next to each other, for clarity,
> please?

Done.

https://codereview.chromium.org/230413002/diff/470001/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest.cc:394: "irt_exception_test.html"));
On 2014/04/24 23:39:28, Mark Seaborn wrote:
> Nit: fits on previous line?

Done.

https://codereview.chromium.org/230413002/diff/470001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_exception_handling.cc (right):

https://codereview.chromium.org/230413002/diff/470001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:33: volatile
NaClExceptionHandler signal_handler_function_pointer = NULL;
On 2014/04/24 23:39:28, Mark Seaborn wrote:
> Doesn't need to be volatile

Done.

https://codereview.chromium.org/230413002/diff/470001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:35: // Signal handler,
responsible for calling the registered handlers.
On 2014/04/24 23:39:28, Mark Seaborn wrote:
> Nit: "handler" singular

Done.

https://codereview.chromium.org/230413002/diff/470001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:100: NaClLog(LOG_FATAL,
"sigaction to register signals failed.");
On 2014/04/24 23:39:28, Mark Seaborn wrote:
> Nit: needs a "\n" at the end (NaClLog doesn't add one)

Done.

[Mark Seaborn, 2014-04-25 15:42:01]

https://codereview.chromium.org/230413002/diff/490001/chrome/test/data/nacl/n...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/230413002/diff/490001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:405: ['disable_pnacl==0 and
target_arch=="ia32" and OS=="linux"', {
Only the "enable_x86_32_nonsfi" part should be conditionalised, just like in
https://src.chromium.org/viewvc/chrome/trunk/src/ppapi/ppapi_nacl.gyp?r1=2613....
 See the current version of ppapi/ppapi_nacl.gyp.  Otherwise, you'll probably
find this fails on the trybots, since the test files won't get built in most
cases.

https://codereview.chromium.org/230413002/diff/490001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:406: 'variables': {
This block should be indented by 2 more spaces relative to the line above

https://codereview.chromium.org/230413002/diff/490001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:409: 'destination_dir':
'<(PRODUCT_DIR)/>(nexe_destination_dir)/nonsfi',
Can you omit this destination_dir setting for consistency with the other tests
in this file?

https://codereview.chromium.org/230413002/diff/490001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:413: 'nonsfi_test_files': [
Does it work to just set "test_files", for consistency with the other tests
here?

https://codereview.chromium.org/230413002/diff/490001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:421: 'copies': [
ppapi_nacl.gyp doesn't have a "copies" step so I think you shouldn't need one
here?

https://codereview.chromium.org/230413002/diff/490001/chrome/test/nacl/nacl_b...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://codereview.chromium.org/230413002/diff/490001/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest.cc:159:
IN_PROC_BROWSER_TEST_F(NaClBrowserTestNewlib, IrtException) {
Nit: can you swap these two so that NaClBrowserTestNewlib is first, just to
follow the usual ordering?

https://codereview.chromium.org/230413002/diff/490001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_exception_handling.cc (right):

https://codereview.chromium.org/230413002/diff/490001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:9: #include
"base/atomicops.h"
Not used?

https://codereview.chromium.org/230413002/diff/490001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:27: static const int
kSignals[] = {
Don't need "static" here since this is in an anon namespace.

Same for others below.

https://codereview.chromium.org/230413002/diff/490001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:74: return -ENOSYS;
This should be "return ENOSYS".  IRT functions return positive errno values.

[Junichi Uekawa, 2014-04-25 22:13:52]

https://codereview.chromium.org/230413002/diff/490001/chrome/test/data/nacl/n...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/230413002/diff/490001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:405: ['disable_pnacl==0 and
target_arch=="ia32" and OS=="linux"', {
On 2014/04/25 15:42:02, Mark Seaborn wrote:
> Only the "enable_x86_32_nonsfi" part should be conditionalised, just like in
>
https://src.chromium.org/viewvc/chrome/trunk/src/ppapi/ppapi_nacl.gyp?r1=2613....
>  See the current version of ppapi/ppapi_nacl.gyp.  Otherwise, you'll probably
> find this fails on the trybots, since the test files won't get built in most
> cases.

The things here are only necessary for nonsfi, because they are fixups for
missing functionality (generate_nmf:1 will only generate nmf for pexe and not
the nexe for nonsfi).

https://codereview.chromium.org/230413002/diff/490001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:406: 'variables': {
On 2014/04/25 15:42:02, Mark Seaborn wrote:
> This block should be indented by 2 more spaces relative to the line above

Done.

https://codereview.chromium.org/230413002/diff/490001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:409: 'destination_dir':
'<(PRODUCT_DIR)/>(nexe_destination_dir)/nonsfi',
On 2014/04/25 15:42:02, Mark Seaborn wrote:
> Can you omit this destination_dir setting for consistency with the other tests
> in this file?

I can call it something else, but we need this for nonsfi

https://codereview.chromium.org/230413002/diff/490001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:413: 'nonsfi_test_files': [
On 2014/04/25 15:42:02, Mark Seaborn wrote:
> Does it work to just set "test_files", for consistency with the other tests
> here?

test_files is already set and used and confuses the pexe/nexe generation which
has other utilities copy nmf/nexe and js files, that's why I'm using something
different here.

https://codereview.chromium.org/230413002/diff/490001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:421: 'copies': [
On 2014/04/25 15:42:02, Mark Seaborn wrote:
> ppapi_nacl.gyp doesn't have a "copies" step so I think you shouldn't need one
> here?

ppapi_nacl.gyp dumps files under out/Debug 
here we are putting files under out/Debug/nacl_test_data/{nonsfi,pnacl,...}

https://codereview.chromium.org/230413002/diff/490001/chrome/test/nacl/nacl_b...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://codereview.chromium.org/230413002/diff/490001/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest.cc:159:
IN_PROC_BROWSER_TEST_F(NaClBrowserTestNewlib, IrtException) {
On 2014/04/25 15:42:02, Mark Seaborn wrote:
> Nit: can you swap these two so that NaClBrowserTestNewlib is first, just to
> follow the usual ordering?

Done.

https://codereview.chromium.org/230413002/diff/490001/components/nacl/loader/...
File components/nacl/loader/nonsfi/irt_exception_handling.cc (right):

https://codereview.chromium.org/230413002/diff/490001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:9: #include
"base/atomicops.h"
On 2014/04/25 15:42:02, Mark Seaborn wrote:
> Not used?

Done.

https://codereview.chromium.org/230413002/diff/490001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:27: static const int
kSignals[] = {
On 2014/04/25 15:42:02, Mark Seaborn wrote:
> Don't need "static" here since this is in an anon namespace.
> 
> Same for others below.

Done.

https://codereview.chromium.org/230413002/diff/490001/components/nacl/loader/...
components/nacl/loader/nonsfi/irt_exception_handling.cc:74: return -ENOSYS;
On 2014/04/25 15:42:02, Mark Seaborn wrote:
> This should be "return ENOSYS".  IRT functions return positive errno values.

Done.

[Mark Seaborn, 2014-04-25 22:51:33]

LGTM, thanks.  Just one nit below...

https://codereview.chromium.org/230413002/diff/530001/chrome/test/data/nacl/n...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/230413002/diff/530001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:371: 'target_name':
'irt_exception_test',
Nit about ordering: Putting this in the middle of the "ppapi_crash_*" tests
isn't great.  Can you either add this at the end of the file, or put it next to
what it's next to in nacl_browsertest.cc?

[Junichi Uekawa, 2014-04-25 23:11:25]

https://codereview.chromium.org/230413002/diff/530001/chrome/test/data/nacl/n...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/230413002/diff/530001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:371: 'target_name':
'irt_exception_test',
On 2014/04/25 22:51:34, Mark Seaborn wrote:
> Nit about ordering: Putting this in the middle of the "ppapi_crash_*" tests
> isn't great.  Can you either add this at the end of the file, or put it next
to
> what it's next to in nacl_browsertest.cc?

Done.

[Junichi Uekawa, 2014-04-25 23:13:23]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-04-25 23:15:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/uekawa@chromium.org/230413002/550001

[commit-bot: I haz the power, 2014-04-28 09:07:21]

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

[commit-bot: I haz the power, 2014-04-28 09:07:22]

Failed to apply patch for components/nacl/loader/nacl_helper_linux.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file components/nacl/loader/nacl_helper_linux.cc
  Hunk #1 succeeded at 33 with fuzz 2.
  Hunk #2 FAILED at 124.
  1 out of 2 hunks FAILED -- saving rejects to file
components/nacl/loader/nacl_helper_linux.cc.rej

Patch:       components/nacl/loader/nacl_helper_linux.cc
Index: components/nacl/loader/nacl_helper_linux.cc
diff --git a/components/nacl/loader/nacl_helper_linux.cc
b/components/nacl/loader/nacl_helper_linux.cc
index
15f58eb3a5fba07e50a7b578f240a261be23384e..fe37602546255fa7da6daa1ca2d79ad0ca5dc70e
100644
--- a/components/nacl/loader/nacl_helper_linux.cc
+++ b/components/nacl/loader/nacl_helper_linux.cc
@@ -33,6 +33,7 @@
 #include "components/nacl/common/nacl_switches.h"
 #include "components/nacl/loader/nacl_listener.h"
 #include "components/nacl/loader/nacl_sandbox_linux.h"
+#include "components/nacl/loader/nonsfi/irt_exception_handling.h"
 #include "components/nacl/loader/nonsfi/nonsfi_sandbox.h"
 #include "content/public/common/zygote_fork_delegate_linux.h"
 #include "crypto/nss_util.h"
@@ -123,6 +124,8 @@ void BecomeNaClLoader(const std::vector<int>& child_fds,
   // don't need zygote FD any more
   if (IGNORE_EINTR(close(kNaClZygoteDescriptor)) != 0)
     LOG(ERROR) << "close(kNaClZygoteDescriptor) failed.";
+  if (uses_nonsfi_mode)
+    nacl::nonsfi::InitializeSignalHandler();
   InitializeLayerTwoSandbox(uses_nonsfi_mode);
   base::GlobalDescriptors::GetInstance()->Set(
       kPrimaryIPCChannel,

[Junichi Uekawa, 2014-04-28 22:01:29]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-04-28 22:02:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/uekawa@chromium.org/230413002/560001

[commit-bot: I haz the power, 2014-04-28 22:05:57]

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

[commit-bot: I haz the power, 2014-04-28 22:05:59]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[Junichi Uekawa, 2014-04-28 22:35:57]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-04-28 22:36:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/uekawa@chromium.org/230413002/560001

[commit-bot: I haz the power, 2014-04-28 22:56:14]

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

[commit-bot: I haz the power, 2014-04-28 22:56:14]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Junichi Uekawa, 2014-04-28 23:36:07]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-04-28 23:37:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/uekawa@chromium.org/230413002/580001

[commit-bot: I haz the power, 2014-04-29 02:43:43]

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

[commit-bot: I haz the power, 2014-04-29 02:43:43]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_x64_rel

[Junichi Uekawa, 2014-04-29 05:38:18]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-04-29 05:38:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/uekawa@chromium.org/230413002/580001

[commit-bot: I haz the power, 2014-04-29 05:39:02]

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

[commit-bot: I haz the power, 2014-04-29 05:39:03]

Failed to apply patch for components/nacl/loader/nacl_helper_linux.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file components/nacl/loader/nacl_helper_linux.cc
  Hunk #1 FAILED at 33.
  Hunk #2 FAILED at 147.
  2 out of 2 hunks FAILED -- saving rejects to file
components/nacl/loader/nacl_helper_linux.cc.rej

Patch:       components/nacl/loader/nacl_helper_linux.cc
Index: components/nacl/loader/nacl_helper_linux.cc
diff --git a/components/nacl/loader/nacl_helper_linux.cc
b/components/nacl/loader/nacl_helper_linux.cc
index
a7c6615e51b11973b9c98d6d1f762de23d48fa31..60db2fb918501fb7de8b1c5d47253f640226f6b2
100644
--- a/components/nacl/loader/nacl_helper_linux.cc
+++ b/components/nacl/loader/nacl_helper_linux.cc
@@ -33,6 +33,7 @@
 #include "components/nacl/common/nacl_switches.h"
 #include "components/nacl/loader/nacl_listener.h"
 #include "components/nacl/loader/nacl_sandbox_linux.h"
+#include "components/nacl/loader/nonsfi/irt_exception_handling.h"
 #include "components/nacl/loader/nonsfi/nonsfi_sandbox.h"
 #include "content/public/common/content_descriptors.h"
 #include "content/public/common/zygote_fork_delegate_linux.h"
@@ -147,8 +148,10 @@ void BecomeNaClLoader(const std::vector<int>& child_fds,
         base::GlobalDescriptors::kBaseDescriptor + kSandboxIPCChannel;
 
     ReplaceFDWithDummy(sandbox_ipc_channel);
-  }
 
+    // Install crash signal handlers before disallowing system calls.
+    nacl::nonsfi::InitializeSignalHandler();
+  }
   InitializeLayerTwoSandbox(uses_nonsfi_mode);
   base::GlobalDescriptors::GetInstance()->Set(
       kPrimaryIPCChannel,

[Junichi Uekawa, 2014-04-29 10:01:18]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-04-29 10:02:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/uekawa@chromium.org/230413002/600001

[commit-bot: I haz the power, 2014-04-29 10:05:09]

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

[commit-bot: I haz the power, 2014-04-29 10:05:09]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium

[Junichi Uekawa, 2014-04-29 11:19:12]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-04-29 11:19:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/uekawa@chromium.org/230413002/620001

[commit-bot: I haz the power, 2014-04-29 11:27:53]

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

[commit-bot: I haz the power, 2014-04-29 11:27:54]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[Junichi Uekawa, 2014-04-29 11:41:32]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-04-29 11:41:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/uekawa@chromium.org/230413002/640001

[commit-bot: I haz the power, 2014-04-29 11:44:17]

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

[commit-bot: I haz the power, 2014-04-29 11:44:19]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[Junichi Uekawa, 2014-04-29 11:46:44]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-04-29 11:46:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/uekawa@chromium.org/230413002/640001

[commit-bot: I haz the power, 2014-04-29 11:52:50]

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

[commit-bot: I haz the power, 2014-04-29 11:52:51]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium

[Junichi Uekawa, 2014-04-29 11:53:16]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-04-29 11:53:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/uekawa@chromium.org/230413002/640001

[commit-bot: I haz the power, 2014-04-29 11:57:21]

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

[commit-bot: I haz the power, 2014-04-29 11:57:22]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[Junichi Uekawa, 2014-04-29 11:58:23]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-04-29 11:58:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/uekawa@chromium.org/230413002/640001

[commit-bot: I haz the power, 2014-04-29 12:02:49]

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

[commit-bot: I haz the power, 2014-04-29 12:02:50]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium

[Junichi Uekawa, 2014-04-29 12:03:29]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-04-29 12:03:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/uekawa@chromium.org/230413002/640001

[commit-bot: I haz the power, 2014-04-29 12:05:50]

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

[commit-bot: I haz the power, 2014-04-29 12:05:51]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium

[Junichi Uekawa, 2014-04-29 12:21:17]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-04-29 12:21:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/uekawa@chromium.org/230413002/660001

[Junichi Uekawa, 2014-04-29 12:21:40]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-04-29 12:21:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/uekawa@chromium.org/230413002/660001

[commit-bot: I haz the power, 2014-04-29 12:36:39]

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

[commit-bot: I haz the power, 2014-04-29 12:36:40]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium

[Junichi Uekawa, 2014-04-29 12:45:33]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-04-29 12:45:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/uekawa@chromium.org/230413002/660001

[commit-bot: I haz the power, 2014-04-29 12:58:33]

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

[commit-bot: I haz the power, 2014-04-29 12:58:34]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium

[Junichi Uekawa, 2014-04-29 13:00:32]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-04-29 13:00:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/uekawa@chromium.org/230413002/680001

[commit-bot: I haz the power, 2014-04-29 13:50:16]

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

[commit-bot: I haz the power, 2014-04-29 13:50:17]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium

[Junichi Uekawa, 2014-04-29 21:48:40]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-04-29 21:50:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/uekawa@chromium.org/230413002/700001

[commit-bot: I haz the power, 2014-04-29 22:00:23]

Change committed as 266968

[Junichi Uekawa, 2014-04-30 05:07:12]

https://codereview.chromium.org/230413002/diff/750001/components/nacl/loader/...
File components/nacl/loader/nonsfi/DEPS (right):

https://codereview.chromium.org/230413002/diff/750001/components/nacl/loader/...
components/nacl/loader/nonsfi/DEPS:4:
"+native_client/src/shared/platform/nacl_log.h",
it seems like on debug build and release build, includes are different and thus
I need to explicitly include this file to get NaClLog().

I tried looking at base/logging.h but it conflicts with nacl_check for CHECK and
DCHECK at least, which I think we should fix but since that is inside
native_client/ it is somewhat more tricky.

[Junichi Uekawa, 2014-04-30 05:07:48]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-04-30 05:11:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/uekawa@chromium.org/230413002/750001

[commit-bot: I haz the power, 2014-04-30 07:29:32]

Change committed as 267120

[Mark Seaborn, 2014-04-30 18:51:41]

So patchsets 38 and 41 are the two committed versions?  LGTM for the difference
between those.