Re: [llvm-dev] GCC 5 and -Wstrict-aliasing in JSON.h

[Kim Gräsman]

On Fri, Aug 10, 2018 at 6:08 PM, Sam McCall  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  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

Re: [llvm-dev] GCC 5 and -Wstrict-aliasing in JSON.h

[Sam McCall]

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  wrote:

> On 08/10/2018 05:30 AM, Liu Hao wrote:
> > Only an lvalue of a pointer to (possibly CV-qualified) `void` or a
> > pointer to a character type (in C) / any of `char`, `unsigned char` or
> > `std::byte` (in C++) can alias objects.
>
> Yes.
>
> > That is to say, in order to eliminate the aliasing problem an
> > intermediate lvalue pointer is required.
>
> 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.
>
> What you can't do is store as a double, cast the double pointer to a
> character pointer, cast that pointer to some other pointer type, and
> read from memory.  GCC won't give you a warning for that, but it's
> still undefined.
>
> JSON.h seems to hope that if you cast a pointer to T to a pointer to
> some union type, magic will happen.  It won't work, unless the object
> stored in the memory at that address was stored as the union type.
>
> Do not lie to the compiler or it will get its revenge.
>
> --
> Andrew Haley
> Java Platform Lead Engineer
> Red Hat UK Ltd. 
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
>

Re: [llvm-dev] GCC 5 and -Wstrict-aliasing in JSON.h

[Andrew Haley]

On 08/10/2018 05:30 AM, Liu Hao wrote:
> Only an lvalue of a pointer to (possibly CV-qualified) `void` or a 
> pointer to a character type (in C) / any of `char`, `unsigned char` or 
> `std::byte` (in C++) can alias objects.

Yes.

> That is to say, in order to eliminate the aliasing problem an 
> intermediate lvalue pointer is required.

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.

What you can't do is store as a double, cast the double pointer to a
character pointer, cast that pointer to some other pointer type, and
read from memory.  GCC won't give you a warning for that, but it's
still undefined.

JSON.h seems to hope that if you cast a pointer to T to a pointer to
some union type, magic will happen.  It won't work, unless the object
stored in the memory at that address was stored as the union type.

Do not lie to the compiler or it will get its revenge.

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. 
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: [llvm-dev] GCC 5 and -Wstrict-aliasing in JSON.h

[Liu Hao]

在 2018-08-10 18:53, Kim Gräsman 写道:

I'm worried that this might only serve to trick the compiler.



It shouldn't. If it was merely a trick then `std::aligned_storage` would 
be completely unusable.



Explicitly using `-fno-strict-aliasing` for GCC < 6 would seem more
direct to me -- as Jonathan says, if the compiler classifies a strict
aliasing rule violation as undefined behavior, and that is further
used to optimize in an unexpected manner, it doesn't matter whether it
warns or not. Then again, I guess disabling strict aliasing would also
disable optimizations that are generally useful for LLVM as a whole.

I'm reading up on safe aliasing techniques, but so far nothing stands
out to me as applicable in this scenario.



The C++ standard requires creation of objects in such ways to use new 
expressions (a.k.a. placement new). Athough [intro.object]-3 only 
defines /provides storage/ for arrays of a character type or 
`std::byte`, the specification of `aligned_storage` and `aligned_union` 
in [meta.trans.other] doesn't require the type used as uninitialized 
storage to be an array of a character type or `std::byte` - in fact it 
cannot be, because alignment information is not part of the nominal type 
system of C and will be lost when obtaining the `type` member.


Focusing on the cast: As long as the compiler is unable to know whether 
a placement new has been made on the union (i.e. whether it is providing 
storage for another object), I don't think a standard-conforming 
compiler is ever allowed to ignore such possibility.



- Kim




--
Best regards,
LH_Mouse

Re: [llvm-dev] GCC 5 and -Wstrict-aliasing in JSON.h

[Kim Gräsman]

Hi LH_Mouse,

Thanks!

