RFR: 8186166: Generalize Atomic::cmpxchg with templates

Fri Aug 18 13:21:25 UTC 2017

On 14/08/17 03:41, Kim Barrett wrote:
> Please review this change to Atomic::cmpxchg, making it a function
> template with a per-platform underlying implementation.
> 
> This has been broken out from 8184334: "Generalizing Atomic with
> templates", as the originally proposed change there encountered
> various objections.  This change takes a somewhat different approach
> to the same problem.
> 
> One goal is to generalize the API and make it more type-safe,
> eliminating (nearly) all need for casts by callers.  Another is to
> allow more direct per-platform implementations, especially where there
> are appropriate compiler intrinsics available.
> 
> If all goes well and this change is accepted, it is intended that the
> remainder of Atomic, and later OrderAccess, will be changed in a
> similar manner as one or more followups.
> 
> This change to cmpxchg deprecates cmpxchg_ptr.  Followup changes will
> convert existing uses of cmpxchg_ptr and eventually remove it.
> Followup changes will also update uses of cmpxchg that can be
> simplified because of the generalized API.
> 
> The cmpxchg function calls private Atomic::CmpxchgImpl, to select
> among several cases, based on the argument types.  Each case performs
> some checking and possibly conversion to reach a standard form of the
> arguments (or fail and report a compile time error), which are then
> passed to private Atomic::PlatformCmpxchg.  The PlatformCmpxchg class
> is provided by each platform, and performs additional case analysis to
> generate the appropriate code for the platform.
> 
> This change involves modifications to all of the existing platform
> implementations of cmpxchg.  I've updated all of the platforms, but
> only tested those supported by Oracle.  I'll need help from the
> various platform maintainers to check and test my changes.
> 
> Changes to files can be categorized as follows:
> 
> New metaprogramming utilities:
> integerTypes.hpp
> test_integerTypes.cpp  
> isRegisteredEnum.hpp
> test_isRegisteredEnum.cpp
> 
> Change cmpxchg to a template:
> atomic.hpp
> 
> Oracle-supported platforms:
> atomic_bsd_x86.hpp - 64bit only
> atomic_linux_arm.hpp
> atomic_linux_x86.hpp
> atomic_solaris_sparc.hpp
> solaris_sparc.il
> atomic_solaris_x86.hpp
> atomic_windows_x86
> 
> Non-Oracle platform changes:
> atomic_aix_ppc.hpp
> atomic_bsd_x86.hpp - 32bit
> atomic_bsd_zero.hpp
> atomic_linux_aarch.hpp
> atomic_linux_ppc.hpp
> atomic_linux_s390.hpp
> atomic_linux_sparc.hpp
> atomic_linux_zero.hpp
> 
> Usage changes required by API change:
> os_bsd.cpp
> os_solaris.cpp
> aotCodeHeap.cpp
> aotCodeHeap.hpp
> psParallelCompact.hpp
> workgroup.cpp
> oopsHierarchy.hpp
> os.cpp
> 
> Example updates of cmpxchg_ptr to cmpxchg:
> oop.inline.hpp
> bitMap.cpp
> bitMap.inline.hpp
> 
> Some specific issues:
> 
> - For Solaris (both SPARC and x86), we were using helper functions
> defined in corresponding .il files.  Using the cmpxchg_using_stub
> helper didn't work; passing a "defined in .il" function as a template
> parameter didn't work, leading to linker errors.  The ideal solution
> is to use gcc-style inline assembly, which is now supported by Solaris
> Studio.  That change was made for SPARC.  However, making that change
> for x86 ran into related segfaults, so using the .il definition with
> direct conversions for now, until that problem can be resolved.
> 
> - I've added the function template Atomic::condition_store_ptr, which
> is a cmpxchg for pointer types with a NULL compare_value.  That might
> not be the best name for it.
> 
> - The name IntegerTypes was retained from the original RFR for
> JDK-8184334.  However, that name no longer fits the provided
> functionality very well, and I think a new name is needed.  Something
> involving Bitwise, or ValueRepresentation perhaps?  I'm open to
> suggestions.
> 
> - Issues with casts/conversions provided by IntegerTypes.
> 
> * Conversions between integers and floating point values are
> supported.  It was suggested during the discussion of the RFR for
> JDK-8184334 that this was not needed, as there are no uses of
> atomic/ordered floating point values in Hotspot.  However, I'm told
> VarHandles may include atomic floating point values.

I am very strongly opposed to checking unused code into HotSpot.  Apart
from the obvious obervation that it's impossible to test, it makes far
more sense to add it if it is ever needed.

> * Conversions between integers and floating point values are presently
> implemented using the memcpy technique.  While this technique is
> correct, it is known to be quite poorly optimized by some compilers.
> It may be necessary to use the "union trick" for performance reasons,
> even though it is technically undefined behavior.  Hotspot already
> does this elsewhere (and we should merge some of the different
> conversion handling).
> 
> * In the original RFR for JDK-8184334, conversions between different
> integral types of the same size (along with enums) were performed
> using
> 
>   reinterpret_cast<ToType&>(from_value)
> 
> This was based on discussion from several months ago, referring to
> C++03 3.10/15.  However, re-reading that section in response to
> discussion of the RFR for JDK-8184334, I now think that implementation
> is not supported in many cases needed here, and indeed invokes
> undefined behavior in those cases.  In particular, where 3.10/15 says
> 
>   "- a type that is the signed or unsigned type corresponding to the
>      dynamic type of the object,"

That's true.  In general, trying to do type conversion on lvalues is a
bad idea.  On the other hand, converting rvalues isn't such an awful
sin: it has implementation-defined effects on all platforms, but the
ones that are well-suited to Java (i.e. byted addressed, native 8-,
16-, 32-, and 64-bit types) have the behaviour we want.  Or are you
seriously worried that some of them will misbehave?

> I interpret this to mean that if the dynamic type of an object is an
> integral type of a given rank, the value may be accessed using an
> lvalue of an integral type of the same rank and the opposite sign.
> (Using the same rank and sign is already permitted, since that's the
> same type.)  I don't think it allows access using an lvalue of an
> integral type with a different rank. 

That's right, it doesn't.

> So, for example, a signed int value may be accessed via an unsigned
> int lvalue.  But access via a signed long lvalue is not permitted,
> even if int and long have the same size.  I don't think there is a
> good way to perform the integral and enum conversions we need,
> without invoking implementation-defined, unspecified, or undefined
> behavior.

Implementation-defined should be fine, surely.

> ** The memcpy technique avoids such problems, but as noted above, is
> known to be quite poorly optimized by some compilers.
> 
> ** Using static_cast avoids any undefined behavior, but does have
> cases of implementation-defined or unspecified behavior.  (When
> casting an unsigned value to a signed type, the result may be
> implementation defined; 5.2.9/2 and 4.7/3.

Right, but do we actually care about any compilers which misbehave
when converting between signed and unsigned values of the same rank?

More seriously, IMO: accessing a stored pointer to X as though it had
been stored as a void* is certainly undefined behaviour, by the rule
you point out above.  It seems to me that you are worried about some
things that are relatively minor, while ignoring things that are
serious.

If you followed the approach of never converting the type of the
pointer to dest to some other point, *but instead converting the types
of exchange_value and compare_value* everything would be fine.  This
whole thing could be done with no UB at all.

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671