Reland "Non-SFI mode: Add Linux asynchronous signal support"

Reland "Non-SFI mode: Add Linux asynchronous signal support"

This change adds a mechanism to both receive and send signals to other
threads in the process.  A single signal is implemented with no option
to mask it, but user code can use this to multiplex multiple signals and
implement something that looks more like POSIX.

Enabling signals will require allowing the tgkill system call (with the
current pid and SIGUSR1 as signum) and adding the CLONE_PARENT_SETTID
flag to clone.

* Refactored some functions common to both signal and exception
  handling from src/nonsfi/linux/irt_exception_handling.c to
  src/nonsfi/linux/irt_signal_handling.c.  That way, other code
  can still call |nonsfi_initialize_signal_handler| and it will
  install both.
* Added a test for user async-signals.
* Added documentation for this feature in
  documentation/nonsfi_mode_async_signals.txt

BUG=https://code.google.com/p/nativeclient/issues/detail?id=4065
TEST=./scons run_user_async_signal_test_irt bitcode=1 nonsfi_nacl=1 \
     --mode=nacl,dbg-host,nacl_irt_test [platform={arm,x86_32}]
TEST=git try

Committed: https://chromium.googlesource.com/native_client/src/native_client/+/821cf4bb69650569eed52c7fa6309333c1f72654

[Luis Héctor Chávez, 2015-08-20 00:14:45]

lhchavez@chromium.org changed reviewers:
+ mseaborn@chromium.org

[Luis Héctor Chávez, 2015-08-20 00:14:46]

Please take a look.

The previous version of the change made some bots fail (not the trybots,
though). It turned out that the |fpstate| field in the context was NULL in that
hardware, so it was segfaulting. While investigating this issue, I ran into a
timing issue with the signal sending code at the end of each test that I also
fixed.

The original patch was uploaded as-is as Patch Set 1 and the fixes can be found
in Patch Set 2.

[Mark Seaborn, 2015-08-20 15:44:30]

Some nits...

https://codereview.chromium.org/1293793009/diff/20001/documentation/nonsfi_mo...
File documentation/nonsfi_mode_async_signals.txt (right):

https://codereview.chromium.org/1293793009/diff/20001/documentation/nonsfi_mo...
documentation/nonsfi_mode_async_signals.txt:54: thread_create, or belonged to a
thread that has already terminated will produce
Instead of saying "will produce undefined behavior", you could say:

Using the ID of a thread that has exited might result in sending the signal to
the wrong thread, if the ID is reused for a new thread.

https://codereview.chromium.org/1293793009/diff/20001/src/untrusted/irt/irt.h
File src/untrusted/irt/irt.h (right):

https://codereview.chromium.org/1293793009/diff/20001/src/untrusted/irt/irt.h...
src/untrusted/irt/irt.h:517: * When |tid| is invalid, the result is unspecified.
 This IRT function
Here you say "unspecified".  In documentation/nonsfi_mode_async_signals.txt, you
say "undefined".

In C, they are different:
https://en.wikipedia.org/wiki/Unspecified_behavior
https://en.wikipedia.org/wiki/Undefined_behavior

https://codereview.chromium.org/1293793009/diff/20001/tests/nonsfi/user_async...
File tests/nonsfi/user_async_signal_test.cc (right):

https://codereview.chromium.org/1293793009/diff/20001/tests/nonsfi/user_async...
tests/nonsfi/user_async_signal_test.cc:95: /* Thread might have exited before we
sent the signal. */
This is problematic, because in documentation/nonsfi_mode_async_signals.txt you
say:

"Providing a thread identifier that is not NACL_IRT_MAIN_THREAD_TID, was not
obtained from thread_create, or belonged to a thread that has already terminated
will produce undefined behavior."

So your test is doing something which you say is undefined behaviour.

Is it practical to use this interface without it invoking this undefined
behaviour?  I suspect you may end up invoking this undefined behaviour in ARC.

BTW, there's a similar issue in glibc:
https://sourceware.org/bugzilla/show_bug.cgi?id=12889
(filed by the musl-libc developer Rich Felker, who has been noticing a lot of
these corner-case bugs)

No change is required here, though.  It's OK for a test to cover
implementation-specific details.  I'm just pointing out a possible problem.

https://codereview.chromium.org/1293793009/diff/20001/tests/nonsfi/user_async...
tests/nonsfi/user_async_signal_test.cc:176: /* Send a last signal to make sure
any waiting syscalls get interrupted. */
How about splitting these 6 lines into a separate function so that the comments
and check aren't duplicated?

[Luis Héctor Chávez, 2015-08-20 16:13:49]

https://codereview.chromium.org/1293793009/diff/20001/documentation/nonsfi_mo...
File documentation/nonsfi_mode_async_signals.txt (right):

https://codereview.chromium.org/1293793009/diff/20001/documentation/nonsfi_mo...
documentation/nonsfi_mode_async_signals.txt:54: thread_create, or belonged to a
thread that has already terminated will produce
On 2015/08/20 15:44:30, Mark Seaborn wrote:
> Instead of saying "will produce undefined behavior", you could say:
> 
> Using the ID of a thread that has exited might result in sending the signal to
> the wrong thread, if the ID is reused for a new thread.

