[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: (S) RFR: 8157904: Atomic::cmpxchg for jbyte is missing a fence on initial failure
From:       "Daniel D. Daugherty" <daniel.daugherty () oracle ! com>
Date:       2016-08-29 22:26:25
Message-ID: 45c5d86b-7e44-7bda-7e99-3ac3105a57e4 () oracle ! com
[Download RAW message or body]

On 8/23/16 11:21 PM, David Holmes wrote:
> Hi Kim,
>
> Thanks for looking at this.
>
> Webrev updated in-place. Comments inline.

Yes, I know this already pushed. Sorry for the late review.

 > http://cr.openjdk.java.net/~dholmes/8157904/webrev.v2/

src/share/vm/runtime/atomic.hpp
     No comments.

src/share/vm/utilities/globalDefinitions.hpp
     No comments.

Thumbs up! Thanks for adding more comments to make it more
clear what this code is doing.

Dan


>
> On 24/08/2016 6:25 AM, Kim Barrett wrote:
>>> On Aug 23, 2016, at 4:55 AM, David Holmes <david.holmes@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.
>>
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]