RFR (M): 8184334: Generalizing Atomic with templates

classic Classic list List threaded Threaded
59 messages Options
123
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR (M): 8184334: Generalizing Atomic with templates

Erik Osterlund
Hi,

Bug:
https://bugs.openjdk.java.net/browse/JDK-8184334 <https://bugs.openjdk.java.net/browse/JDK-8184334>

Webrev:
http://cr.openjdk.java.net/~eosterlund/8184334/webrev.00/ <http://cr.openjdk.java.net/~eosterlund/8184334/webrev.00/>

The Atomic class mostly uses the JNI-specific j-types (jbyte, jshort, jint, jlong). This has led to wide-spread pollution of j-types in the C++ code base.

Another problem is that when dealing with certain inherently not j-typed types like pointers, hacks were built that allowed them using _ptr member functions that had to be specialized on every architecture supported by Java.

Yet another problem is yet another exception, like size_t, that does not quite fit being a pointer and isn't necessarily mapped to any j-type. Then yet more exceptions were built on top. And what about intx? You get the idea by now.

As a remedy to the above mentioned problems, I propose to generalize the Atomic implementation to use templates instead.
The new Atomic API handles pointers, all integer and floating point types that the underlying architecture supports.

To achieve this, a new metaprogramming tool called IntegerTypes was built, that allows us to safely cast between different integral, floating point and pointer types of the same size while preserving the bit pattern of the values. This allows Atomic to take primitive types and canonicalize them to the same integer type that is passed to the platform layer.

As for atomic operations on integral types like add/inc/dec, pointer scaling is now performed. That is, the functional behaviour is modelled after the +=, ++ and -- operators correspondingly. The X_ptr member functions have been deprecated, but are still there and can be used with identical behaviour as they had before. But new code should just use the non-ptr member functions instead.

The platform layer specializes the specialized_X member functions that take canonicalized signed integers as input, and turn them to some platform-specific instruction sequence.
A nice benefit with these changes is that the platform-specific code has gotten a lot of pointless redundancies removed such as how to perform atomic loads and stores on native word or less sized integers, or having inc/dec implementations that just reuse add.

As for testing, new gtests were written for IntegerTypes as well as Atomic to make it more convincing that the functional behaviour works.
Apart from that, I have run JPRT, JTREG, RBT hs-tier3 and a few local tonga test lists including nsk.jdi, nsk.monitoring, nsk.stress, vm.gc on Solaris SPARC T7-4 and Linux AMD64.

As for oracle-external ports, I would appreciate if port maintainers could check if it seems to make sense. I have done my best to try to make the appropriate changes where needed.

Special thanks go to Kim Barrett that has worked closely with me on this and whom I exchanged many emails with, and so I would like him to be a co-author.

Thanks,
/Erik
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Kim Barrett
> On Jul 14, 2017, at 12:28 PM, Erik Österlund <[hidden email]> wrote:
>
> Hi,
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8184334
>
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8184334/webrev.00/

A few small things I overlooked in our pre-review discussions.

------------------------------------------------------------------------------
src/share/vm/metaprogramming/integerTypes.hpp
 225 template<typename T, typename U>
 226 struct IntegerTypes::Cast<T, U, true, true, false> VALUE_OBJ_CLASS_SPEC {
 227   T operator()(U x) const { return (T)((void*)x); }
 228 };

One of my pre-review comments seems to have been passed over here.

I don't think we need the void* cast at all.  And the rest of this
file consistently prefers C++ cast operators over C-style casts,
suggesting the cast to T should be performed using reinterpret_cast.

------------------------------------------------------------------------------
src/share/vm/runtime/atomic.hpp
  29 #include "metaprogramming/conditional.hpp"
  30 #include "metaprogramming/enableIf.hpp"
...
  34 #include "metaprogramming/isSame.hpp"
  35 #include "metaprogramming/removePointer.hpp"

I think these are remnents of earlier versions of this code, and have
ended up not being used here after all.

------------------------------------------------------------------------------
src/os_cpu/linux_x86/vm/atomic_linux_x86.hpp
 143   volatile jlong dest;

jlong => int64_t ?

------------------------------------------------------------------------------
src/os_cpu/linux_arm/vm/atomic_linux_arm.hpp
 150   jlong rv;
jlong => int64_t ?

 176   intptr_t val;
 192   intptr_t old_val;
intptr_t => int64_t ?

------------------------------------------------------------------------------
src/os_cpu/bsd_x86/vm/atomic_bsd_x86.hpp
 143   volatile jlong dest;
jlong => int64_t ?

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Kim Barrett
In reply to this post by Erik Osterlund
> On Jul 14, 2017, at 12:28 PM, Erik Österlund <[hidden email]> wrote:
>
> Hi,
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8184334
>
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8184334/webrev.00/

------------------------------------------------------------------------------
Another pre-review comment that seems to have been missed.  (Your
reply indicated the suggestion was accepted.)

src/share/vm/runtime/atomic.hpp
 250   typedef typename IntegerTypes::Signed<intptr_t>::type Raw;
intptr_t => U*

 269     typedef typename IntegerTypes::Signed<intptr_t>::type Raw;
 286     typedef typename IntegerTypes::Signed<intptr_t>::type Raw;
intptr_t => T*

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Erik Osterlund
Hi Kim,

Thank you for the review. I have now fixed all the issues you mentioned.

New full webrev:
http://cr.openjdk.java.net/~eosterlund/8184334/webrev.01/

New incremental webrev:
http://cr.openjdk.java.net/~eosterlund/8184334/webrev.00_01/

Thanks,
/Erik

On 2017-07-14 20:56, Kim Barrett wrote:

>> On Jul 14, 2017, at 12:28 PM, Erik Österlund <[hidden email]> wrote:
>>
>> Hi,
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8184334
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8184334/webrev.00/
> ------------------------------------------------------------------------------
> Another pre-review comment that seems to have been missed.  (Your
> reply indicated the suggestion was accepted.)
>
> src/share/vm/runtime/atomic.hpp
>   250   typedef typename IntegerTypes::Signed<intptr_t>::type Raw;
> intptr_t => U*
>
>   269     typedef typename IntegerTypes::Signed<intptr_t>::type Raw;
>   286     typedef typename IntegerTypes::Signed<intptr_t>::type Raw;
> intptr_t => T*
>
> ------------------------------------------------------------------------------
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Roman Kennke-6
Hi Erik,

I like the stuff. Cannot find anything wrong in the patch (i.e. ok from
me as non-reviewer), but have some questions:
- Do you plan to fix+remove the now deprecated methods, e.g. store_ptr()
soon?
- Have you given any thought about C++11 atomics? I don't know if that
will ever be feasible, but part of me would find it nice to just use
that in the future? Or at least get/stay as close to it API-wise as
possible?

Roman

Am 17.07.2017 um 11:04 schrieb Erik Österlund:

> Hi Kim,
>
> Thank you for the review. I have now fixed all the issues you mentioned.
>
> New full webrev:
> http://cr.openjdk.java.net/~eosterlund/8184334/webrev.01/
>
> New incremental webrev:
> http://cr.openjdk.java.net/~eosterlund/8184334/webrev.00_01/
>
> Thanks,
> /Erik
>
> On 2017-07-14 20:56, Kim Barrett wrote:
>>> On Jul 14, 2017, at 12:28 PM, Erik Österlund
>>> <[hidden email]> wrote:
>>>
>>> Hi,
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8184334
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8184334/webrev.00/
>> ------------------------------------------------------------------------------
>>
>> Another pre-review comment that seems to have been missed.  (Your
>> reply indicated the suggestion was accepted.)
>>
>> src/share/vm/runtime/atomic.hpp
>>   250   typedef typename IntegerTypes::Signed<intptr_t>::type Raw;
>> intptr_t => U*
>>
>>   269     typedef typename IntegerTypes::Signed<intptr_t>::type Raw;
>>   286     typedef typename IntegerTypes::Signed<intptr_t>::type Raw;
>> intptr_t => T*
>>
>> ------------------------------------------------------------------------------
>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Erik Osterlund
In reply to this post by Erik Osterlund
Hi Andrew,

