| commit f0419e6a10740a672b28e112c409ae24f5e890ab |
| Author: Jakub Jelinek <jakub@redhat.com> |
| Date: Thu Mar 4 15:15:33 2021 +0100 |
| |
| [PATCH] pthread_once hangs when init routine throws an exception [BZ #18435] |
| |
| This is another attempt at making pthread_once handle throwing exceptions |
| from the init routine callback. As the new testcases show, just switching |
| to the cleanup attribute based cleanup does fix the tst-once5 test, but |
| breaks the new tst-oncey3 test. That is because when throwing exceptions, |
| only the unwind info registered cleanups (i.e. C++ destructors or cleanup |
| attribute), when cancelling threads and there has been unwind info from the |
| cancellation point up to whatever needs cleanup both unwind info registered |
| cleanups and THREAD_SETMEM (self, cleanup, ...) registered cleanups are |
| invoked, but once we hit some frame with no unwind info, only the |
| THREAD_SETMEM (self, cleanup, ...) registered cleanups are invoked. |
| So, to stay fully backwards compatible (allow init routines without |
| unwind info which encounter cancellation points) and handle exception throwing |
| we actually need to register the pthread_once cleanups in both unwind info |
| and in the THREAD_SETMEM (self, cleanup, ...) way. |
| If an exception is thrown, only the former will happen and we in that case |
| need to also unregister the THREAD_SETMEM (self, cleanup, ...) registered |
| handler, because otherwise after catching the exception the user code could |
| call deeper into the stack some cancellation point, get cancelled and then |
| a stale cleanup handler would clobber stack and probably crash. |
| If a thread calling init routine is cancelled and unwind info ends before |
| the pthread_once frame, it will be cleaned up through self->cleanup as |
| before. And if unwind info is present, unwind_stop first calls the |
| self->cleanup registered handler for the frame, then it will call the |
| unwind info registered handler but that will already see __do_it == 0 |
| and do nothing. |
| |
| # Conflicts: |
| # nptl/Makefile |
| # (The usual cleanups because they don't match.) |
| # sysdeps/pthread/Makefile |
| # (The usual cleanups because all the other tests aren't moved.) |
| |
| diff --git a/nptl/Makefile b/nptl/Makefile |
| index dcf3868869767015..70a3be23ecfcd9c9 100644 |
| |
| |
| @@ -334,10 +334,6 @@ xtests = tst-setuid1 tst-setuid1-static tst-setuid2 \ |
| tst-mutexpp1 tst-mutexpp6 tst-mutexpp10 |
| test-srcs = tst-oddstacklimit |
| |
| -# Test expected to fail on most targets (except x86_64) due to bug |
| -# 18435 - pthread_once hangs when init routine throws an exception. |
| -test-xfail-tst-once5 = yes |
| - |
| # Files which must not be linked with libpthread. |
| tests-nolibpthread = tst-unload |
| |
| diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h |
| index a2d48b2015cd385c..7ddc166cf32414c4 100644 |
| |
| |
| @@ -571,6 +571,67 @@ extern void __pthread_cleanup_pop (struct _pthread_cleanup_buffer *buffer, |
| # undef pthread_cleanup_pop |
| # define pthread_cleanup_pop(execute) \ |
| __pthread_cleanup_pop (&_buffer, (execute)); } |
| + |
| +# if defined __EXCEPTIONS && !defined __cplusplus |
| +/* Structure to hold the cleanup handler information. */ |
| +struct __pthread_cleanup_combined_frame |
| +{ |
| + void (*__cancel_routine) (void *); |
| + void *__cancel_arg; |
| + int __do_it; |
| + struct _pthread_cleanup_buffer __buffer; |
| +}; |
| + |
| +/* Special cleanup macros which register cleanup both using |
| + __pthread_cleanup_{push,pop} and using cleanup attribute. This is needed |
| + for pthread_once, so that it supports both throwing exceptions from the |
| + pthread_once callback (only cleanup attribute works there) and cancellation |
| + of the thread running the callback if the callback or some routines it |
| + calls don't have unwind information. */ |
| + |
| +static __always_inline void |
| +__pthread_cleanup_combined_routine (struct __pthread_cleanup_combined_frame |
| + *__frame) |
| +{ |
| + if (__frame->__do_it) |
| + { |
| + __frame->__cancel_routine (__frame->__cancel_arg); |
| + __frame->__do_it = 0; |
| + __pthread_cleanup_pop (&__frame->__buffer, 0); |
| + } |
| +} |
| + |
| +static inline void |
| +__pthread_cleanup_combined_routine_voidptr (void *__arg) |
| +{ |
| + struct __pthread_cleanup_combined_frame *__frame |
| + = (struct __pthread_cleanup_combined_frame *) __arg; |
| + if (__frame->__do_it) |
| + { |
| + __frame->__cancel_routine (__frame->__cancel_arg); |
| + __frame->__do_it = 0; |
| + } |
| +} |
| + |
| +# define pthread_cleanup_combined_push(routine, arg) \ |
| + do { \ |
| + void (*__cancel_routine) (void *) = (routine); \ |
| + struct __pthread_cleanup_combined_frame __clframe \ |
| + __attribute__ ((__cleanup__ (__pthread_cleanup_combined_routine))) \ |
| + = { .__cancel_routine = __cancel_routine, .__cancel_arg = (arg), \ |
| + .__do_it = 1 }; \ |
| + __pthread_cleanup_push (&__clframe.__buffer, \ |
| + __pthread_cleanup_combined_routine_voidptr, \ |
| + &__clframe); |
| + |
| +# define pthread_cleanup_combined_pop(execute) \ |
| + __pthread_cleanup_pop (&__clframe.__buffer, 0); \ |
| + __clframe.__do_it = 0; \ |
| + if (execute) \ |
| + __cancel_routine (__clframe.__cancel_arg); \ |
| + } while (0) |
| + |
| +# endif |
| #endif |
| |
| extern void __pthread_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer, |
| diff --git a/nptl/pthread_once.c b/nptl/pthread_once.c |
| index 1653226286dc3539..45e965e8743d9412 100644 |
| |
| |
| @@ -111,11 +111,11 @@ __pthread_once_slow (pthread_once_t *once_control, void (*init_routine) (void)) |
| /* This thread is the first here. Do the initialization. |
| Register a cleanup handler so that in case the thread gets |
| interrupted the initialization can be restarted. */ |
| - pthread_cleanup_push (clear_once_control, once_control); |
| + pthread_cleanup_combined_push (clear_once_control, once_control); |
| |
| init_routine (); |
| |
| - pthread_cleanup_pop (0); |
| + pthread_cleanup_combined_pop (0); |
| |
| |
| /* Mark *once_control as having finished the initialization. We need |
| diff --git a/nptl/tst-once5.cc b/nptl/tst-once5.cc |
| index d232266c3ace89d9..dda18e610c9114bc 100644 |
| |
| |
| @@ -59,7 +59,7 @@ do_test (void) |
| " throwing an exception", stderr); |
| } |
| catch (OnceException) { |
| - if (1 < niter) |
| + if (niter > 1) |
| fputs ("pthread_once unexpectedly threw", stderr); |
| result = 0; |
| } |
| @@ -75,7 +75,5 @@ do_test (void) |
| return result; |
| } |
| |
| -// The test currently hangs and is XFAILed. Reduce the timeout. |
| -#define TIMEOUT 1 |
| #define TEST_FUNCTION do_test () |
| #include "../test-skeleton.c" |
| diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile |
| index 14ef04247cb84ad3..80a71f3f9f0e72ae 100644 |
| |
| |
| @@ -35,7 +35,7 @@ tst-create1mod.so-no-z-defs = yes |
| |
| tests += tst-once1 tst-once2 tst-once3 tst-once4 |
| |
| -tests += tst-oncex3 tst-oncex4 |
| +tests += tst-oncex3 tst-oncex4 tst-oncey3 tst-oncey4 |
| |
| ifeq ($(build-shared),yes) |
| # Build all the modules even when not actually running test programs. |
| @@ -44,6 +44,8 @@ endif |
| |
| CFLAGS-tst-oncex3.c += -fexceptions |
| CFLAGS-tst-oncex4.c += -fexceptions |
| +CFLAGS-tst-oncey3.c += -fno-exceptions -fno-asynchronous-unwind-tables |
| +CFLAGS-tst-oncey4.c += -fno-exceptions -fno-asynchronous-unwind-tables |
| |
| modules-names += tst-create1mod |
| test-modules = $(addprefix $(objpfx),$(addsuffix .so,$(modules-names))) |
| diff --git a/sysdeps/pthread/tst-oncey3.c b/sysdeps/pthread/tst-oncey3.c |
| new file mode 100644 |
| index 0000000000000000..08225b88dc06b979 |
| |
| |
| @@ -0,0 +1 @@ |
| +#include "tst-once3.c" |
| diff --git a/sysdeps/pthread/tst-oncey4.c b/sysdeps/pthread/tst-oncey4.c |
| new file mode 100644 |
| index 0000000000000000..9b4d98f3f13c265a |
| |
| |
| @@ -0,0 +1 @@ |
| +#include "tst-once4.c" |