[PATCH 14/19] nptl: Change type of __default_pthread_attr

Carlos O'Donell carlos@redhat.com
Tue Jun 2 03:39:31 GMT 2020


On 5/19/20 6:44 AM, Florian Weimer via Libc-alpha wrote:
> union pthread_attr_transparent has always the correct size, even if
> pthread_attr_t has padding that is not present in struct pthread_attr.
> 
> This should not result in an observable behavioral change.  The
> existing code appears to have been correct, but it was brittle because
> it was not clear which functions were allowed to write to an entire
> pthread_attr_t argument (e.g., by copying it).

OK for master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  nptl/allocatestack.c              | 2 +-
>  nptl/nptl-init.c                  | 4 ++--
>  nptl/pthreadP.h                   | 2 +-
>  nptl/pthread_attr_getstacksize.c  | 2 +-
>  nptl/pthread_create.c             | 8 ++++----
>  nptl/pthread_getattr_default_np.c | 3 +--
>  nptl/pthread_setattr_default_np.c | 6 +++---
>  nptl/vars.c                       | 2 +-
>  8 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index c94980c21c..d16f3d71f8 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -425,7 +425,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
>    else
>      {
>        lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
> -      size = __default_pthread_attr.stacksize;
> +      size = __default_pthread_attr.internal.stacksize;

OK.

>        lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
>      }
>  
> diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
> index 96b1444a01..d4cf20e3d1 100644
> --- a/nptl/nptl-init.c
> +++ b/nptl/nptl-init.c
> @@ -318,8 +318,8 @@ __pthread_initialize_minimal_internal (void)
>    /* Round the resource limit up to page size.  */
>    limit.rlim_cur = ALIGN_UP (limit.rlim_cur, pagesz);
>    lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
> -  __default_pthread_attr.stacksize = limit.rlim_cur;
> -  __default_pthread_attr.guardsize = GLRO (dl_pagesize);
> +  __default_pthread_attr.internal.stacksize = limit.rlim_cur;
> +  __default_pthread_attr.internal.guardsize = GLRO (dl_pagesize);

OK.

>    lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
>  
>  #ifdef SHARED
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 9c6dd41b7c..acc8e88e4a 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -199,7 +199,7 @@ enum
>  
>  
>  /* Default pthread attributes.  */
> -extern struct pthread_attr __default_pthread_attr attribute_hidden;
> +extern union pthread_attr_transparent __default_pthread_attr attribute_hidden;

OK.

>  extern int __default_pthread_attr_lock attribute_hidden;
>  
>  /* Size and alignment of static TLS block.  */
> diff --git a/nptl/pthread_attr_getstacksize.c b/nptl/pthread_attr_getstacksize.c
> index 346b375690..9830a635a6 100644
> --- a/nptl/pthread_attr_getstacksize.c
> +++ b/nptl/pthread_attr_getstacksize.c
> @@ -33,7 +33,7 @@ __pthread_attr_getstacksize (const pthread_attr_t *attr, size_t *stacksize)
>    if (size == 0)
>      {
>        lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
> -      size = __default_pthread_attr.stacksize;
> +      size = __default_pthread_attr.internal.stacksize;

OK.

>        lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
>      }
>    *stacksize = size;
> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index 347d510707..86fbeb5218 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -612,16 +612,16 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>    STACK_VARIABLES;
>  
>    const struct pthread_attr *iattr = (struct pthread_attr *) attr;
> -  struct pthread_attr default_attr;
> +  union pthread_attr_transparent default_attr;

OK.

>    bool destroy_default_attr = false;
>    bool c11 = (attr == ATTR_C11_THREAD);
>    if (iattr == NULL || c11)
>      {
> -      int ret = __pthread_getattr_default_np ((pthread_attr_t *) &default_attr);
> +      int ret = __pthread_getattr_default_np (&default_attr.external);

OK. Uses external type for public API.

>        if (ret != 0)
>  	return ret;
>        destroy_default_attr = true;
> -      iattr = &default_attr;
> +      iattr = &default_attr.internal;

OK.

>      }
>  
>    struct pthread *pd = NULL;
> @@ -852,7 +852,7 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>  
>   out:
>    if (destroy_default_attr)
> -    __pthread_attr_destroy ((pthread_attr_t *) &default_attr);
> +    __pthread_attr_destroy (&default_attr.external);

OK.

>  
>    return retval;
>  }
> diff --git a/nptl/pthread_getattr_default_np.c b/nptl/pthread_getattr_default_np.c
> index 5c99f980e2..f3ce1c2885 100644
> --- a/nptl/pthread_getattr_default_np.c
> +++ b/nptl/pthread_getattr_default_np.c
> @@ -22,8 +22,7 @@ int
>  __pthread_getattr_default_np (pthread_attr_t *out)
>  {
>    lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
> -  int ret = __pthread_attr_copy (out,
> -                                 (pthread_attr_t *) &__default_pthread_attr);
> +  int ret = __pthread_attr_copy (out, &__default_pthread_attr.external);

OK.

>    lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
>    return ret;
>  }
> diff --git a/nptl/pthread_setattr_default_np.c b/nptl/pthread_setattr_default_np.c
> index eb5d24d3bd..c4cfb4e8ef 100644
> --- a/nptl/pthread_setattr_default_np.c
> +++ b/nptl/pthread_setattr_default_np.c
> @@ -68,15 +68,15 @@ pthread_setattr_default_np (const pthread_attr_t *in)
>  
>    /* Preserve the previous stack size (see above).  */
>    if (temp.internal.stacksize == 0)
> -    temp.internal.stacksize = __default_pthread_attr.stacksize;
> +    temp.internal.stacksize = __default_pthread_attr.internal.stacksize;
>  
>    /* Destroy the old attribute structure because it will be
>       overwritten.  */
> -  __pthread_attr_destroy ((pthread_attr_t *) &__default_pthread_attr);
> +  __pthread_attr_destroy (&__default_pthread_attr.external);
>  
>    /* __default_pthread_attr takes ownership, so do not free
>       attrs.internal after this point.  */
> -  __default_pthread_attr = temp.internal;
> +  __default_pthread_attr = temp;

OK.

>  
>    lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
>    return ret;
> diff --git a/nptl/vars.c b/nptl/vars.c
> index b88300d9b4..3696020145 100644
> --- a/nptl/vars.c
> +++ b/nptl/vars.c
> @@ -22,7 +22,7 @@
>  
>  /* Default thread attributes for the case when the user does not
>     provide any.  */
> -struct pthread_attr __default_pthread_attr attribute_hidden;
> +union pthread_attr_transparent __default_pthread_attr attribute_hidden;

OK.

>  
>  /* Mutex protecting __default_pthread_attr.  */
>  int __default_pthread_attr_lock = LLL_LOCK_INITIALIZER;
> 


-- 
Cheers,
Carlos.



More information about the Libc-alpha mailing list