RFR: 8166651: OrderAccess::load_acquire &etc should have const parameters

[Andrew Dinn]

Hi Kim,

On 25/05/17 15:41, Andrew Dinn wrote:
> On 25/05/17 00:42, Kim Barrett wrote:
>> <snip>
>> Aside regarding aarch64: I don't know why gcc's __atomic_load doesn't
>> const-qualify the source argument; that seems like a bug.  Or maybe
>> they are, but not documented that way.  And I wonder why the aarch64
>> port uses __atomic_load rather than __atomic_load_n.

Hmm, this does not appear to be borne out by my experience (see below).

> This is breaking when I compile it on AArch64. Specifically, it's the
> void * atomic load definition in orderAccess_linux_aarch64.inline.hpp
> that is causing the problem. Here is one of many errors:
> . . .
> 
> I'm still puzzling over what is actually needed to do the right job
> here. I can see why you have made the change the way you have but the
> compiler definitely does not want to eat it. I'll play with the code and
> see what does compile.

Ok, so I have several interesting things to report.

Your patch states

  // __atomic_load's ptr parameter is non-const, so need casts.

and then proceeds to insert const_cast into each inline load_acquire
definition:

  inline jbyte    OrderAccess::load_acquire(const volatile jbyte*   p)
  { jbyte data; __atomic_load(const_cast<volatile jbyte*>(p), &data,
__ATOMIC_ACQUIRE); return data; }
  inline jshort   OrderAccess::load_acquire(const volatile jshort*  p)
  { jshort data; __atomic_load(const_cast<volatile jshort*>(p), &data,
__ATOMIC_ACQUIRE); return data; }
  ...

I don't know what version of the compiler or the lib code for
__atomic_load you derived that from (or maybe you read the fine
manual???) but, leaving aside the vodi* flavour method one which blew up
earlier, the const_cast is not needed on my machine for any of the other
load_acquire methods. So, I can compile all of them quite happily with
no const_cast:

  inline jbyte    OrderAccess::load_acquire(const volatile jbyte*   p)
  { jbyte data; __atomic_load(p, &data, __ATOMIC_ACQUIRE); return data; }
  inline jshort   OrderAccess::load_acquire(const volatile jshort*  p)
  { jshort data; __atomic_load(p, &data, __ATOMIC_ACQUIRE); return data; }
  ...

As regards the void* flavour which /does/ blow up the problem does not
seem strictly to be the constness but the fact that you are trying to
pass a void* in a context where a void** is really needed. The const and
volatile qualifiers simply get in the way of making that recast work in
one go. I did manage to get it to compile with two casts:

  inline void*  OrderAccess::load_ptr_acquire(const volatile void* p)
  { void* data; __atomic_load(static_cast<void* volatile
*>(const_cast<volatile void*>(p)), &data, __ATOMIC_ACQUIRE); return data; }

i.e. if you cast away the constness as a prior step then you can happily
static_cast the resulting (volatile void*) to a (void * volatile *). Of
course, I'll happily bow to your superior knowledge of C++ if you think
this is wrong.

> Oddly enough, I have just posted a fix to jdk10-dev (I forwarded the
> original note to hotspot-dev but the discussion now seems to be
> progressing in jdk10-dev -- apologies) that relates to a use of this
> code and to the correct declaration of volatile fields that store
> pointers vs fields that store volatile pointers). It might be worth
> looking at that to see if it bears on this issue.

Please ignore that last remark. The problem was only in jdk8 not jdk9/10.

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander