RFR (M): 8184334: Generalizing Atomic with templates

[Roman Kennke]

+1 for enforcing same type for cmpxchg parameters
+1 for keeping it as simple as possible

Roman

Just Am 28.07.2017 um 16:15 schrieb Andrew Haley:
> Hi,
>
> On 27/07/17 18:27, Erik Osterlund wrote:
>> 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:
>>> 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.
> OK.  It looked to me like some much-needed discipline for HotSpot
> programmers, but I guess that's a matter of taste.  :-)
>
>>>> 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.
> Hmm.  This too is perhaps a matter of taste: I'm a great believer in
> not speculatively coding.  But that's a discussion for another day.
>
>>> 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).
> I believe not.
>
>>>> 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/
> Alright, let's see where this takes us.  It's be good to see if we can
> get the desirable properties of only allowing safe type conversions
> but still passing all of the types down to the native layer where
> possible.
>
>>> 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.
> It certainly wouldn't with GCC, because GCC defines that as an
> extension, and it is thus implementation-defined.  I suspect that is
> true of other compilers used to build HotSpot.  I don't much care
> about theoretical niceties, but I do care about incorrect code.  The
> union trick is so widely used that its UB may not matter, but I don't
> have much experience of proprietary C++ compilers so I might be wrong
> about that.
>
> But never mind, we're using -fno-strict-aliasing anyway.
>
>> Hope we are converging!
> I think so.
>