> Date: Tue, 29 Dec 2020 12:21:25 +0100
> From: Otto Moerbeek <[email protected]>
>
> Hi,
>
> this is a continuation from the threads on bugs@
>
> This version makes it explicit to *only* setup "TLS" (which actually
> is just a pointer to static data) with the data provided if we're
> running single threaded (ie.. no -pthread or -pthread but no pthread
> function that triggers multi-threaded init called yet). In all other
> cases the real allocated TLS is zero'ed. This avoids the setup issue
> asr is having. This diff also makes sure asr uses a specialized
> destructor for it's TLS to kill a mem leak that occurs on thread
> destruction.
>
> All in all I'm still a bit fond of the constructor approach to
> librthread init: it makes the distinction between having a single
> thread and being the main thread in a multi-threaded setup moot.
>
> Anyway, we can consider that later, please test this.
This isn't right. The memcpy() call you're removing is there such
that the contents of the per-thread storage for the initial (main)
thread remains the same.
With your change it reverts back to zero when the first thread is
created. In the particular case at hand that means the initial thread
would get a fresh resolver context at that point.
> Index: asr/asr.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/asr/asr.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 asr.c
> --- asr/asr.c 6 Jul 2020 13:33:05 -0000 1.64
> +++ asr/asr.c 29 Dec 2020 11:10:08 -0000
> @@ -117,7 +117,7 @@ _asr_resolver_done(void *arg)
> _asr_ctx_unref(ac);
> return;
> } else {
> - priv = _THREAD_PRIVATE(_asr, _asr, &_asr);
> + priv = _THREAD_PRIVATE_DT(_asr, _asr, NULL, &_asr);
> if (*priv == NULL)
> return;
> asr = *priv;
> @@ -128,6 +128,21 @@ _asr_resolver_done(void *arg)
> free(asr);
> }
>
> +static void
> +_asr_resolver_done_tp(void *arg)
> +{
> + struct asr **priv = arg;
> + struct asr *asr;
> +
> + if (*priv == NULL)
> + return;
> + asr = *priv;
> +
> + _asr_ctx_unref(asr->a_ctx);
> + free(asr);
> + free(priv);
> +}
> +
> void *
> asr_resolver_from_string(const char *str)
> {
> @@ -349,7 +364,8 @@ _asr_use_resolver(void *arg)
> }
> else {
> DPRINT("using thread-local resolver\n");
> - priv = _THREAD_PRIVATE(_asr, _asr, &_asr);
> + priv = _THREAD_PRIVATE_DT(_asr, _asr, _asr_resolver_done_tp,
> + &_asr);
> if (*priv == NULL) {
> DPRINT("setting up thread-local resolver\n");
> *priv = _asr_resolver();
> Index: include/thread_private.h
> ===================================================================
> RCS file: /cvs/src/lib/libc/include/thread_private.h,v
> retrieving revision 1.35
> diff -u -p -r1.35 thread_private.h
> --- include/thread_private.h 13 Feb 2019 13:22:14 -0000 1.35
> +++ include/thread_private.h 29 Dec 2020 11:10:08 -0000
> @@ -51,7 +51,7 @@ PROTO_NORMAL(_malloc_init);
> * tc_tag_storage:
> * Returns a pointer to per-thread instance of data associated
> * with the given tag. If the given tag is NULL a tag is
> - * allocated and initialized automatically.
> + * allocated and cleared automatically.
> *
> * tc_fork, tc_vfork:
> * If not NULL, they are called instead of the syscall stub, so that
> @@ -98,7 +98,7 @@ struct thread_callbacks {
> void (*tc_mutex_destroy)(void **);
> void (*tc_tag_lock)(void **);
> void (*tc_tag_unlock)(void **);
> - void *(*tc_tag_storage)(void **, void *, size_t, void *);
> + void *(*tc_tag_storage)(void **, size_t, void (*)(void *), void *);
> __pid_t (*tc_fork)(void);
> __pid_t (*tc_vfork)(void);
> void (*tc_thread_release)(struct pthread *);
> @@ -142,6 +142,7 @@ __END_HIDDEN_DECLS
> #define _THREAD_PRIVATE_MUTEX_LOCK(name) do {} while (0)
> #define _THREAD_PRIVATE_MUTEX_UNLOCK(name) do {} while (0)
> #define _THREAD_PRIVATE(keyname, storage, error) &(storage)
> +#define _THREAD_PRIVATE_DT(keyname, storage, dt, error) &(storage)
> #define _MUTEX_LOCK(mutex) do {} while (0)
> #define _MUTEX_UNLOCK(mutex) do {} while (0)
> #define _MUTEX_DESTROY(mutex) do {} while (0)
> @@ -168,7 +169,12 @@ __END_HIDDEN_DECLS
> #define _THREAD_PRIVATE(keyname, storage, error) \
> (_thread_cb.tc_tag_storage == NULL ? &(storage) : \
> _thread_cb.tc_tag_storage(&(__THREAD_NAME(keyname)), \
> - &(storage), sizeof(storage), error))
> + sizeof(storage), NULL, (error)))
> +
> +#define _THREAD_PRIVATE_DT(keyname, storage, dt, error)
> \
> + (_thread_cb.tc_tag_storage == NULL ? &(storage) : \
> + _thread_cb.tc_tag_storage(&(__THREAD_NAME(keyname)), \
> + sizeof(storage), (dt), (error)))
>
> /*
> * Macros used in libc to access mutexes.
> Index: thread/rthread_cb.h
> ===================================================================
> RCS file: /cvs/src/lib/libc/thread/rthread_cb.h,v
> retrieving revision 1.2
> diff -u -p -r1.2 rthread_cb.h
> --- thread/rthread_cb.h 5 Sep 2017 02:40:54 -0000 1.2
> +++ thread/rthread_cb.h 29 Dec 2020 11:10:08 -0000
> @@ -35,5 +35,5 @@ void _thread_mutex_unlock(void **);
> void _thread_mutex_destroy(void **);
> void _thread_tag_lock(void **);
> void _thread_tag_unlock(void **);
> -void *_thread_tag_storage(void **, void *, size_t, void *);
> +void *_thread_tag_storage(void **, size_t, void (*)(void*), void *);
> __END_HIDDEN_DECLS
> Index: thread/rthread_libc.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/thread/rthread_libc.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 rthread_libc.c
> --- thread/rthread_libc.c 10 Jan 2019 18:45:33 -0000 1.3
> +++ thread/rthread_libc.c 29 Dec 2020 11:10:08 -0000
> @@ -31,7 +31,7 @@ static pthread_mutex_t _thread_tag_mutex
> * This function will never return NULL.
> */
> static void
> -_thread_tag_init(void **tag)
> +_thread_tag_init(void **tag, void (*dt)(void *))
> {
> struct _thread_tag *tt;
> int result;
> @@ -42,7 +42,8 @@ _thread_tag_init(void **tag)
> tt = malloc(sizeof *tt);
> if (tt != NULL) {
> result = pthread_mutex_init(&tt->m, NULL);
> - result |= pthread_key_create(&tt->k, free);
> + result |= pthread_key_create(&tt->k, dt ? dt :
> + free);
> *tag = tt;
> }
> }
> @@ -62,7 +63,7 @@ _thread_tag_lock(void **tag)
>
> if (__isthreaded) {
> if (*tag == NULL)
> - _thread_tag_init(tag);
> + _thread_tag_init(tag, NULL);
> tt = *tag;
> if (pthread_mutex_lock(&tt->m) != 0)
> _rthread_debug(1, "tag mutex lock failure");
> @@ -79,7 +80,7 @@ _thread_tag_unlock(void **tag)
>
> if (__isthreaded) {
> if (*tag == NULL)
> - _thread_tag_init(tag);
> + _thread_tag_init(tag, NULL);
> tt = *tag;
> if (pthread_mutex_unlock(&tt->m) != 0)
> _rthread_debug(1, "tag mutex unlock failure");
> @@ -88,28 +89,26 @@ _thread_tag_unlock(void **tag)
>
> /*
> * return the thread specific data for the given tag. If there
> - * is no data for this thread initialize it from 'storage'.
> + * is no data for this thread allocate and clear it.
> * On any error return 'err'.
> */
> void *
> -_thread_tag_storage(void **tag, void *storage, size_t sz, void *err)
> +_thread_tag_storage(void **tag, size_t sz, void (*dt)(void *), void *err)
> {
> struct _thread_tag *tt;
> void *ret;
>
> if (*tag == NULL)
> - _thread_tag_init(tag);
> + _thread_tag_init(tag, dt);
> tt = *tag;
>
> ret = pthread_getspecific(tt->k);
> if (ret == NULL) {
> - ret = malloc(sz);
> + ret = calloc(1, sz);
> if (ret == NULL)
> ret = err;
> else {
> - if (pthread_setspecific(tt->k, ret) == 0)
> - memcpy(ret, storage, sz);
> - else {
> + if (pthread_setspecific(tt->k, ret) != 0) {
> free(ret);
> ret = err;
> }
>
>
