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

I did some more extensive testing and found that all GCCs older than 7
trigger the warning, but only if CMAKE_BUILD_TYPE=Release (which I
guess indicates optimizations are enabled).

There's a patch up for disabling the warning here:
https://reviews.llvm.org/D50607.

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.

While writing this I realized that LH_Mouse's double static_cast also
silences the warning, and I think that's a nicer workaround, so I put
up a patch for that too, here:
https://reviews.llvm.org/D50608

Let me know what you think,
- Kim
On Fri, Aug 10, 2018 at 11:30 PM Kim Gräsman <kim.grasman at gmail.com>
wrote:
>
> On Fri, Aug 10, 2018 at 6:08 PM, Sam McCall <sam.mccall at gmail.com>
wrote:
> > json::Value in JSON.h is a discriminated union.
> > The storage is a char array of appropriate type and alignment. The
storage
> > holds one object at a time, it's initialized (and for nontrivial
types,
> > destroyed) at the right times to ensure this. The cast is only to the
type
> > of object that's already there, there's no magic here.
> >
> >
> > On Fri, Aug 10, 2018, 17:52 Andrew Haley <aph at redhat.com>
wrote:
> >>
> >> Not exactly.  You can cast a pointer to a pointer to some
character
> >> type or the type of the object stored in memory.  It does not
matter
> >> whether you use an intermediate type or not.  Having not seen the
test
> >> case, I can't tell whether this rule is followed.
>
> Yes, if Andrew's explanation agrees with the standard, then this code
> should be benign (if it breaks any of these rules, that's a bug in and
> of itself).
>
> So maybe we're back to figuring out how to silence GCC's warning
machinery :)
>
> - Kim

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

Whether all compilers see through this is another question, and I
trust you that there may be dragons.

Thanks,
- Kim

在 2018-08-14 17:51, 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 this piece of code occurs in 'JSON.h' I presume it is sort of variant
implementation, then handwritten specializations of this template, 
taking the member expected, might be a bit superfluous.
> I don't think that GCC would produce this warning unnecessarily.  You
> are in dangerous waters.
> 
It it still in doubt that whether a union can /provide storage for/ 
other objects. Other than that, I think it should be safe to assume the 
behavior is quite defined here, as long as the object constructed in the 
union is always referenced through the _correct_ type of reference (that 
is, a reference type to the dynamic type, or a type similar to the 
dynamic type, possibly cv-qualified or different only in sign-ness, of 
the object that has been constructed in the union), and the union itself 
is never accessed directly, because no rule in [basic.lval]-11 is ever 
violated.

-- 
Best regards,
LH_Mouse

On Sun, 12 Aug 2018, Kim Gräsman wrote:
> I did some more extensive testing and found that all GCCs older than 7
> trigger the warning, but only if CMAKE_BUILD_TYPE=Release (which I
> guess indicates optimizations are enabled).
> 
> There's a patch up for disabling the warning here:
> https://reviews.llvm.org/D50607.
> 
> 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.
The warning code is simply broken - do not trust it in any way to
decide whether GCC will later optimize sth.  But removing it wasn't
met with approval because we do not have anything better and it
did point out issues in the past.

Richard.
> While writing this I realized that LH_Mouse's double static_cast also
> silences the warning, and I think that's a nicer workaround, so I put
> up a patch for that too, here:
> https://reviews.llvm.org/D50608
> 
> Let me know what you think,
> - Kim
> On Fri, Aug 10, 2018 at 11:30 PM Kim Gräsman <kim.grasman at
gmail.com> wrote:
> >
> > On Fri, Aug 10, 2018 at 6:08 PM, Sam McCall <sam.mccall at
gmail.com> wrote:
> > > json::Value in JSON.h is a discriminated union.
> > > The storage is a char array of appropriate type and alignment.
The storage
> > > holds one object at a time, it's initialized (and for
nontrivial types,
> > > destroyed) at the right times to ensure this. The cast is only to
the type
> > > of object that's already there, there's no magic here.
> > >
> > >
> > > On Fri, Aug 10, 2018, 17:52 Andrew Haley <aph at
redhat.com> wrote:
> > >>
> > >> Not exactly.  You can cast a pointer to a pointer to some
character
> > >> type or the type of the object stored in memory.  It does not
matter
> > >> whether you use an intermediate type or not.  Having not seen
the test
> > >> case, I can't tell whether this rule is followed.
> >
> > Yes, if Andrew's explanation agrees with the standard, then this
code
> > should be benign (if it breaks any of these rules, that's a bug in
and
> > of itself).
> >
> > So maybe we're back to figuring out how to silence GCC's
warning machinery :)
> >
> > - Kim
> 
> 
-- 
Richard Biener <rguenther at suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284
(AG Nuernberg)