GCC 5 and -Wstrict-aliasing in JSON.h

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

WoW! Andrew Haley :slight_smile:

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

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

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.

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

Thanks Richard (and all) good info!

- Kim