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

[Volker Simonis]

Looks good now.

Thanks,
Volker


On Wed, Aug 24, 2016 at 7:21 AM, David Holmes <david.holmes at oracle.com> wrote:
> 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.
>>
>>
>