RFR (M): 8184334: Generalizing Atomic with templates

[Andrew Haley]

On 18/07/17 16:26, Erik Österlund wrote:
> 
> 
> On 2017-07-18 16:41, Andrew Haley wrote:
>> On 18/07/17 14:46, Erik Österlund wrote:
>>>
>>> On 2017-07-18 11:57, Andrew Haley wrote:
>>>> On 18/07/17 10:38, Erik Österlund wrote:

>> It is more general than that.  If you access the stored value of an
>> object through a glvalue of other than one of the allowed types, then
>> your *whole program is undefined*.
> 
> And I think it is in that list of allowed types. Specifically, it is my 
> interpretation of the standard that intptr_t is "a type that is the 
> signed or unsigned type corresponding to the dynamic type of the object" 
> (cf. 5.2.10) if the dynamic type is e.g. void*.
> 
>>> and that therefore it is fine to load something (that was stored as
>>> A*) as intptr_t and subsequently cast it to A* before the A itself
>>> is being accessed. Am I wrong?
>> No, that is correct.  The question is whether intptr_t is a type
>> compatible with a pointer type, and I don't think you will find any
>> language to the effect that it is.
> 
> I think 5.2.10 describing reinterpret_cast seems to suggest that they 
> are compatible by saying "A pointer converted to an integer of 
> sufficient size (if any such exists on the implementation) and back to 
> the same pointer type will have its original value". That is precisely 
> what is done.

This is the core of our disagreement.

In a sense, it doesn't matter which of us is right; what really
matters is the opinion of the compiler.  GCC does not put intptr_t and
pointer types into the same alias set, and even if this did turn out
to be a bug in GCC it wouldn't help us because we often use
unsupported old versions of GCC to build OpenJDK, so it wouldn't get
fixed anyway.

So what are we to do?  Do you believe me when I say that GCC does
treat A* and intptr_t to be compatible types?  If not, what would it
take to convince you?

> Conversely, I do not find any language suggesting that intptr_t would 
> not be compatible with pointer types.
> 
>> Pointer types are distinct from one another and from all integer
>> types.  People sometimes get confused by this: the fact that you can
>> cast from A* to e.g. void*, doesn't tell you that you can cast from
>> A** to void** and use the result: they are different types.
> 
> Agreed. But I do not see any problem with reinterpret_cast from A* to 
> intptr_t back to A* and using the A from that result. That is explicitly 
> allowed as per 5.2.10 as previously described.

I don't think that's what you're doing.  I think you're casting from
A** to intptr_t*, dereferencing the intptr_t*, and casting the result
to A*.

> That looks like the correct (and indeed expected) output. iptr should be 
> the same as &b, and iptr->_x should be 0, and &a should be different 
> from &b and iptr. I tried this experiment locally too with similar 
> correct result. Am I missing something here?

No, I was; I misinterpreted the result, and it's a red herring.
Sorry.

I believe this patch to be dangerous.  We might well get away with it,
given sufficient larding of volatile, but it's still undefined
behaviour.  We have far too much UB in HotSpot already, and we should
not be adding any more.

And I still believe that this patch is more complex than is needed,
and with many (if not all) targets we could do something much simpler,
and which does not need to cast pointer types.  It could also map
cleanly onto C++11.

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