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

[David Holmes]

Thanks Volker!

Just waiting for Kim to give the all clear.

David

On 24/08/2016 10:56 PM, Volker Simonis wrote:
> 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.
>>>
>>>
>>