Re: [PATCH V2] Initialize pad outside the conditional to prevent uninitialized data warnings.

[Carlos O'Donell <carlos at redhat dot com>]

On 06/26/2018 11:13 AM, Patsy Franklin wrote:
> Updated patch attached.

This version looks good to me.

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

> commit afcfe098889bb3e5e04c662e9047f20864d55f8e
> Author: Patsy Franklin <pfrankli@redhat.com>
> Date:   Tue Jun 26 10:35:03 2018 -0400
> 
>     In sem_open.c,  pad was not initialized when __HAVE_64B_ATOMICS was
>     true.  On some arches this caused valgrind to warn about uninitialized
>     bytes when the struct was written to the file system.

OK, commit message resolves Florian's question about why this raises an error.
It's an error because we write uninitialized bytes to disk which can be seen
by a reader.

>     
>     This patch moves the initialization of pad outside of the
>     conditional.
> 
> diff --git a/ChangeLog b/ChangeLog
> index eaee727..83539e3 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,8 @@
> +2018-06-26  Patsy Franklin  <pfrankli@redhat.com>
> +
> +	* nptl/sem_open.c [!__HAVE_64B_ATOMICS] (sem_open): Don't update pad.
> +	(sem_open): Set sem.newsem.pad to zero for valgrind.
> +

OK.

>  2018-06-26  Florian Weimer  <fweimer@redhat.com>
>  
>  	Run thread shutdown functions in an explicit order.
> diff --git a/nptl/sem_open.c b/nptl/sem_open.c
> index 1d7f142..c5389f6 100644
> --- a/nptl/sem_open.c
> +++ b/nptl/sem_open.c
> @@ -215,10 +215,11 @@ sem_open (const char *name, int oflag, ...)
>        sem.newsem.data = value;
>  #else
>        sem.newsem.value = value << SEM_VALUE_SHIFT;
> -      /* pad is used as a mutex on pre-v9 sparc and ignored otherwise.  */
> -      sem.newsem.pad = 0;
>        sem.newsem.nwaiters = 0;
>  #endif
> +      /* pad is used as a mutex on pre-v9 sparc and ignored otherwise.  */
> +      sem.newsem.pad = 0;

OK.

> +
>        /* This always is a shared semaphore.  */
>        sem.newsem.private = FUTEX_SHARED;
>  

Cheers,
Carlos.