(S) RFR: 8157904: Atomic::cmpxchg for jbyte is missing a fence on initial failure

David Holmes david.holmes at oracle.com
Wed Aug 24 05:21:56 UTC 2016

Hi Kim,

Thanks for looking at this.

Webrev updated in-place. Comments inline.

On 24/08/2016 6:25 AM, Kim Barrett wrote:
>> On Aug 23, 2016, at 4:55 AM, David Holmes <david.holmes at oracle.com> wrote:
>> Hi Volker, Andrew,
>> On 23/08/2016 12:27 AM, Volker Simonis wrote:
>>> Hi,
>>> I don't particularly like the const_casts as well.
>> I would have thought this was exactly the kind of thing const_cast was good for - avoiding the need to define multiple overloads to deal with volatile, non-volatile, const etc.
>>> Why not change pointer_delta to accept pointers to volatiles as well:
>>> pointer_delta(const volatile void* left, const volatile void* right,
>> I can do that. I also have to make a similar change to align_ptr_down. Now should I also change align_ptr_up for consistency (though I note they are already inconsistent in that one takes void* and one takes const void*) ?
>> Alternative webrev at:
>> http://cr.openjdk.java.net/~dholmes/8157904/webrev.v2/
> ------------------------------------------------------------------------------
> src/share/vm/runtime/atomic.hpp
>  155   assert(sizeof(jbyte) == 1, "assumption");
> STATIC_ASSERT would be better here.


> ------------------------------------------------------------------------------
> src/share/vm/utilities/globalDefinitions.hpp
>  524 inline void* align_ptr_down(volatile void* ptr, size_t alignment) {
>  525   return (void*)align_size_down((intptr_t)ptr, (intptr_t)alignment);
>  526 }
> I think implicitly (to the caller of align_ptr_down) casting away
> volatile like this is a mistake.  I disagree with the rationale for
> this change; stripping off volatile (or const) *should* be annoyingly
> in your face with a const_cast.

Yep my bad - volatile in, volatile out:

inline volatile void* align_ptr_down(volatile void* ptr, size_t alignment) {
   return (volatile void*)align_size_down((intptr_t)ptr, 

This also leads to a change to the static_cast to be "volatile jint*".

> The addition of volatile to pointer_delta is not the same sort of
> thing.  I think that change is good, except I agree with Volker that
> only the one version is needed.

Fixed. I hadn't appreciated what Volker was saying about one version.

> ------------------------------------------------------------------------------
> Otherwise looks good to me.
> Regarding:
>   Now should I also change align_ptr_up for consistency (though I note
>   they are already inconsistent in that one takes void* and one takes
>   const void*) ?
> I think there should be two overloads of each of these, one with const
> qualified argument and result, and one without const qualification for
> either.  That way the result has the same const-ness as the argument.
> We could double the number of overloads by similarly dealing with
> volatile, but I doubt there are enough relevant callers for that to be
> worthwhile; just use const_cast to deal with volatile at the call
> sites.  But this is all a different issue...

Agreed - separate issue if when it becomes an issue.


> Another option would be to make the argument and result
> const-qualified, and make callers deal with the result, but there are
> probably enough call sites to make the second overload worthwhile.

More information about the hotspot-runtime-dev mailing list