Re: glibc 2.21 - Machine maintainers, please test your machines.

[Torvald Riegel <triegel at redhat dot com>]

On Sun, 2015-01-25 at 11:45 -0500, Chris Metcalf wrote:
> On 1/25/2015 10:18 AM, Chris Metcalf wrote:
> > I'm now running with the following change, to see if tilegx32 will
> > also pass with it; this implements my suggestion of rounding new_sem to
> > an 8-byte boundary explicitly on ILP32 platforms. 
> 
> With my proposed change, tilegx32 (and tilegx64) pass all the tests.  Repeated
> here with a suitable ChangeLog.  Let me know if this is acceptable to commit
> for 2.21.

I think this is the right direction, but I have a few comments below.

> 
> 2015-01-25  Chris Metcalf  <cmetcalf@ezchip.com>
> 
>          * sysdeps/nptl/internaltypes.h (to_new_sem): Define.  Provides new
>          behavior for [__HAVE_64B_ATOMICS && !defined (_LP64)].
>          * nptl/sem_getvalue.c (__new_sem_getvalue): Use to_new_sem.
>          * nptl/sem_init.c (__new_sem_init): Likewise.
>          * nptl/sem_post.c (__new_sem_post): Likewise.
>          * nptl/sem_wait.c (__new_sem_wait): Likewise.
>          (__new_sem_trywait): Likewise.
>          * nptl/sem_timedwait.c (sem_timedwait): Likewise.
>          * nptl/sem_open.c (sem_open): Add comment about to_new_sem.
> 
> diff --git a/nptl/sem_getvalue.c b/nptl/sem_getvalue.c
> index 1432cc7..08a0789 100644
> --- a/nptl/sem_getvalue.c
> +++ b/nptl/sem_getvalue.c
> @@ -25,7 +25,7 @@
>   int
>   __new_sem_getvalue (sem_t *sem, int *sval)
>   {
> -  struct new_sem *isem = (struct new_sem *) sem;
> +  struct new_sem *isem = to_new_sem (sem);
> 
>     /* XXX Check for valid SEM parameter.  */
>     /* FIXME This uses relaxed MO, even though POSIX specifies that this function
> diff --git a/nptl/sem_init.c b/nptl/sem_init.c
> index 575b661..aaa6af8 100644
> --- a/nptl/sem_init.c
> +++ b/nptl/sem_init.c
> @@ -50,7 +50,7 @@ __new_sem_init (sem_t *sem, int pshared, unsigned int value)
>       }
> 
>     /* Map to the internal type.  */
> -  struct new_sem *isem = (struct new_sem *) sem;
> +  struct new_sem *isem = to_new_sem (sem);
> 
>     /* Use the values the caller provided.  */
>   #if __HAVE_64B_ATOMICS
> diff --git a/nptl/sem_open.c b/nptl/sem_open.c
> index bfd2dea..9c1f279 100644
> --- a/nptl/sem_open.c
> +++ b/nptl/sem_open.c
> @@ -186,7 +186,9 @@ sem_open (const char *name, int oflag, ...)
>            return SEM_FAILED;
>          }
> 
> -      /* Create the initial file content.  */
> +      /* Create the initial file content.  Note that the new_sem

I think this should be NEWSEM.

> +         will have the necessary alignment in this case, so we are
> +         not obliged to use the to_new_sem macro in this case.  */

I think we should just use to_new_sem here too for consistency, but
change the comment to point out that the address to which we will copy
newsem via the file write and mmap later will have a compatible
alignment with the natural alignment of NEWSEM.

