Hi, > On Oct 1, 2017, at 9:30 PM, Scott Cheloha <scottchel...@gmail.com> wrote: > > Per this bit from pthread_once(3): > >> The function pthread_once() is not a cancellation point. >> However, if init_routine() is a cancellation point and is >> cancelled, the effect on once_control is as if pthread_once() >> was never called. > > it should be possible to cancel a thread from init_routine() and > send in another thread to retry. But the attached regression > test fails, so my guess is that there needs to be something like a > pthread_cleanup wrapper around init_routine() in pthread_once() in > order for it to be POSIX-compliant. > > [...] > > [...] pthread_once(3) is in libc now while pthread_cleanup_push and > pthread_cleanup_pop remain in librthread, so you have a choice: > > 1. Move the pthread_cleanup interfaces into libc, or > > 2. Copy their contents and do the cleanup push/pop on the > stack in pthread_once().
Absent any other feedback, the attached patch moves the pthread_cleanup routines from librthread to libc and adds a cleanup wrapper around the call to init_routine() in pthread_once(3). These changes allow the initial parts of the attached regression test to succeed. I believe this means pthread_once(3) is now more (fully?) POSIX.1-compliant, like it says it is in the man page. Moving the routines into libc and calling them there seems cleaner than copying their contents out into a macro. Calling the routines from within the library was libthr's original solution, though according to the commit record they now use a macro for performance reasons. Relevant commits here: https://svnweb.freebsd.org/base/head/lib/libthr/thread/thr_once.c?r1=112918&r2=144518 https://svnweb.freebsd.org/base/head/lib/libthr/thread/thr_once.c?r1=155739&r2=172695 Doing it in a macro would also require changing the cleanup_fn structure and adding a "this is on the stack" boolean. Misc. notes: * We're adding interfaces to libc so minor version bump there. * We're removing interfaces from librthread so major version bump there. Tangential questions: * Because the cleanup functions are now exported from and called from within libc they should be PROTO_NORMAL and DEF_STRONG'd, right? * pthread_self(3) and pthread_exit(3) are not called from within libc but they are PROTO_NORMAL'd. Is this because they are called from within librthread? Some kind of exception case? * Why does pthread_equal(3) not have a DEF*? Same question for pthread_once(3). CC guenther@ and kettenis@ because they wrote/reviewed the recent interface migration from librthread to libc. Thoughts/Feedback? -- Scott Cheloha Index: lib/libc/Symbols.list =================================================================== RCS file: /cvs/src/lib/libc/Symbols.list,v retrieving revision 1.57 diff -u -p -r1.57 Symbols.list --- lib/libc/Symbols.list 5 Sep 2017 06:35:19 -0000 1.57 +++ lib/libc/Symbols.list 9 Oct 2017 12:56:06 -0000 @@ -1667,6 +1667,8 @@ _thread_atfork _thread_dofork _thread_set_callbacks pthread_atfork +pthread_cleanup_pop +pthread_cleanup_push pthread_cond_broadcast pthread_cond_destroy pthread_cond_init Index: lib/libc/shlib_version =================================================================== RCS file: /cvs/src/lib/libc/shlib_version,v retrieving revision 1.192 diff -u -p -r1.192 shlib_version --- lib/libc/shlib_version 5 Sep 2017 02:40:54 -0000 1.192 +++ lib/libc/shlib_version 9 Oct 2017 12:56:06 -0000 @@ -1,4 +1,4 @@ major=90 -minor=0 +minor=1 # note: If changes were made to include/thread_private.h or if system # calls were added/changed then librthread/shlib_version also be updated. Index: lib/libc/hidden/pthread.h =================================================================== RCS file: /cvs/src/lib/libc/hidden/pthread.h,v retrieving revision 1.2 diff -u -p -r1.2 pthread.h --- lib/libc/hidden/pthread.h 5 Sep 2017 02:40:54 -0000 1.2 +++ lib/libc/hidden/pthread.h 9 Oct 2017 12:56:06 -0000 @@ -21,6 +21,8 @@ #include_next <pthread.h> PROTO_STD_DEPRECATED(pthread_atfork); +PROTO_NORMAL(pthread_cleanup_pop); +PROTO_NORMAL(pthread_cleanup_push); PROTO_STD_DEPRECATED(pthread_cond_broadcast); PROTO_STD_DEPRECATED(pthread_cond_destroy); PROTO_NORMAL(pthread_cond_init); Index: lib/libc/thread/rthread.c =================================================================== RCS file: /cvs/src/lib/libc/thread/rthread.c,v retrieving revision 1.4 diff -u -p -r1.4 rthread.c --- lib/libc/thread/rthread.c 5 Sep 2017 02:40:54 -0000 1.4 +++ lib/libc/thread/rthread.c 9 Oct 2017 12:56:07 -0000 @@ -147,6 +147,38 @@ pthread_exit(void *retval) } DEF_STRONG(pthread_exit); +void +pthread_cleanup_push(void (*fn)(void *), void *arg) +{ + struct rthread_cleanup_fn *clfn; + pthread_t self = pthread_self(); + + clfn = calloc(1, sizeof(*clfn)); + if (!clfn) + return; + clfn->fn = fn; + clfn->arg = arg; + clfn->next = self->cleanup_fns; + self->cleanup_fns = clfn; +} +DEF_STRONG(pthread_cleanup_push); + +void +pthread_cleanup_pop(int execute) +{ + struct rthread_cleanup_fn *clfn; + pthread_t self = pthread_self(); + + clfn = self->cleanup_fns; + if (clfn) { + self->cleanup_fns = clfn->next; + if (execute) + clfn->fn(clfn->arg); + free(clfn); + } +} +DEF_STRONG(pthread_cleanup_pop); + int pthread_equal(pthread_t t1, pthread_t t2) { Index: lib/libc/thread/rthread_once.c =================================================================== RCS file: /cvs/src/lib/libc/thread/rthread_once.c,v retrieving revision 1.1 diff -u -p -r1.1 rthread_once.c --- lib/libc/thread/rthread_once.c 15 Aug 2017 06:13:24 -0000 1.1 +++ lib/libc/thread/rthread_once.c 9 Oct 2017 12:56:07 -0000 @@ -18,12 +18,20 @@ #include <pthread.h> +static void +once_cleanup(void *mutex) +{ + pthread_mutex_unlock(mutex); +} + int pthread_once(pthread_once_t *once_control, void (*init_routine)(void)) { pthread_mutex_lock(&once_control->mutex); if (once_control->state == PTHREAD_NEEDS_INIT) { + pthread_cleanup_push(once_cleanup, &once_control->mutex); init_routine(); + pthread_cleanup_pop(0); once_control->state = PTHREAD_DONE_INIT; } pthread_mutex_unlock(&once_control->mutex); Index: lib/librthread/Symbols.map =================================================================== RCS file: /cvs/src/lib/librthread/Symbols.map,v retrieving revision 1.4 diff -u -p -r1.4 Symbols.map --- lib/librthread/Symbols.map 5 Sep 2017 02:40:54 -0000 1.4 +++ lib/librthread/Symbols.map 9 Oct 2017 12:56:07 -0000 @@ -29,8 +29,6 @@ pthread_barrierattr_init; pthread_barrierattr_setpshared; pthread_cancel; - pthread_cleanup_pop; - pthread_cleanup_push; pthread_create; pthread_detach; pthread_getconcurrency; Index: lib/librthread/pthread.h =================================================================== RCS file: /cvs/src/lib/librthread/pthread.h,v retrieving revision 1.4 diff -u -p -r1.4 pthread.h --- lib/librthread/pthread.h 5 Sep 2017 02:40:54 -0000 1.4 +++ lib/librthread/pthread.h 9 Oct 2017 12:56:07 -0000 @@ -51,8 +51,6 @@ PROTO_STD_DEPRECATED(pthread_barrierattr PROTO_STD_DEPRECATED(pthread_barrierattr_init); PROTO_STD_DEPRECATED(pthread_barrierattr_setpshared); PROTO_STD_DEPRECATED(pthread_cancel); -PROTO_STD_DEPRECATED(pthread_cleanup_pop); -PROTO_STD_DEPRECATED(pthread_cleanup_push); PROTO_STD_DEPRECATED(pthread_condattr_getclock); PROTO_STD_DEPRECATED(pthread_condattr_setclock); PROTO_STD_DEPRECATED(pthread_create); Index: lib/librthread/rthread.c =================================================================== RCS file: /cvs/src/lib/librthread/rthread.c,v retrieving revision 1.96 diff -u -p -r1.96 rthread.c --- lib/librthread/rthread.c 5 Sep 2017 02:40:54 -0000 1.96 +++ lib/librthread/rthread.c 9 Oct 2017 12:56:07 -0000 @@ -494,36 +494,6 @@ pthread_setcanceltype(int type, int *old return (0); } -void -pthread_cleanup_push(void (*fn)(void *), void *arg) -{ - struct rthread_cleanup_fn *clfn; - pthread_t self = pthread_self(); - - clfn = calloc(1, sizeof(*clfn)); - if (!clfn) - return; - clfn->fn = fn; - clfn->arg = arg; - clfn->next = self->cleanup_fns; - self->cleanup_fns = clfn; -} - -void -pthread_cleanup_pop(int execute) -{ - struct rthread_cleanup_fn *clfn; - pthread_t self = pthread_self(); - - clfn = self->cleanup_fns; - if (clfn) { - self->cleanup_fns = clfn->next; - if (execute) - clfn->fn(clfn->arg); - free(clfn); - } -} - int pthread_getconcurrency(void) { Index: lib/librthread/shlib_version =================================================================== RCS file: /cvs/src/lib/librthread/shlib_version,v retrieving revision 1.25 diff -u -p -r1.25 shlib_version --- lib/librthread/shlib_version 5 Sep 2017 02:40:54 -0000 1.25 +++ lib/librthread/shlib_version 9 Oct 2017 12:56:07 -0000 @@ -1,2 +1,2 @@ -major=24 +major=25 minor=0 Index: regress/lib/libpthread/Makefile =================================================================== RCS file: /cvs/src/regress/lib/libpthread/Makefile,v retrieving revision 1.48 diff -u -p -r1.48 Makefile --- regress/lib/libpthread/Makefile 7 Jul 2017 23:55:21 -0000 1.48 +++ regress/lib/libpthread/Makefile 9 Oct 2017 12:56:07 -0000 @@ -18,7 +18,7 @@ SUBDIR+= barrier blocked_shutdown \ group netdb pcap poll preemption preemption_float \ pthread_atfork pthread_cond_timedwait pthread_create \ pthread_join pthread_kill pthread_mutex \ - pthread_rwlock pthread_specific \ + pthread_once pthread_rwlock pthread_specific \ readdir restart \ select semaphore setjmp sigdeliver siginfo \ siginterrupt signal signals signodefer sigsuspend sigwait sleep \ Index: regress/lib/libpthread/pthread_once/Makefile =================================================================== RCS file: regress/lib/libpthread/pthread_once/Makefile diff -N regress/lib/libpthread/pthread_once/Makefile --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ regress/lib/libpthread/pthread_once/Makefile 9 Oct 2017 12:56:07 -0000 @@ -0,0 +1,5 @@ +# $OpenBSD$ + +PROG= pthread_once + +.include <bsd.regress.mk> Index: regress/lib/libpthread/pthread_once/pthread_once.c =================================================================== RCS file: regress/lib/libpthread/pthread_once/pthread_once.c diff -N regress/lib/libpthread/pthread_once/pthread_once.c --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ regress/lib/libpthread/pthread_once/pthread_once.c 9 Oct 2017 12:56:07 -0000 @@ -0,0 +1,67 @@ +/* $OpenBSD$ */ + +/* + * Scott Cheloha <scottchel...@gmail.com>, 2017. Public Domain. + */ + +#include <pthread.h> +#include <unistd.h> + +#include "test.h" + +pthread_once_t once_control = PTHREAD_ONCE_INIT; +int done = 0; +int more_than_once = 0; + +void +init_routine(void) +{ + static int first = 1; + + if (first) { + first = 0; + CHECKr(pthread_cancel(pthread_self())); + pthread_testcancel(); + } + if (done) + more_than_once = 1; + done = 1; +} + +void * +thread_main(void *arg) +{ + pthread_once(&once_control, init_routine); + return NULL; +} + +int +main(int argc, char *argv[]) +{ + pthread_t tid; + + /* + * The secondary thread should cancel itself from init_routine, + * leaving it incomplete (!done). + */ + CHECKr(pthread_create(&tid, NULL, thread_main, NULL)); + CHECKr(pthread_join(tid, NULL)); + ASSERT(!done); + + /* + * The main thread should then be able to call and return from + * init_routine, leaving it completed (done). + * + * This should not take very long at all. If the alarm goes off, + * something is likely wrong. + */ + alarm(5); + CHECKr(pthread_once(&once_control, init_routine)); + alarm(0); + ASSERT(done); + + /* init_routine, once returned from, should not be called again. */ + CHECKr(pthread_once(&once_control, init_routine)); + ASSERT(!more_than_once); + SUCCEED; +}
