RFR: 8166651: OrderAccess::load_acquire &etc should have const parameters (original) (raw)

Andrew Dinn adinn at redhat.com
Thu May 25 15:54:56 UTC 2017

Hi Kim,

On 25/05/17 15:41, Andrew Dinn wrote:

On 25/05/17 00:42, Kim Barrett wrote:

_Aside regarding aarch64: I don't know why gcc's atomicload 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 _atomicload rather than atomicloadn.

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

More information about the hotspot-dev mailing list