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

[Kim Barrett]

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

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.

------------------------------------------------------------------------------

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

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.