RFR: 8253970: Build error: address argument to atomic builtin must be a pointer to integer or pointer ('volatile narrowOop *' invalid)

[Kim Barrett]

> On Oct 13, 2020, at 9:27 AM, Jie Fu <jiefu at openjdk.java.net> wrote:
> 
> On Sun, 4 Oct 2020 11:57:34 GMT, Jie Fu <jiefu at openjdk.org> wrote:
> 
>>> What version of clang?
>>> 
>>> gcc (at least recent versions) allows enum types (both scoped and unscoped) for both the __sync_xxx functions and for
>>> the __atomic_xxx functions. (The documentation for both say the type can be an integral scalar or pointer type. Enums
>>> are not integral types.)  So this seems to be a clang incompatibility with gcc, which may be a clang bug.
>>> 
>>> The __sync_xxx functions have been legacy since gcc4.7, having been superseded by the __atomic_xxx functions. Could
>>> zero be updated here?  Would that help? If clang is incompatible with gcc for the __sync_xxx functions, it might also
>>> be incompatible for the __atomic_xxx functions.  BTW, the memory ordering by the zero implementation of Atomic::xchg in
>>> terms of __sync_lock_test_and_set looks wrong to me.  I think the fence() is on the wrong side of the __sync_xxx
>>> operation.
>> 
>> The following two versions of clang can reproduce the bug.
>> 
>> $ clang -v
>> Apple clang version 11.0.0 (clang-1100.0.33.16)
>> Target: x86_64-apple-darwin19.0.0
>> Thread model: posix
>> 
>> # clang -v
>> clang version 10.0.0-4ubuntu1
>> Target: x86_64-pc-linux-gnu
>> Thread model: posix
> 
> Hi @kimbarrett ,
> 
> I replace __sync_val_compare_and_swap whith __atomic_compare_exchange and it really woks.
> Thanks for your help.

Good news that __atomic_compare_exchange works where
__sync_val_compare_and_swap doesn't.

However, __ATOMIC_SEQ_CST doesn't provide the same semantics as the __sync
function, nor the required semantics for HotSpot. I think something like
what's in atomic_linux_aarch64.hpp is needed, i.e.

    T value = compare_value;
    FULL_MEM_BARRIER;
    __atomic_compare_exchange(dest, &value, &exchange_value, /*weak/false,
                              __ATOMIC_RELAXED, __ATOMIC_RELAXED);
    FULL_MEM_BARRIER;
    return value;

See the discussion related to that linux_aarch64 code here:
https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2019-November/008164.html

> I also fix the following bug when building zero VM on MacOS.
> --------------------------------------------------------------
>   * For target hotspot_variant-zero_libjvm_objs_os_bsd_zero.o:
>   ./src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp:176:21: error: no member named 'in_stack_yellow_reserved_zone' in
>   'JavaThread'
>           if (thread->in_stack_yellow_reserved_zone(addr)) {
>               ~~~~~~  ^
>   ./src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp:177:19: error: no member named 'disable_stack_yellow_reserved_zone' in
>   'JavaThread'
>             thread->disable_stack_yellow_reserved_zone();
>             ~~~~~~  ^
>   ./src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp:180:26: error: no member named 'in_stack_red_zone' in 'JavaThread'
>           else if (thread->in_stack_red_zone(addr)) {
>                    ~~~~~~  ^
>   ./src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp:181:19: error: no member named 'disable_stack_red_zone' in 'JavaThread'
>             thread->disable_stack_red_zone();
>             ~~~~~~  ^

I think this should be a separate bug and PR.  This seems to be a result of a recent change
(JDK-8253717, pushed a little less than a week ago) not accounting for bsd_zero; the needed
change seems to have only been made to linux_zero.

> As for the memory ordering of the zero implementation, it might be better to file another bug to fix it.
> What do you think?

I’m okay with that.  The linux_aarch64 implementation looks like a good model for zero.