On Fri, Aug 10, 2018 at 6:30 AM, Liu Hao  wrote:
>
> When I used to do such type punning in C, I got similar warnings. Then I
> looked for some solutions... I can't recall the principle now and I fail to
> find it in the C or C++ standard. Despite that, the solution is simple:
>
> Only an lvalue of a pointer to (possibly CV-qualified) `void` or a pointer
> to a character type (in C) / any of `char`, `unsigned char` or `std::byte`
> (in C++) can alias objects.
>
> That is to say, in order to eliminate the aliasing problem an intermediate
> lvalue pointer is required.
>
> Hence, altering
> ```
> return *reinterpret_cast(Union.buffer);
> ```
> to
> ```
> auto p = static_cast(Union.buffer);
> return *static_cast(p);
> ```
> will probably resolve this problem.

I'm worried that this might only serve to trick the compiler.

Explicitly using `-fno-strict-aliasing` for GCC < 6 would seem more
direct to me -- as Jonathan says, if the compiler classifies a strict
aliasing rule violation as undefined behavior, and that is further
used to optimize in an unexpected manner, it doesn't matter whether it
warns or not. Then again, I guess disabling strict aliasing would also
disable optimizations that are generally useful for LLVM as a whole.

I'm reading up on safe aliasing techniques, but so far nothing stands
out to me as applicable in this scenario.

- Kim

Re: [llvm-dev] GCC 5 and -Wstrict-aliasing in JSON.h

[Liu Hao]

在 2018-08-10 06:20, Kim Gräsman 写道:

On Fri, Aug 10, 2018 at 12:02 AM, Jonathan Wakely 
wrote:


If GCC 4.9.3 thinks there's an aliasing violation it might
misoptimise. It doesn't matter if it's right or not, it matters if it
treats the code as undefined or not.

And apparently GCC does think there's a violation, because it warns.

Unless you're sure that not only is the code OK, but GCC is just being
noisy and doesn't misoptimise, then I think using -fno-strict-aliasing
is safer than just suppressing the warning.


Good point, I can see how that would play out nicer.

So this would probably need to be addressed in the LLVM build system, I'll
try and work up a patch tomorrow.



When I used to do such type punning in C, I got similar warnings. Then I 
looked for some solutions... I can't recall the principle now and I fail 
to find it in the C or C++ standard. Despite that, the solution is simple:


Only an lvalue of a pointer to (possibly CV-qualified) `void` or a 
pointer to a character type (in C) / any of `char`, `unsigned char` or 
`std::byte` (in C++) can alias objects.


That is to say, in order to eliminate the aliasing problem an 
intermediate lvalue pointer is required.


Hence, altering
```
return *reinterpret_cast(Union.buffer);
```
to
```
auto p = static_cast(Union.buffer);
return *static_cast(p);
```
will probably resolve this problem.



Thanks,
- Kim




--
Best regards,
LH_Mouse

Re: [llvm-dev] GCC 5 and -Wstrict-aliasing in JSON.h

[Kim Gräsman]

On Fri, Aug 10, 2018 at 12:02 AM, Jonathan Wakely 
wrote:
>
> If GCC 4.9.3 thinks there's an aliasing violation it might
> misoptimise. It doesn't matter if it's right or not, it matters if it
> treats the code as undefined or not.
>
> And apparently GCC does think there's a violation, because it warns.
>
> Unless you're sure that not only is the code OK, but GCC is just being
> noisy and doesn't misoptimise, then I think using -fno-strict-aliasing
> is safer than just suppressing the warning.

Good point, I can see how that would play out nicer.

So this would probably need to be addressed in the LLVM build system, I'll
try and work up a patch tomorrow.

Thanks,
- Kim

Re: [llvm-dev] GCC 5 and -Wstrict-aliasing in JSON.h

[Jonathan Wakely]

On Thu, 9 Aug 2018 at 22:59, Kim Gräsman  wrote:
>
> Thanks all for pitching in to help!
>
> On Thu, Aug 9, 2018 at 1:25 PM, Sam McCall  wrote:
> >
> > Obviously if there really is something illegal here we should fix it in
> > LLVM, but it looks like this warning is a false positive (anyone disagree?)
>
> The little I've read about strict aliasing rules leaves me firmly
> incompetent to judge what's valid and not :)
>
> But since both Clang and GCC 6+ are happy with this, it seems
> plausible that this would be a false positive.

If GCC 4.9.3 thinks there's an aliasing violation it might
misoptimise. It doesn't matter if it's right or not, it matters if it
treats the code as undefined or not.

