RFR (M): 8184334: Generalizing Atomic with templates

[Andrew Haley]

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