I am glad that it seems to work. Thank you for reviewing.

As for the over engineered casting mechanism, the IntegerTypes class in
metaprogramming/integerTypes.hpp has lots of commentary about what the
different casting mechanisms do. Hope that helps.

To explain the reasoning, I will go through an example. Let's take
atomic::cmpxchg.
Each platform has a specialization for different supported cmpxchg
implementations in terms of int32_t and int64_t.
Since the Atomic::cmpxchg parameter now takes a generic primitive type
T, we need to cast T without changing its binary representation, and
without losing type safety, into *exactly* those intX_t types. If even
slightly different in any way, the specializations will not match.

So for example, we need to be able to perform the following conversions:
1) Floating point to integer without changing bit representaiton. It
seems like the only way of doing this without violating the C++03
standard is to use memcpy. (no, the union trick in e.g. jfloat_cast is
not standards compliant)
2) Pointer to integer
3) Anything with not matching CV qualifiers to matching CV qualifiers
4) Integer of wrong signedness to matching signedness
5) Integer with same signedness but declared differently, e.g. signed
char is not the same as char, and will miss that specialization. For
example on Solaris, int8_t is char, but jbyte is signed char, and those
types are different. Sometimes even intptr_t is neither int32_t or
int64_t as I have seen on clang.

Due to these issues, the IntegerTypes class was created to solve the
casting problems in one single place while remaining type safe. So it
solves the problem of canonicalizing primitive types and casting them
into exactly the same canonical integer type that can be safely passed
into code that specializes on that canonical integer type, without
changing the bit pattern or slipping in type safety.

By solving this general casting problem problem in IntegerTypes, it
turned the casting issues in Atomic into trivial non-problems, and
hopefully can be used elsewhere in hotspot. For example the jfloat_cast
with friend functions are not standards compliant and can now use
IntegerTypes instead, for that mindblowingly seemless casting experience.

I hope this explanation makes sense.

Thanks,
/Erik

On 2017-07-17 16:21, Andrew Haley wrote:

> On 14/07/17 17:28, Erik Österlund wrote:
>> As for oracle-external ports, I would appreciate if port maintainers
>> could check if it seems to make sense. I have done my best to try to
>> make the appropriate changes where needed.
> It seems to, but it looks over-engineered.  We're relying on the C++
> compiler to get rid of all of the casts and calls, but it's always
> better not to write code we don't want to execute.  So, could you add
> a little commentary explaining what all this SignedType stuff does?  I
> guess it must be necessary or you wouldn't have written it, but I
> don't know why.
>
> Monitor::TryFast
> ->  Atomic::cmpxchg_ptr
>    ->  Atomic::cmpxchg<long, long, long>
>      ->  Atomic::specialized_cmpxchg<long>
>        ->  generic_cmpxchg<long>
>          ->  __sync_val_compare_and_swap
>
> I wonder if it's intended to avoid undefine behaviour by always
> guaranteeing that we store a signed type, but on the other hand
> cmpxchg_ptr doesn't much seem to worry about handing a void** as an
> intptr_t*, so it can't be that, and besides we compile with
> -fno-strict-aliasing (or it equivalent on other machines).
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Aleksey Shipilev-4
In reply to this post by Erik Osterlund
On 07/17/2017 04:21 PM, Andrew Haley wrote:
> On 14/07/17 17:28, Erik Österlund wrote:
>>
>> As for oracle-external ports, I would appreciate if port maintainers
>> could check if it seems to make sense. I have done my best to try to
>> make the appropriate changes where needed.
>
> It seems to, but it looks over-engineered.  We're relying on the C++
> compiler to get rid of all of the casts and calls, but it's always
> better not to write code we don't want to execute.

I have a related question: how are fastdebug builds doing? This is important,
because we run lots of tests on fastdebug builds, and making them slower would
be inconvenient.

Something like the Unsafe.{get|put|CAS} -Xint performance test would be nice to
have.

Thanks,
-Aleksey

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Aleksey Shipilev-4
On 07/17/2017 05:49 PM, Erik Österlund wrote:

>> Something like the Unsafe.{get|put|CAS} -Xint performance test would be nice to
>> have.
>
> Sorry, but I do not understand what you mean here.
>
> 1) The code only boils down to performing casts that do not change any bit
> pattern, and is by contract forced to be expanded and inlined at compile time,
> so I would be very surprised about any regressions.
> 2) This code executes when we are about to perform atomic operations, so any
> accidental code generated for the casting (which seems odd in the first place),
> I would be very surprised to see.
> 3) Even if there was some barely noticeable regression in fastdebug atomics for
> whatever reason, I do not know if I think it makes sense to pollute the code
> with ugly hacks to micro optimize fastdebug execution time.

I agree that fastdebug should not be our primary optimization target. I was
wondering if all the template magic actually works well in fastdebug builds.

Or product builds, for that matter: because while templates are indeed done
during compile-time, say, the inliner that has to deal with new code shapes can
have opinions.

There are some VarHandles perf tests I have lying around, I can check this
myself today.

-Aleksey

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Andrew Haley
In reply to this post by Erik Osterlund
Hi,

On 17/07/17 16:00, Erik Österlund wrote:

> I am glad that it seems to work. Thank you for reviewing.
>
> As for the over engineered casting mechanism, the IntegerTypes class in
> metaprogramming/integerTypes.hpp has lots of commentary about what the
> different casting mechanisms do. Hope that helps.

It doesn't, because most HotSpot programmers know what this stuff
does, but not why you're doing it.

> To explain the reasoning, I will go through an example. Let's take
> atomic::cmpxchg.
> Each platform has a specialization for different supported cmpxchg
> implementations in terms of int32_t and int64_t.

OK, I get that.  Some platforms presumably requires such
implementations, but would it not make more sense to restrict the
complexity to just those platforms?  None of the GCC targets need it.

> So for example, we need to be able to perform the following conversions:
> 1) Floating point to integer without changing bit representaiton. It
> seems like the only way of doing this without violating the C++03
> standard is to use memcpy. (no, the union trick in e.g. jfloat_cast is
> not standards compliant)

I'm not entirely convinced that memcpy does it either for "truly
portable" C++, but perhaps it's as good as we're going to get.

> 2) Pointer to integer
> 3) Anything with not matching CV qualifiers to matching CV qualifiers
> 4) Integer of wrong signedness to matching signedness
> 5) Integer with same signedness but declared differently, e.g. signed
> char is not the same as char, and will miss that specialization. For
> example on Solaris, int8_t is char, but jbyte is signed char, and those
> types are different. Sometimes even intptr_t is neither int32_t or
> int64_t as I have seen on clang.

If you have something like

long n;
long *p = &n;

The you can access the stored value in p with any type compatible with
long*.  However, this:

  Atomic::cmpxchg_ptr(0, &p, &n);

casts &n to (signed) intptr_t: this is guaranteed to work.
Unfortunately, it then calls generic_cmpxchg<long>(0, &p, &n).  This
accesses the stored pointer in p as a long, which is undefined
behaviour.

It works on GCC if we compile everything with -fno-strict-aliasing (at
least if intptr_t has the same representation in memory as every
pointer type, which every machine we care about does) but in that case
there's little point casting pointers back and forth to integer types.

> Due to these issues, the IntegerTypes class was created to solve the
> casting problems in one single place while remaining type safe. So it
> solves the problem of canonicalizing primitive types and casting them
> into exactly the same canonical integer type that can be safely passed
> into code that specializes on that canonical integer type, without
> changing the bit pattern or slipping in type safety.

It is not type safe.

------------------------------------------------------------------------
3.10, Lvalues and rvalues

If a program attempts to access the stored value of an object through
a glvalue of other than one of the following types the behavior is
undefined:

— the dynamic type of the object,

— a cv-qualified version of the dynamic type of the object,

— a type similar (as defined in 4.4) to the dynamic type of the object,

