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

[Chris Metcalf <cmetcalf at ezchip dot com>]

I'm out and just have my phone but my quick answer is that uint64_t does  have 8-byte alignment on tilegx32 and should force that on a containing struct in the absence of a packed directive. I didn't do anything special with sem_t during the port so it should match x86 etc. 

I'd advocate for fixing the alignment rather than avoiding register-sized 64-bit atomics, as long as it doesn't break the ABI.

Removing the pad for ILP32 seems correct too. 

Thanks. 

> On Jan 24, 2015, at 12:13 PM, Torvald Riegel <triegel@redhat.com> wrote:
> 
>> On Sat, 2015-01-24 at 10:14 -0500, Chris Metcalf wrote:
>>> On 1/23/2015 4:32 PM, Carlos O'Donell wrote:
>>> Dear Machine Maintainers,
>>> 
>>> Please start testing your machines against glibc
>>> master.
>>> 
>>> Please update the glibc 2.21 release page with your
>>> testing results:
>>> 
>>> https://sourceware.org/glibc/wiki/Release/2.21
>>> 
>>> If nobody objects I want to cut the release as soon
>>> as we have results for all the machines.
>>> 
>>> Cheers,
>>> Carlos.
>> 
>> tilegx64 is fine (modulo the one bug compiler bug that has been outstanding for multiple glibc releases now).
>> 
>> tilegx32 has a bunch of new failures, all of which manifest as bus errors:
>> 
>> FAIL: nptl/tst-sem14
>> FAIL: nptl/tst-sem3
>> FAIL: nptl/tst-sem6
>> FAIL: nptl/tst-signal3
>> FAIL: nptl/tst-tls2
>> FAIL: nptl/tst-tls3
>> 
>> I assume these are all from the new semaphore code but have not had an opportunity to look more closely.  One thing that could cause this is if somehow we are trying to do atomic operations on 64-bit values that aren't aligned to an 8-byte boundary.
>> 
> 
> One thing you could test is set __HAVE_64B_ATOMICS to 0 on tilegx32, and
> see whether it makes the semaphore tests pass.  If it avoids the issue,
> what is __alignof (sem_t) on tilegx32?
> 
> struct new_sem (sysdeps/nptl/internaltypes.h) was changed from having an
> "unsigned long int" as biggest field to having "unsigned long long int"
> or "uint64_t" fields.  On the x86_64 ABI, this should both result in an
> 8-byte alignment I believe.
> 
> [I'm still looking at how the different archs define sem_t.  But saw
> your email, so dumping this email here for now...]
> 
> I suppose that's not the case on the tilegx32 ABI?  If it is, we can't
> use the 64b algorithm there if we don't want to change the glibc ABI,
> AFAICT.
> 
> A simple "fix" is to not make 64B atomics available.  However, the issue
> is the alignment change; IOW, whether "unsigned long" is 8B-aligned or
> not.  Adding such a check to struct new_sem and in the semaphore code
> would be better, I suppose.  It would be even better if we would add
> another flag to each atomic.h stating the minimum alignment requirements
> for 64b atomic ops.
> 
> HJ, do we have the same issue on x32?  IOW, same alignment issue, and
> 64b atomics need to be targetting naturally aligned data?
> 
> I'd appreciate feedback on other archs wrt the alignment question and
> alignment requirements for 64b atomics.
> 
> We have several options here, and while I agree there's an issue and I
> hadn't considered the alignment of struct new_sem sufficiently, just
> using the 32b semaphore variant is always an easy option.
> 
> 
> There might be another issue in the 32b variant of struct new_sem, which
> changed like this:
> -  unsigned long int nwaiters;
> +  int pad;
> +  unsigned int nwaiters;
> 
> I'll investigate this further.  The size increase might be an issue.
> 
> Do we know whether all our targets with just 32b atomic operations are
> ILP32?  If yes, then removing the addition of "int pad;" should fix
> this.  If not, then would adding a union like this be most useful?
>  union { unsigned long int dummy; 
> 
> 
> Sorry about the fallout!  I'll try to fix that up quickly.
>