Non-SFI mode: Add Linux asynchronous signal support

src/untrusted/irt/irt.h

Index: src/untrusted/irt/irt.h
diff --git a/src/untrusted/irt/irt.h b/src/untrusted/irt/irt.h
index 979614a10e02430c54e7b7c393745d06793662c3..82f6f1f2ea93a701826f2077c1ae934d683a0bc5 100644
--- a/src/untrusted/irt/irt.h
+++ b/src/untrusted/irt/irt.h
@@ -21,6 +21,8 @@ struct NaClMemMappingInfo;
 
 typedef int64_t nacl_irt_off_t;
 typedef uint32_t nacl_irt_clockid_t;
+typedef uintptr_t nacl_irt_tid_t;
+#define NACL_IRT_MAIN_THREAD_TID 0
 
 #if defined(__cplusplus)
 extern "C" {
@@ -241,6 +243,23 @@ struct nacl_irt_thread {
 };
 
 /*
+ * This interface is only available on Non-SFI Mode.
+ */
+#define NACL_IRT_THREAD_v0_2   "nacl-irt-thread-0.2"
+struct nacl_irt_thread_v0_2 {
+  /*
+   * Now this interface assigns the thread ID of the child thread into

[Mark Seaborn, 2015/08/12 01:43:07]

Remove "Now".  Can you start by saying "This interface is the same as
nacl_irt_thread_v0_1, with the additional feature that it assigns the thread
ID...".

[Luis Héctor Chávez, 2015/08/12 22:14:27]

Done.

+   * |child_tid| atomically when the thread is created.  It will be the

[Mark Seaborn, 2015/08/12 01:43:07]

Nit: the assignment is not atomic.  It just happens before the child thread
starts and before thread_create() returns in the parent thread.

(Pedantically, "atomic" would mean that a third thread could read *child_tid
while the assignment is happening without getting undefined behaviour.)

[Luis Héctor Chávez, 2015/08/12 22:14:27]

Done.

+   * |parent_tid| argument to the clone() system call.  |child_tid| can be

[Junichi Uekawa, 2015/07/21 23:36:03]

This comment is strange.

clone looks something like:
clone(flags, child_stack, parent_tid, child_tid, regs)
(there are three different orderings depending on target architecture, so the
exact ordering is sometimes different).

But clone has parent_tid and child_tid parameters, why parent_tid?


CLONE_CHILD_SETTID should set child tid at ctid, CLONE_PARENT_SETTID should set
parent tid at ptid.

[Luis Héctor Chávez, 2015/07/21 23:42:11]

IIUC when calling the clone syscall with the CLONE_VM flag like we do,
CLONE_PARENT_SETTID and CLONE_CHILD_SETTID have the same effect, so which one to
choose is arbitrary. ptid was chosen since CLONE_PARENT_SETTID is already
allowed by the sandbox in SFI mode, so it required less modification.

[Mark Seaborn, 2015/08/12 01:43:07]

Nit: don't mention clone() here.  That's an implementation detail.

If you were to mention it, it should be like "The child_tid argument works like
argument blah of Linux's clone() system call".

+   * NULL.
+   */
+  int (*thread_create)(void (*start_func)(void), void *stack, void *thread_ptr,
+                       nacl_irt_tid_t *child_tid);
+  void (*thread_exit)(int32_t *stack_flag);
+  int (*thread_nice)(const int nice);
+};
+
+/*
  * The irt_futex interface is based on Linux's futex() system call.
  *
  * irt_futex provides process-private futexes, so futex wait queues are
@@ -436,6 +455,54 @@ struct nacl_irt_icache {
   int (*clear_cache)(void *addr, size_t size);
 };
 
+/*
+ * This interface is only available on Non-SFI Mode.

[Mark Seaborn, 2015/08/12 01:43:07]

Can you refer back to nonsfi_mode_async_signals.txt somewhere in the comments
for this interface?

[Luis Héctor Chávez, 2015/08/12 22:14:27]

Done.

+ */
+#define NACL_IRT_ASYNC_SIGNAL_HANDLING_v0_1 "nacl-irt-async-signal-handling-0.1"
+typedef void (*NaClIrtSignalHandler)(struct NaClExceptionContext *context);

[Mark Seaborn, 2015/08/12 01:43:07]

Nit: don't put this between the #define and the struct.  Put it before, with
empty lines before and after?

Also, please add "Async" into the type name

[Luis Héctor Chávez, 2015/08/12 22:14:27]

Done.

+struct nacl_irt_async_signal_handling {
+  /*
+   * NaCl applications can register a single, global async signal handler that
+   * will be invoked on the thread that receives the signal.  When
+   * send_async_signal is called, |handler| is invoked on the thread specified
+   * by |tid| on the thread's stack and without modifying the underlying signal
+   * mask (as opposed to the POSIX behavior), so |handler| must be reentrant.

[Mark Seaborn, 2015/08/12 01:43:07]

See my comment in nonsfi_mode_async_signals.txt about re-entrancy.

[Luis Héctor Chávez, 2015/08/12 22:14:27]

Reworded it a bit.

+   *
+   * If the thread was executing an IRT function, the suspended thread will
+   * continue it after |handler| finishes.  In other words, no IRT function will
+   * be aborted.
+   *
+   * You can safely use only async-signal-safe operations in |handler|. The
+   * following NaCl IRT functions are async-signal-safe:
+   *
+   * - tls_get
+   * - futex_wait_abs
+   * - futex_wake
+   * - send_async_signal
+   *
+   * This function in particular is not async-signal-safe and must not be called
+   * from signal handlers.
+   */
+  int (*set_async_signal_handler)(NaClIrtSignalHandler handler);
+  /*
+   * Asynchronously delivers a signal to the thread identified by |tid| within
+   * the same thread group as the caller.  This function requires

[Mark Seaborn, 2015/08/12 01:43:07]

"Thread group" is a Linux concept that shouldn't be referenced here.

[Luis Héctor Chávez, 2015/08/12 22:14:27]

Changed to "process" to clarify that this can't be used to communicate outside
of the app.

+   * nacl_signal_set_handler to be called first, otherwise it will fail with

[Mark Seaborn, 2015/08/12 01:43:07]

"otherwise it will fail with ESRCH" -- presumably this won't be true if the
signal handler is initialised on startup by nacl_helper_nonsfi?

Maybe you don't need to document this case?

[Luis Héctor Chávez, 2015/08/12 22:14:27]

Removed.

+   * ESRCH.  |tid| should be a valid thread identifier obtained when creating a
+   * thread (through the |parent_tid| parameter) and the thread must still be

[Mark Seaborn, 2015/08/12 01:43:07]

You mean child_tid.

[Luis Héctor Chávez, 2015/08/12 22:14:27]

Done.

+   * alive.  The constant NACL_IRT_MAIN_THREAD_TID can be used to refer to the
+   * main thread.
+   *
+   * As opposed to POSIX, NaCl only provides a way to send a single signal.
+   * Delivery of different kinds of signals to be POSIX-compliant can be
+   * implemented in userspace.
+   *
+   * When |tid| is invalid, the result is unspecified.  This IRT function
+   * returns 0 on success or may return ESRCH or EINVAL on failure.
+   */
+  int (*send_async_signal)(nacl_irt_tid_t tid);
+};
+
 #if defined(__cplusplus)
 }
 #endif