Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1212613002:
Non-SFI mode: Add Linux asynchronous signal support (Closed)
|
Created:
5 years, 6 months ago by Luis Héctor Chávez Modified:
5 years, 4 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/src/native_client.git@master Target Ref:
refs/heads/master Project:
nacl Visibility:
Public. |
DescriptionNon-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]
TEST=git try
Committed: https://chromium.googlesource.com/native_client/src/native_client/+/6a0ce065f481a3eb0bcd63ed62755d82d339c617
Patch Set 1 #
Total comments: 40
Patch Set 2 : Addressed feedback and an almost working ARM implementation #Patch Set 3 : Made the test much less racy. ARM still has some issues. #Patch Set 4 : Fixed tests and disabling one test in ARM #Patch Set 5 : Fixed a small window where signals could corrupt the stack #
Total comments: 28
Patch Set 6 : Addressed feedback #Patch Set 7 : Disabled the async signal test for glibc #
Total comments: 104
Patch Set 8 : Added SA_RESTART to sigaction and addressed feedback #Patch Set 9 : Rebased change for trybots #
Total comments: 6
Patch Set 10 : Restored irt_exception_handling.h for clean DEPS roll #Patch Set 11 : Added a test for FUTEX_WAIT with no timeout #
Total comments: 4
Patch Set 12 : Added a TODO to remove code after roll #
Messages
Total messages: 38 (4 generated)
lhchavez@chromium.org changed reviewers: + mseaborn@chromium.org
Please take a look. The implementation of _nonsfi_restore_context for ARM is not finished, though.
https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/irt/irt_interfac... File src/nonsfi/irt/irt_interfaces.c (right): https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/irt/irt_interfac... src/nonsfi/irt/irt_interfaces.c:42: #include "native_client/src/untrusted/pthread/pthread_types.h" Not used? irt_interfaces.c shouldn't depend on src/untrusted/pthread/ anyway. Oh, I see you are depending on this for "tid->native_tid" below... https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/irt/irt_interfac... src/nonsfi/irt/irt_interfaces.c:382: *child_tid = tid->native_tid; This creates an unfortunate dependency on libpthread's internals. (And adding tid->native_tid to libpthread has problems -- see other comment.) This would fail when compiling irt_interfaces.c for Linux or Mac (rather than for Non-SFI mode) -- which we test on the PNaCl toolchain trybots for "PNaCl unsandboxed mode". What I would recommend here is: Change src/nonsfi/linux/linux_pthread_private.c to export a user_thread_create() function (or nacl_irt_thread structs). In irt_interfaces.c, when __native_client__ is #defined, we can then use that user_thread_create() instead of calling pthread_create(). Note that this user_thread_create() would need to do more than the existing nacl_irt_thread_create() in linux_pthread_private.c, because it would need to initialise the IRT's pthread state for the new thread. See nacl_irt_thread_create() and irt_start_thread() in src/untrusted/irt/irt_thread.c for how to do that. This is admittedly a bit fiddly, so you could do it as a separate refactoring change. But it does have the benefit that we would no longer be allocating two stacks for every thread. Currently, when a Bare Metal Mode app creates a thread, it allocates a stack and passes it to thread_create(), but the IRT ignores that stack and uses one it allocates itself (allocated by the IRT's internal pthread_create()). This refactoring would fix that: the IRT would start using the stack that the app passes to thread_create(). https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/linux/irt_except... File src/nonsfi/linux/irt_exception_handling.c (left): https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/linux/irt_except... src/nonsfi/linux/irt_exception_handling.c:23: typedef struct compat_sigaltstack { If you're moving large chunks of code around, would you mind summarising what's moved in the commit message, to make review easier? https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/linux/linux_sys_... File src/nonsfi/linux/linux_sys_private.c (right): https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/linux/linux_sys_... src/nonsfi/linux/linux_sys_private.c:613: int linux_tgkill(int tgid, Nit: args would fit on one line https://codereview.chromium.org/1212613002/diff/1/src/public/nonsfi/irt_signa... File src/public/nonsfi/irt_signal_handling.h (right): https://codereview.chromium.org/1212613002/diff/1/src/public/nonsfi/irt_signa... src/public/nonsfi/irt_signal_handling.h:14: /* Initialize signal handler for exceptions and regular signals at startup Nit: Use the NaCl style for multiline comments, with "/*" and "*/" on separate lines, like this: /* * Blah... * blah. */ https://codereview.chromium.org/1212613002/diff/1/src/untrusted/irt/irt.h File src/untrusted/irt/irt.h (right): https://codereview.chromium.org/1212613002/diff/1/src/untrusted/irt/irt.h#new... src/untrusted/irt/irt.h:244: #define NACL_IRT_THREAD_v0_2 "nacl-irt-thread-0.2" Please comment that this is implemented for Non-SFI NaCl only. https://codereview.chromium.org/1212613002/diff/1/src/untrusted/irt/irt.h#new... src/untrusted/irt/irt.h:247: * Now this interface returns the thread ID of the child thread as Nit: It assigns it to *child_tid rather than returning it. Please document when it does the assignment. https://codereview.chromium.org/1212613002/diff/1/src/untrusted/irt/irt.h#new... src/untrusted/irt/irt.h:406: #define NACL_IRT_SIGNAL_HANDLING_v0_1 "nacl-irt-signal-handling-0.1" Please comment that this is implemented for Non-SFI NaCl only (just as nacl_irt_icache is). For that reason, it should probably go at the end of the file, next to nacl_irt_icache. https://codereview.chromium.org/1212613002/diff/1/src/untrusted/irt/irt.h#new... src/untrusted/irt/irt.h:408: struct nacl_irt_signal_handling { Let's add "async" into the name to make it clear that this is for async signals only. https://codereview.chromium.org/1212613002/diff/1/src/untrusted/pthread/nc_th... File src/untrusted/pthread/nc_thread.c (right): https://codereview.chromium.org/1212613002/diff/1/src/untrusted/pthread/nc_th... src/untrusted/pthread/nc_thread.c:419: retval = __libnacl_irt_thread.thread_create(nc_thread_starter, esp, new_tp, This isn't going to work for SFI NaCl, because you're making this depend on nacl_irt_thread_v0_2, which isn't defined for SFI NaCl. I'm not sure if this change passes on the trybots? (Can you figure out how to get trybot access?) I don't think it's really necessary to add support for TIDs to src/untrusted/pthread/ (i.e. to nacl-newlib's libpthread), because in the NaCl codebase we *should* only need access to them for testing. See my other comment on the test code. https://codereview.chromium.org/1212613002/diff/1/src/untrusted/pthread/nc_th... src/untrusted/pthread/nc_thread.c:421: (*thread_id)->native_tid = native_tid; BTW, assigning to (*thread_id)->native_tid as a separate step defeats the purpose of having thread_create() set the TID rather than having a separate gettid() syscall, which is to make both the assignment and the use of the TID thread-safe. Doing the assignment here means that it's not thread-safe for the child thread to cause pthread_kill(child_thread, sig) to be called. Also, the thread could have exited by this point, so (*thread_id)->native_tid might point to deallocated memory. The idea would have been to pass &(*thread_id)->native_tid to thread_create(). https://codereview.chromium.org/1212613002/diff/1/tests/signal_handler/nacl.s... File tests/signal_handler/nacl.scons (right): https://codereview.chromium.org/1212613002/diff/1/tests/signal_handler/nacl.s... tests/signal_handler/nacl.scons:8: if env.Bit('nonsfi_nacl'): Can you put this test in tests/nonsfi/, please? I think that would be more appropriate. tests/signal_handler/ is really for SFI NaCl's hardware exception handling, which is quite different from this async signal facility. https://codereview.chromium.org/1212613002/diff/1/tests/signal_handler/nacl.s... tests/signal_handler/nacl.scons:12: 'user_signal_test.cc', Can you add "async" to the test names? https://codereview.chromium.org/1212613002/diff/1/tests/signal_handler/user_s... File tests/signal_handler/user_signal_test.cc (right): https://codereview.chromium.org/1212613002/diff/1/tests/signal_handler/user_s... tests/signal_handler/user_signal_test.cc:35: * Check that nacl_tls_get() is signal-async-safe. Nit: "async-signal-safe" https://codereview.chromium.org/1212613002/diff/1/tests/signal_handler/user_s... tests/signal_handler/user_signal_test.cc:42: sem_post(&g_sem); Nit: check return value on all calls (sem_post(), pthread_join() etc.) https://codereview.chromium.org/1212613002/diff/1/tests/signal_handler/user_s... tests/signal_handler/user_signal_test.cc:66: nacl_signal_send_async_signal(tid->native_tid); Using tid->native_tid here is unfortunate because it requires our libpthread to support saving the TID, which we don't really need apart from for this test. Here's an alternative: In main(), modify __libnacl_irt_thread.thread_create to point to a wrapper function like this one: static __thread int native_tid; int thread_create_wrapper(func, stack, thread_ptr) { /* Trick to figure out address of thread-local var in the new thread. */ uintptr_t var_offset = (uintptr_t) &native_tid - get_tls(); uintptr_t new_tid_ptr = (uintptr_t) thread_ptr + var_offset; return thread_create_v2(func, stack, thread_ptr, new_tid_ptr); }
uekawa@chromium.org changed reviewers: + uekawa@chromium.org
https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/linux/irt_signal... File src/nonsfi/linux/irt_signal_handling.c (right): https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/linux/irt_signal... src/nonsfi/linux/irt_signal_handling.c:119: _nonsfi_restore_context(const linux_mcontext_t *mctx) { this is not python code, don't prepend function name with _ please. https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/linux/irt_signal... src/nonsfi/linux/irt_signal_handling.c:202: /* TODO(lhchavez): Implement this. */ erm... is this required ? https://codereview.chromium.org/1212613002/diff/1/src/public/linux_syscalls/s... File src/public/linux_syscalls/sys/syscall.h (right): https://codereview.chromium.org/1212613002/diff/1/src/public/linux_syscalls/s... src/public/linux_syscalls/sys/syscall.h:130: # define __NR_tgkill 286 double checking; 268?
https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/irt/irt_interfac... File src/nonsfi/irt/irt_interfaces.c (right): https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/irt/irt_interfac... src/nonsfi/irt/irt_interfaces.c:42: #include "native_client/src/untrusted/pthread/pthread_types.h" On 2015/06/26 18:31:59, Mark Seaborn wrote: > Not used? irt_interfaces.c shouldn't depend on src/untrusted/pthread/ anyway. > > Oh, I see you are depending on this for "tid->native_tid" below... Removed. https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/irt/irt_interfac... src/nonsfi/irt/irt_interfaces.c:382: *child_tid = tid->native_tid; On 2015/06/26 18:31:59, Mark Seaborn wrote: > This creates an unfortunate dependency on libpthread's internals. (And adding > tid->native_tid to libpthread has problems -- see other comment.) > > This would fail when compiling irt_interfaces.c for Linux or Mac (rather than > for Non-SFI mode) -- which we test on the PNaCl toolchain trybots for "PNaCl > unsandboxed mode". > > What I would recommend here is: > > Change src/nonsfi/linux/linux_pthread_private.c to export a user_thread_create() > function (or nacl_irt_thread structs). In irt_interfaces.c, when > __native_client__ is #defined, we can then use that user_thread_create() instead > of calling pthread_create(). > > Note that this user_thread_create() would need to do more than the existing > nacl_irt_thread_create() in linux_pthread_private.c, because it would need to > initialise the IRT's pthread state for the new thread. See > nacl_irt_thread_create() and irt_start_thread() in > src/untrusted/irt/irt_thread.c for how to do that. > > This is admittedly a bit fiddly, so you could do it as a separate refactoring > change. But it does have the benefit that we would no longer be allocating two > stacks for every thread. Currently, when a Bare Metal Mode app creates a > thread, it allocates a stack and passes it to thread_create(), but the IRT > ignores that stack and uses one it allocates itself (allocated by the IRT's > internal pthread_create()). This refactoring would fix that: the IRT would > start using the stack that the app passes to thread_create(). Split that off into https://codereview.chromium.org/1222753005/ that can be applied before. https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/linux/irt_except... File src/nonsfi/linux/irt_exception_handling.c (left): https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/linux/irt_except... src/nonsfi/linux/irt_exception_handling.c:23: typedef struct compat_sigaltstack { On 2015/06/26 18:31:59, Mark Seaborn wrote: > If you're moving large chunks of code around, would you mind summarising what's > moved in the commit message, to make review easier? Done. https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/linux/irt_signal... File src/nonsfi/linux/irt_signal_handling.c (right): https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/linux/irt_signal... src/nonsfi/linux/irt_signal_handling.c:119: _nonsfi_restore_context(const linux_mcontext_t *mctx) { On 2015/07/01 02:14:57, Junichi Uekawa wrote: > this is not python code, don't prepend function name with _ please. Done. https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/linux/irt_signal... src/nonsfi/linux/irt_signal_handling.c:202: /* TODO(lhchavez): Implement this. */ On 2015/07/01 02:14:57, Junichi Uekawa wrote: > erm... is this required ? It is. I wanted to have feedback on the rest of the change before finishing this particular code. https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/linux/linux_sys_... File src/nonsfi/linux/linux_sys_private.c (right): https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/linux/linux_sys_... src/nonsfi/linux/linux_sys_private.c:613: int linux_tgkill(int tgid, On 2015/06/26 18:31:59, Mark Seaborn wrote: > Nit: args would fit on one line Done. https://codereview.chromium.org/1212613002/diff/1/src/public/linux_syscalls/s... File src/public/linux_syscalls/sys/syscall.h (right): https://codereview.chromium.org/1212613002/diff/1/src/public/linux_syscalls/s... src/public/linux_syscalls/sys/syscall.h:130: # define __NR_tgkill 286 On 2015/07/01 02:14:57, Junichi Uekawa wrote: > double checking; 268? Yes, it should have been 268. https://codereview.chromium.org/1212613002/diff/1/src/public/nonsfi/irt_signa... File src/public/nonsfi/irt_signal_handling.h (right): https://codereview.chromium.org/1212613002/diff/1/src/public/nonsfi/irt_signa... src/public/nonsfi/irt_signal_handling.h:14: /* Initialize signal handler for exceptions and regular signals at startup On 2015/06/26 18:31:59, Mark Seaborn wrote: > Nit: Use the NaCl style for multiline comments, with "/*" and "*/" on separate > lines, like this: > /* > * Blah... > * blah. > */ Done. https://codereview.chromium.org/1212613002/diff/1/src/untrusted/irt/irt.h File src/untrusted/irt/irt.h (right): https://codereview.chromium.org/1212613002/diff/1/src/untrusted/irt/irt.h#new... src/untrusted/irt/irt.h:244: #define NACL_IRT_THREAD_v0_2 "nacl-irt-thread-0.2" On 2015/06/26 18:32:00, Mark Seaborn wrote: > Please comment that this is implemented for Non-SFI NaCl only. Done. https://codereview.chromium.org/1212613002/diff/1/src/untrusted/irt/irt.h#new... src/untrusted/irt/irt.h:247: * Now this interface returns the thread ID of the child thread as On 2015/06/26 18:31:59, Mark Seaborn wrote: > Nit: It assigns it to *child_tid rather than returning it. Please document when > it does the assignment. Done. https://codereview.chromium.org/1212613002/diff/1/src/untrusted/irt/irt.h#new... src/untrusted/irt/irt.h:406: #define NACL_IRT_SIGNAL_HANDLING_v0_1 "nacl-irt-signal-handling-0.1" On 2015/06/26 18:31:59, Mark Seaborn wrote: > Please comment that this is implemented for Non-SFI NaCl only (just as > nacl_irt_icache is). For that reason, it should probably go at the end of the > file, next to nacl_irt_icache. Done. https://codereview.chromium.org/1212613002/diff/1/src/untrusted/irt/irt.h#new... src/untrusted/irt/irt.h:408: struct nacl_irt_signal_handling { On 2015/06/26 18:31:59, Mark Seaborn wrote: > Let's add "async" into the name to make it clear that this is for async signals > only. Done. https://codereview.chromium.org/1212613002/diff/1/src/untrusted/pthread/nc_th... File src/untrusted/pthread/nc_thread.c (right): https://codereview.chromium.org/1212613002/diff/1/src/untrusted/pthread/nc_th... src/untrusted/pthread/nc_thread.c:419: retval = __libnacl_irt_thread.thread_create(nc_thread_starter, esp, new_tp, On 2015/06/26 18:32:00, Mark Seaborn wrote: > This isn't going to work for SFI NaCl, because you're making this depend on > nacl_irt_thread_v0_2, which isn't defined for SFI NaCl. > > I'm not sure if this change passes on the trybots? (Can you figure out how to > get trybot access?) > > I don't think it's really necessary to add support for TIDs to > src/untrusted/pthread/ (i.e. to nacl-newlib's libpthread), because in the NaCl > codebase we *should* only need access to them for testing. > > See my other comment on the test code. Done. https://codereview.chromium.org/1212613002/diff/1/src/untrusted/pthread/nc_th... src/untrusted/pthread/nc_thread.c:421: (*thread_id)->native_tid = native_tid; On 2015/06/26 18:32:00, Mark Seaborn wrote: > BTW, assigning to (*thread_id)->native_tid as a separate step defeats the > purpose of having thread_create() set the TID rather than having a separate > gettid() syscall, which is to make both the assignment and the use of the TID > thread-safe. > > Doing the assignment here means that it's not thread-safe for the child thread > to cause pthread_kill(child_thread, sig) to be called. > > Also, the thread could have exited by this point, so (*thread_id)->native_tid > might point to deallocated memory. > > The idea would have been to pass &(*thread_id)->native_tid to thread_create(). Done. https://codereview.chromium.org/1212613002/diff/1/tests/signal_handler/nacl.s... File tests/signal_handler/nacl.scons (right): https://codereview.chromium.org/1212613002/diff/1/tests/signal_handler/nacl.s... tests/signal_handler/nacl.scons:8: if env.Bit('nonsfi_nacl'): On 2015/06/26 18:32:00, Mark Seaborn wrote: > Can you put this test in tests/nonsfi/, please? I think that would be more > appropriate. > > tests/signal_handler/ is really for SFI NaCl's hardware exception handling, > which is quite different from this async signal facility. Done. https://codereview.chromium.org/1212613002/diff/1/tests/signal_handler/nacl.s... tests/signal_handler/nacl.scons:12: 'user_signal_test.cc', On 2015/06/26 18:32:00, Mark Seaborn wrote: > Can you add "async" to the test names? Done. https://codereview.chromium.org/1212613002/diff/1/tests/signal_handler/user_s... File tests/signal_handler/user_signal_test.cc (right): https://codereview.chromium.org/1212613002/diff/1/tests/signal_handler/user_s... tests/signal_handler/user_signal_test.cc:35: * Check that nacl_tls_get() is signal-async-safe. On 2015/06/26 18:32:00, Mark Seaborn wrote: > Nit: "async-signal-safe" Done. https://codereview.chromium.org/1212613002/diff/1/tests/signal_handler/user_s... tests/signal_handler/user_signal_test.cc:42: sem_post(&g_sem); On 2015/06/26 18:32:00, Mark Seaborn wrote: > Nit: check return value on all calls (sem_post(), pthread_join() etc.) Done. https://codereview.chromium.org/1212613002/diff/1/tests/signal_handler/user_s... tests/signal_handler/user_signal_test.cc:66: nacl_signal_send_async_signal(tid->native_tid); On 2015/06/26 18:32:00, Mark Seaborn wrote: > Using tid->native_tid here is unfortunate because it requires our libpthread to > support saving the TID, which we don't really need apart from for this test. > > Here's an alternative: In main(), modify __libnacl_irt_thread.thread_create to > point to a wrapper function like this one: > > static __thread int native_tid; > > int thread_create_wrapper(func, stack, thread_ptr) { > /* Trick to figure out address of thread-local var in the new thread. */ > uintptr_t var_offset = (uintptr_t) &native_tid - get_tls(); > uintptr_t new_tid_ptr = (uintptr_t) thread_ptr + var_offset; > > return thread_create_v2(func, stack, thread_ptr, new_tid_ptr); > } The trick was not needed, since I need to be able to read the native_tid from the caller thread, not the thread itself.
All right, everything works reliably now and tests are passing. Please take a look.
There is one spot in linux_clone_wrapper (linux_sys_private.c) where some elements in the stack above esp are being accessed. Since we don't have an alternate stack for signal delivery, this overwrites that slot if a signal is delivered just at the right time. Should we try to fix that code or is it better to have an alternate signal stack?
On 13 July 2015 at 11:14, <lhchavez@chromium.org> wrote: > There is one spot in linux_clone_wrapper (linux_sys_private.c) where some > elements in the stack above esp are being accessed. Since we don't have an > alternate stack for signal delivery, this overwrites that slot if a signal > is > delivered just at the right time. > Thanks for catching that! You mean below rather above, right? I think you're referring to "call <http://localhost:8000/search?dir=nacl&sym=call> *-4 <http://localhost:8000/search?dir=nacl&sym=4>(%%esp <http://localhost:8000/search?dir=nacl&sym=esp>)\n <http://localhost:8000/search?dir=nacl&sym=n>". > Should we try to fix that code or is it better to have an alternate signal > stack? Might as well fix it, since it would be simple enough to fix. Cheers, Mark -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
On 2015/07/13 18:42:27, Mark Seaborn wrote: > On 13 July 2015 at 11:14, <mailto:lhchavez@chromium.org> wrote: > > > There is one spot in linux_clone_wrapper (linux_sys_private.c) where some > > elements in the stack above esp are being accessed. Since we don't have an > > alternate stack for signal delivery, this overwrites that slot if a signal > > is > > delivered just at the right time. > > > > Thanks for catching that! > > You mean below rather above, right? I think you're referring to "call > <http://localhost:8000/search?dir=nacl&sym=call> *-4 > <http://localhost:8000/search?dir=nacl&sym=4%3E(%25%25esp > <http://localhost:8000/search?dir=nacl&sym=esp%3E)%5Cn > <http://localhost:8000/search?dir=nacl&sym=n%3E%22. Yes, sorry. > > > > Should we try to fix that code or is it better to have an alternate signal > > stack? > > > Might as well fix it, since it would be simple enough to fix. This is now fixed and the tests seem to be passing. > > Cheers, > Mark > > -- > You received this message because you are subscribed to the Google Groups > "Native-Client-Reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:native-client-reviews+unsubscribe@googlegroups.com. > To post to this group, send email to mailto:native-client-reviews@googlegroups.com. > Visit this group at http://groups.google.com/group/native-client-reviews. > For more options, visit https://groups.google.com/d/optout.
On 13 July 2015 at 11:42, Mark Seaborn <mseaborn@chromium.org> wrote: > On 13 July 2015 at 11:14, <lhchavez@chromium.org> wrote: > >> There is one spot in linux_clone_wrapper (linux_sys_private.c) where some >> elements in the stack above esp are being accessed. Since we don't have an >> alternate stack for signal delivery, this overwrites that slot if a >> signal is >> delivered just at the right time. >> > > Thanks for catching that! > > You mean below rather above, right? I think you're referring to "call > <http://localhost:8000/search?dir=nacl&sym=call> *-4 > <http://localhost:8000/search?dir=nacl&sym=4>(%%esp > <http://localhost:8000/search?dir=nacl&sym=esp>)\n > <http://localhost:8000/search?dir=nacl&sym=n>". > Oops, that quoted code got linkified. That should have been "call *-4(%%esp)\n". Mark -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
Seem to me like the ARM bot has failed a test, did you take a look ? http://build.chromium.org/p/tryserver.nacl/builders/nacl-arm_hw_opt_panda/bui...
On 2015/07/15 11:35:55, Junichi Uekawa wrote: > Seem to me like the ARM bot has failed a test, did you take a look ? > > http://build.chromium.org/p/tryserver.nacl/builders/nacl-arm_hw_opt_panda/bui... also, could you test that ARC crash dump / reporting still works ? something like launch_chrome run --chrome-binary=xxx JniCrash.apk (found in Arc drive) should work
These comments are mostly just about code organisation... https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/linux/irt_except... File src/nonsfi/linux/irt_exception_handling.c (left): https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/linux/irt_except... src/nonsfi/linux/irt_exception_handling.c:23: typedef struct compat_sigaltstack { On 2015/07/06 23:44:59, lhc wrote: > On 2015/06/26 18:31:59, Mark Seaborn wrote: > > If you're moving large chunks of code around, would you mind > > summarising what's moved in the commit message, to make > > review easier? > > Done. Hmm, I don't see a summary in the commit message... https://codereview.chromium.org/1212613002/diff/80001/src/include/nacl/nacl_s... File src/include/nacl/nacl_signal.h (right): https://codereview.chromium.org/1212613002/diff/80001/src/include/nacl/nacl_s... src/include/nacl/nacl_signal.h:17: For documentation, could you add a text file in documentation/ (e.g. nonsfi_mode_async_signals.txt) that gives just a quick overview of the new feature and its status. i.e.: * This is for Non-SFI mode only. * It treats async signals entirely separate from hardware exceptions (which are synchronous). * There is no notion of signal masks or multiple signal types. These must be implemented in user mode, on top of the IRT interfaces provided. * There is no equivalent of sigaltstack() for async signals yet. * It adds the concepts of: * thread IDs (TIDs) * async signal safety * In contrast to POSIX's async signals, this is not intended to abort any in-progress operations (such as syscalls, IRT calls or PPAPI calls). * We don't provide any libc wrapper functions for this interface in libnacl at the moment. * Link to https://code.google.com/p/nativeclient/issues/detail?id=4065 We've tended to have a documentation deficit for NaCl, and I'm trying to avoid continuing that for new features. https://codereview.chromium.org/1212613002/diff/80001/src/include/nacl/nacl_s... src/include/nacl/nacl_signal.h:22: * Sets the global signal handler. Generally, everywhere you say "signal" (in comments and code), can you say "async signal" to clarify, please? (Except where there's a signal handler that really does cover both sync and async signals.) https://codereview.chromium.org/1212613002/diff/80001/src/include/nacl/nacl_s... src/include/nacl/nacl_signal.h:24: * NaCl applications can register a single, global signal handler that will be These comments are duplicating those in irt.h. How about merging the two sets of comments, and just referring to irt.h here? https://codereview.chromium.org/1212613002/diff/80001/src/include/nacl/nacl_s... src/include/nacl/nacl_signal.h:33: int nacl_signal_set_handler(nacl_signal_handler_t handler); The way you've currently got these functions defined, they are IRT-internal functions -- i.e. there's no definition for linking into a user nexe. Which is fine, but it means this .h doesn't belong in src/include/nacl/. Putting it in src/nonsfi/linux/ next to the definitions of these functions would be more appropriate. For comparison: src/include/nacl/nacl_exception.h declares functions that have definitions in src/untrusted/nacl/nacl_exception.c that can be linked into user nexes. https://codereview.chromium.org/1212613002/diff/80001/src/nonsfi/irt/irt_inte... File src/nonsfi/irt/irt_interfaces.c (left): https://codereview.chromium.org/1212613002/diff/80001/src/nonsfi/irt/irt_inte... src/nonsfi/irt/irt_interfaces.c:361: #if defined(__native_client__) By removing this, you're changing NACL_IRT_THREAD_v0_1 back to ignoring the stack provided and allocating its own. That's a bit icky. It would be better if the only difference between NACL_IRT_THREAD_v0_{1,2} is whether the TID can optionally be set. https://codereview.chromium.org/1212613002/diff/80001/src/untrusted/irt/irt.h File src/untrusted/irt/irt.h (right): https://codereview.chromium.org/1212613002/diff/80001/src/untrusted/irt/irt.h... src/untrusted/irt/irt.h:255: nacl_irt_tid_t *child_tid); Can the child_tid pointer be NULL? Please clarify in comment. I think it should be allowed to be NULL. https://codereview.chromium.org/1212613002/diff/80001/src/untrusted/pthread/p... File src/untrusted/pthread/pthread_types.h (right): https://codereview.chromium.org/1212613002/diff/80001/src/untrusted/pthread/p... src/untrusted/pthread/pthread_types.h:76: pid_t native_tid; Not used now https://codereview.chromium.org/1212613002/diff/80001/tests/nonsfi/nacl.scons File tests/nonsfi/nacl.scons (right): https://codereview.chromium.org/1212613002/diff/80001/tests/nonsfi/nacl.scons... tests/nonsfi/nacl.scons:26: if env.Bit('tests_use_irt'): By putting the test after this, you're not testing the interface that the IRT provides. You actually want to declare your test inside an "if env.Bit('tests_use_irt')" block. https://codereview.chromium.org/1212613002/diff/80001/tests/nonsfi/nacl.scons... tests/nonsfi/nacl.scons:35: sel_ldr_flags=['-S'], exit_status='0', These args shouldn't be needed https://codereview.chromium.org/1212613002/diff/80001/tests/nonsfi/nacl.scons... tests/nonsfi/nacl.scons:36: using_nacl_signal_handler=True) Ditto https://codereview.chromium.org/1212613002/diff/80001/tests/nonsfi/user_async... File tests/nonsfi/user_async_signal_test.cc (right): https://codereview.chromium.org/1212613002/diff/80001/tests/nonsfi/user_async... tests/nonsfi/user_async_signal_test.cc:20: extern struct nacl_irt_thread_v0_2 __libnacl_irt_thread_v0_2; Use the appropriate #include instead of an extern here https://codereview.chromium.org/1212613002/diff/80001/tests/nonsfi/user_async... tests/nonsfi/user_async_signal_test.cc:25: void* g_expected_tls; Nit: "*" spacing https://codereview.chromium.org/1212613002/diff/80001/tests/nonsfi/user_async... tests/nonsfi/user_async_signal_test.cc:234: nacl_signal_set_handler(main_signal_handler); By calling this function, you're not testing that the IRT actually provides your new "nacl_irt_async_signal_handling" interface. Instead, you should use nacl_interface_query() and call the function pointers it returns. https://codereview.chromium.org/1212613002/diff/80001/tests/nonsfi/user_async... tests/nonsfi/user_async_signal_test.cc:251: * native tid, mock that functionality so the tid is stored in a thread-local Nit: "mock" -> "wrap"
On 15 July 2015 at 13:49, Luis Hector Chavez <lhchavez@google.com> wrote: > Hi Mark, > > At first I thought it was a flake, but it looks pretty consistent. Can you > point me to a doc that tells me how to set up a test environment to > reproduce this issue? > > On Wed, Jul 15, 2015 at 4:35 AM, <uekawa@chromium.org> wrote: > >> Seem to me like the ARM bot has failed a test, did you take a look ? >> >> >> http://build.chromium.org/p/tryserver.nacl/builders/nacl-arm_hw_opt_panda/bui... > > That's a timeout in "run_thread_suspension_test". That tests SFI NaCl. Your change shouldn't affect that, so I don't think the failure is caused by your change. The ARM hardware bots can be rather flaky. It might be that the hardware is unreliable -- they are fairly old ARM boards, running fairly old versions of the Linux kernel. dschuff@ set them up; you could ask him. You should be able to run run_thread_suspension_test on any ARM Linux system though. Do you want any pointers on how to do that? Cheers, Mark -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
Yes, please. I just want to make sure this is not breaking anything else. On Thu, Jul 16, 2015 at 4:39 PM, Mark Seaborn <mseaborn@chromium.org> wrote: > On 15 July 2015 at 13:49, Luis Hector Chavez <lhchavez@google.com> wrote: > >> Hi Mark, >> >> At first I thought it was a flake, but it looks pretty consistent. Can >> you point me to a doc that tells me how to set up a test environment to >> reproduce this issue? >> > > >> On Wed, Jul 15, 2015 at 4:35 AM, <uekawa@chromium.org> wrote: >> >>> Seem to me like the ARM bot has failed a test, did you take a look ? >>> >>> >>> http://build.chromium.org/p/tryserver.nacl/builders/nacl-arm_hw_opt_panda/bui... >> >> > That's a timeout in "run_thread_suspension_test". That tests SFI NaCl. > Your change shouldn't affect that, so I don't think the failure is caused > by your change. > > The ARM hardware bots can be rather flaky. It might be that the hardware > is unreliable -- they are fairly old ARM boards, running fairly old > versions of the Linux kernel. dschuff@ set them up; you could ask him. > > You should be able to run run_thread_suspension_test on any ARM Linux > system though. Do you want any pointers on how to do that? > > Cheers, > Mark > > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
On 16 July 2015 at 16:42, Luis Hector Chavez <lhchavez@google.com> wrote: > Yes, please. I just want to make sure this is not breaking anything else. > One way to run the test on ARM, if you have an ARM device that you can SSH into, is to use the "run_test_via_ssh.py" helper script, which is documented in SConstruct: # test_wrapper specifies a wrapper program such as # tools/run_test_via_ssh.py, which runs tests on a remote host # using rsync and SSH. Example usage: # ./scons run_hello_world_test platform=arm force_emulator= \ # test_wrapper="./tools/run_test_via_ssh.py --host=armbox --subdir=tmp" Just replace "run_hello_world_test" with "run_thread_suspension_test". Cheers, Mark > On Thu, Jul 16, 2015 at 4:39 PM, Mark Seaborn <mseaborn@chromium.org> > wrote: > >> On 15 July 2015 at 13:49, Luis Hector Chavez <lhchavez@google.com> wrote: >> >>> Hi Mark, >>> >>> At first I thought it was a flake, but it looks pretty consistent. Can >>> you point me to a doc that tells me how to set up a test environment to >>> reproduce this issue? >>> >> >> >>> On Wed, Jul 15, 2015 at 4:35 AM, <uekawa@chromium.org> wrote: >>> >>>> Seem to me like the ARM bot has failed a test, did you take a look ? >>>> >>>> >>>> http://build.chromium.org/p/tryserver.nacl/builders/nacl-arm_hw_opt_panda/bui... >>> >>> >> That's a timeout in "run_thread_suspension_test". That tests SFI NaCl. >> Your change shouldn't affect that, so I don't think the failure is caused >> by your change. >> >> The ARM hardware bots can be rather flaky. It might be that the hardware >> is unreliable -- they are fairly old ARM boards, running fairly old >> versions of the Linux kernel. dschuff@ set them up; you could ask him. >> >> You should be able to run run_thread_suspension_test on any ARM Linux >> system though. Do you want any pointers on how to do that? >> >> Cheers, >> Mark >> >> > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/linux/irt_except... File src/nonsfi/linux/irt_exception_handling.c (left): https://codereview.chromium.org/1212613002/diff/1/src/nonsfi/linux/irt_except... src/nonsfi/linux/irt_exception_handling.c:23: typedef struct compat_sigaltstack { On 2015/07/16 23:21:16, Mark Seaborn wrote: > On 2015/07/06 23:44:59, lhc wrote: > > On 2015/06/26 18:31:59, Mark Seaborn wrote: > > > If you're moving large chunks of code around, would you mind > > > summarising what's moved in the commit message, to make > > > review easier? > > > > Done. > > Hmm, I don't see a summary in the commit message... I did not realize that `git cl upload` does not replace the commit message with the one provided to git. Updating manually. https://codereview.chromium.org/1212613002/diff/80001/src/include/nacl/nacl_s... File src/include/nacl/nacl_signal.h (right): https://codereview.chromium.org/1212613002/diff/80001/src/include/nacl/nacl_s... src/include/nacl/nacl_signal.h:17: On 2015/07/16 23:21:16, Mark Seaborn wrote: > For documentation, could you add a text file in documentation/ (e.g. > nonsfi_mode_async_signals.txt) that gives just a quick overview of the new > feature and its status. i.e.: > > * This is for Non-SFI mode only. > > * It treats async signals entirely separate from hardware exceptions (which are > synchronous). > > * There is no notion of signal masks or multiple signal types. These must be > implemented in user mode, on top of the IRT interfaces provided. > > * There is no equivalent of sigaltstack() for async signals yet. > > * It adds the concepts of: > * thread IDs (TIDs) > * async signal safety > > * In contrast to POSIX's async signals, this is not intended to abort any > in-progress operations (such as syscalls, IRT calls or PPAPI calls). > > * We don't provide any libc wrapper functions for this interface in libnacl at > the moment. > > * Link to https://code.google.com/p/nativeclient/issues/detail?id=4065 > > We've tended to have a documentation deficit for NaCl, and I'm trying to avoid > continuing that for new features. Done. https://codereview.chromium.org/1212613002/diff/80001/src/include/nacl/nacl_s... src/include/nacl/nacl_signal.h:22: * Sets the global signal handler. On 2015/07/16 23:21:16, Mark Seaborn wrote: > Generally, everywhere you say "signal" (in comments and code), can you say > "async signal" to clarify, please? > > (Except where there's a signal handler that really does cover both sync and > async signals.) Merged the docs with unrusted/irt/irt.h https://codereview.chromium.org/1212613002/diff/80001/src/include/nacl/nacl_s... src/include/nacl/nacl_signal.h:24: * NaCl applications can register a single, global signal handler that will be On 2015/07/16 23:21:16, Mark Seaborn wrote: > These comments are duplicating those in irt.h. How about merging the two sets > of comments, and just referring to irt.h here? Done. https://codereview.chromium.org/1212613002/diff/80001/src/include/nacl/nacl_s... src/include/nacl/nacl_signal.h:33: int nacl_signal_set_handler(nacl_signal_handler_t handler); On 2015/07/16 23:21:16, Mark Seaborn wrote: > The way you've currently got these functions defined, they are IRT-internal > functions -- i.e. there's no definition for linking into a user nexe. > > Which is fine, but it means this .h doesn't belong in src/include/nacl/. > Putting it in src/nonsfi/linux/ next to the definitions of these functions would > be more appropriate. > > For comparison: src/include/nacl/nacl_exception.h declares functions that have > definitions in src/untrusted/nacl/nacl_exception.c that can be linked into user > nexes. Done. https://codereview.chromium.org/1212613002/diff/80001/src/nonsfi/irt/irt_inte... File src/nonsfi/irt/irt_interfaces.c (left): https://codereview.chromium.org/1212613002/diff/80001/src/nonsfi/irt/irt_inte... src/nonsfi/irt/irt_interfaces.c:361: #if defined(__native_client__) On 2015/07/16 23:21:16, Mark Seaborn wrote: > By removing this, you're changing NACL_IRT_THREAD_v0_1 back to ignoring the > stack provided and allocating its own. That's a bit icky. It would be better > if the only difference between NACL_IRT_THREAD_v0_{1,2} is whether the TID can > optionally be set. Done. https://codereview.chromium.org/1212613002/diff/80001/src/untrusted/irt/irt.h File src/untrusted/irt/irt.h (right): https://codereview.chromium.org/1212613002/diff/80001/src/untrusted/irt/irt.h... src/untrusted/irt/irt.h:255: nacl_irt_tid_t *child_tid); On 2015/07/16 23:21:16, Mark Seaborn wrote: > Can the child_tid pointer be NULL? Please clarify in comment. I think it > should be allowed to be NULL. Done. https://codereview.chromium.org/1212613002/diff/80001/src/untrusted/pthread/p... File src/untrusted/pthread/pthread_types.h (right): https://codereview.chromium.org/1212613002/diff/80001/src/untrusted/pthread/p... src/untrusted/pthread/pthread_types.h:76: pid_t native_tid; On 2015/07/16 23:21:16, Mark Seaborn wrote: > Not used now Done. https://codereview.chromium.org/1212613002/diff/80001/tests/nonsfi/nacl.scons File tests/nonsfi/nacl.scons (right): https://codereview.chromium.org/1212613002/diff/80001/tests/nonsfi/nacl.scons... tests/nonsfi/nacl.scons:26: if env.Bit('tests_use_irt'): On 2015/07/16 23:21:16, Mark Seaborn wrote: > By putting the test after this, you're not testing the interface that the IRT > provides. > > You actually want to declare your test inside an "if env.Bit('tests_use_irt')" > block. Done. https://codereview.chromium.org/1212613002/diff/80001/tests/nonsfi/nacl.scons... tests/nonsfi/nacl.scons:35: sel_ldr_flags=['-S'], exit_status='0', On 2015/07/16 23:21:17, Mark Seaborn wrote: > These args shouldn't be needed Done. https://codereview.chromium.org/1212613002/diff/80001/tests/nonsfi/nacl.scons... tests/nonsfi/nacl.scons:36: using_nacl_signal_handler=True) On 2015/07/16 23:21:16, Mark Seaborn wrote: > Ditto Done. https://codereview.chromium.org/1212613002/diff/80001/tests/nonsfi/user_async... File tests/nonsfi/user_async_signal_test.cc (right): https://codereview.chromium.org/1212613002/diff/80001/tests/nonsfi/user_async... tests/nonsfi/user_async_signal_test.cc:20: extern struct nacl_irt_thread_v0_2 __libnacl_irt_thread_v0_2; On 2015/07/16 23:21:17, Mark Seaborn wrote: > Use the appropriate #include instead of an extern here Done. https://codereview.chromium.org/1212613002/diff/80001/tests/nonsfi/user_async... tests/nonsfi/user_async_signal_test.cc:25: void* g_expected_tls; On 2015/07/16 23:21:17, Mark Seaborn wrote: > Nit: "*" spacing Done. https://codereview.chromium.org/1212613002/diff/80001/tests/nonsfi/user_async... tests/nonsfi/user_async_signal_test.cc:234: nacl_signal_set_handler(main_signal_handler); On 2015/07/16 23:21:17, Mark Seaborn wrote: > By calling this function, you're not testing that the IRT actually provides your > new "nacl_irt_async_signal_handling" interface. > > Instead, you should use nacl_interface_query() and call the function pointers it > returns. Done. https://codereview.chromium.org/1212613002/diff/80001/tests/nonsfi/user_async... tests/nonsfi/user_async_signal_test.cc:251: * native tid, mock that functionality so the tid is stored in a thread-local On 2015/07/16 23:21:17, Mark Seaborn wrote: > Nit: "mock" -> "wrap" Done.
On 2015/07/15 22:27:14, Junichi Uekawa wrote: > On 2015/07/15 11:35:55, Junichi Uekawa wrote: > > Seem to me like the ARM bot has failed a test, did you take a look ? > > > > > http://build.chromium.org/p/tryserver.nacl/builders/nacl-arm_hw_opt_panda/bui... > > also, could you test that ARC crash dump / reporting still works ? > > something like > launch_chrome run --chrome-binary=xxx JniCrash.apk (found in Arc drive) > should work Minidump was still generated and breakpad could correctly walk the stacks until it hit an ART frame.
Tests are passing, please take a look. The ARM bot did not fail at the same step as before, so it was probably a flake.
https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.h File src/untrusted/irt/irt.h (right): https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:253: * |parent_tid| argument to the clone() system call. |child_tid| can be 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.
https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.h File src/untrusted/irt/irt.h (right): https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:253: * |parent_tid| argument to the clone() system call. |child_tid| can be On 2015/07/21 23:36:03, Junichi Uekawa wrote: > 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. 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.
On 2015/07/21 23:42:11, lhc wrote: > https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.h > File src/untrusted/irt/irt.h (right): > > https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... > src/untrusted/irt/irt.h:253: * |parent_tid| argument to the clone() system call. > |child_tid| can be > On 2015/07/21 23:36:03, Junichi Uekawa wrote: > > 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. > > 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. Ah I see. ptid stores tid in the parent memory space (in do_fork), and ctid stores tid in child memory space (in schedule_tail via copy_process),
On 2015/07/20 20:24:58, lhc wrote: > Tests are passing, please take a look. The ARM bot did not fail at the same step > as before, so it was probably a flake. ping?
This is getting closer. The comments below are mostly minor, except for handling of SSE/FP registers (which you can do separately) and EINTR. https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... File documentation/nonsfi_mode_async_signals.txt (right): https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:6: This provides a way to asynchronously send a notification to another thread in How about "asynchronously interrupt another thread"? That's stronger than "send a notification". https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:9: Can you add "see nacl_irt_async_signal_handling in src/untrusted/irt/irt.h" to make it clear what interface you're referring to? https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:10: Async signals have several differences with POSIX signals: Nit: "differences from"? https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:12: * Hardware/synchonous signals are separate and their behavior is not changed. "synchronous" Maybe say "Synchronous signals (from hardware exceptions)" to make it clear that they're the same thing. Otherwise "Hardware/synchonous signals" could refer to two types. https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:18: This means that the signal handler must be reentrant. This sentence is not necessarily true. User code might only send a signal once. Or it might use some mutual exclusion to ensure that the body of the handler never gets interrupted. https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:22: moment. If full POSIX support is needed, it can be implemented in userspace, Nit: "user space" -> "user code" https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:26: return EINTR, but users must not assume this will always be true. No, the intent is you *shouldn't* get EINTR. Are there any cases where the current implementation gives EINTR? See my other comment in tests/nonsfi/user_async_signal_test.cc... https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:36: is undefined. NaCl only guarantees that the following functions are "functions" -> "IRT interfaces" https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:39: * tls_get Can you add "()" to these? https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:47: parameter. Since the main thread is not created using nacl_irt_thread, the Nit: "main thread" -> "initial thread" https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:49: identifier can be used as a parameter to send_async_signal. Providing a thread Nit: Add "()" to "send_async_signal()" https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/irt/irt_int... File src/nonsfi/irt/irt_interfaces.c (right): https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/irt/irt_int... src/nonsfi/irt/irt_interfaces.c:48: # include "native_client/src/public/nonsfi/irt_signal_handling.h" This one is not needed, I think? https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/irt/irt_int... src/nonsfi/irt/irt_interfaces.c:362: void *thread_ptr, nacl_irt_tid_t *child_tid) { Nit: align indentation https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_e... File src/nonsfi/linux/irt_exception_handling.c (right): https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_e... src/nonsfi/linux/irt_exception_handling.c:17: #include "native_client/src/nonsfi/linux/nonsfi_signal.h" Nit: sort #includes https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_e... src/nonsfi/linux/irt_exception_handling.c:35: extern pthread_mutex_t g_signal_handler_mutex; Not needed? Use the header file's decl instead. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_e... src/nonsfi/linux/irt_exception_handling.c:38: /* Signal handlers, responsible for calling the registered handlers. */ Use singular "handler", in both cases? https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... File src/nonsfi/linux/irt_signal_handling.c (right): https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... src/nonsfi/linux/irt_signal_handling.c:31: Nit: be consistent about whether you have 1 or 2 empty lines between top-level decl https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... src/nonsfi/linux/irt_signal_handling.c:34: struct linux_ucontext_t { You've moved a lot of this from src/nonsfi/linux/irt_exception_handling.c. How about moving the rest of irt_exception_handling.c into this file too? Then that will show up as renaming irt_exception_handling.c to irt_signal_handling.c. More definitions can stay as "static". There's a lot of overlap between these two pieces of functionality -- I don't think having two separate files is necessary. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... src/nonsfi/linux/irt_signal_handling.c:132: /* Restore floating-point environment */ What about restoring SSE/AVX registers? I think they're all non-callee-saved. I would suggest you do this as a follow-on change. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... src/nonsfi/linux/irt_signal_handling.c:207: /* Restore flags */ What about FP registers too? Some of them are non-callee-saved. I would suggest you do this as a follow-on change. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... src/nonsfi/linux/irt_signal_handling.c:212: "ldmia r14, {r0-r10}\n" How about "{r0-pc}"? Then you shouldn't need the further code below. On ARM it's really easy to restore all the general purpose registers (+ PC) in one instruction. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... src/nonsfi/linux/irt_signal_handling.c:299: g_tgid = getpid(); You should check for errors in these two syscalls, since they could easily be blocked by seccomp-bpf. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... File src/nonsfi/linux/irt_signal_handling.h (right): https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... src/nonsfi/linux/irt_signal_handling.h:18: int nacl_signal_set_handler(NaClIrtSignalHandler handler); The names in irt.h have "async" in them. Could you add that here too? https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/linux... File src/nonsfi/linux/linux_pthread_private.c (right): https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/linux... src/nonsfi/linux/linux_pthread_private.c:21: struct nacl_irt_thread_v0_2 __libnacl_irt_thread_v0_2; I don't think you need this, or the assignment to it below. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/nonsf... File src/nonsfi/linux/nonsfi_signal.h (right): https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/nonsf... src/nonsfi/linux/nonsfi_signal.h:85: void exception_frame_from_signal_context( How about prefixing this with "nonsfi_" too? https://codereview.chromium.org/1212613002/diff/120001/src/public/nonsfi/irt_... File src/public/nonsfi/irt_exception_handling.h (left): https://codereview.chromium.org/1212613002/diff/120001/src/public/nonsfi/irt_... src/public/nonsfi/irt_exception_handling.h:15: void nonsfi_initialize_signal_handler(void); Removing this from src/public/ would break the DEPS roll into Chromium. This is referred to by components/nacl/loader/nacl_helper_linux.cc. Please stage the change so that a trivial DEPS roll will work. How about just keeping the function name and header file name the same? https://codereview.chromium.org/1212613002/diff/120001/src/public/nonsfi/irt_... File src/public/nonsfi/irt_signal_handling.h (right): https://codereview.chromium.org/1212613002/diff/120001/src/public/nonsfi/irt_... src/public/nonsfi/irt_signal_handling.h:15: * Initialize signal handler for exceptions and regular signals at startup What does "regular signals" mean? https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.h File src/untrusted/irt/irt.h (right): https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:251: * Now this interface assigns the thread ID of the child thread into 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...". https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:252: * |child_tid| atomically when the thread is created. It will be the 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.) https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:253: * |parent_tid| argument to the clone() system call. |child_tid| can be 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". https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:459: * This interface is only available on Non-SFI Mode. Can you refer back to nonsfi_mode_async_signals.txt somewhere in the comments for this interface? https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:462: typedef void (*NaClIrtSignalHandler)(struct NaClExceptionContext *context); 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 https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:469: * mask (as opposed to the POSIX behavior), so |handler| must be reentrant. See my comment in nonsfi_mode_async_signals.txt about re-entrancy. https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:489: * the same thread group as the caller. This function requires "Thread group" is a Linux concept that shouldn't be referenced here. https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:490: * nacl_signal_set_handler to be called first, otherwise it will fail with "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? https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:492: * thread (through the |parent_tid| parameter) and the thread must still be You mean child_tid. https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/nacl/nac... File src/untrusted/nacl/nacl_irt.h (right): https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/nacl/nac... src/untrusted/nacl/nacl_irt.h:30: extern struct nacl_irt_thread_v0_2 __libnacl_irt_thread_v0_2; Not needed -- see my comment in linux_pthread_private.c. https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/nacl.scons File tests/nonsfi/nacl.scons (right): https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/nacl.scon... tests/nonsfi/nacl.scons:30: '${EXCEPTION_LIBS}']) I don't think this executable uses EXCEPTION_LIBS. https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... File tests/nonsfi/user_async_signal_test.cc (right): https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:11: #include "native_client/src/nonsfi/linux/irt_signal_handling.h" Shouldn't be needed now -- this is an IRT-internal header. https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:12: #include "native_client/src/public/nonsfi/irt_signal_handling.h" Also shouldn't be needed now -- this is an IRT-internal header. https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:14: #include "native_client/src/untrusted/nacl/nacl_irt.h" Not needed now? https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:16: #include "native_client/src/untrusted/pthread/pthread_types.h" Not needed now? https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:20: struct nacl_irt_thread_v0_2 __libnacl_irt_thread_v0_2; You shouldn't need the "__" prefix here (or in the next decl). Can you also put all these definitions in a top-level anon namespace (which would replace uses of "static")? Then we get warnings about unused decls. https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:48: void test_send_signal_before_init() { How about "before_set_handler", since there is no public "init" function? https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:109: * handler, so it did not actually counts as a wakeup. "count" https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:135: ASSERT_EQ(retval, EINTR); Is the current implementation giving us EINTR? I don't think we want that. The idea is that user code should not have to retry syscalls. That's a common source of bugs, when programmers don't put a retry loop around a syscall that can be interrupted. Should we be registering signal handlers with SA_RESTART? https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:282: * opposed to interrupting it), which breaks this test. Do you mean the futex_wait syscall doesn't get interrupted at all? Does this problem only occur under QEMU? If so, can you say that in the comment, please?
https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... File documentation/nonsfi_mode_async_signals.txt (right): https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:6: This provides a way to asynchronously send a notification to another thread in On 2015/08/12 01:43:06, Mark Seaborn wrote: > How about "asynchronously interrupt another thread"? That's stronger than "send > a notification". Done. https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:9: On 2015/08/12 01:43:06, Mark Seaborn wrote: > Can you add "see nacl_irt_async_signal_handling in src/untrusted/irt/irt.h" to > make it clear what interface you're referring to? Done. https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:10: Async signals have several differences with POSIX signals: On 2015/08/12 01:43:06, Mark Seaborn wrote: > Nit: "differences from"? Done. https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:12: * Hardware/synchonous signals are separate and their behavior is not changed. On 2015/08/12 01:43:06, Mark Seaborn wrote: > "synchronous" > > Maybe say "Synchronous signals (from hardware exceptions)" to make it clear that > they're the same thing. Otherwise "Hardware/synchonous signals" could refer to > two types. Done. https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:18: This means that the signal handler must be reentrant. On 2015/08/12 01:43:06, Mark Seaborn wrote: > This sentence is not necessarily true. User code might only send a signal once. > Or it might use some mutual exclusion to ensure that the body of the handler > never gets interrupted. Removed https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:22: moment. If full POSIX support is needed, it can be implemented in userspace, On 2015/08/12 01:43:06, Mark Seaborn wrote: > Nit: "user space" -> "user code" Done. https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:26: return EINTR, but users must not assume this will always be true. On 2015/08/12 01:43:06, Mark Seaborn wrote: > No, the intent is you *shouldn't* get EINTR. > > Are there any cases where the current implementation gives EINTR? > > See my other comment in tests/nonsfi/user_async_signal_test.cc... Yes: futex_wait (with non-null timeout) and nanosleep (the two things I use in the test). I reworded this to mention that the norm is that you don't normally get EINTR except in the two cases above. https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:36: is undefined. NaCl only guarantees that the following functions are On 2015/08/12 01:43:06, Mark Seaborn wrote: > "functions" -> "IRT interfaces" Done. https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:39: * tls_get On 2015/08/12 01:43:06, Mark Seaborn wrote: > Can you add "()" to these? Done. https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:47: parameter. Since the main thread is not created using nacl_irt_thread, the On 2015/08/12 01:43:06, Mark Seaborn wrote: > Nit: "main thread" -> "initial thread" Done. https://codereview.chromium.org/1212613002/diff/120001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:49: identifier can be used as a parameter to send_async_signal. Providing a thread On 2015/08/12 01:43:06, Mark Seaborn wrote: > Nit: Add "()" to "send_async_signal()" Done. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/irt/irt_int... File src/nonsfi/irt/irt_interfaces.c (right): https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/irt/irt_int... src/nonsfi/irt/irt_interfaces.c:48: # include "native_client/src/public/nonsfi/irt_signal_handling.h" On 2015/08/12 01:43:06, Mark Seaborn wrote: > This one is not needed, I think? Done. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/irt/irt_int... src/nonsfi/irt/irt_interfaces.c:362: void *thread_ptr, nacl_irt_tid_t *child_tid) { On 2015/08/12 01:43:07, Mark Seaborn wrote: > Nit: align indentation Done. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_e... File src/nonsfi/linux/irt_exception_handling.c (right): https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_e... src/nonsfi/linux/irt_exception_handling.c:17: #include "native_client/src/nonsfi/linux/nonsfi_signal.h" On 2015/08/12 01:43:07, Mark Seaborn wrote: > Nit: sort #includes Done. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_e... src/nonsfi/linux/irt_exception_handling.c:35: extern pthread_mutex_t g_signal_handler_mutex; On 2015/08/12 01:43:07, Mark Seaborn wrote: > Not needed? Use the header file's decl instead. Done. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_e... src/nonsfi/linux/irt_exception_handling.c:38: /* Signal handlers, responsible for calling the registered handlers. */ On 2015/08/12 01:43:07, Mark Seaborn wrote: > Use singular "handler", in both cases? Done. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... File src/nonsfi/linux/irt_signal_handling.c (right): https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... src/nonsfi/linux/irt_signal_handling.c:31: On 2015/08/12 01:43:07, Mark Seaborn wrote: > Nit: be consistent about whether you have 1 or 2 empty lines between top-level > decl Done. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... src/nonsfi/linux/irt_signal_handling.c:34: struct linux_ucontext_t { On 2015/08/12 01:43:07, Mark Seaborn wrote: > You've moved a lot of this from src/nonsfi/linux/irt_exception_handling.c. How > about moving the rest of irt_exception_handling.c into this file too? > > Then that will show up as renaming irt_exception_handling.c to > irt_signal_handling.c. More definitions can stay as "static". There's a lot of > overlap between these two pieces of functionality -- I don't think having two > separate files is necessary. Done. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... src/nonsfi/linux/irt_signal_handling.c:132: /* Restore floating-point environment */ On 2015/08/12 01:43:07, Mark Seaborn wrote: > What about restoring SSE/AVX registers? I think they're all non-callee-saved. > > I would suggest you do this as a follow-on change. Acknowledged. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... src/nonsfi/linux/irt_signal_handling.c:207: /* Restore flags */ On 2015/08/12 01:43:07, Mark Seaborn wrote: > What about FP registers too? Some of them are non-callee-saved. > > I would suggest you do this as a follow-on change. Acknowledged. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... src/nonsfi/linux/irt_signal_handling.c:212: "ldmia r14, {r0-r10}\n" On 2015/08/12 01:43:07, Mark Seaborn wrote: > How about "{r0-pc}"? Then you shouldn't need the further code below. On ARM > it's really easy to restore all the general purpose registers (+ PC) in one > instruction. The docs mentioned two deprecations for this instruction: * SP must not be in the list * If PC is in the list, LR must not be in the list. GCC prints a warning but the test passes. FWIW, glibc (longjmp) and the kernel (sigreturn) have a similar implementation. If we are OK with the warning, I can change it to {r0-pc}. Stress testing did not show anything wrong. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... src/nonsfi/linux/irt_signal_handling.c:299: g_tgid = getpid(); On 2015/08/12 01:43:07, Mark Seaborn wrote: > You should check for errors in these two syscalls, since they could easily be > blocked by seccomp-bpf. Added an abort() on failure for both (although the purpose of this function is that it should be called before seccomp-bpf is enabled, since nonsfi_install_.*_locked() above call sigaction()). https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... File src/nonsfi/linux/irt_signal_handling.h (right): https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... src/nonsfi/linux/irt_signal_handling.h:18: int nacl_signal_set_handler(NaClIrtSignalHandler handler); On 2015/08/12 01:43:07, Mark Seaborn wrote: > The names in irt.h have "async" in them. Could you add that here too? Done. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/linux... File src/nonsfi/linux/linux_pthread_private.c (right): https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/linux... src/nonsfi/linux/linux_pthread_private.c:21: struct nacl_irt_thread_v0_2 __libnacl_irt_thread_v0_2; On 2015/08/12 01:43:07, Mark Seaborn wrote: > I don't think you need this, or the assignment to it below. Done. https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/nonsf... File src/nonsfi/linux/nonsfi_signal.h (right): https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/nonsf... src/nonsfi/linux/nonsfi_signal.h:85: void exception_frame_from_signal_context( On 2015/08/12 01:43:07, Mark Seaborn wrote: > How about prefixing this with "nonsfi_" too? Done. https://codereview.chromium.org/1212613002/diff/120001/src/public/nonsfi/irt_... File src/public/nonsfi/irt_exception_handling.h (left): https://codereview.chromium.org/1212613002/diff/120001/src/public/nonsfi/irt_... src/public/nonsfi/irt_exception_handling.h:15: void nonsfi_initialize_signal_handler(void); On 2015/08/12 01:43:06, Mark Seaborn wrote: > Removing this from src/public/ would break the DEPS roll into Chromium. This is > referred to by components/nacl/loader/nacl_helper_linux.cc. Please stage the > change so that a trivial DEPS roll will work. > > How about just keeping the function name and header file name the same? The DEPS roll would also be broken by the flags change in the call to clone(). Should I do the roll to make sure both this and the corresponding sandbox change are done in the same change? https://codereview.chromium.org/1212613002/diff/120001/src/public/nonsfi/irt_... File src/public/nonsfi/irt_signal_handling.h (right): https://codereview.chromium.org/1212613002/diff/120001/src/public/nonsfi/irt_... src/public/nonsfi/irt_signal_handling.h:15: * Initialize signal handler for exceptions and regular signals at startup On 2015/08/12 01:43:07, Mark Seaborn wrote: > What does "regular signals" mean? Done. https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.h File src/untrusted/irt/irt.h (right): https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:251: * Now this interface assigns the thread ID of the child thread into On 2015/08/12 01:43:07, Mark Seaborn wrote: > 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...". Done. https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:252: * |child_tid| atomically when the thread is created. It will be the On 2015/08/12 01:43:07, Mark Seaborn wrote: > 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.) Done. https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:459: * This interface is only available on Non-SFI Mode. On 2015/08/12 01:43:07, Mark Seaborn wrote: > Can you refer back to nonsfi_mode_async_signals.txt somewhere in the comments > for this interface? Done. https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:462: typedef void (*NaClIrtSignalHandler)(struct NaClExceptionContext *context); On 2015/08/12 01:43:07, Mark Seaborn wrote: > 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 Done. https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:469: * mask (as opposed to the POSIX behavior), so |handler| must be reentrant. On 2015/08/12 01:43:07, Mark Seaborn wrote: > See my comment in nonsfi_mode_async_signals.txt about re-entrancy. Reworded it a bit. https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:489: * the same thread group as the caller. This function requires On 2015/08/12 01:43:07, Mark Seaborn wrote: > "Thread group" is a Linux concept that shouldn't be referenced here. Changed to "process" to clarify that this can't be used to communicate outside of the app. https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:490: * nacl_signal_set_handler to be called first, otherwise it will fail with On 2015/08/12 01:43:07, Mark Seaborn wrote: > "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? Removed. https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/irt/irt.... src/untrusted/irt/irt.h:492: * thread (through the |parent_tid| parameter) and the thread must still be On 2015/08/12 01:43:07, Mark Seaborn wrote: > You mean child_tid. Done. https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/nacl/nac... File src/untrusted/nacl/nacl_irt.h (right): https://codereview.chromium.org/1212613002/diff/120001/src/untrusted/nacl/nac... src/untrusted/nacl/nacl_irt.h:30: extern struct nacl_irt_thread_v0_2 __libnacl_irt_thread_v0_2; On 2015/08/12 01:43:07, Mark Seaborn wrote: > Not needed -- see my comment in linux_pthread_private.c. Done. https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/nacl.scons File tests/nonsfi/nacl.scons (right): https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/nacl.scon... tests/nonsfi/nacl.scons:30: '${EXCEPTION_LIBS}']) On 2015/08/12 01:43:07, Mark Seaborn wrote: > I don't think this executable uses EXCEPTION_LIBS. Done. https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... File tests/nonsfi/user_async_signal_test.cc (right): https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:11: #include "native_client/src/nonsfi/linux/irt_signal_handling.h" On 2015/08/12 01:43:08, Mark Seaborn wrote: > Shouldn't be needed now -- this is an IRT-internal header. Done. https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:12: #include "native_client/src/public/nonsfi/irt_signal_handling.h" On 2015/08/12 01:43:08, Mark Seaborn wrote: > Also shouldn't be needed now -- this is an IRT-internal header. Done. https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:14: #include "native_client/src/untrusted/nacl/nacl_irt.h" On 2015/08/12 01:43:08, Mark Seaborn wrote: > Not needed now? This is used for __libnacl_irt_thread. We cannot use a local copy since we are using the thread_create wrapper. https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:16: #include "native_client/src/untrusted/pthread/pthread_types.h" On 2015/08/12 01:43:08, Mark Seaborn wrote: > Not needed now? Done. https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:20: struct nacl_irt_thread_v0_2 __libnacl_irt_thread_v0_2; On 2015/08/12 01:43:08, Mark Seaborn wrote: > You shouldn't need the "__" prefix here (or in the next decl). > > Can you also put all these definitions in a top-level anon namespace (which > would replace uses of "static")? Then we get warnings about unused decls. Done. https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:48: void test_send_signal_before_init() { On 2015/08/12 01:43:08, Mark Seaborn wrote: > How about "before_set_handler", since there is no public "init" function? Done. https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:109: * handler, so it did not actually counts as a wakeup. On 2015/08/12 01:43:08, Mark Seaborn wrote: > "count" Done. https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:135: ASSERT_EQ(retval, EINTR); On 2015/08/12 01:43:08, Mark Seaborn wrote: > Is the current implementation giving us EINTR? I don't think we want that. > > The idea is that user code should not have to retry syscalls. That's a common > source of bugs, when programmers don't put a retry loop around a syscall that > can be interrupted. > > Should we be registering signal handlers with SA_RESTART? It appears that FUTEX_WAIT with a timeout does return EINTR when interrupted, even with SA_RESTART (could not find docs that mention this, but verified experimentally). With no timeout, SA_RESTART does what's expected. I'll add a comment to clarify and will add SA_RESTART to the sigaction flags. https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:282: * opposed to interrupting it), which breaks this test. On 2015/08/12 01:43:08, Mark Seaborn wrote: > Do you mean the futex_wait syscall doesn't get interrupted at all? > > Does this problem only occur under QEMU? If so, can you say that in the > comment, please? Yes, futex_wait_abs returns ETIMEDOUT and the signal is delivered afterwards. I also ran the test on real ARM hardware and does not seem to happen. Amended the comment.
https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... File src/nonsfi/linux/irt_signal_handling.c (right): https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... src/nonsfi/linux/irt_signal_handling.c:212: "ldmia r14, {r0-r10}\n" On 2015/08/12 22:14:26, Luis Héctor Chávez wrote: > On 2015/08/12 01:43:07, Mark Seaborn wrote: > > How about "{r0-pc}"? Then you shouldn't need the further code below. On ARM > > it's really easy to restore all the general purpose registers (+ PC) in one > > instruction. > > The docs mentioned two deprecations for this instruction: > > * SP must not be in the list > * If PC is in the list, LR must not be in the list. Can you give me a pointer to which docs say that, and/or what they say? > GCC prints a warning but the test passes. > FWIW, glibc (longjmp) and the kernel > (sigreturn) have a similar implementation. Do you mean glibc and Linux use "ldm ... {r0-pc}"? > If we are OK with the warning, I can change it to {r0-pc}. Stress testing did > not show anything wrong. I think if Linux and glibc use {r0-pc}, that would be preferred. What is the warning? Did you find a way to turn it off? https://codereview.chromium.org/1212613002/diff/120001/src/public/nonsfi/irt_... File src/public/nonsfi/irt_exception_handling.h (left): https://codereview.chromium.org/1212613002/diff/120001/src/public/nonsfi/irt_... src/public/nonsfi/irt_exception_handling.h:15: void nonsfi_initialize_signal_handler(void); On 2015/08/12 22:14:25, Luis Héctor Chávez wrote: > On 2015/08/12 01:43:06, Mark Seaborn wrote: > > Removing this from src/public/ would break the DEPS roll into Chromium. This > is > > referred to by components/nacl/loader/nacl_helper_linux.cc. Please stage the > > change so that a trivial DEPS roll will work. > > > > How about just keeping the function name and header file name the same? > > The DEPS roll would also be broken by the flags change in the call to clone(). > Should I do the roll to make sure both this and the corresponding sandbox change > are done in the same change? No, it's better to avoid non-trivial DEPS rolls whenever possible. See: https://docs.google.com/document/d/1jHoLo9I3CCS1_-4KlIq1OiEMv9cmMuXES2Z9JVpmP... Can you change the Chromium side to allow the new clone() flags first? https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... File tests/nonsfi/user_async_signal_test.cc (right): https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:109: * handler, so it did not actually counts as a wakeup. On 2015/08/12 22:14:27, Luis Héctor Chávez wrote: > On 2015/08/12 01:43:08, Mark Seaborn wrote: > > "count" > > Done. Er, I didn't mean you should replace '|count|' with '"count"'. The former was fine. I put the comment on the wrong line. It should have been the next line... https://codereview.chromium.org/1212613002/diff/160001/documentation/nonsfi_m... File documentation/nonsfi_mode_async_signals.txt (right): https://codereview.chromium.org/1212613002/diff/160001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:12: * Synchonous signals (from hardware exceptions) are separate and their behavior Typo is still there: "Synchronous" https://codereview.chromium.org/1212613002/diff/160001/tests/nonsfi/user_asyn... File tests/nonsfi/user_async_signal_test.cc (right): https://codereview.chromium.org/1212613002/diff/160001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:29: } // namespace Can you put all of the functions (except main()) in the anon namespace too, please? https://codereview.chromium.org/1212613002/diff/160001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:110: * handler, so it did not actually counts as a wakeup. "so it did not actually count as a wakeup"
https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... File src/nonsfi/linux/irt_signal_handling.c (right): https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... src/nonsfi/linux/irt_signal_handling.c:212: "ldmia r14, {r0-r10}\n" On 2015/08/13 21:17:50, Mark Seaborn wrote: > On 2015/08/12 22:14:26, Luis Héctor Chávez wrote: > > On 2015/08/12 01:43:07, Mark Seaborn wrote: > > > How about "{r0-pc}"? Then you shouldn't need the further code below. On > ARM > > > it's really easy to restore all the general purpose registers (+ PC) in one > > > instruction. > > > > The docs mentioned two deprecations for this instruction: > > > > * SP must not be in the list > > * If PC is in the list, LR must not be in the list. > > Can you give me a pointer to which docs say that, and/or what they say? http://infocenter.arm.com/help/topic/com.arm.doc.dui0489i/DUI0489I_arm_assemb... page 3-85 (187 in the .pdf) "In 32-bit Thumb instructions: • the SP cannot be in the list • the PC and LR cannot both be in the list • there must be two or more registers in the list ARM load instructions can have SP and PC in the reglist but these instructions that include SP in the reglist or both PC and LR in the reglist are deprecated in ARMv6T2 and above." > > > GCC prints a warning but the test passes. > > > FWIW, glibc (longjmp) and the kernel > > (sigreturn) have a similar implementation. > > Do you mean glibc and Linux use "ldm ... {r0-pc}"? glibc in ARM splits the loading of the registers in several steps. It only uses ldmia for a few registers and then the rest are loaded individually. In Linux the example I looked at was that arch/arm/kernel/entry-header.S has two implementations for svc_exit: one for non-Thumb2 that does use ldmia {r0-pc} and another one for Thumb2 where "stm/ldm rd, {sp, lr} is not available" where they do something similar to what I'm doing (although they just load r0-r12 and then deal with the rest of the registers in a much more straightforward fashion). > > > If we are OK with the warning, I can change it to {r0-pc}. Stress testing did > > not show anything wrong. > > I think if Linux and glibc use {r0-pc}, that would be preferred. What is the > warning? Did you find a way to turn it off? pnacl-translate issues the following warning: <inline asm>:3:1: warning: use of SP in the list is deprecated ldmia r14, {r0-pc} I'm not very familiar with clang and had no success in finding a way to silence it. https://codereview.chromium.org/1212613002/diff/120001/src/public/nonsfi/irt_... File src/public/nonsfi/irt_exception_handling.h (left): https://codereview.chromium.org/1212613002/diff/120001/src/public/nonsfi/irt_... src/public/nonsfi/irt_exception_handling.h:15: void nonsfi_initialize_signal_handler(void); On 2015/08/13 21:17:50, Mark Seaborn wrote: > On 2015/08/12 22:14:25, Luis Héctor Chávez wrote: > > On 2015/08/12 01:43:06, Mark Seaborn wrote: > > > Removing this from src/public/ would break the DEPS roll into Chromium. > This > > is > > > referred to by components/nacl/loader/nacl_helper_linux.cc. Please stage > the > > > change so that a trivial DEPS roll will work. > > > > > > How about just keeping the function name and header file name the same? > > > > The DEPS roll would also be broken by the flags change in the call to clone(). > > Should I do the roll to make sure both this and the corresponding sandbox > change > > are done in the same change? > > No, it's better to avoid non-trivial DEPS rolls whenever possible. > > See: > https://docs.google.com/document/d/1jHoLo9I3CCS1_-4KlIq1OiEMv9cmMuXES2Z9JVpmP... > > Can you change the Chromium side to allow the new clone() flags first? Done. https://codereview.chromium.org/1212613002/diff/160001/documentation/nonsfi_m... File documentation/nonsfi_mode_async_signals.txt (right): https://codereview.chromium.org/1212613002/diff/160001/documentation/nonsfi_m... documentation/nonsfi_mode_async_signals.txt:12: * Synchonous signals (from hardware exceptions) are separate and their behavior On 2015/08/13 21:17:50, Mark Seaborn wrote: > Typo is still there: "Synchronous" Fixed for real this time. https://codereview.chromium.org/1212613002/diff/160001/tests/nonsfi/user_asyn... File tests/nonsfi/user_async_signal_test.cc (right): https://codereview.chromium.org/1212613002/diff/160001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:29: } // namespace On 2015/08/13 21:17:50, Mark Seaborn wrote: > Can you put all of the functions (except main()) in the anon namespace too, > please? Done. https://codereview.chromium.org/1212613002/diff/160001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:110: * handler, so it did not actually counts as a wakeup. On 2015/08/13 21:17:50, Mark Seaborn wrote: > "so it did not actually count as a wakeup" Done.
https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... File src/nonsfi/linux/irt_signal_handling.c (right): https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... src/nonsfi/linux/irt_signal_handling.c:212: "ldmia r14, {r0-r10}\n" On 2015/08/13 22:48:12, Luis Héctor Chávez wrote: > > > If we are OK with the warning, I can change it to {r0-pc}. Stress > > > testing did not show anything wrong. > > > > I think if Linux and glibc use {r0-pc}, that would be preferred. What > > is the warning? Did you find a way to turn it off? > > pnacl-translate issues the following warning: > > <inline asm>:3:1: warning: use of SP in the list is deprecated > ldmia r14, {r0-pc} > > I'm not very familiar with clang and had no success in finding a way to > silence it. OK, you can leave the ARM code as it is. Could you just add a brief comment saying why the code doesn't use "ldmia r14, {r0-pc}", please? https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... File tests/nonsfi/user_async_signal_test.cc (right): https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:135: ASSERT_EQ(retval, EINTR); On 2015/08/12 22:14:27, Luis Héctor Chávez wrote: > On 2015/08/12 01:43:08, Mark Seaborn wrote: > > Is the current implementation giving us EINTR? I don't think we want that. > > > > The idea is that user code should not have to retry syscalls. That's a common > > source of bugs, when programmers don't put a retry loop around a syscall that > > can be interrupted. > > > > Should we be registering signal handlers with SA_RESTART? > > It appears that FUTEX_WAIT with a timeout does return EINTR when interrupted, > even with SA_RESTART (could not find docs that mention this, but verified > experimentally). With no timeout, SA_RESTART does what's expected. > > I'll add a comment to clarify and will add SA_RESTART to the sigaction flags. OK. Are there any test cases in this file that do cover a syscall being restarted automatically? If not, could you add one? e.g. You could cover futex_wait() without a timeout.
https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... File src/nonsfi/linux/irt_signal_handling.c (right): https://codereview.chromium.org/1212613002/diff/120001/src/nonsfi/linux/irt_s... src/nonsfi/linux/irt_signal_handling.c:212: "ldmia r14, {r0-r10}\n" On 2015/08/17 23:55:56, Mark Seaborn wrote: > On 2015/08/13 22:48:12, Luis Héctor Chávez wrote: > > > > If we are OK with the warning, I can change it to {r0-pc}. Stress > > > > testing did not show anything wrong. > > > > > > I think if Linux and glibc use {r0-pc}, that would be preferred. What > > > is the warning? Did you find a way to turn it off? > > > > pnacl-translate issues the following warning: > > > > <inline asm>:3:1: warning: use of SP in the list is deprecated > > ldmia r14, {r0-pc} > > > > I'm not very familiar with clang and had no success in finding a way to > > silence it. > > OK, you can leave the ARM code as it is. Could you just add a brief comment > saying why the code doesn't use "ldmia r14, {r0-pc}", please? Done. https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... File tests/nonsfi/user_async_signal_test.cc (right): https://codereview.chromium.org/1212613002/diff/120001/tests/nonsfi/user_asyn... tests/nonsfi/user_async_signal_test.cc:135: ASSERT_EQ(retval, EINTR); On 2015/08/17 23:55:56, Mark Seaborn wrote: > On 2015/08/12 22:14:27, Luis Héctor Chávez wrote: > > On 2015/08/12 01:43:08, Mark Seaborn wrote: > > > Is the current implementation giving us EINTR? I don't think we want that. > > > > > > The idea is that user code should not have to retry syscalls. That's a > common > > > source of bugs, when programmers don't put a retry loop around a syscall > that > > > can be interrupted. > > > > > > Should we be registering signal handlers with SA_RESTART? > > > > It appears that FUTEX_WAIT with a timeout does return EINTR when interrupted, > > even with SA_RESTART (could not find docs that mention this, but verified > > experimentally). With no timeout, SA_RESTART does what's expected. > > > > I'll add a comment to clarify and will add SA_RESTART to the sigaction flags. > > OK. Are there any test cases in this file that do cover a syscall being > restarted automatically? If not, could you add one? e.g. You could cover > futex_wait() without a timeout. Done. There is one small caveat: the test will still need a retry loop since apparently at some point, Linux started allowing spurious wakeups with futex_wait() also when it returns 0. Here is the thread in LKML where they are trying to update the documentation to reflect this: http://thread.gmane.org/gmane.linux.kernel/1919555/focus=9795 The important snippet: RETURN VALUE [...] FUTEX_WAIT Returns 0 if the caller was woken up. Note that a wake-up can also be caused by common futex usage patterns in unrelated code that happened to have previously used the futex word's memory location (e.g., typical futex-based implementations of Pthreads mutexes can cause this under some conditions). Therefore, call‐ ers should always conservatively assume that a return value of 0 can mean a spurious wake-up, and use the futex word's value (i.e., the user space synchronization scheme) to decide whether to continue to block or not.
The commit message refers to "src/nonsfi/linux/nonsfi_signal.h" which doesn't exist now -- can you update it, please? Then LGTM. https://codereview.chromium.org/1212613002/diff/200001/src/nonsfi/irt/irt_int... File src/nonsfi/irt/irt_interfaces.c (right): https://codereview.chromium.org/1212613002/diff/200001/src/nonsfi/irt/irt_int... src/nonsfi/irt/irt_interfaces.c:6: Since you're changing this file, can you manually run the "nacl-toolchain-linux-pnacl-x86_64" trybot and check that it passes before committing, please? (That tests "unsandboxed PNaCl" mode.) https://codereview.chromium.org/1212613002/diff/200001/src/public/nonsfi/irt_... File src/public/nonsfi/irt_exception_handling.h (right): https://codereview.chromium.org/1212613002/diff/200001/src/public/nonsfi/irt_... src/public/nonsfi/irt_exception_handling.h:10: /* Staging for trivial DEPS roll. */ Can you add a TODO to remove this after the roll?
https://codereview.chromium.org/1212613002/diff/200001/src/nonsfi/irt/irt_int... File src/nonsfi/irt/irt_interfaces.c (right): https://codereview.chromium.org/1212613002/diff/200001/src/nonsfi/irt/irt_int... src/nonsfi/irt/irt_interfaces.c:6: On 2015/08/18 20:35:46, Mark Seaborn wrote: > Since you're changing this file, can you manually run the > "nacl-toolchain-linux-pnacl-x86_64" trybot and check that it passes before > committing, please? (That tests "unsandboxed PNaCl" mode.) Acknowledged. https://codereview.chromium.org/1212613002/diff/200001/src/public/nonsfi/irt_... File src/public/nonsfi/irt_exception_handling.h (right): https://codereview.chromium.org/1212613002/diff/200001/src/public/nonsfi/irt_... src/public/nonsfi/irt_exception_handling.h:10: /* Staging for trivial DEPS roll. */ On 2015/08/18 20:35:47, Mark Seaborn wrote: > Can you add a TODO to remove this after the roll? Done.
The CQ bit was checked by lhchavez@chromium.org
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/1212613002/#ps220001 (title: "Added a TODO to remove code after roll")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1212613002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1212613002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/native_client/src/native_client/+/6a0ce065f...
Message was sent while issue was closed.
On 2015/08/19 16:35:31, commit-bot: I haz the power wrote: > Committed patchset #12 (id:220001) as > https://chromium.googlesource.com/native_client/src/native_client/+/6a0ce065f... This seems to have broken the x86-32 nonsfi tests: http://build.chromium.org/p/client.nacl.toolchain/builders/linux-pnacl-x86_64... specifically, /usr/bin/python tools/command_tester.py --name pnacl_newlib.run_user_async_signal_test_irt --time_warning 10 --time_error 100 --perf_env_description x86-32_with_irt_pnacl_newlib_static --using_nacl_signal_handler True --subarch 32 --arch x86 --output_stamp /mnt/data/b/build/slave/nacl-toolchain/build/native_client/scons-out/nacl_irt_test-x86-32-pnacl-pexe-nonsfi-clang/test_results/user_async_signal_test.out scons-out/nacl-x86-32-pnacl-pexe-nonsfi-clang/staging/nonsfi_loader.nexe scons-out/nacl_irt_test-x86-32-pnacl-pexe-nonsfi-clang/obj/tests/nonsfi/user_async_signal_test.nexe [ RUN ] pnacl_newlib.run_user_async_signal_test_irt scons-out/nacl-x86-32-pnacl-pexe-nonsfi-clang/staging/nonsfi_loader.nexe scons-out/nacl_irt_test-x86-32-pnacl-pexe-nonsfi-clang/obj/tests/nonsfi/user_async_signal_test.nexe ERROR: Command returned: exit status 245 (0xf5) and signal info None but we expected: exit status 0 (0x0) and signal info None ====================================================================== Stdout for pnacl_newlib.run_user_async_signal_test_irt: ====================================================================== Running test_send_signal_before_set_handler... Running test_async_safe_tls_get... Running test_async_safe_futex... Running test_futex_wait_restart... Running test_async_safe_signal... ====================================================================== Stderr for pnacl_newlib.run_user_async_signal_test_irt: ====================================================================== [ FAILED ] pnacl_newlib.run_user_async_signal_test_irt (40 ms)
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/1300883003/ by lhchavez@chromium.org. The reason for reverting is: test_async_safe_signal seems to be broken on x86_32 bots, but not reproducible on trybots or local machine.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