— a type that is the signed or unsigned type corresponding to the
  dynamic type of the object,

— a type that is the signed or unsigned type corresponding to a
  cv-qualified version of the dynamic type of the object,

— an aggregate or union type that includes one of the aforementioned
  types among its elements or non- static data members (including,
  recursively, an element or non-static data member of a subaggregate
  or contained union),

— a type that is a (possibly cv-qualified) base class type of the
  dynamic type of the object,

— a char or unsigned char type.
------------------------------------------------------------------------

You only have permission to convert pointers to intptr_t and back: you
do not have permission to access the stored value of a pointer an an
intptr_t.

> By solving this general casting problem problem in IntegerTypes, it
> turned the casting issues in Atomic into trivial non-problems, and
> hopefully can be used elsewhere in hotspot. For example the jfloat_cast
> with friend functions are not standards compliant and can now use
> IntegerTypes instead, for that mindblowingly seemless casting experience.
>
> I hope this explanation makes sense.

I don't believe that it entirely does, no.  But it is the sort of
commentary which ought to be in the source code.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Aleksey Shipilev-4
In reply to this post by Aleksey Shipilev-4
On 07/17/2017 05:56 PM, Aleksey Shipilev wrote:

> On 07/17/2017 05:49 PM, Erik Österlund wrote:
>>> Something like the Unsafe.{get|put|CAS} -Xint performance test would be nice to
>>> have.
>>
>> Sorry, but I do not understand what you mean here.
>>
>> 1) The code only boils down to performing casts that do not change any bit
>> pattern, and is by contract forced to be expanded and inlined at compile time,
>> so I would be very surprised about any regressions.
>> 2) This code executes when we are about to perform atomic operations, so any
>> accidental code generated for the casting (which seems odd in the first place),
>> I would be very surprised to see.
>> 3) Even if there was some barely noticeable regression in fastdebug atomics for
>> whatever reason, I do not know if I think it makes sense to pollute the code
>> with ugly hacks to micro optimize fastdebug execution time.
>
> I agree that fastdebug should not be our primary optimization target. I was
> wondering if all the template magic actually works well in fastdebug builds.
>
> Or product builds, for that matter: because while templates are indeed done
> during compile-time, say, the inliner that has to deal with new code shapes can
> have opinions.
>
> There are some VarHandles perf tests I have lying around, I can check this
> myself today.

Checked, times are the same for baseline/patched builds with either fastdebug or
release bits. Ditto the instruction counts, number of memory accesses, branches,
etc. Seems good!

Thanks,
-Aleksey

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Thomas Stüfe-2
In reply to this post by Erik Osterlund
Hi Eric, Volker,

builds fine on AIX 5.3 and 7.1.

gtests show no errors.

Kind Regards, Thomas


On Fri, Jul 14, 2017 at 6:28 PM, Erik Österlund <[hidden email]>
wrote:

> Hi,
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8184334 <
> https://bugs.openjdk.java.net/browse/JDK-8184334>
>
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8184334/webrev.00/ <
> http://cr.openjdk.java.net/~eosterlund/8184334/webrev.00/>
>
> The Atomic class mostly uses the JNI-specific j-types (jbyte, jshort,
> jint, jlong). This has led to wide-spread pollution of j-types in the C++
> code base.
>
> Another problem is that when dealing with certain inherently not j-typed
> types like pointers, hacks were built that allowed them using _ptr member
> functions that had to be specialized on every architecture supported by
> Java.
>
> Yet another problem is yet another exception, like size_t, that does not
> quite fit being a pointer and isn't necessarily mapped to any j-type. Then
> yet more exceptions were built on top. And what about intx? You get the
> idea by now.
>
> As a remedy to the above mentioned problems, I propose to generalize the
> Atomic implementation to use templates instead.
> The new Atomic API handles pointers, all integer and floating point types
> that the underlying architecture supports.
>
> To achieve this, a new metaprogramming tool called IntegerTypes was built,
> that allows us to safely cast between different integral, floating point
> and pointer types of the same size while preserving the bit pattern of the
> values. This allows Atomic to take primitive types and canonicalize them to
> the same integer type that is passed to the platform layer.
>
> As for atomic operations on integral types like add/inc/dec, pointer
> scaling is now performed. That is, the functional behaviour is modelled
> after the +=, ++ and -- operators correspondingly. The X_ptr member
> functions have been deprecated, but are still there and can be used with
> identical behaviour as they had before. But new code should just use the
> non-ptr member functions instead.
>
> The platform layer specializes the specialized_X member functions that
> take canonicalized signed integers as input, and turn them to some
> platform-specific instruction sequence.
> A nice benefit with these changes is that the platform-specific code has
> gotten a lot of pointless redundancies removed such as how to perform
> atomic loads and stores on native word or less sized integers, or having
> inc/dec implementations that just reuse add.
>
> As for testing, new gtests were written for IntegerTypes as well as Atomic
> to make it more convincing that the functional behaviour works.
> Apart from that, I have run JPRT, JTREG, RBT hs-tier3 and a few local
> tonga test lists including nsk.jdi, nsk.monitoring, nsk.stress, vm.gc on
> Solaris SPARC T7-4 and Linux AMD64.
>
> As for oracle-external ports, I would appreciate if port maintainers could
> check if it seems to make sense. I have done my best to try to make the
> appropriate changes where needed.
>
> Special thanks go to Kim Barrett that has worked closely with me on this
> and whom I exchanged many emails with, and so I would like him to be a
> co-author.
>
> Thanks,
> /Erik
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Erik Osterlund
In reply to this post by Andrew Haley
Hi Andrew,

On 2017-07-17 17:59, Andrew Haley wrote:

> Hi,
>
> On 17/07/17 16:00, Erik Österlund wrote:
>
>> I am glad that it seems to work. Thank you for reviewing.
>>
>> As for the over engineered casting mechanism, the IntegerTypes class in
>> metaprogramming/integerTypes.hpp has lots of commentary about what the
>> different casting mechanisms do. Hope that helps.
> It doesn't, because most HotSpot programmers know what this stuff
> does, but not why you're doing it.

I am not sure what 'this stuff' is in this context, so I am afraid I can
not answer why I am doing it at this point.

>> To explain the reasoning, I will go through an example. Let's take
>> atomic::cmpxchg.
>> Each platform has a specialization for different supported cmpxchg
>> implementations in terms of int32_t and int64_t.
> OK, I get that.  Some platforms presumably requires such
> implementations, but would it not make more sense to restrict the
> complexity to just those platforms?  None of the GCC targets need it.

The Atomic API in hotspot has a frontend and backend part. The frontend
is shared, and mediates the atomic operations to the platform-specific
backends. What I have worked on is the frontend. The backend is platform
specific. Some platforms might want to use GCC intrinsics, while others
might want to do something else.

Are you proposing that the shared mediator to the backend should have a
GCC-only path?

>> So for example, we need to be able to perform the following conversions:
>> 1) Floating point to integer without changing bit representaiton. It
>> seems like the only way of doing this without violating the C++03
>> standard is to use memcpy. (no, the union trick in e.g. jfloat_cast is
>> not standards compliant)
> I'm not entirely convinced that memcpy does it either for "truly
> portable" C++, but perhaps it's as good as we're going to get.

I am curious what you mean here.

>> 2) Pointer to integer
>> 3) Anything with not matching CV qualifiers to matching CV qualifiers
>> 4) Integer of wrong signedness to matching signedness
>> 5) Integer with same signedness but declared differently, e.g. signed
>> char is not the same as char, and will miss that specialization. For
>> example on Solaris, int8_t is char, but jbyte is signed char, and those
>> types are different. Sometimes even intptr_t is neither int32_t or
>> int64_t as I have seen on clang.
> If you have something like
>
> long n;
> long *p = &n;
>
> The you can access the stored value in p with any type compatible with
> long*.  However, this:
>
>    Atomic::cmpxchg_ptr(0, &p, &n);
>
> casts &n to (signed) intptr_t: this is guaranteed to work.

