RFR: 8290324: Move atomic operations outside of os_xxx.hpp [v2]

Ioi Lam iklam at openjdk.org
Mon Jul 18 21:37:20 UTC 2022


On Mon, 18 Jul 2022 20:54:22 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
>> 
>>  - Merge branch 'master' into 8290324-move-atomic-ops-outside-os-hpp
>>  - 8290324: Move atomic operations outside of os_xxx.hpp
>
> src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 587:
> 
>> 585: 
>> 586:   inline void atomic_copy64(const volatile void *src, volatile void *dst) {
>> 587:     *(jlong *) dst = *(const jlong *) src;
> 
> s/jlong/uint64_t/ (or int64_t).  And casting away volatile seems kind of sketchy.  This could be
> 
> Atomic::store((uint64_t*)dst, Atomic::load((const uint64_t*)src));
> 
> which seems clearer to me.

This is also from the original version. I think we can fix this in a separate PR.

> src/hotspot/os_cpu/bsd_zero/atomic_bsd_zero.hpp line 312:
> 
>> 310:   STATIC_ASSERT(8 == sizeof(T));
>> 311:   volatile int64_t dest;
>> 312:   atomic_copy64(reinterpret_cast<const volatile int64_t*>(src), reinterpret_cast<volatile int64_t*>(&dest));
> 
> [pre-existing] Why do we have these casts when `atomic_copy64` takes `volatile void*`?  (There are more like this that I didn't bother to comment directly.)

To be honest I don't know :-)

-------------

PR: https://git.openjdk.org/jdk/pull/9501


More information about the hotspot-runtime-dev mailing list