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

[David Holmes]

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.

Changed.

> ------------------------------------------------------------------------------
> 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, 
(intptr_t)alignment);
}

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.

Thanks,
David

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