Done, and reworded the case where the ID is invalid/already dead to say it will
fail with ESRCH.

https://codereview.chromium.org/1293793009/diff/20001/src/untrusted/irt/irt.h
File src/untrusted/irt/irt.h (right):

https://codereview.chromium.org/1293793009/diff/20001/src/untrusted/irt/irt.h...
src/untrusted/irt/irt.h:517: * When |tid| is invalid, the result is unspecified.
 This IRT function
On 2015/08/20 15:44:30, Mark Seaborn wrote:
> Here you say "unspecified".  In documentation/nonsfi_mode_async_signals.txt,
you
> say "undefined".
> 
> In C, they are different:
> https://en.wikipedia.org/wiki/Unspecified_behavior
> https://en.wikipedia.org/wiki/Undefined_behavior

Done.

https://codereview.chromium.org/1293793009/diff/20001/tests/nonsfi/user_async...
File tests/nonsfi/user_async_signal_test.cc (right):

https://codereview.chromium.org/1293793009/diff/20001/tests/nonsfi/user_async...
tests/nonsfi/user_async_signal_test.cc:95: /* Thread might have exited before we
sent the signal. */
On 2015/08/20 15:44:30, Mark Seaborn wrote:
> This is problematic, because in documentation/nonsfi_mode_async_signals.txt
you
> say:
> 
> "Providing a thread identifier that is not NACL_IRT_MAIN_THREAD_TID, was not
> obtained from thread_create, or belonged to a thread that has already
terminated
> will produce undefined behavior."
> 
> So your test is doing something which you say is undefined behaviour.
> 
> Is it practical to use this interface without it invoking this undefined
> behaviour?  I suspect you may end up invoking this undefined behaviour in ARC.
> 
> BTW, there's a similar issue in glibc:
> https://sourceware.org/bugzilla/show_bug.cgi?id=12889
> (filed by the musl-libc developer Rich Felker, who has been noticing a lot of
> these corner-case bugs)
> 
> No change is required here, though.  It's OK for a test to cover
> implementation-specific details.  I'm just pointing out a possible problem.

I think that the doc change is enough. Upstream users should also take this into
account.

https://codereview.chromium.org/1293793009/diff/20001/tests/nonsfi/user_async...
tests/nonsfi/user_async_signal_test.cc:176: /* Send a last signal to make sure
any waiting syscalls get interrupted. */
On 2015/08/20 15:44:30, Mark Seaborn wrote:
> How about splitting these 6 lines into a separate function so that the
comments
> and check aren't duplicated?

Done.

[Mark Seaborn, 2015-08-20 21:42:35]

LGTM

https://codereview.chromium.org/1293793009/diff/40001/documentation/nonsfi_mo...
File documentation/nonsfi_mode_async_signals.txt (right):

https://codereview.chromium.org/1293793009/diff/40001/documentation/nonsfi_mo...
documentation/nonsfi_mode_async_signals.txt:55: send_async_signal() fail with
ESRCH.  Using the ID of a thread that has exited
Saying it *will* fail with ESRCH is also technically not true.

The PPAPI proxy creates threads internally.  So a thread ID may be reused for an
internal thread -- one that wasn't created by thread_create().

Hmm, given that, maybe "undefined behaviour" is appropriate after all.  So maybe
something like:

"...may cause undefined behaviour.  For example, this could include sending the
signal to an internally-created thread, if the thread ID is reused for a new
thread."

[Luis Héctor Chávez, 2015-08-20 21:50:54]

https://codereview.chromium.org/1293793009/diff/40001/documentation/nonsfi_mo...
File documentation/nonsfi_mode_async_signals.txt (right):

https://codereview.chromium.org/1293793009/diff/40001/documentation/nonsfi_mo...
documentation/nonsfi_mode_async_signals.txt:55: send_async_signal() fail with
ESRCH.  Using the ID of a thread that has exited
On 2015/08/20 21:42:35, Mark Seaborn wrote:
> Saying it *will* fail with ESRCH is also technically not true.
> 
> The PPAPI proxy creates threads internally.  So a thread ID may be reused for
an
> internal thread -- one that wasn't created by thread_create().
> 
> Hmm, given that, maybe "undefined behaviour" is appropriate after all.  So
maybe
> something like:
> 
> "...may cause undefined behaviour.  For example, this could include sending
the
> signal to an internally-created thread, if the thread ID is reused for a new
> thread."

Done.

[Luis Héctor Chávez, 2015-08-21 03:24:03]

The CQ bit was checked by lhchavez@chromium.org

[Luis Héctor Chávez, 2015-08-21 03:24:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
mseaborn@chromium.org
Link to the patchset: https://codereview.chromium.org/1293793009/#ps60001
(title: "Updated documentation")

[commit-bot: I haz the power, 2015-08-21 03:24:21]

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

[commit-bot: I haz the power, 2015-08-21 03:25:25]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/native_client/src/native_client/+/821cf4bb6...