RFR: 8067790: Better support for native implementations of Atomic::cmpxchg

[Kim Barrett]

On Dec 17, 2014, at 11:38 AM, Erik Österlund <erik.osterlund at lnu.se> wrote:
> 
> RFE: https://bugs.openjdk.java.net/browse/JDK-8067790
> Webrev: http://cr.openjdk.java.net/~jwilhelm/8067790/webrev.00/

This seems like a lot of infrastructure, just to eliminate a
per-platform conditionally defined macro and the corresponding test
for its definition. If there were more atomic operations that could be
similarly conditionalized, with a default version that can be
overridden by a platform-specific version, some additional
infrastructure might be warranted. But as it stands, I don't think it
is.

The rest of these comments only matter if, contrary to my suggestion
above, some additional infrastructure is thought necessary.

The AtomicBase and various AtomicPlatform classes should not provide
public cmpxchg(jbyte...) functions. They should instead provide
protected cmpxchg_jbyte() functions, to be called by the public Atomic
interface function (whch can make an unqualified call to
cmpxchg_jbyte() and get the right version).
(AtomicBase::cmpxchg_general should be renamed to cmpxchg_jbyte.)

All of the template metaprogramming infrastructure proposed by the
offered change set can instead be accomplished by one conditionally
defined macro, e.g.

- Along with each definition of AtomicPlatform, also define
VM_HAS_ATOMIC_PLATFORM.

- After all the atomic_<os_arch>.hpp headers have been conditionally
included, add the following:

#ifdef VM_HAS_ATOMIC_PLATFORM
typedef AtomicPlatform AtomicSuper;
#else
typedef AtomicBase AtomicSuper;
#endif

[An alternative to the introduction of the AtomicSuper typedef would
be to just conditionalize the base class for Atomic.]

This scales with the number of AtomicPlatform definitions, rather than
the number of specialized functions as happens with the existing code.

The rest of these comments only matter if, contrary to my suggestion
above, the additional template metaprogramming infrastructure is
thought necessary.

I think the name "IsBaseOfAndDerived" would be better as
"IsBaseAndDerived".  That would also match the naming used by Boost,
and several other similar libraries one can find via web search (some
of which make explicit nod to the Boost name).

The current implementation of IsBaseOfAndDerived (and therefore
IsBaseOf) may (silently) provide incorrect answers when called with
incomplete types. C++11 is_base_of requires Derived to be complete if
it differs from Base. Boost has the stronger requirement that both
Base and Derived must be complete. This is an important flaw in the
offered implementations.

The actual usage in this proposed change-set even relies on this
flawed behavior of these metafunctions, since AtomicPlatform isn't
always defined. This seems rather icky to me. The only way to address
this would be to ensure that AtomicPlatform is defined for all
targets, either via explicit empty definitions or via the
VM_HAS_ATOMIC_PLATFORM technique described above.  But if the latter,
then we don't need all this TMP infrastructure.