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

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

On Tue, Aug 14, 2018 at 4:49 PM Andrew Haley <aph at redhat.com>
wrote:
>
> 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.
Union is of type llvm::AlignedCharArrayUnion<...>. It looks like a
predecessor to aligned_union. If you're willing/allowed to look at
LLVM source code, it's declared here:
https://github.com/llvm-mirror/llvm/blob/master/include/llvm/Support/AlignOf.h#L138

As far as I can tell, it works like aligned_union, except instead of
declaring a nested type with the right alignment and size, it directly
declares a char buffer with the same qualities.

The union-ed types are:
llvm::AlignedCharArrayUnion<bool, double, int64_t, llvm::StringRef,
std::string, json::Array, json::Object> Union;

- Kim

On Tue, 14 Aug 2018, Andrew Haley wrote:
> 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.
If you look at the implementation of the strict-aliasing warning
you will immediately notice that it's stupid and the only way to
"fix"
it is to remove it.

Properly warning for strict-aliasing violations isn't possible - you'd
need to conservatively detect must-aliases and track dynamic types.
If you'd detect a violation the compiler should better not exploit it.
You may have noticed we do not "miscompile"

int foo (int *p)
{
  *p = 1;
  *(float *)p = 0.0;
  return *p;
}

since quite some time but correctly zero *p and return zero.  We
do not warn for the above though because the access uses a pointer.
We only warn when we see a declared type of the storage in the very
same GENERIC expression(!).
> So what exactly is the type Union?  It has a field called buffer.
GCC 6+ contain fixes for sth that later C++ standards appearantly
need, namely that copy-assignment of aggregates with embedded
storage copies the dynamic type of said storage.  Otherwise placement
new should work just fine.

Richard.

On Fri, Aug 17, 2018 at 1:17 PM Richard Biener <rguenther at suse.de>
wrote:
>
> Properly warning for strict-aliasing violations isn't possible -
you'd
> need to conservatively detect must-aliases and track dynamic types.
> If you'd detect a violation the compiler should better not exploit it.
> You may have noticed we do not "miscompile"
>
> int foo (int *p)
> {
>   *p = 1;
>   *(float *)p = 0.0;
>   return *p;
> }
>
> since quite some time but correctly zero *p and return zero.  We
> do not warn for the above though because the access uses a pointer.
> We only warn when we see a declared type of the storage in the very
> same GENERIC expression(!).
>
> > So what exactly is the type Union?  It has a field called buffer.
>
> GCC 6+ contain fixes for sth that later C++ standards appearantly
> need, namely that copy-assignment of aggregates with embedded
> storage copies the dynamic type of said storage.  Otherwise placement
> new should work just fine.
Thanks Richard (and all) good info!

- Kim