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

[Kim Barrett]

> On May 31, 2019, at 12:11 AM, Nick Gasson <Nick.Gasson at arm.com> wrote:
>>> /home/nicgas01/jdk/src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp:43:39: error: cannot initialize a parameter of type 'char *' with an lvalue of type 'unsigned long'
>>>    return __sync_add_and_fetch(dest, add_value);
>>>                                      ^~~~~~~~~
>>> 
>>> If the add_and_fetch template is instantiated with I=integer and
>>> D=pointer then we need an explicit cast here.
>> 
>> Please don't. Please have a look at where this happens and fix the
>> types there, as appropriate. What do other ports do?
> 
> It's instantiated here:
> 
> /home/nicgas01/jdk/src/hotspot/share/gc/g1/g1PageBasedVirtualSpace.cpp:259:34: note: in instantiation of function template specialization 'Atomic::add<unsigned long, char *>' requested here
>      char* touch_addr = Atomic::add(actual_chunk_size, &_cur_addr) - actual_chunk_size;
>                                 ^
> 
> We're adding an unsigned long to a char* which seems legitimate.
> 
> The other ports except Arm32 and Zero use inline assembly so don't have
> problems with mismatched types. Zero uses __sync_fetch_and_add so would
> hit this too if it used G1.
> 
> I'm not sure how to resolve this. We could add a specialisation of
> PlatformAdd for byte_size=8 and cast to (u)intptr_t?
> 
> template<>
> struct Atomic::PlatformAdd<8>
>  : Atomic::AddAndFetch<Atomic::PlatformAdd<8> >
> {
>  template<typename I, typename D>
>  D add_and_fetch(I add_value, D volatile* dest, atomic_memory_order order) const {
>    STATIC_ASSERT(8 == sizeof(I));
>    STATIC_ASSERT(8 == sizeof(D));
> 
>    typedef typename Conditional<IsSigned<I>::value, intptr_t, uintptr_t>::type PI;
> 
>    PI res = __sync_add_and_fetch(reinterpret_cast<volatile PI *>(dest),
>                                  static_cast<PI>(add_value));
>    return reinterpret_cast<D>(res);
>  }
> }
> 
> I think this is safe.

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.)

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));
  }
}

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.

The documentation also says "That is, they are not scaled by the size
of the type to which the pointer points."  Any needed scaling of
add_value has already been done in the calling Atomic::AddImpl.