RFR: 8224851: AArch64: fix warnings and errors with Clang and GCC 8.3

[Andrew Haley]

On 6/3/19 7:58 AM, Nick Gasson wrote:
> 
>> If I'm reading the gcc documentation for __sync_add_and_fetch
>> correctly, I think the following (completely untested) should work,
>> and won't cause Andrew to (I think rightly) complain about
>> type-punning reinterpret_casts.
>>
>> Though I wonder if this is a clang bug. (See below.)

It's arguably a mistake in the GCC documentation, but adding two
pointers is not well defined whereas adding a pointer and an integer
is.

>> template<size_t byte_size>
>> struct Atomic::PlatformAdd
>>   : Atomic::AddAndFetch<Atomic::PlatformAdd<byte_size> >
>> {
>>   template<typename I, typename D>
>>   D add_and_fetch(I add_value, D volatile* dest, atomic_memory_order order) const {
>>     // Cast to work around clang pointer arithmetic bug.
>>     return PrimitiveConversions::cast<D>(__sync_add_and_fetch(dest, add_value));
>>   }
>> }
> 
> This fails with:
> 
> /home/nicgas01/jdk/src/hotspot/share/metaprogramming/primitiveConversions.hpp:162:10: error: invalid use of incomplete type 'struct PrimitiveConversions::Cast<char*, char*, true, void>'
>    return Cast<T, U>()(x);
>           ^~~~~~~~~~~~
> 
> I think because all the specialisations of PrimitiveConversions are only
> enabled if T is an integral type and here it's char*?
> 
>>
>> The key bit from the documentation is "Operations on pointer arguments
>> are performed as if the operands were of the uintptr_t type."
>>
>> The bug description says this warning only arises with clang.  It
>> seems like clang is overdoing that as-if, and treating it literally as
>> uintptr_t all the way through to the result type.
>>
> 
> I think Clang is actually complaining about the argument types
> mismatching here. According to the GCC docs the __sync_add_and_fetch
> prototype looks like:
> 
> type __sync_add_and_fetch (type *ptr, type value, ...)
> 
> I guess Clang infers type=char* from the first argument, and complains
> that the second argument is unsigned long. So Clang gives an error for
> the following but GCC accepts it:
> 
>   char *ptr;
>   unsigned long val;
>   __sync_add_and_fetch(&ptr, val);
> 
> Another way round this is to use __atomic_add_fetch instead which both
> Clang and GCC accept:
> 
>   template<typename I, typename D>
>   D add_and_fetch(I add_value, D volatile* dest, atomic_memory_order order) const {
>     D res = __atomic_add_fetch(dest, add_value, __ATOMIC_RELEASE);
>     FULL_MEM_BARRIER;
>     return res;
>   }

I could live with that.

>> Or don't change the code at all here, and say that clang is not
>> (currently) a supported compiler for this platform.
> 
> Yes, that's an option too :-). But we should change the configure script
> to give an error on --with-toolchain-type=clang on AArch64.

I don't think there's any need to boycott the whole compiler because
of this bug. It would make more sense to try to get clang fixed.

Maybe for now put the cast in #ifdef CLANG ?

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671