>         union
>         {
>          sem_t initsem;
> diff --git a/nptl/sem_post.c b/nptl/sem_post.c
> index 6e495ed..71818ea 100644
> --- a/nptl/sem_post.c
> +++ b/nptl/sem_post.c
> @@ -56,7 +56,7 @@ futex_wake (unsigned int* futex, int processes_to_wake, int private)
>   int
>   __new_sem_post (sem_t *sem)
>   {
> -  struct new_sem *isem = (struct new_sem *) sem;
> +  struct new_sem *isem = to_new_sem (sem);
>     int private = isem->private;
> 
>   #if __HAVE_64B_ATOMICS
> diff --git a/nptl/sem_timedwait.c b/nptl/sem_timedwait.c
> index 042b0ac..5e650e0 100644
> --- a/nptl/sem_timedwait.c
> +++ b/nptl/sem_timedwait.c
> @@ -30,8 +30,8 @@ sem_timedwait (sem_t *sem, const struct timespec *abstime)
>         return -1;
>       }
> 
> -  if (__new_sem_wait_fast ((struct new_sem *) sem, 0) == 0)
> +  if (__new_sem_wait_fast (to_new_sem (sem), 0) == 0)
>       return 0;
>     else
> -    return __new_sem_wait_slow((struct new_sem *) sem, abstime);
> +    return __new_sem_wait_slow(to_new_sem (sem), abstime);
>   }
> diff --git a/nptl/sem_wait.c b/nptl/sem_wait.c
> index c1fd10c..3dade0c 100644
> --- a/nptl/sem_wait.c
> +++ b/nptl/sem_wait.c
> @@ -22,10 +22,10 @@
>   int
>   __new_sem_wait (sem_t *sem)
>   {
> -  if (__new_sem_wait_fast ((struct new_sem *) sem, 0) == 0)
> +  if (__new_sem_wait_fast (to_new_sem (sem), 0) == 0)
>       return 0;
>     else
> -    return __new_sem_wait_slow((struct new_sem *) sem, NULL);
> +    return __new_sem_wait_slow(to_new_sem (sem), NULL);
>   }
>   versioned_symbol (libpthread, __new_sem_wait, sem_wait, GLIBC_2_1);
> 
> @@ -65,7 +65,7 @@ __new_sem_trywait (sem_t *sem)
>   {
>     /* We must not fail spuriously, so require a definitive result even if this
>        may lead to a long execution time.  */
> -  if (__new_sem_wait_fast ((struct new_sem *) sem, 1) == 0)
> +  if (__new_sem_wait_fast (to_new_sem (sem), 1) == 0)
>       return 0;
>     __set_errno (EAGAIN);
>     return -1;
> diff --git a/sysdeps/nptl/internaltypes.h b/sysdeps/nptl/internaltypes.h
> index 8f5cfa4..45ba99a 100644
> --- a/sysdeps/nptl/internaltypes.h
> +++ b/sysdeps/nptl/internaltypes.h
> @@ -168,6 +168,22 @@ struct new_sem
>   #endif
>   };
> 
> +/* The sem_t alignment is typically just 4 bytes for ILP32, whereas
> +   new_sem is 8 bytes.  For better atomic performance on platforms
> +   that tolerate unaligned atomics (and to make it work at all on
> +   platforms that don't), round up the new_sem pointer within the
> +   sem_t object to an 8-byte boundary.
> +
> +   Note that in this case, the "pad" variable at the end of the
> +   new_sem object extends beyond the end of the memory allocated
> +   for the sem_t, so it's important that it not be initialized
> +   or otherwise used.  */

Please just remove "pad", and add a comment like this:
Note that sem_t is always at least 16 bytes in size, so moving new_sem,
which is 12 bytes in size, to offset of 4 bytes within sem_t is okay.
Also, a bitwise memcpy of sem_t is not allowed, and when mapping the
page a sem_t is in to another page, the offset within the page will not
change.

(BTW, I believe that a memcpy is not allowed -- but I couldn't point out
where in POSIX this is required.  Does somebody know, just to be sure?)

> +#if __HAVE_64B_ATOMICS && !defined (_LP64)
> +# define to_new_sem(s) ((struct new_sem *)(((uintptr_t)(s) + 4) & -8))
> +#else
> +# define to_new_sem(s) ((struct new_sem *)(s))
> +#endif
> +

Is the compiler aware that if s is actually of a type that is properly
aligned, this can be a noop?  If not, have you considered using
__alignof__ ?  This would help on new targets that are ILP32 with 64b
atomics and that align sem_t properly.