RFR (M): 8184334: Generalizing Atomic with templates

[Andrew Haley]

Hi,

On 27/07/17 15:54, Erik Österlund wrote:

> Thank you for having another look at this. I do see appeal with allowing 
> platforms to specialize before truncating types. I also recognize that 
> your implementation is simpler. So thank you for your efforts helping 
> out here.

Thank you.  You're being very gracious about this.

> However... I do think that the simplification comes at a cost. Since we 
> are now introducing templates that accept any types, I would like to 
> protect the user of the API from doing things that are not a good idea. 

Well, I think that was you, not me: it's simpler for everyone if we
insist that all of the arguments to these functions are the same type.
Some of the callers will have to change, sure.

> That was one of the key reasons why the IntegerTypes casting mechanisms 
> were employed. To safely cast and sanity check the passed in types to 
> make sense.
> 
> Take for example a user passing in to your cmpxchg method a new_value of 
> float, a destination of volatile int*, and an expected value of float. 

Perhaps I wasn't clear enough.  I think I implied that that there
should be specializations which would do the bitwise mapping from
e.g. double to uint64_t and call the generic cmpxchg.  I didn't
describe these for the sake of brevity and because they aren't used
anywhere in HotSpot.

> This clearly makes no sense, and I would like the Atomic API to not 
> allow passing in things that clearly do not make sense.

I agree.

> The proposed mechanism will explicitly convert the expected and new
> values to ints, which will truncate their values under the hood,
> resulting in a different value being written to what was
> expected.

Sure, I get that.

> Similarly, I believe similar problems may apply when different
> integer types of with different sizes are passed in that would not
> be implicitly convertible, but sure are explicitly convertible (with
> possibly unintentional truncation). You mentioned the case of
> extending a 32 bit integer to a 64 bit integer, which indeed is
> fine. Unfortunately, the reverse is not fine yet possible in this
> API. For example, if the destination is of type volatile jint* and
> the passed in expected value is a jlong, then that is clearly
> wrong. Yet the API will accept these types and explicitly convert
> the jlong to jint, truncating it in the process without
> warning. This is the kind of thing that the casting mechanism me and
> Kim worked on prevents you from doing accidentally.

OK, I see.  But it's a heck of a lot of code just to do that.

Instead, we could simply insist that the types must be the same, and
fix the callers.  This makes everything clearer, including the point
where the Atomic API is called.  Mismatched types at this level are
either unintentional oversights or plain bugs.  I have made this
change, and it took a few minutes to fix all eight of the callers
in HotSpot which have mismatched types.

> All in all, I do like the idea of allowing platforms that do not
> want to rely on -fno-strict-aliasing to keep as much type
> information as possible, and give it to the compiler. Having said
> that, I do think the API should protect users from messing up
> instead of silently accepting not compliant types and truncating
> their values.

OK, good.  We're on the same page.

> How would you feel about a mechanism where we allow each platform to
> specialize on the top level with all type information available, but
> by default send everything in to the Atomic implementation I have
> proposed that canonicalizes and sanity checks all values. Then we
> have solved a bunch of existing problems, not introduced any new
> problems, yet are future proof for allowing arbitrary specialization
> on each platform to allow further improving the solution on
> platforms that have cooperative compilers that allow such
> improvements, on a platform by platform basis.
> 
> What do you think?

It's a better solution, and it is much more well suited to the future,
when we can move over to compiler intrinsics.  That's really
important, IMO: it gives us an escape hatch from the past.

But I think it's not really necessary: if we insist on something like

  template <typename T>
  inline static T cmpxchg(T exchange_value, volatile T* dest, T compare_value,
                          cmpxchg_memory_order order = memory_order_conservative);

then all of the casts go away because they're unneeded.  We'd have
e.g.

template <typename T>
struct Atomic::CmpxchgHelper<T, 8> {
  T operator()(T exchange_value, volatile T* dest, T compare_value,
               cmpxchg_memory_order order) {
    T result;
    __asm__ __volatile__ ("lock cmpxchgq %1,(%3)"
                          : "=a" (result)
                          : "r" (exchange_value), "a" (compare_value), "r" (dest)
                          : "cc", "memory");
    return result;
  }
};

(Look ma, no casts!)

However, this would certainly fail to compile if anyone passed a
double.  To handle floating-point operands I would do one of two
things:

Create explicit specializations for doubles and floats: these do
bitwise copies into integer values when calling the assembly-language
sinippets, or

Make all of the snippets copy their arguments bitwise into integer
values and back.  I believe that the best-supported way of doing that
is the "union trick", which is widely supported by C++ compilers, even
when using strict aliasing.

Andrew.