Okay, so far so good then.

> Unfortunately, it then calls generic_cmpxchg<long>(0, &p, &n).  This
> accesses the stored pointer in p as a long, which is undefined
> behaviour.

As generic_cmpxchg is in the backend of Linux AArch64, I assume you are
referring to the code in that backend, which should not come as a surprise.
Further on, I am assuming you are referring to how for a pointer,
previously intptr_t would be sent into generic_cmpxchg, and now we send
in int64_t. But you will find that the following assert is true with GCC
on AArch64 (the backend you are interested in):

#include <metaprogramming/isSame.hpp>

STATIC_ASSERT(IsSame<intptr_t, int64_t>::value);

That is, intptr_t and int64_t is the exact same type by definition, and
hence the type of the instantiated generic_cmpxchg is exactly the same.
So this particular backend we would previously send in intptr_t, and now
send in a type that IsSame as intptr_t. The behaviour looks identical to me.

So as long as intptr_t and T* are compatible types (which they are by
definition), I do not see how this is undefined behaviour.

> It works on GCC if we compile everything with -fno-strict-aliasing (at
> least if intptr_t has the same representation in memory as every
> pointer type, which every machine we care about does) but in that case
> there's little point casting pointers back and forth to integer types.

I hope my answer above convinces you that this is not a problem.

>> Due to these issues, the IntegerTypes class was created to solve the
>> casting problems in one single place while remaining type safe. So it
>> solves the problem of canonicalizing primitive types and casting them
>> into exactly the same canonical integer type that can be safely passed
>> into code that specializes on that canonical integer type, without
>> changing the bit pattern or slipping in type safety.
> It is not type safe.
>
> ------------------------------------------------------------------------
> 3.10, Lvalues and rvalues
>
> If a program attempts to access the stored value of an object through
> a glvalue of other than one of the following types the behavior is
> undefined:
>
> — the dynamic type of the object,
>
> — a cv-qualified version of the dynamic type of the object,
>
> — a type similar (as defined in 4.4) to the dynamic type of the object,
>
> — a type that is the signed or unsigned type corresponding to the
>    dynamic type of the object,
>
> — a type that is the signed or unsigned type corresponding to a
>    cv-qualified version of the dynamic type of the object,
>
> — an aggregate or union type that includes one of the aforementioned
>    types among its elements or non- static data members (including,
>    recursively, an element or non-static data member of a subaggregate
>    or contained union),
>
> — a type that is a (possibly cv-qualified) base class type of the
>    dynamic type of the object,
>
> — a char or unsigned char type.
> ------------------------------------------------------------------------
>
> You only have permission to convert pointers to intptr_t and back: you
> do not have permission to access the stored value of a pointer an an
> intptr_t.

I would say the scenario you describe goes under "the dynamic type of
the object" or "a type that is the signed or unsigned type corresponding
to the dynamic type of the object", in the quoted section 3.10 of the
standard, depending on specific use case.
The problem that type aliasing is aimed at is if you store an A* and
then load it as a B*, then the dynamic type is A*, yet it is loaded as
B*, where B is not compatible with A. This is invariant of whether the
value of the store is indirectly performed through intptr_t or loaded
indirectly through intptr_t, as the dynamic type of the stored value is
still A*.

If you now perform a store of A* through Atomic that casts the pointer
to intptr_t, and stores it, then the dynamic type is still A*. If a
subsequent load of A* observes that store, then the type of that loads
needs to be compatible with the dynamic type, which is A*. In this
scenario, the observed value is the dynamic type A* and is loaded as A*,
so that is all good.

Conversely, if you perform a normal store of A*, and observe that value
inside of Atomic, then the dynamic type is still A*, and the value
observed in atomic is the signed type corresponding to the dynamic type
A*, so that is all good. And the signed type corresponding to the
dynamic type A* may safely be casted to A*.

In summary, my point is that as long as the performed stores and loads
in Atomic preserves the bit representation of whatever pointer was
passed in, the dynamic type of that pointer is unchanged, invariantly of
casting it to/from intptr_t, and hence the aliasing is allowed.

Do you have a different interpretation of the standard than I do?

>
>> By solving this general casting problem problem in IntegerTypes, it
>> turned the casting issues in Atomic into trivial non-problems, and
>> hopefully can be used elsewhere in hotspot. For example the jfloat_cast
>> with friend functions are not standards compliant and can now use
>> IntegerTypes instead, for that mindblowingly seemless casting experience.
>>
>> I hope this explanation makes sense.
> I don't believe that it entirely does, no.  But it is the sort of
> commentary which ought to be in the source code.
>

I can put some commentary in atomic.hpp describing the use of
IntegerTypes for casting if you like.

Thanks,
/Erik
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Erik Osterlund
In reply to this post by Aleksey Shipilev-4
Hi Aleksey,

Thank you for checking this. Glad it turned out as I expected.

/Erik

On 2017-07-17 18:49, Aleksey Shipilev wrote:

> On 07/17/2017 05:56 PM, Aleksey Shipilev wrote:
>> On 07/17/2017 05:49 PM, Erik Österlund wrote:
>>>> Something like the Unsafe.{get|put|CAS} -Xint performance test would be nice to
>>>> have.
>>> Sorry, but I do not understand what you mean here.
>>>
>>> 1) The code only boils down to performing casts that do not change any bit
>>> pattern, and is by contract forced to be expanded and inlined at compile time,
>>> so I would be very surprised about any regressions.
>>> 2) This code executes when we are about to perform atomic operations, so any
>>> accidental code generated for the casting (which seems odd in the first place),
>>> I would be very surprised to see.
>>> 3) Even if there was some barely noticeable regression in fastdebug atomics for
>>> whatever reason, I do not know if I think it makes sense to pollute the code
>>> with ugly hacks to micro optimize fastdebug execution time.
>> I agree that fastdebug should not be our primary optimization target. I was
>> wondering if all the template magic actually works well in fastdebug builds.
>>
>> Or product builds, for that matter: because while templates are indeed done
>> during compile-time, say, the inliner that has to deal with new code shapes can
>> have opinions.
>>
>> There are some VarHandles perf tests I have lying around, I can check this
>> myself today.
> Checked, times are the same for baseline/patched builds with either fastdebug or
> release bits. Ditto the instruction counts, number of memory accesses, branches,
> etc. Seems good!
>
> Thanks,
> -Aleksey
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Erik Osterlund
In reply to this post by Thomas Stüfe-2
Hi Thomas,

Thank you for checking this.

/Erik

On 2017-07-18 11:13, Thomas Stüfe wrote:

