RFR (M): 8184334: Generalizing Atomic with templates

[Erik Osterlund]

Hi Andrew,

> On 27 Jul 2017, at 17:35, Andrew Haley <aph at redhat.com> wrote:
> 
> 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.

I did indeed introduce the ability to match any types, but with applied sanity checks protecting users from being able to compile if incompatible types are passed in.

I did try earlier on to see what would happen if we forced the types to be identical as you propose. That turned out to be too strict. It made it impossible to pass in perfectly fine literals like NULL or 0 where it should be fine (among other things). This would force too many call sites to have to be changed to explicitly cast passed in values to types that they could perfectly safely convert to implicitly. Therefore I decided against it.

> 
>> 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.

Okay, sorry I did not catch that.

> 
>> 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.

Perhaps. But IntegerTypes is a general reusable tool that might find further use later on.

> 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.

I think I already covered why this was not done. Also, that is also a "heck of a lot of code" invested in being able to reduce the anount of code in Atomic. I think you will come to the same conclusion once you apply that pattern to all atomic operations as well as OrderAccess (coming up after we are done here as it builds on Atomic).

> 
>> 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.

Excellent.

> 
>> 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.

Okay. I am currently on my way home, but here is an untested prototype:

http://cr.openjdk.java.net/~eosterlund/8184334/webrev.02/

This variant allows you to declare an AllStatic PlatformAtomic class in your favourite platfoem file that it should automatically pick up (unless I messed up - I have not tried it yet) as an override for AtomicImpl which Atomic forwards all operations to too. This allows overriding Atomic completely on a per platform basis.

That is, when no such PlatformAtomic declaration is found from the platform specific files, there is only a forward declaration of PlatformAtomic, which is detected, and the AtomicImpl will redirect its type to GeneralizedAtomic instead, which canonicalizes the types safely according to current mechanisms.

Now this webrev is only a sneak preview of that concept for quicker turnaround of the idea. If we decide we like the concept, then I will send out a new webrev that has been tested and fixed indentation issues that I know are present after the mechanical change from Atomic:: to GeneralizedAtomic:: in platform specific files.

> 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!)

This does indeed not use casts. However, I believe that would either suffer from the problem I mentioned earlier about not accepting perfectly reasonable literals eithout explicit casting, propagating to a lot of call sites and being generally annoying, or miss e.g. required sign extension in places if the types are allowed to be different and passed straight into the inline assembly.

> 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.

That makes sense. Unfortunately though, that would still violate section 3.10 and remain undefined behaviour. That is, a double field could be initialized as 0.0, and then be loaded as an int64_t through e.g. Atomic::xchg. It would be unfortunate if a mechanism was built with the sole purpose of cutting reliance on 3.10 if it does not actually solve that problem. But I am happy to defer that discussion about a possible new specialization to when we actually get there. But perhaps we could do that in a subsequent RFR.

Hope we are converging!

Thanks,
/Erik

> Andrew.