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

[Torvald Riegel <triegel at redhat dot com>]

On Mon, 2015-01-26 at 15:21 -0500, Chris Metcalf wrote:
> On 1/26/2015 7:44 AM, Torvald Riegel wrote:

> >> +         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.
> 
> The diff to make it look like that is more complicated, so I resisted doing
> it, particularly late in the freeze.
> 
> All that said, I'm happy to do v2 that way, and you can see what you think.
> I don't actually think it's necessary to explain that to_new_sem() is a known
> no-op in this case; we're using it just to be consistent anyway.

Sorry, what I wrote was misleading, and incomplete:  I still think using
to_new_sem would be consistent -- but if we do that, we need to force
the same alignment as we will have when effectively copy it via file
write and mmap later on.  I don't think the current code does that.

If we don't use to_new_sem as you had in your patch, then we don't need
to force the alignment -- but in this case we have to make sure that the
code behaves as if to_new_sem didn't adapt the offset of new_sem within
sem_t (which your previous code made sure by using the union).
Nonetheless, I think that then, we need to point out that we must not
use to_new_sem, so that it doesn't assume a wrong alignment (based on
sem).

I hope that is clearer :)  (Sorry, it's been a long day here...)

> >> --- 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.
> 
> The thing is, new_sem is actually 16 bytes in size with __HAVE_64B_ATOMICS,
> because the int64 type causes the size to round up to a multiple of eight.  So I'd
> rather be explicit about that padding in the structure definition, which is why
> I ended up using the language I did.

Good point, sizeof() vs. the amount of bytes actually accessed.

> I'll tweak my comment to discuss the lengths of the non-padding parts of
> the structures more explicitly for v2.

Thanks.

> > 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?)
> 
> I'm not actually sure what you mean about bitwise memcpy here.  I think
> it should be fine to do; and if you memcpy it from an alignof-4 address to
> an alignof-8 address or vice versa, that should be fine, as long as you always
> pass it through to_new_sem() before using the fields.

I don't think so, because in a badly 4B-aligned (ie, no 8B-aligned) use,
struct new_sem is at offset 4 of the semaphore.  If you copy that to a
8B-aligned semaphore just bit-by-bit, then the actual data will still
start at offset 4, but to_new_sem will assume offset 0.

> >> +#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.
> 
> Yes, I agree.  Rather than using the preprocessor, v2 has:
> 
> #define to_new_sem(s)                                                \
>    ((__alignof__ (sem_t) == 4 && __alignof__ (struct new_sem) == 8) ? \
>     (struct new_sem *) (((uintptr_t) (s) + 4) & -8) :                 \
>     (struct new_sem *) (s))
> 
> The compiler evaluates the constant condition and produces no extra
> code for 64-bit targets or for 32-bit targets without __HAVE_64B_ATOMICS.
> 
> We can't use an inline function since sem_t is not defined here,
> and I'm reluctant to try to #include <semaphore.h> at this point.

Good, thanks!