> Hi Eric, Volker,
>
> builds fine on AIX 5.3 and 7.1.
>
> gtests show no errors.
>
> Kind Regards, Thomas
>
>
> On Fri, Jul 14, 2017 at 6:28 PM, Erik Österlund
> <[hidden email] <mailto:[hidden email]>> wrote:
>
>     Hi,
>
>     Bug:
>     https://bugs.openjdk.java.net/browse/JDK-8184334
>     <https://bugs.openjdk.java.net/browse/JDK-8184334>
>     <https://bugs.openjdk.java.net/browse/JDK-8184334
>     <https://bugs.openjdk.java.net/browse/JDK-8184334>>
>
>     Webrev:
>     http://cr.openjdk.java.net/~eosterlund/8184334/webrev.00/
>     <http://cr.openjdk.java.net/%7Eeosterlund/8184334/webrev.00/>
>     <http://cr.openjdk.java.net/~eosterlund/8184334/webrev.00/
>     <http://cr.openjdk.java.net/%7Eeosterlund/8184334/webrev.00/>>
>
>     The Atomic class mostly uses the JNI-specific j-types (jbyte,
>     jshort, jint, jlong). This has led to wide-spread pollution of
>     j-types in the C++ code base.
>
>     Another problem is that when dealing with certain inherently not
>     j-typed types like pointers, hacks were built that allowed them
>     using _ptr member functions that had to be specialized on every
>     architecture supported by Java.
>
>     Yet another problem is yet another exception, like size_t, that
>     does not quite fit being a pointer and isn't necessarily mapped to
>     any j-type. Then yet more exceptions were built on top. And what
>     about intx? You get the idea by now.
>
>     As a remedy to the above mentioned problems, I propose to
>     generalize the Atomic implementation to use templates instead.
>     The new Atomic API handles pointers, all integer and floating
>     point types that the underlying architecture supports.
>
>     To achieve this, a new metaprogramming tool called IntegerTypes
>     was built, that allows us to safely cast between different
>     integral, floating point and pointer types of the same size while
>     preserving the bit pattern of the values. This allows Atomic to
>     take primitive types and canonicalize them to the same integer
>     type that is passed to the platform layer.
>
>     As for atomic operations on integral types like add/inc/dec,
>     pointer scaling is now performed. That is, the functional
>     behaviour is modelled after the +=, ++ and -- operators
>     correspondingly. The X_ptr member functions have been deprecated,
>     but are still there and can be used with identical behaviour as
>     they had before. But new code should just use the non-ptr member
>     functions instead.
>
>     The platform layer specializes the specialized_X member functions
>     that take canonicalized signed integers as input, and turn them to
>     some platform-specific instruction sequence.
>     A nice benefit with these changes is that the platform-specific
>     code has gotten a lot of pointless redundancies removed such as
>     how to perform atomic loads and stores on native word or less
>     sized integers, or having inc/dec implementations that just reuse add.
>
>     As for testing, new gtests were written for IntegerTypes as well
>     as Atomic to make it more convincing that the functional behaviour
>     works.
>     Apart from that, I have run JPRT, JTREG, RBT hs-tier3 and a few
>     local tonga test lists including nsk.jdi, nsk.monitoring,
>     nsk.stress, vm.gc on Solaris SPARC T7-4 and Linux AMD64.
>
>     As for oracle-external ports, I would appreciate if port
>     maintainers could check if it seems to make sense. I have done my
>     best to try to make the appropriate changes where needed.
>
>     Special thanks go to Kim Barrett that has worked closely with me
>     on this and whom I exchanged many emails with, and so I would like
>     him to be a co-author.
>
>     Thanks,
>     /Erik
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Andrew Haley
In reply to this post by Erik Osterlund
On 18/07/17 10:38, Erik Österlund wrote:

>> ------------------------------------------------------------------------
>> 3.10, Lvalues and rvalues
>>
>> If a program attempts to access the stored value of an object through
>> a glvalue of other than one of the following types the behavior is
>> undefined:
>>
>> — the dynamic type of the object,
>>
>> — a cv-qualified version of the dynamic type of the object,
>>
>> — a type similar (as defined in 4.4) to the dynamic type of the object,
>>
>> — a type that is the signed or unsigned type corresponding to the
>>    dynamic type of the object,
>>
>> — a type that is the signed or unsigned type corresponding to a
>>    cv-qualified version of the dynamic type of the object,
>>
>> — an aggregate or union type that includes one of the aforementioned
>>    types among its elements or non- static data members (including,
>>    recursively, an element or non-static data member of a subaggregate
>>    or contained union),
>>
>> — a type that is a (possibly cv-qualified) base class type of the
>>    dynamic type of the object,
>>
>> — a char or unsigned char type.
>> ------------------------------------------------------------------------
>>
>> You only have permission to convert pointers to intptr_t and back: you
>> do not have permission to access the stored value of a pointer an an
>> intptr_t.
>
> I would say the scenario you describe goes under "the dynamic type of
> the object" or "a type that is the signed or unsigned type corresponding
> to the dynamic type of the object",

OK.

> in the quoted section 3.10 of the standard, depending on specific
> use case.  The problem that type aliasing is aimed at is if you
> store an A* and then load it as a B*, then the dynamic type is A*,
> yet it is loaded as B*, where B is not compatible with A.

Precisely.  That is what is happening in this case.  A is, say, void*
and B is intptr_t.  void* is not compatible with intptr_t.

> This is invariant of whether the value of the store is indirectly
> performed through intptr_t or loaded indirectly through intptr_t, as
> the dynamic type of the stored value is still A*.

Exactly so.  The stored object is an A*.  We're accessing it as an
intptr_t.  A* and intptr_t are not compatible types.  This is true
even if the intptr_t was the result of casting from an A*.  The
compiler doesn't necessarily know that the intptr_t was originally an
A*: they might even be in separate compilation units.  The compiler is
entitled to assume that the object in memory of type A* does not alias
with any value of type intptr_t.

> If you now perform a store of A* through Atomic that casts the pointer
> to intptr_t, and stores it, then the dynamic type is still A*.

This isn't how it works.  (BTW, in case it helps understand where I'm
coming from, although I am not the greatest expert in this area, I am
familiar with GCC's alias analysis because I am a GCC author.)

> In summary, my point is that as long as the performed stores and loads
> in Atomic preserves the bit representation of whatever pointer was
> passed in, the dynamic type of that pointer is unchanged, invariantly of
> casting it to/from intptr_t, and hence the aliasing is allowed.

That is incorrect.  Rules about aliasing are nothing to do with the
bit representation.

Let me reiterate: you may cast from any pointer type to intptr_t.  You
may not access a pointer in memory as though it were an intptr_t.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Erik Osterlund


On 2017-07-18 11:57, Andrew Haley wrote:

> On 18/07/17 10:38, Erik Österlund wrote:
>
>>> ------------------------------------------------------------------------
>>> 3.10, Lvalues and rvalues
>>>
>>> If a program attempts to access the stored value of an object through
>>> a glvalue of other than one of the following types the behavior is
>>> undefined:
>>>
>>> — the dynamic type of the object,
>>>
>>> — a cv-qualified version of the dynamic type of the object,
>>>
>>> — a type similar (as defined in 4.4) to the dynamic type of the object,
>>>
>>> — a type that is the signed or unsigned type corresponding to the
>>>     dynamic type of the object,
>>>
>>> — a type that is the signed or unsigned type corresponding to a
>>>     cv-qualified version of the dynamic type of the object,
>>>
>>> — an aggregate or union type that includes one of the aforementioned
>>>     types among its elements or non- static data members (including,
>>>     recursively, an element or non-static data member of a subaggregate
>>>     or contained union),
>>>
>>> — a type that is a (possibly cv-qualified) base class type of the
>>>     dynamic type of the object,
>>>
>>> — a char or unsigned char type.
>>> ------------------------------------------------------------------------
>>>
>>> You only have permission to convert pointers to intptr_t and back: you
>>> do not have permission to access the stored value of a pointer an an
>>> intptr_t.
>> I would say the scenario you describe goes under "the dynamic type of
>> the object" or "a type that is the signed or unsigned type corresponding
>> to the dynamic type of the object",
> OK.
>
>> in the quoted section 3.10 of the standard, depending on specific
>> use case.  The problem that type aliasing is aimed at is if you
>> store an A* and then load it as a B*, then the dynamic type is A*,
>> yet it is loaded as B*, where B is not compatible with A.
> Precisely.  That is what is happening in this case.  A is, say, void*
> and B is intptr_t.  void* is not compatible with intptr_t.

My interpretation is that the aliasing rules are for points-to analysis
being able to alias that if somebody stores A* and then other code loads
that as B* and accesses B, then it is assumed that the B* does not
points-to the A* as they are of incompatible types, and that therefore
it is fine to load something (that was stored as A*) as intptr_t and
subsequently cast it to A* before the A itself is being accessed. Am I
wrong?

For example, the following test program compiles and runs with g++
-fstrict-aliasing -Wstrict-aliasing=3 -O3 -std=c++03:

#include <stdio.h>
#include <stdint.h>

class A{
public:
   int _x;
   A() : _x(0) {}
};