And apparently GCC does think there's a violation, because it warns.

Unless you're sure that not only is the code OK, but GCC is just being
noisy and doesn't misoptimise, then I think using -fno-strict-aliasing
is safer than just suppressing the warning.


> > Still if there's a simple source-level workaround, or we can suppress the
> > warning with a #pragma, I'd be happy to do that. GCC 4.9.3 is a supported
> > compiler for LLVM and the more configurations we build cleanly in, the
> > better.
>
> I *think* Leslie's warning-disable patch should work even with MSVC
> (it should ignore unknown #pragmas, right?). I can't think of a
> straight-up code change that would fix this.
>
> Cheers,
> - Kim

Re: [llvm-dev] GCC 5 and -Wstrict-aliasing in JSON.h

[Kim Gräsman]

Thanks all for pitching in to help!

On Thu, Aug 9, 2018 at 1:25 PM, Sam McCall  wrote:
>
> Obviously if there really is something illegal here we should fix it in
> LLVM, but it looks like this warning is a false positive (anyone disagree?)

The little I've read about strict aliasing rules leaves me firmly
incompetent to judge what's valid and not :)

But since both Clang and GCC 6+ are happy with this, it seems
plausible that this would be a false positive.

> Still if there's a simple source-level workaround, or we can suppress the
> warning with a #pragma, I'd be happy to do that. GCC 4.9.3 is a supported
> compiler for LLVM and the more configurations we build cleanly in, the
> better.

I *think* Leslie's warning-disable patch should work even with MSVC
(it should ignore unknown #pragmas, right?). I can't think of a
straight-up code change that would fix this.

Cheers,
- Kim

Re: [llvm-dev] GCC 5 and -Wstrict-aliasing in JSON.h

[Leslie Zhai]

Hi Sam,

Thanks for your response!

Please review it, thanks a lot!

A patch by Loongson!

diff --git a/include/llvm/Support/JSON.h b/include/llvm/Support/JSON.h
index da3c5ea..fd60b40 100644
--- a/include/llvm/Support/JSON.h
+++ b/include/llvm/Support/JSON.h
@@ -452,7 +452,10 @@ private:
 new (reinterpret_cast(Union.buffer)) T(std::forward(V)...);
   }
   template  T () const {
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wstrict-aliasing"
 return *reinterpret_cast(Union.buffer);
+#pragma GCC diagnostic pop
   }

   template 


在 2018年08月09日 19:25, Sam McCall 写道:
Author of the problematic code here. Thanks everyone, and sorry to 
have caused difficulty!


Obviously if there really is something illegal here we should fix it 
in LLVM, but it looks like this warning is a false positive (anyone 
disagree?)

False positive!



Still if there's a simple source-level workaround, or we can suppress 
the warning with a #pragma, I'd be happy to do that. GCC 4.9.3 is a 
supported compiler for LLVM and the more configurations we build 
cleanly in, the better.


If this is a useful direction, could someone with an affected 
environment send me a small patch? I don't have the right setup to 
verify myself.

I test it for mips64el built with gcc-4-branch:
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/mips64el-redhat-linux/4.9.3/lto-wrapper
Target: mips64el-redhat-linux
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man 
--infodir=/usr/share/info 
--with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap 
--enable-shared --enable-threads=posix --enable-checking=release 
--enable-multilib --with-system-zlib --enable-__cxa_atexit 
--disable-libunwind-exceptions --enable-gnu-unique-object 
--enable-linker-build-id --with-arch=loongson3a 
--enable-languages=c,c++,objc,obj-c++,fortran,go,lto --enable-plugin 
--disable-libgcj 
--with-isl=/builddir/build/BUILD/gcc-4.9.3/obj-mips64el-redhat-linux/isl-install 
--with-cloog=/builddir/build/BUILD/gcc-4.9.3/obj-mips64el-redhat-linux/cloog-install 
--enable-gnu-indirect-function --with-long-double-128 
--build=mips64el-redhat-linux

Thread model: posix
gcc version 4.9.3 20150626 (Red Hat 4.9.3-8) (GCC)



Cheers, Sam


--
Regards,
Leslie Zhai

Re: [llvm-dev] GCC 5 and -Wstrict-aliasing in JSON.h

[Sam McCall]

Author of the problematic code here. Thanks everyone, and sorry to have
caused difficulty!

Obviously if there really is something illegal here we should fix it in
LLVM, but it looks like this warning is a false positive (anyone disagree?)

Still if there's a simple source-level workaround, or we can suppress the
warning with a #pragma, I'd be happy to do that. GCC 4.9.3 is a supported
compiler for LLVM and the more configurations we build cleanly in, the
better.

If this is a useful direction, could someone with an affected environment
send me a small patch? I don't have the right setup to verify myself.

Cheers, Sam

Re: [llvm-dev] GCC 5 and -Wstrict-aliasing in JSON.h

[Jonathan Wakely]

On Thu, 9 Aug 2018 at 12:04, Leslie Zhai wrote:
>
> Hi Jonathan,
>
> Thanks for your response!
>
> So workaround for Kim's issue is bootstrap old version LLVM with GCC 4/5
> to build old clang, then bootstrap latest LLVM with old clang.

It would be much easier to just use -fno-strict-aliasing with the older GCC.

Re: [llvm-dev] GCC 5 and -Wstrict-aliasing in JSON.h

[Leslie Zhai]

Hi Jonathan,

Thanks for your response!

So workaround for Kim's issue is bootstrap old version LLVM with GCC 4/5 
to build old clang, then bootstrap latest LLVM with old clang.



在 2018年08月09日 17:16, Jonathan Wakely 写道:

On Thu, 9 Aug 2018 at 03:09, Leslie Zhai wrote:

Could you test to compile LLVM with GCC old versions 4/5/6? Does it need
to backport your patch to GCC old version?

GCC versions before 6.4 are not supported, so no backports will happen
to 4.x or 5 releases and I doubt anybody's going to routinely test
anything with them. If there's a bug in GCC 4.9.3 that you can't work
around then you should upgrade to a supported release (or use a
distribution that provides support for older releases).


--
Regards,
Leslie Zhai

Re: [llvm-dev] GCC 5 and -Wstrict-aliasing in JSON.h

[Jonathan Wakely]

On Thu, 9 Aug 2018 at 03:09, Leslie Zhai wrote:
> Could you test to compile LLVM with GCC old versions 4/5/6? Does it need
> to backport your patch to GCC old version?

GCC versions before 6.4 are not supported, so no backports will happen
to 4.x or 5 releases and I doubt anybody's going to routinely test
anything with them. If there's a bug in GCC 4.9.3 that you can't work
around then you should upgrade to a supported release (or use a
distribution that provides support for older releases).

Re: [llvm-dev] GCC 5 and -Wstrict-aliasing in JSON.h

[Leslie Zhai]

Hi Kim,

Thanks for your report!

My GCC 4.9.3 is *not* able to reproduce the issue with reduced testcase[1]

But it is able to reproduce the issue by compiling LLVM with GCC 
toolchain[2]



In file included from /home/loongson/llvm/lib/Support/JSON.cpp:10:0:
/home/loongson/llvm/include/llvm/Support/JSON.h: In instantiation of ‘T& 
llvm::json::Value::as() const [with T = bool]’:

/home/loongson/llvm/include/llvm/Support/JSON.h:393:23:   required from here
/home/loongson/llvm/include/llvm/Support/JSON.h:455:47: warning: 
dereferencing type-punned pointer will break strict-aliasing rules 
[-Wstrict-aliasing]

 return *reinterpret_cast(Union.buffer);
   ^
/home/loongson/llvm/include/llvm/Support/JSON.h: In instantiation of ‘T& 
llvm::json::Value::as() const [with T = double]’:

/home/loongson/llvm/include/llvm/Support/JSON.h:398:25:   required from here
/home/loongson/llvm/include/llvm/Support/JSON.h:455:47: warning: 
dereferencing type-punned pointer will break strict-aliasing rules 
[-Wstrict-aliasing]
/home/loongson/llvm/include/llvm/Support/JSON.h: In instantiation of ‘T& 
llvm::json::Value::as() const [with T = long int]’:

/home/loongson/llvm/include/llvm/Support/JSON.h:400:26:   required from here
/home/loongson/llvm/include/llvm/Support/JSON.h:455:47: warning: 
dereferencing type-punned pointer will break strict-aliasing rules 
[-Wstrict-aliasing]
/home/loongson/llvm/include/llvm/Support/JSON.h: In instantiation of ‘T& 
llvm::json::Value::as() const [with T = std::basic_string]’:

/home/loongson/llvm/include/llvm/Support/JSON.h:418:46:   required from here
/home/loongson/llvm/include/llvm/Support/JSON.h:455:47: warning: 
dereferencing type-punned pointer will break strict-aliasing rules 
[-Wstrict-aliasing]
/home/loongson/llvm/include/llvm/Support/JSON.h: In instantiation of ‘T& 
llvm::json::Value::as() const [with T = llvm::StringRef]’:

/home/loongson/llvm/include/llvm/Support/JSON.h:420:34:   required from here
/home/loongson/llvm/include/llvm/Support/JSON.h:455:47: warning: 
dereferencing type-punned pointer will break strict-aliasing rules 
[-Wstrict-aliasing]
/home/loongson/llvm/include/llvm/Support/JSON.h: In instantiation of ‘T& 
llvm::json::Value::as() const [with T = llvm::json::Object]’:

/home/loongson/llvm/include/llvm/Support/JSON.h:424:62:   required from here
/home/loongson/llvm/include/llvm/Support/JSON.h:455:47: warning: 
dereferencing type-punned pointer will break strict-aliasing rules 
[-Wstrict-aliasing]
/home/loongson/llvm/include/llvm/Support/JSON.h: In instantiation of ‘T& 
llvm::json::Value::as() const [with T = llvm::json::Array]’:

/home/loongson/llvm/include/llvm/Support/JSON.h:430:60:   required from here
/home/loongson/llvm/include/llvm/Support/JSON.h:455:47: warning: 
dereferencing type-punned pointer will break strict-aliasing rules 
[-Wstrict-aliasing]

In file included from /home/loongson/llvm/lib/TableGen/JSONBackend.cpp:20:0:
/home/loongson/llvm/include/llvm/Support/JSON.h: In instantiation of ‘T& 
llvm::json::Value::as() const [with T = bool]’:

/home/loongson/llvm/include/llvm/Support/JSON.h:393:23:   required from here
/home/loongson/llvm/include/llvm/Support/JSON.h:455:47: warning: 
dereferencing type-punned pointer will break strict-aliasing rules 
[-Wstrict-aliasing]

 return *reinterpret_cast(Union.buffer);
   ^
/home/loongson/llvm/include/llvm/Support/JSON.h: In instantiation of ‘T& 
llvm::json::Value::as() const [with T = double]’:

/home/loongson/llvm/include/llvm/Support/JSON.h:398:25:   required from here
/home/loongson/llvm/include/llvm/Support/JSON.h:455:47: warning: 
dereferencing type-punned pointer will break strict-aliasing rules 
[-Wstrict-aliasing]
/home/loongson/llvm/include/llvm/Support/JSON.h: In instantiation of ‘T& 
llvm::json::Value::as() const [with T = long int]’:

/home/loongson/llvm/include/llvm/Support/JSON.h:400:26:   required from here
/home/loongson/llvm/include/llvm/Support/JSON.h:455:47: warning: 
dereferencing type-punned pointer will break strict-aliasing rules 
[-Wstrict-aliasing]
/home/loongson/llvm/include/llvm/Support/JSON.h: In instantiation of ‘T& 
llvm::json::Value::as() const [with T = std::basic_string]’:

/home/loongson/llvm/include/llvm/Support/JSON.h:418:46:   required from here
/home/loongson/llvm/include/llvm/Support/JSON.h:455:47: warning: 
dereferencing type-punned pointer will break strict-aliasing rules 
[-Wstrict-aliasing]
/home/loongson/llvm/include/llvm/Support/JSON.h: In instantiation of ‘T& 
llvm::json::Value::as() const [with T = llvm::StringRef]’:

/home/loongson/llvm/include/llvm/Support/JSON.h:420:34:   required from here
/home/loongson/llvm/include/llvm/Support/JSON.h:455:47: warning: 
dereferencing type-punned pointer will break strict-aliasing rules 
[-Wstrict-aliasing]
/home/loongson/llvm/include/llvm/Support/JSON.h: In instantiation of ‘T& 
llvm::json::Value::as() const [with T =