llvm dev - Aug 2018 - GCC 5 and -Wstrict-aliasing in JSON.h

On 08/12/2018 02:19 PM, Kim Gräsman wrote:
> I still feel a little uncomfortable, because I think Jonathan makes an
> excellent point -- if GCC thinks there's a strict-aliasing violation
> (whether the standard agrees or not) and classifies this as undefined
> behavior, we might invoke the optimizers wrath. The warning is a nice
> hint that this could happen.
Indeed.  And I'm not convinced that the pointer cast is necessary anyway:
if the type passed in is a union, why not simply take the union member of
the appropriate type?

I don't think that GCC would produce this warning unnecessarily.  You
are in dangerous waters.

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

On Tue, 14 Aug 2018 at 11:56, Kim Gräsman <kim.grasman at gmail.com>
wrote:
>
> On Tue, Aug 14, 2018 at 11:51 AM Andrew Haley <aph at redhat.com>
wrote:
> >
> > On 08/12/2018 02:19 PM, Kim Gräsman wrote:
> > > I still feel a little uncomfortable, because I think Jonathan
makes an
> > > excellent point -- if GCC thinks there's a strict-aliasing
violation
> > > (whether the standard agrees or not) and classifies this as
undefined
> > > behavior, we might invoke the optimizers wrath. The warning is a
nice
> > > hint that this could happen.
> >
> > Indeed.  And I'm not convinced that the pointer cast is necessary
anyway:
> > if the type passed in is a union, why not simply take the union member
of
> > the appropriate type?
>
> As it turns out, Union is not a union ¯\_(ツ)_/¯. (I thought it was, up
> until this point.)
>
> It's a template producing a char array suitably aligned for any number
> of mutually exclusive types, much like
> https://en.cppreference.com/w/cpp/types/aligned_union.
>
> I can't tell if that makes the waters less dangerous, but there's
an
> invariant for this code that it only reinterpret_casts out Ts from the
> char buffer that it has previously put in there using placement new.
> As far as I can tell, that should be safe. The local construct in the
> as<T>() member function template, something like 'return
> *reinterpret<T *>(buffer);' is generally unsafe. But the calling
code
> always maintains the type invariant (only invoking as<T> for the
right
> T), so I'd argue this is fine.
It's fine if the T object was placed in *that* buffer using placement
new. We've had problems in libstdc++ in the past where we used
placement new to create an object in a char buffer, then copied the
char buffer (using a trivial assignment on the struct containing the
buffer) and then tried to access a T from the copy. GCC didn't like
that. No T object was ever constructed in the second buffer, because a
copy of the underlying bytes is not a copy of the object. As long as
that's not the case here, GCC's warning might well be a false
positive.

Hi all,

Thanks for all your response!

It is a good teaching case I can learn from it, and I forgot to paste 
what I mentioned before:

* Almost the same usecase, but they argue that it should be fixed in the 
compiler side https://github.com/mapbox/variant/issues/148

* Reduced testcase punning.cpp is *not* reproduce for gcc 4.9.3 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80593


在 2018年08月14日 18:55, Kim Gräsman 写道:
> On Tue, Aug 14, 2018 at 11:51 AM Andrew Haley <aph at redhat.com>
wrote:
WoW! Andrew Haley :)
>> On 08/12/2018 02:19 PM, Kim Gräsman wrote:
>>> I still feel a little uncomfortable, because I think Jonathan makes
an
>>> excellent point -- if GCC thinks there's a strict-aliasing
violation
>>> (whether the standard agrees or not) and classifies this as
undefined
>>> behavior, we might invoke the optimizers wrath. The warning is a
nice
>>> hint that this could happen.
>> Indeed.  And I'm not convinced that the pointer cast is necessary
anyway:
>> if the type passed in is a union, why not simply take the union member
of
>> the appropriate type?
> As it turns out, Union is not a union ¯\_(ツ)_/¯. (I thought it was, up
> until this point.)
>
> It's a template producing a char array suitably aligned for any number
> of mutually exclusive types, much like
> https://en.cppreference.com/w/cpp/types/aligned_union.
>
> I can't tell if that makes the waters less dangerous, but there's
an
> invariant for this code that it only reinterpret_casts out Ts from the
> char buffer that it has previously put in there using placement new.
> As far as I can tell, that should be safe. The local construct in the
> as<T>() member function template, something like 'return
> *reinterpret<T *>(buffer);' is generally unsafe. But the calling
code
> always maintains the type invariant (only invoking as<T> for the
right
> T), so I'd argue this is fine.
>
> Whether all compilers see through this is another question, and I
> trust you that there may be dragons.
>
> Thanks,
> - Kim
-- 
Regards,
Leslie Zhai

On 08/14/2018 11:55 AM, Kim Gräsman wrote:
> On Tue, Aug 14, 2018 at 11:51 AM Andrew Haley <aph at redhat.com>
wrote:
>>
>> On 08/12/2018 02:19 PM, Kim Gräsman wrote:
>>> I still feel a little uncomfortable, because I think Jonathan makes
an
>>> excellent point -- if GCC thinks there's a strict-aliasing
violation
>>> (whether the standard agrees or not) and classifies this as
undefined
>>> behavior, we might invoke the optimizers wrath. The warning is a
nice
>>> hint that this could happen.
>>
>> Indeed.  And I'm not convinced that the pointer cast is necessary
anyway:
>> if the type passed in is a union, why not simply take the union member
of
>> the appropriate type?
> 
> As it turns out, Union is not a union ¯\_(ツ)_/¯. (I thought it was, up
> until this point.)
> 
> It's a template producing a char array suitably aligned for any number
> of mutually exclusive types, much like
> https://en.cppreference.com/w/cpp/types/aligned_union.
> 
> I can't tell if that makes the waters less dangerous, but there's
an
> invariant for this code that it only reinterpret_casts out Ts from the
> char buffer that it has previously put in there using placement new.
So I think GCC is wrong here. The intent of aligned_union is clearly that
it is a POD-type suitable for any of its declared types.

So what exactly is the type Union?  It has a field called buffer.

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