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

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.

On Tue, Aug 14, 2018 at 1:16 PM Jonathan Wakely <jwakely.gcc at gmail.com>
wrote:
>
> 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.
Interesting! That json::Value type wrapping all this does indeed have
a copy constructor that just does memcpy of primitive values between
buffers.

I tried changing it to placement-new the copy into the buffer instead,
but GCC 5 still warns, so there's something else afoot.

- Kim