int main(int argc, char* argv[]) {
   A a;
   A b;
   A* ptr = &a;
   A** ptr_ptr = &ptr;
   intptr_t* iptr_ptr = reinterpret_cast<intptr_t*>(ptr_ptr);

   *ptr_ptr = &b;
   intptr_t iptr = *iptr_ptr;
   A* ptr2 = reinterpret_cast<A*>(iptr);

   printf("iptr = %lx, &a = %lx, &b = %lx, iptr->_x = %d\n", iptr,
          reinterpret_cast<intptr_t>(&a),
reinterpret_cast<intptr_t>(&b), ptr2->_x);

   return 0;
}

The program stores an A*, reads it as intptr_t and casts it to A*, and
then dereferences into the A. Even with -Wstrict-aliasing=3 GCC does not
complain about this. Is GCC wrong about not complaining about this?

The way I interpret the standard, intptr_t is the signed type
corresponding to the dynamic type of A*, which seems compliant to me. Of
course the way it is stated in the standard is a bit vague (as usual),
but the compilers seem to support my interpretation. Is my
interpretation and GCC -Wstrict-aliasing=3 wrong here in allowing this?

>> This is invariant of whether the value of the store is indirectly
>> performed through intptr_t or loaded indirectly through intptr_t, as
>> the dynamic type of the stored value is still A*.
> Exactly so.  The stored object is an A*.  We're accessing it as an
> intptr_t.  A* and intptr_t are not compatible types.  This is true
> even if the intptr_t was the result of casting from an A*.  The
> compiler doesn't necessarily know that the intptr_t was originally an
> A*: they might even be in separate compilation units.  The compiler is
> entitled to assume that the object in memory of type A* does not alias
> with any value of type intptr_t.
>
>> If you now perform a store of A* through Atomic that casts the pointer
>> to intptr_t, and stores it, then the dynamic type is still A*.
> This isn't how it works.  (BTW, in case it helps understand where I'm
> coming from, although I am not the greatest expert in this area, I am
> familiar with GCC's alias analysis because I am a GCC author.)
>
>> In summary, my point is that as long as the performed stores and loads
>> in Atomic preserves the bit representation of whatever pointer was
>> passed in, the dynamic type of that pointer is unchanged, invariantly of
>> casting it to/from intptr_t, and hence the aliasing is allowed.
> That is incorrect.  Rules about aliasing are nothing to do with the
> bit representation.

What I meant is that if the bit representation changed, it would point
to something different, hence the dynamic type of whatever is pointed at
would not (necessarily) be the same.

> Let me reiterate: you may cast from any pointer type to intptr_t.  You
> may not access a pointer in memory as though it were an intptr_t.
>

Are you 100% sure about this? Again, I do not know where in the standard
this is stated, but -Wstrict-aliasing=3 allows me to do that as long as
the intptr_t is appropriately casted to the A* before the A behind the
dynamic A* type is used. Perhaps I have missed something?

Thanks,
/Erik
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Andrew Haley
On 18/07/17 14:46, Erik Österlund wrote:

>
>
> On 2017-07-18 11:57, Andrew Haley wrote:
>> On 18/07/17 10:38, Erik Österlund wrote:
>>
>>>> ------------------------------------------------------------------------
>>>> 3.10, Lvalues and rvalues
>>>>
>>>> If a program attempts to access the stored value of an object through
>>>> a glvalue of other than one of the following types the behavior is
>>>> undefined:
>>>>
>>>> — the dynamic type of the object,
>>>>
>>>> — a cv-qualified version of the dynamic type of the object,
>>>>
>>>> — a type similar (as defined in 4.4) to the dynamic type of the object,
>>>>
>>>> — a type that is the signed or unsigned type corresponding to the
>>>>     dynamic type of the object,
>>>>
>>>> — a type that is the signed or unsigned type corresponding to a
>>>>     cv-qualified version of the dynamic type of the object,
>>>>
>>>> — an aggregate or union type that includes one of the aforementioned
>>>>     types among its elements or non- static data members (including,
>>>>     recursively, an element or non-static data member of a subaggregate
>>>>     or contained union),
>>>>
>>>> — a type that is a (possibly cv-qualified) base class type of the
>>>>     dynamic type of the object,
>>>>
>>>> — a char or unsigned char type.
>>>> ------------------------------------------------------------------------
>>>>
>>>> You only have permission to convert pointers to intptr_t and back: you
>>>> do not have permission to access the stored value of a pointer an an
>>>> intptr_t.
>>> I would say the scenario you describe goes under "the dynamic type of
>>> the object" or "a type that is the signed or unsigned type corresponding
>>> to the dynamic type of the object",
>> OK.
>>
>>> in the quoted section 3.10 of the standard, depending on specific
>>> use case.  The problem that type aliasing is aimed at is if you
>>> store an A* and then load it as a B*, then the dynamic type is A*,
>>> yet it is loaded as B*, where B is not compatible with A.
>> Precisely.  That is what is happening in this case.  A is, say, void*
>> and B is intptr_t.  void* is not compatible with intptr_t.
>
> My interpretation is that the aliasing rules are for points-to analysis
> being able to alias that if somebody stores A* and then other code loads
> that as B* and accesses B, then it is assumed that the B* does not
> points-to the A* as they are of incompatible types,

It is more general than that.  If you access the stored value of an
object through a glvalue of other than one of the allowed types, then
your *whole program is undefined*.

> and that therefore it is fine to load something (that was stored as
> A*) as intptr_t and subsequently cast it to A* before the A itself
> is being accessed. Am I wrong?

No, that is correct.  The question is whether intptr_t is a type
compatible with a pointer type, and I don't think you will find any
language to the effect that it is.

Pointer types are distinct from one another and from all integer
types.  People sometimes get confused by this: the fact that you can
cast from A* to e.g. void*, doesn't tell you that you can cast from
A** to void** and use the result: they are different types.

> For example, the following test program compiles and runs with g++
> -fstrict-aliasing -Wstrict-aliasing=3 -O3 -std=c++03:
>
> #include <stdio.h>
> #include <stdint.h>
>
> class A{
> public:
>    int _x;
>    A() : _x(0) {}
> };
>
> int main(int argc, char* argv[]) {
>    A a;
>    A b;
>    A* ptr = &a;
>    A** ptr_ptr = &ptr;
>    intptr_t* iptr_ptr = reinterpret_cast<intptr_t*>(ptr_ptr);
>
>    *ptr_ptr = &b;
>    intptr_t iptr = *iptr_ptr;
>    A* ptr2 = reinterpret_cast<A*>(iptr);
>
>    printf("iptr = %lx, &a = %lx, &b = %lx, iptr->_x = %d\n", iptr,
>           reinterpret_cast<intptr_t>(&a),
> reinterpret_cast<intptr_t>(&b), ptr2->_x);
>
>    return 0;
> }
>
> The program stores an A*, reads it as intptr_t and casts it to A*, and
> then dereferences into the A. Even with -Wstrict-aliasing=3 GCC does not
> complain about this. Is GCC wrong about not complaining about this?

No GCC is not wrong because GCC does not have to complain about this.
You are, however, wrong to write it!

Try

  g++ -fsanitize=undefined pp.cc

and you'll get

  iptr = 7ffffcf68ec0, &a = 7ffffcf68ed0, &b = 7ffffcf68ec0, iptr->_x = 0

at one point of undefined behaviour.

[ A warning here: don't assume that the sanitizer can detect all UB
caused by messing with pointer types, but it can detect this
example. ]

> The way I interpret the standard, intptr_t is the signed type
> corresponding to the dynamic type of A*, which seems compliant to me.

But that's not what the standard says.  Remember that whatever is
allowed is explicitly allowed, and if something is not allowed it is
forbidden.

> Of course the way it is stated in the standard is a bit vague (as
> usual), but the compilers seem to support my interpretation. Is my
> interpretation and GCC -Wstrict-aliasing=3 wrong here in allowing
> this?

Your interpretation is wrong.  But a C++ compiler can do anything with
undefined behaviour, which includes the possibility that it does what
the programmer expected.

>> Let me reiterate: you may cast from any pointer type to intptr_t.  You
>> may not access a pointer in memory as though it were an intptr_t.
>
> Are you 100% sure about this? Again, I do not know where in the standard
> this is stated,

It's in my quote above.

> but -Wstrict-aliasing=3 allows me to do that as long as the intptr_t
> is appropriately casted to the A* before the A behind the dynamic A*
> type is used. Perhaps I have missed something?

Yes, I am sure.  Warnings don't help you here because if you throw in
enough casts you can suppress any type-based warnings.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Erik Osterlund


On 2017-07-18 16:41, Andrew Haley wrote:

> On 18/07/17 14:46, Erik Österlund wrote:
>>
>> On 2017-07-18 11:57, Andrew Haley wrote:
>>> On 18/07/17 10:38, Erik Österlund wrote:
>>>
>>>>> ------------------------------------------------------------------------
>>>>> 3.10, Lvalues and rvalues
>>>>>
>>>>> If a program attempts to access the stored value of an object through
>>>>> a glvalue of other than one of the following types the behavior is
>>>>> undefined:
>>>>>
>>>>> — the dynamic type of the object,
>>>>>
>>>>> — a cv-qualified version of the dynamic type of the object,
>>>>>
>>>>> — a type similar (as defined in 4.4) to the dynamic type of the object,
>>>>>
>>>>> — a type that is the signed or unsigned type corresponding to the
>>>>>      dynamic type of the object,
>>>>>
>>>>> — a type that is the signed or unsigned type corresponding to a
>>>>>      cv-qualified version of the dynamic type of the object,
>>>>>
>>>>> — an aggregate or union type that includes one of the aforementioned
>>>>>      types among its elements or non- static data members (including,
>>>>>      recursively, an element or non-static data member of a subaggregate
>>>>>      or contained union),
>>>>>
>>>>> — a type that is a (possibly cv-qualified) base class type of the
>>>>>      dynamic type of the object,
>>>>>
>>>>> — a char or unsigned char type.
>>>>> ------------------------------------------------------------------------
>>>>>
>>>>> You only have permission to convert pointers to intptr_t and back: you
>>>>> do not have permission to access the stored value of a pointer an an
>>>>> intptr_t.
>>>> I would say the scenario you describe goes under "the dynamic type of
>>>> the object" or "a type that is the signed or unsigned type corresponding
>>>> to the dynamic type of the object",
>>> OK.
>>>
>>>> in the quoted section 3.10 of the standard, depending on specific
>>>> use case.  The problem that type aliasing is aimed at is if you
>>>> store an A* and then load it as a B*, then the dynamic type is A*,
>>>> yet it is loaded as B*, where B is not compatible with A.
>>> Precisely.  That is what is happening in this case.  A is, say, void*
>>> and B is intptr_t.  void* is not compatible with intptr_t.
>> My interpretation is that the aliasing rules are for points-to analysis
>> being able to alias that if somebody stores A* and then other code loads
>> that as B* and accesses B, then it is assumed that the B* does not
>> points-to the A* as they are of incompatible types,
> It is more general than that.  If you access the stored value of an
> object through a glvalue of other than one of the allowed types, then
> your *whole program is undefined*.

And I think it is in that list of allowed types. Specifically, it is my
interpretation of the standard that intptr_t is "a type that is the
signed or unsigned type corresponding to the dynamic type of the object"
(cf. 5.2.10) if the dynamic type is e.g. void*.

>> and that therefore it is fine to load something (that was stored as
>> A*) as intptr_t and subsequently cast it to A* before the A itself
>> is being accessed. Am I wrong?
> No, that is correct.  The question is whether intptr_t is a type
> compatible with a pointer type, and I don't think you will find any
> language to the effect that it is.

I think 5.2.10 describing reinterpret_cast seems to suggest that they
are compatible by saying "A pointer converted to an integer of
sufficient size (if any such exists on the implementation) and back to
the same pointer type will have its original value". That is precisely
what is done.
Conversely, I do not find any language suggesting that intptr_t would
not be compatible with pointer types.

> Pointer types are distinct from one another and from all integer
> types.  People sometimes get confused by this: the fact that you can
> cast from A* to e.g. void*, doesn't tell you that you can cast from
> A** to void** and use the result: they are different types.

Agreed. But I do not see any problem with reinterpret_cast from A* to
intptr_t back to A* and using the A from that result. That is explicitly
allowed as per 5.2.10 as previously described.

>> For example, the following test program compiles and runs with g++
>> -fstrict-aliasing -Wstrict-aliasing=3 -O3 -std=c++03:
>>
>> #include <stdio.h>
>> #include <stdint.h>
>>
>> class A{
>> public:
>>     int _x;
>>     A() : _x(0) {}
>> };
>>
>> int main(int argc, char* argv[]) {
>>     A a;
>>     A b;
>>     A* ptr = &a;
>>     A** ptr_ptr = &ptr;
>>     intptr_t* iptr_ptr = reinterpret_cast<intptr_t*>(ptr_ptr);
>>
>>     *ptr_ptr = &b;
>>     intptr_t iptr = *iptr_ptr;
>>     A* ptr2 = reinterpret_cast<A*>(iptr);
>>
>>     printf("iptr = %lx, &a = %lx, &b = %lx, iptr->_x = %d\n", iptr,
>>            reinterpret_cast<intptr_t>(&a),
>> reinterpret_cast<intptr_t>(&b), ptr2->_x);
>>
>>     return 0;
>> }
>>
>> The program stores an A*, reads it as intptr_t and casts it to A*, and
>> then dereferences into the A. Even with -Wstrict-aliasing=3 GCC does not
>> complain about this. Is GCC wrong about not complaining about this?
> No GCC is not wrong because GCC does not have to complain about this.
> You are, however, wrong to write it!

Oh dear. My fingers slipped.

> Try
>
>    g++ -fsanitize=undefined pp.cc
>
> and you'll get
>
>    iptr = 7ffffcf68ec0, &a = 7ffffcf68ed0, &b = 7ffffcf68ec0, iptr->_x = 0
>
> at one point of undefined behaviour.
>
> [ A warning here: don't assume that the sanitizer can detect all UB
> caused by messing with pointer types, but it can detect this
> example. ]

That looks like the correct (and indeed expected) output. iptr should be
the same as &b, and iptr->_x should be 0, and &a should be different
from &b and iptr. I tried this experiment locally too with similar
correct result. Am I missing something here?

>> The way I interpret the standard, intptr_t is the signed type
>> corresponding to the dynamic type of A*, which seems compliant to me.
> But that's not what the standard says.  Remember that whatever is
> allowed is explicitly allowed, and if something is not allowed it is
> forbidden.

Agreed. But it is my interpretation is that this is explicitly allowed.

In the case where A* is stored, and subsequently loaded as intptr_t (and
casted to A*), the dynamic type observed by the load is A* and intptr_t
is the signed type of A*. This is explicitly allowed w.r.t. aliasing
because the type of the load may be:
3.10: "a type that is the signed or unsigned type corresponding to the
dynamic type of the object"
...which it is as intptr_t is the signed type corresponding to A*.

This intptr_t is then casted back to the dynamic type A* through
reinterpret_cast, which is explicitly allowed because:
5.2.10: "A pointer converted to an integer of sufficient size (if any
such exists on the implementation) and back to the same pointer type
will have its original value"

Thanks,
/Erik

>> Of course the way it is stated in the standard is a bit vague (as
>> usual), but the compilers seem to support my interpretation. Is my
>> interpretation and GCC -Wstrict-aliasing=3 wrong here in allowing
>> this?
> Your interpretation is wrong.  But a C++ compiler can do anything with
> undefined behaviour, which includes the possibility that it does what
> the programmer expected.
>
>>> Let me reiterate: you may cast from any pointer type to intptr_t.  You
>>> may not access a pointer in memory as though it were an intptr_t.
>> Are you 100% sure about this? Again, I do not know where in the standard
>> this is stated,
> It's in my quote above.
>
>> but -Wstrict-aliasing=3 allows me to do that as long as the intptr_t
>> is appropriately casted to the A* before the A behind the dynamic A*
>> type is used. Perhaps I have missed something?
> Yes, I am sure.  Warnings don't help you here because if you throw in
> enough casts you can suppress any type-based warnings.
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Andrew Haley
On 18/07/17 16:26, Erik Österlund wrote:
>
>
> On 2017-07-18 16:41, Andrew Haley wrote:
>> On 18/07/17 14:46, Erik Österlund wrote:
>>>
>>> On 2017-07-18 11:57, Andrew Haley wrote:
>>>> On 18/07/17 10:38, Erik Österlund wrote:

>> It is more general than that.  If you access the stored value of an
>> object through a glvalue of other than one of the allowed types, then
>> your *whole program is undefined*.
>
> And I think it is in that list of allowed types. Specifically, it is my
> interpretation of the standard that intptr_t is "a type that is the
> signed or unsigned type corresponding to the dynamic type of the object"
> (cf. 5.2.10) if the dynamic type is e.g. void*.
>
>>> and that therefore it is fine to load something (that was stored as
>>> A*) as intptr_t and subsequently cast it to A* before the A itself
>>> is being accessed. Am I wrong?
>> No, that is correct.  The question is whether intptr_t is a type
>> compatible with a pointer type, and I don't think you will find any
>> language to the effect that it is.
>
> I think 5.2.10 describing reinterpret_cast seems to suggest that they
> are compatible by saying "A pointer converted to an integer of
> sufficient size (if any such exists on the implementation) and back to
> the same pointer type will have its original value". That is precisely
> what is done.

This is the core of our disagreement.

In a sense, it doesn't matter which of us is right; what really
matters is the opinion of the compiler.  GCC does not put intptr_t and
pointer types into the same alias set, and even if this did turn out
to be a bug in GCC it wouldn't help us because we often use
unsupported old versions of GCC to build OpenJDK, so it wouldn't get
fixed anyway.

So what are we to do?  Do you believe me when I say that GCC does
treat A* and intptr_t to be compatible types?  If not, what would it
take to convince you?

> Conversely, I do not find any language suggesting that intptr_t would
> not be compatible with pointer types.
>
>> Pointer types are distinct from one another and from all integer
>> types.  People sometimes get confused by this: the fact that you can
>> cast from A* to e.g. void*, doesn't tell you that you can cast from
>> A** to void** and use the result: they are different types.
>
> Agreed. But I do not see any problem with reinterpret_cast from A* to
> intptr_t back to A* and using the A from that result. That is explicitly
> allowed as per 5.2.10 as previously described.

I don't think that's what you're doing.  I think you're casting from
A** to intptr_t*, dereferencing the intptr_t*, and casting the result
to A*.

> That looks like the correct (and indeed expected) output. iptr should be
> the same as &b, and iptr->_x should be 0, and &a should be different
> from &b and iptr. I tried this experiment locally too with similar
> correct result. Am I missing something here?

No, I was; I misinterpreted the result, and it's a red herring.
Sorry.

I believe this patch to be dangerous.  We might well get away with it,
given sufficient larding of volatile, but it's still undefined
behaviour.  We have far too much UB in HotSpot already, and we should
not be adding any more.

And I still believe that this patch is more complex than is needed,
and with many (if not all) targets we could do something much simpler,
and which does not need to cast pointer types.  It could also map
cleanly onto C++11.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (M): 8184334: Generalizing Atomic with templates

Kim Barrett
> On Jul 19, 2017, at 12:21 PM, Andrew Haley <[hidden email]> wrote:
> I believe this patch to be dangerous.  We might well get away with it,
> given sufficient larding of volatile, but it's still undefined
> behaviour.  We have far too much UB in HotSpot already, and we should
> not be adding any more.
>
> And I still believe that this patch is more complex than is needed,
> and with many (if not all) targets we could do something much simpler,
> and which does not need to cast pointer types.  It could also map
> cleanly onto C++11.

I agree with Andrew that we're violating strict aliasing rules [1].
But I disagree with Andrew's assessement of the consequences.

Yes, we're violating strict aliasing.  Existing code is already doing
that.  We've been relying on volatile larding (I love that phrasing),
opacity of assembly code, and things like -fno-strict-aliasing to get
away with that.  This change doesn't change any of that.

What this change does do is allow us to centralize and isolate where
that's happening.  Current uses of Atomic are littered with casts,
along with some in the existing Atomic implementation.  By presenting
a general, type-safe, interface, we are eliminating the need for those
casts in API uses (or the oft-used alternative of making unnatural
type choices in order to match the existing limited API).  This new
implementation also does a lot of validation to improve code
correctness; that's simply missing in the existing casts-in-usage
approach.

So I think this change makes some things better (in some ways
significantly), while not being any worse than we already are on the
strict aliasing question.

Once we have this new API in place, we can go to work on cleaning up
the uses.  Independently, we can work on the implementation, which may
eventually include using C++11 atomics, at least on some platforms.
(Where applicable, that looks like it should be pretty easy with the
new API. [2]) The limitations of the present API lead to a strong
coupling between usage and implementation that make those kinds of
changes difficult.  Providing a better interface removes that coupling
and makes it easier to work on the various parts in isolation.

We also need something like this to support the GC interface work that
is in progress.  Some mechanism for connecting higher level operations
to the low-level platform-specific implementations is needed by that
work. Putting that mechanism in the atomics layer (and a forthcoming
proposal for changes to orderaccess) makes it available to more than
just the GC interface access protocol.

I agree with Erik that C++11 atomics cannot be considered a panacea in
this domain.  As he notes, there are implementation choices that are
not exposed, and which require consistency across all uses, but not
all of our interacting uses are in C++.

There is also a problem with using C++11 atomics that it generally
requires enabling C++11 support, which makes it all too easy to
unintentionally use other C++11 features.  This can easily run into
problems because there are some seriously laggard compilers being used
by some folks.  It would be very easy do some work that
unintentionally relies on some set of new language features, test,
review, and push it, and only later find out it all needed to be
backed out and re-thought due to limitations of one of these exotic
platforms.  (See discussion on the build-dev list, subject
"C++11/C++14 support in XLC ?".)  I would like to think we can find
our way through that, but it seems unlikely to happen quickly.

[1] Erik quoted 3.10: "a type that is the signed or unsigned type
corresponding to the dynamic type of the object" and suggested
"...which it is as intptr_t is the signed type corresponding to A*."

While attractive in this situation, I see no basis in the standard for
that interpretation of "corresponding".  All relevant uses of
"corresponding" are wrto integral types only.  My interpretation
(which I think matches what Andrew says about gcc's implementation) is
that "corresponding" here is about the pairs of signed/unsigned
integral types of a given size and rank.  That is, one can read a
dynamically typed signed long as an unsigned long, or vice versa.  No
more than that.

As Andrew pointed out, in the case of "A* is stored, and subsequently
loaded as intptr_t (and casted to A*)", 5.2.10/4 doesn't apply.  This
is not a cast of A* to intptr_t.  Rather, it's a cast of A** to
intptr_t* (or in the code code in question maybe some other I* where I
is an integral type).  The relevant reference is 5.2.10/7, about
converting one pointer type to another.  There's a C++11 addition
there about reinterpret_cast of pointers being equivalent to a two
step sequence of static_cast's through void* under certain conditions;
that addition codified existing practice.  But that doesn't help with
the 3.10 dereferencing issue.

[2] A couple of months ago I built Linux x86_64 with -std=gnu++11 and
ran our nightly tests, and didn't see any surprises.  We haven't
flipped that switch, in part because of the above mentioned unintended
use of new features problem (though with only one platform we
shouldn't get to the actual push part).  Also, one ad hoc run of a
subset of our tests isn't sufficient for such a change.  However, I
don't think there's anything preventing the aarch64 port from
independently doing something like that and reimplementing Atomic
using C++11 atomics, so long as the changes to shared code were done
in an appropriate manner.  I'd review that.  I might even find some
spare time to help.


123
Loading...