[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #103 from Richard Biener  ---
Author: rguenth
Date: Wed Apr 12 07:35:49 2017
New Revision: 246866

URL: https://gcc.gnu.org/viewcvs?rev=246866=gcc=rev
Log:
2017-04-12  Richard Biener  
Bernd Edlinger  

PR middle-end/79671
* alias.c (component_uses_parent_alias_set_from): Handle
TYPE_TYPELESS_STORAGE.
(get_alias_set): Likewise.
* tree-core.h (tree_type_common): Add typeless_storage flag.
* tree.h (TYPE_TYPELESS_STORAGE): New macro.
* stor-layout.c (place_union_field): Set TYPE_TYPELESS_STORAGE
for types containing members with TYPE_TYPELESS_STORAGE.
(place_field): Likewise.
(layout_type): Likewise for ARRAY_TYPE.
* lto-streamer-out.c (hash_tree): Hash TYPE_TYPELESS_STORAGE.
* tree-streamer-in.c (unpack_ts_type_common_value_fields): Stream
TYPE_TYPELESS_STORAGE.
* tree-streamer-out.c (pack_ts_type_common_value_fields): Likewise.

lto/
* lto.c (compare_tree_sccs_1): Compare TYPE_TYPELESS_STORAGE.

cp/
* tree.c (build_cplus_array_type): Set TYPE_TYPELESS_STORAGE
for arrays of character or std::byte type.

* g++.dg/torture/pr79671.C: New testcase.
* g++.dg/lto/pr79671_0.C: Likewise.
* g++.dg/lto/pr79671_1.c: Likewise.

Added:
trunk/gcc/testsuite/g++.dg/lto/pr79671_0.C
trunk/gcc/testsuite/g++.dg/lto/pr79671_1.c
trunk/gcc/testsuite/g++.dg/torture/pr79671.C
Modified:
trunk/gcc/ChangeLog
trunk/gcc/alias.c
trunk/gcc/cp/ChangeLog
trunk/gcc/cp/tree.c
trunk/gcc/lto-streamer-out.c
trunk/gcc/lto/ChangeLog
trunk/gcc/lto/lto.c
trunk/gcc/stor-layout.c
trunk/gcc/testsuite/ChangeLog
trunk/gcc/tree-core.h
trunk/gcc/tree-streamer-in.c
trunk/gcc/tree-streamer-out.c
trunk/gcc/tree.h

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

Richard Biener  changed:

   What|Removed |Added

 Status|REOPENED|RESOLVED
 Resolution|--- |FIXED

--- Comment #102 from Richard Biener  ---
Fixed.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #101 from Bernd Edlinger  ---
(In reply to rguent...@suse.de from comment #100)
> On Tue, 4 Apr 2017, bernd.edlinger at hotmail dot de wrote:
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
> > 
> > --- Comment #99 from Bernd Edlinger  ---
> > (In reply to rguent...@suse.de from comment #95)
> > > >
> > > >this would fix the remaining fall-out.
> > > 
> > > Because that is not how it was designed or documented to work :)
> > 
> > So yes, it seems I misunderstood what may_alias should do.
> > 
> > It is defined on the type, however it has only meaning on the
> > pointers to that type, but not on the instances.
> > 
> > But I wanted to have an attribute to express in the tree
> > that instances and pointers to that type may alias anything.
> > 
> > How about adding a new type attribute, that does what I meant,
> > like always_alias for instance?
> 
> That's what I proposed the C++ FE to do with a new tree type flag.  That
> could be exposed to users with an attribute as well, of course.

good.

I have all the bits together, I just have to rename the flag.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #100 from rguenther at suse dot de  ---
On Tue, 4 Apr 2017, bernd.edlinger at hotmail dot de wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
> 
> --- Comment #99 from Bernd Edlinger  ---
> (In reply to rguent...@suse.de from comment #95)
> > >
> > >this would fix the remaining fall-out.
> > 
> > Because that is not how it was designed or documented to work :)
> 
> So yes, it seems I misunderstood what may_alias should do.
> 
> It is defined on the type, however it has only meaning on the
> pointers to that type, but not on the instances.
> 
> But I wanted to have an attribute to express in the tree
> that instances and pointers to that type may alias anything.
> 
> How about adding a new type attribute, that does what I meant,
> like always_alias for instance?

That's what I proposed the C++ FE to do with a new tree type flag.  That
could be exposed to users with an attribute as well, of course.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #99 from Bernd Edlinger  ---
(In reply to rguent...@suse.de from comment #95)
> >
> >this would fix the remaining fall-out.
> 
> Because that is not how it was designed or documented to work :)

So yes, it seems I misunderstood what may_alias should do.

It is defined on the type, however it has only meaning on the
pointers to that type, but not on the instances.

But I wanted to have an attribute to express in the tree
that instances and pointers to that type may alias anything.

How about adding a new type attribute, that does what I meant,
like always_alias for instance?

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #98 from Bernd Edlinger  ---
I realized that my latest patch triggers a pre-existing
bug in the libstdc++ testsuite => PR80287

So the following patch would be needed on top of it:

Index: gcc/cp/class.c
===
--- gcc/cp/class.c  (revision 246605)
+++ gcc/cp/class.c  (working copy)
@@ -2060,12 +2060,14 @@
 static void
 fixup_may_alias (tree klass)
 {
-  tree t;
+  tree t, v;

   for (t = TYPE_POINTER_TO (klass); t; t = TYPE_NEXT_PTR_TO (t))
-TYPE_REF_CAN_ALIAS_ALL (t) = true;
+for (v = TYPE_MAIN_VARIANT (t); v; v = TYPE_NEXT_VARIANT (v))
+  TYPE_REF_CAN_ALIAS_ALL (v) = true;
   for (t = TYPE_REFERENCE_TO (klass); t; t = TYPE_NEXT_REF_TO (t))
-TYPE_REF_CAN_ALIAS_ALL (t) = true;
+for (v = TYPE_MAIN_VARIANT (t); v; v = TYPE_NEXT_VARIANT (v))
+  TYPE_REF_CAN_ALIAS_ALL (v) = true;
 }

 /* Early variant fixups: we apply attributes at the beginning of the class

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #97 from Bernd Edlinger  ---
I thought that at that point t is the type of to the outer reference.

thus in this example:
cat t1.c
#include 

union xx {
  __m64 m;
  long long l;
};

union xx t;

__m64
test(long long x)
{
 t.l=x;
 return t.m;
}

gcc -S t1.c -fdump-rtl-final

I see this in t1.c.309r.final

(insn 7 6 8 2 (set (mem/j/c:DI (symbol_ref:DI ("t") [flags 0x2]  ) [1 t.l+0 S8 A64])
(reg:DI 0 ax [89])) "t1.c":13 81 {*movdi_internal}
 (nil))
(insn 8 7 11 2 (set (reg:V2SI 0 ax [orig:87 _4 ] [87])
(mem/j/c:V2SI (symbol_ref:DI ("t") [flags 0x2]  ) [1 t.m+0 S8 A64])) "t1.c":14 1100 {*movv2si_internal}
 (nil))

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #96 from Jakub Jelinek  ---
As Richard mentioned on IRC, e.g. the x86intrin.h __m[125]* types have
may_alias attributes and such a change would slow down code that puts those
into structures/unions.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #95 from rguenther at suse dot de  ---
On April 2, 2017 10:54:28 AM GMT+02:00, "bernd.edlinger at hotmail dot de"
 wrote:
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
>
>--- Comment #94 from Bernd Edlinger 
>---
>I always wondered why get_alias_set does not use
>the may_alias attribute like this:
>
>
>Index: alias.c
>===
>--- alias.c (revision 246605)
>+++ alias.c (working copy)
>@@ -928,6 +928,9 @@ get_alias_set (tree t)
>   return 0;
> }
>
>+  if (lookup_attribute ("may_alias", TYPE_ATTRIBUTES (t)))
>+return 0;
>+
>   /* See if the language has special handling for this type.  */
>   set = lang_hooks.get_alias_set (t);
>   if (set != -1)
>
>this would fix the remaining fall-out.

Because that is not how it was designed or documented to work :)

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #94 from Bernd Edlinger  ---
I always wondered why get_alias_set does not use
the may_alias attribute like this:


Index: alias.c
===
--- alias.c (revision 246605)
+++ alias.c (working copy)
@@ -928,6 +928,9 @@ get_alias_set (tree t)
   return 0;
 }

+  if (lookup_attribute ("may_alias", TYPE_ATTRIBUTES (t)))
+return 0;
+
   /* See if the language has special handling for this type.  */
   set = lang_hooks.get_alias_set (t);
   if (set != -1)

this would fix the remaining fall-out.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #93 from Bernd Edlinger  ---
I tried a bit more with lto
and discovered that -flto -O2 removes all the
code from test() although it is marked noinline, noclone

To work around that, I added an asm statement
initializing all these mem-refs, now the test case
looks like that:

cat t.cc
#include 
//namespace std { enum class byte: unsigned char {}; };
struct t  {
  int x;
  //std::byte kk[1];
  unsigned char k;
} __attribute__((may_alias));

t t1;
t t2;
t *t3;
t *t4;

void __attribute__((noinline, noclone))
test()
{
  t1 = t2;
  *t4 = *t3;
}

int main()
{
  asm("":"=m"(t1), "=m"(t2), "=m"(t3), "=m"(t4));
  test();
}

This prevents -flto -O2 from removing the mem-refs.
But I discovered that the asm output arguments
don't use the alias-set zero:

g++ -flto -O2 -fdump-rtl-final t.cc

has this rtl from the asm stmt:

(insn:TI 5 2 6 2 (parallel [
(set (mem/c:DI (symbol_ref:DI ("t1") [flags 0x2]  ) [2 t1+0 S8 A64])
(asm_operands:DI ("") ("=m") 0 []
 []
 [] t.cc:23))
(set (mem/c:DI (symbol_ref:DI ("t2") [flags 0x2]  ) [2 t2+0 S8 A64])
(asm_operands:DI ("") ("=m") 1 []
 []
 [] t.cc:23))
(set (mem/f/c:DI (symbol_ref:DI ("t3") [flags 0x2]  ) [1 t3+0 S8 A64])
(asm_operands:DI ("") ("=m") 2 []
 []
 [] t.cc:23))
(set (mem/f/c:DI (symbol_ref:DI ("t4") [flags 0x2]  ) [1 t4+0 S8 A64])
(asm_operands:DI ("") ("=m") 3 []
 []
 [] t.cc:23))
(clobber (reg:CCFP 18 fpsr))
(clobber (reg:CC 17 flags))
]) "t.cc":23 -1
 (expr_list:REG_UNUSED (reg:CCFP 18 fpsr)
(expr_list:REG_UNUSED (reg:CC 17 flags)
(nil


Again alias set 2 for t1, and t2.
This is identical also for non-lto build.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #92 from Bernd Edlinger  ---
It is an interesting fact that

g++ -flto -fdump-rtl-final t.cc

has correct rtl:
(insn 7 6 8 2 (set (mem/c:DI (symbol_ref:DI ("t1") [flags 0x2]  ) [0 t1+0 S8 A64])
(reg:DI 0 ax [90])) "t.cc":17 81 {*movdi_internal}
 (nil))

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #91 from Bernd Edlinger  ---
I mean here is [2 t1+0 S8 A64] and 2 is the alias set info in
the MEM_REF, I have expected that to be 0.


(insn:TI 7 6 14 2 (set (mem/c:DI (symbol_ref:DI ("t1") [flags 0x2]  ) [2 t1+0 S8 A64])
(reg:DI 0 ax [orig:90 MEM[(const struct t & {ref-all})] ] [90]))
"t.cc":17 81 {*movdi_internal}
 (expr_list:REG_DEAD (reg:DI 0 ax [orig:90 MEM[(const struct t &
{ref-all})] ] [90])
(nil)))

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #90 from rguenther at suse dot de  ---
On April 2, 2017 2:09:07 AM GMT+02:00, "bernd.edlinger at hotmail dot de"
 wrote:
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
>
>Bernd Edlinger  changed:
>
>   What|Removed |Added
>
>  Attachment #41101|0   |1
>is obsolete||
>
>--- Comment #89 from Bernd Edlinger 
>---
>Created attachment 41103
>  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=41103=edit
>updated patch
>
>should work in principle with LTO,
>but there is still another bug
>somewhere which makes __attribute__((may_alias))
>and the std::byte handling fail in the end.
>
>consider this test example:
>
>cat t.cc
>#include 
>//namespace std { enum class byte: unsigned char {}; };
>struct t  {
>  int x;
>  //std::byte kk[1];
>  unsigned char k;
>} __attribute__((may_alias));
>
>t t1;
>t t2;
>t *t3;
>t *t4;
>
>void __attribute__((noinline, noclone))
>test()
>{
>  t1 = t2;

Can't spot the issue in the RTL you quote but may_alias does not apply to this
assignment.

>  *t4 = *t3;
>}
>
>int main()
>{
>  test();
>}
>
>g++ -std=c++17 -O2 -fdump-rtl-final t.cc
>
>t.cc.309r.final:
>(insn:TI 6 2 7 2 (set (reg:DI 0 ax [orig:90 MEM[(const struct t &
>{ref-all})] ] [90])
>  (mem/c:DI (symbol_ref:DI ("t2") [flags 0x2]  t2>) [0 MEM[(const struct t & {ref-all})]+0 S8 A64])) "t.cc":17 81
>{*movdi_internal}
> (expr_list:REG_EQUIV (mem/c:DI (symbol_ref:DI ("t2") [flags 0x2] 
>) [0 MEM[(const struct t & {ref-all})]+0
>S8
>A64])
>(nil)))
>(insn:TI 7 6 14 2 (set (mem/c:DI (symbol_ref:DI ("t1") [flags 0x2] 
>0x7f973b730240 t1>) [2 t1+0 S8 A64])
>(reg:DI 0 ax [orig:90 MEM[(const struct t & {ref-all})] ] [90]))
>"t.cc":17 81 {*movdi_internal}
> (expr_list:REG_DEAD (reg:DI 0 ax [orig:90 MEM[(const struct t &
>{ref-all})] ] [90])
>(nil)))
>(insn 14 7 10 2 (set (reg/f:DI 0 ax [orig:87 t3.1_1 ] [87])
>(mem/f/c:DI (symbol_ref:DI ("t3") [flags 0x2]  t3>) [1 t3+0 S8 A64])) "t.cc":18 81 {*movdi_internal}
>(expr_list:REG_EQUIV (mem/f/c:DI (symbol_ref:DI ("t3") [flags 0x2] 
>) [1 t3+0 S8 A64])
>(nil)))
>(insn:TI 10 14 15 2 (set (reg:DI 1 dx [orig:91 MEM[(const struct t &
>{ref-all})t3.1_1] ] [91])
>(mem:DI (reg/f:DI 0 ax [orig:87 t3.1_1 ] [87]) [0 MEM[(const struct t &
>{ref-all})t3.1_1]+0 S8 A32])) "t.cc":18 81 {*movdi_internal}
> (expr_list:REG_DEAD (reg/f:DI 0 ax [orig:87 t3.1_1 ] [87])
>(nil)))
>(insn 15 10 11 2 (set (reg/f:DI 0 ax [orig:88 t4.2_2 ] [88])
>(mem/f/c:DI (symbol_ref:DI ("t4") [flags 0x2]  t4>) [1 t4+0 S8 A64])) "t.cc":18 81 {*movdi_internal}
>(expr_list:REG_EQUIV (mem/f/c:DI (symbol_ref:DI ("t4") [flags 0x2] 
>) [1 t4+0 S8 A64])
>(nil)))
>(insn:TI 11 15 23 2 (set (mem:DI (reg/f:DI 0 ax [orig:88 t4.2_2 ] [88])
>[0
>*t4.2_2+0 S8 A32])
> (reg:DI 1 dx [orig:91 MEM[(const struct t & {ref-all})t3.1_1] ] [91]))
>"t.cc":18 81 {*movdi_internal}
> (expr_list:REG_DEAD (reg:DI 1 dx [orig:91 MEM[(const struct t &
>{ref-all})t3.1_1] ] [91])
>(expr_list:REG_DEAD (reg/f:DI 0 ax [orig:88 t4.2_2 ] [88])
>(nil
>
>
>
>I think the access to t1 uses alias set 2, and that is wrong.
>The other accesses are ok.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

Bernd Edlinger  changed:

   What|Removed |Added

  Attachment #41101|0   |1
is obsolete||

--- Comment #89 from Bernd Edlinger  ---
Created attachment 41103
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=41103=edit
updated patch

should work in principle with LTO,
but there is still another bug
somewhere which makes __attribute__((may_alias))
and the std::byte handling fail in the end.

consider this test example:

cat t.cc
#include 
//namespace std { enum class byte: unsigned char {}; };
struct t  {
  int x;
  //std::byte kk[1];
  unsigned char k;
} __attribute__((may_alias));

t t1;
t t2;
t *t3;
t *t4;

void __attribute__((noinline, noclone))
test()
{
  t1 = t2;
  *t4 = *t3;
}

int main()
{
  test();
}

g++ -std=c++17 -O2 -fdump-rtl-final t.cc

t.cc.309r.final:
(insn:TI 6 2 7 2 (set (reg:DI 0 ax [orig:90 MEM[(const struct t &
{ref-all})] ] [90])
(mem/c:DI (symbol_ref:DI ("t2") [flags 0x2]  ) [0 MEM[(const struct t & {ref-all})]+0 S8 A64])) "t.cc":17 81
{*movdi_internal}
 (expr_list:REG_EQUIV (mem/c:DI (symbol_ref:DI ("t2") [flags 0x2] 
) [0 MEM[(const struct t & {ref-all})]+0 S8
A64])
(nil)))
(insn:TI 7 6 14 2 (set (mem/c:DI (symbol_ref:DI ("t1") [flags 0x2]  ) [2 t1+0 S8 A64])
(reg:DI 0 ax [orig:90 MEM[(const struct t & {ref-all})] ] [90]))
"t.cc":17 81 {*movdi_internal}
 (expr_list:REG_DEAD (reg:DI 0 ax [orig:90 MEM[(const struct t &
{ref-all})] ] [90])
(nil)))
(insn 14 7 10 2 (set (reg/f:DI 0 ax [orig:87 t3.1_1 ] [87])
(mem/f/c:DI (symbol_ref:DI ("t3") [flags 0x2]  ) [1 t3+0 S8 A64])) "t.cc":18 81 {*movdi_internal}
 (expr_list:REG_EQUIV (mem/f/c:DI (symbol_ref:DI ("t3") [flags 0x2] 
) [1 t3+0 S8 A64])
(nil)))
(insn:TI 10 14 15 2 (set (reg:DI 1 dx [orig:91 MEM[(const struct t &
{ref-all})t3.1_1] ] [91])
(mem:DI (reg/f:DI 0 ax [orig:87 t3.1_1 ] [87]) [0 MEM[(const struct t &
{ref-all})t3.1_1]+0 S8 A32])) "t.cc":18 81 {*movdi_internal}
 (expr_list:REG_DEAD (reg/f:DI 0 ax [orig:87 t3.1_1 ] [87])
(nil)))
(insn 15 10 11 2 (set (reg/f:DI 0 ax [orig:88 t4.2_2 ] [88])
(mem/f/c:DI (symbol_ref:DI ("t4") [flags 0x2]  ) [1 t4+0 S8 A64])) "t.cc":18 81 {*movdi_internal}
 (expr_list:REG_EQUIV (mem/f/c:DI (symbol_ref:DI ("t4") [flags 0x2] 
) [1 t4+0 S8 A64])
(nil)))
(insn:TI 11 15 23 2 (set (mem:DI (reg/f:DI 0 ax [orig:88 t4.2_2 ] [88]) [0
*t4.2_2+0 S8 A32])
(reg:DI 1 dx [orig:91 MEM[(const struct t & {ref-all})t3.1_1] ] [91]))
"t.cc":18 81 {*movdi_internal}
 (expr_list:REG_DEAD (reg:DI 1 dx [orig:91 MEM[(const struct t &
{ref-all})t3.1_1] ] [91])
(expr_list:REG_DEAD (reg/f:DI 0 ax [orig:88 t4.2_2 ] [88])
(nil



I think the access to t1 uses alias set 2, and that is wrong.
The other accesses are ok.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #88 from Bernd Edlinger  ---
(In reply to rguent...@suse.de from comment #87)
> On April 1, 2017 4:40:19 PM GMT+02:00, "bernd.edlinger at hotmail dot de"
>  wrote:
> >https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
> >
> >--- Comment #86 from Bernd Edlinger 
> >---
> >Created attachment 41101 [details]
> >  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=41101=edit
> >another trial patch
> >
> >This is another approach for a patch that reflects what I understood
> >so far what the C++17 spec wants from us.
> >I think it should resolve the issue Richard raised.
> 
> That doesn't properly work with LTO

oops...
I think this does not work either,
at cp/decl.c (start_enum):

  /* std::byte aliases anything.  */
  if (enumtype != error_mark_node
  && TYPE_CONTEXT (enumtype) == std_node
  && !strcmp ("byte", TYPE_NAME_STRING (enumtype)))
TYPE_ALIAS_SET (enumtype) = 0;

I looked at -flto -fdump-rtl-final
and see the alias set is 1:

#include 
//namespace std { enum class byte: unsigned char {}; };
union  t  {
  int x;
  std::byte k[1];
};
t t1;
t t2;
void test()
{
  t1.k[0] = t2.k[0];
}
int main()
{
  test();
}

g++ -std=c++17 -flto -fdump-rtl-final t.cc

/tmp/cc*.final:
(insn 5 2 6 2 (set (reg:QI 0 ax [orig:87 _1 ] [87])
(mem/j/c:QI (symbol_ref:DI ("t2") [flags 0x2]  ) [1 t2.k+0 S1 A32])) "t.cc":11 84 {*movqi_internal}
 (nil))
(insn 6 5 13 2 (set (mem/j/c:QI (symbol_ref:DI ("t1") [flags 0x2]  ) [1 t1.k+0 S1 A32])
(reg:QI 0 ax [orig:87 _1 ] [87])) "t.cc":11 84 {*movqi_internal}
 (nil))


[1 = alias set 1 but should be 0

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #87 from rguenther at suse dot de  ---
On April 1, 2017 4:40:19 PM GMT+02:00, "bernd.edlinger at hotmail dot de"
 wrote:
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
>
>--- Comment #86 from Bernd Edlinger 
>---
>Created attachment 41101
>  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=41101=edit
>another trial patch
>
>This is another approach for a patch that reflects what I understood
>so far what the C++17 spec wants from us.
>I think it should resolve the issue Richard raised.

That doesn't properly work with LTO

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #86 from Bernd Edlinger  ---
Created attachment 41101
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=41101=edit
another trial patch

This is another approach for a patch that reflects what I understood
so far what the C++17 spec wants from us.
I think it should resolve the issue Richard raised.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #85 from Jason Merrill  ---
(In reply to Bernd Edlinger from comment #84)
> all valid examples used a union with an array of unsigned char.  Right?

There doesn't need to be a union, just an array of unsigned char.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #84 from Bernd Edlinger  ---
I think the patch should also handle
arrays of unions like:

union BB {
  unsigned char x[10];
};
struct B {
  BB x[16];
};
struct A {
  B b;
};

A a1, a2;
void test()
{
  a1.b = a2.b;
}

compiles to:
; Function void test() (_Z4testv, funcdef_no=0, decl_uid=2384, cgraph_uid=0,
symbol_order=2)

void test() ()
{
   [0.00%]:
  a1.b = a2.b;
  return;

}


while in this example
struct B {
  unsigned char x[16];
};
struct A {
  B b;
};

A a1, a2;
void test()
{
  a1.b = a2.b;
}

compiles to:
;; Function void test() (_Z4testv, funcdef_no=0, decl_uid=2348, cgraph_uid=0,
symbol_order=2)

void test() ()
{
   [0.00%]:
  MEM[(void *)] = MEM[(void *)];
  return;

}

which is probably unnecessary, because all valid examples
used a union with an array of unsigned char.  Right?

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #83 from Jason Merrill  ---
(In reply to Jonathan Wakely from comment #81)
> Boost 1.63 has an array of char.

Seems they should change that to unsigned char (or std::byte).

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #82 from Richard Biener  ---
Btw, for the attached patch it would fail to handle the case of passing the
aggregate by value and the inliner introducing the aggregate copy:

inline void* operator new(__SIZE_TYPE__, void* __p) { return __p; }

struct X { union { int i; char s[4]; } u; };

static float foo (X y)
{
  return reinterpret_cast (y.u.s);
}

int main ()
{
  X x;
  new () float (1.0);
  if (foo (x) != 1.0)
__builtin_abort ();
}

generates at -O2 after inlining:

int main() ()
{
  void * D.2385;
  float D.2383;
  struct X y;
  struct X x;
  float _7;
  bool retval.0_8;

   [100.00%]:
  MEM[(float *)] = 1.0e+0;
  y = x;
  _7 = MEM[(float &)];
...

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #81 from Jonathan Wakely  ---
(In reply to Bernd Edlinger from comment #80)
> but they use just: "mutable char data;"

No, actually it doesn't, see comment 45. The original testcase that this reprot
came from is https://bugzilla.redhat.com/show_bug.cgi?id=1422456 which uses
Boost 1.63 and that has an array of char. Jakub used the wrong version of Boost
to create the code attached here.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #80 from Bernd Edlinger  ---
but they use just: "mutable char data;"

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #79 from Jonathan Wakely  ---
That's a new change for C++17, and I don't think today's GCC will treat it any
differently if it uses unsigned char[] instead of char[].

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #78 from Bernd Edlinger  ---
Ah, adopted is the opposite of rejected, fine.

Does this mean that the boost code is still incorrect,
because it is not using an array of unsigned char ?

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #77 from Jonathan Wakely  ---
It was rejected in 2010, but see
http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1116 for the later
discussion, and:

"[Adopted at the June, 2016 meeting as document P0137R1.]"

Which Jason already pointed to in Comment 72.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #76 from Bernd Edlinger  ---
FYI this is also on the same topic:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3296.html#US27
Question:
"Related to core issue 1027, consider:

int f() {
union U { double d; } u1, u2;
(int&)u1.d = 1;
u2 = u1;
return (int&)u2.d;
}

Does this involve undefined behavior? 3.8/4 seems to say that it's OK to
clobber u1 with an int object. Then union assignment copies the object
representation, possibly creating an int object in u2 and making the return
statement well-defined. If this is well-defined, compilers are significantly
limited in the assumptions they can make about type aliasing. On the other
hand, the variant where U has an array of unsigned char member must be
well-defined in order to support std::aligned_storage."

Proposed Resolution:
"Clarify that this testcase is undefined, but that adding an array of unsigned
char to union U would make it well-defined--if a storage location is allocated
with a particular type, it should be undefined to create an object in that
storage if it would be undefined to access the stored value of the object
through the allocated type."

Disposition:
"REJECTED

Resolving this question was not deemed essential for this revision of the
Standard, but core issues 1116 remains open for possible consideration in a
future revision."

Does this mean that the boost code is still invalid because there
is no array of unsigned char ?

Or does it not count because it was REJECTED ?

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #75 from rguenther at suse dot de  ---
On Tue, 28 Mar 2017, bernd.edlinger at hotmail dot de wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
> 
> --- Comment #74 from Bernd Edlinger  ---
> (In reply to rguent...@suse.de from comment #73)
> > 
> > Ah, I remember seeing this.  So this introduces object size as a way
> > to allow some TBAA to happen.  Specifically it forbids creating
> > a series of float sub-objects inside a char[sizeof(float)*4] sub-object
> > due to "there is no smaller array object that satisfies these 
> > constraints"(?).  One would need to use sth like
> > 
> >   struct { unsigned char storage[sizeof(float)]; } st[4];
> > 
> 
> I think that is meant differently, but I am not sure if I
> understand this kind of english good enough..., the sample uses:
> 
> template
> struct AlignedUnion {
>   alignas(T...) unsigned char data[max(sizeof(T)...)];
> };
> 
> so that data array is max of sizeof(int) and sizeof(char)
> 
> ... the more they write the less clear it becomes :)

I was thinking about how to arrange for _multiple_ objects to
be constructed inside a memory area, the interesting size clause
seems to require workarounds like above.  Otherwise the "array element"
choice doesn't make much sense I guess.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #74 from Bernd Edlinger  ---
(In reply to rguent...@suse.de from comment #73)
> 
> Ah, I remember seeing this.  So this introduces object size as a way
> to allow some TBAA to happen.  Specifically it forbids creating
> a series of float sub-objects inside a char[sizeof(float)*4] sub-object
> due to "there is no smaller array object that satisfies these 
> constraints"(?).  One would need to use sth like
> 
>   struct { unsigned char storage[sizeof(float)]; } st[4];
> 

I think that is meant differently, but I am not sure if I
understand this kind of english good enough..., the sample uses:

template
struct AlignedUnion {
  alignas(T...) unsigned char data[max(sizeof(T)...)];
};

so that data array is max of sizeof(int) and sizeof(char)

... the more they write the less clear it becomes :)


> to work around that "limitation".  Not if it's really possible to
> take advantage of that size restriction in alias analysis though...
> 
> If we'd want to implement sth like this in the middle-end I'd suggest
> to force frontends to mark these storage areas in some way.
> In the C++ case all unsigned char arrays -- does the "array of N unsigned
> char"
> wording allow for struct C {} to be created within a unsigned char
> (no array!) member or has it to be unsigned char[1]?).
> 
> I suggest sth like
> 
> /* For an ARRAY_TYPE, indicates that when the dynamic type of the storage
>it provides differs from the declard type it is still valid to access
>it via the declared type or the type of a containing object.  */
> #define ARRAY_TYPE_IS_STORAGE (NODE) \
>   (ARRAY_TYPE_CHECK (NODE)->type_common.array_type_is_storage)

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #73 from rguenther at suse dot de  ---
On Mon, 27 Mar 2017, jason at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
> 
> --- Comment #72 from Jason Merrill  ---
> (In reply to rguent...@suse.de from comment #69)
> > As I noted elsewhere union members in C++ seem to be pure convenience and a
> > union contains implicit members of all types (well, somehow factor in
> > alignment).  Of course Jason argues char[] is special and introduces this
> > but I can't find text anywhere to support that or require char[] and not,
> > say int[].
> 
> This is clarified somewhat in C++17, by
> 
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0137r1.html
> 
> Note that this wording makes only unsigned char[] special, not signed or plain
> char[].

Ah, I remember seeing this.  So this introduces object size as a way
to allow some TBAA to happen.  Specifically it forbids creating
a series of float sub-objects inside a char[sizeof(float)*4] sub-object
due to "there is no smaller array object that satisfies these 
constraints"(?).  One would need to use sth like

  struct { unsigned char storage[sizeof(float)]; } st[4];

to work around that "limitation".  Not if it's really possible to
take advantage of that size restriction in alias analysis though...

If we'd want to implement sth like this in the middle-end I'd suggest
to force frontends to mark these storage areas in some way.
In the C++ case all unsigned char arrays -- does the "array of N unsigned char"
wording allow for struct C {} to be created within a unsigned char
(no array!) member or has it to be unsigned char[1]?).

I suggest sth like

/* For an ARRAY_TYPE, indicates that when the dynamic type of the storage
   it provides differs from the declard type it is still valid to access
   it via the declared type or the type of a containing object.  */
#define ARRAY_TYPE_IS_STORAGE (NODE) \
  (ARRAY_TYPE_CHECK (NODE)->type_common.array_type_is_storage)

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #72 from Jason Merrill  ---
(In reply to rguent...@suse.de from comment #69)
> As I noted elsewhere union members in C++ seem to be pure convenience and a
> union contains implicit members of all types (well, somehow factor in
> alignment).  Of course Jason argues char[] is special and introduces this
> but I can't find text anywhere to support that or require char[] and not,
> say int[].

This is clarified somewhat in C++17, by

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0137r1.html

Note that this wording makes only unsigned char[] special, not signed or plain
char[].

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #71 from Jonathan Wakely  ---
(In reply to Michael Matz from comment #68)
> (In reply to Jonathan Wakely from comment #65)
> > It accesses b, but it doesn't access the object stored in b's char[N] member
> > via placement new.
> 
> Okay, let's go with this.  So the copying of the union is then defined
> (as a memcpy equivalent).  Then there's still the question if the following
> sequences are valid:
> 
> // assume T1 and T2 are some types and new is trivial placement new
> union U {T1 a; char b[sizeof T2];} x,y;
> new (x.b) T2();  // 1
> y = x;   // 2
> T2 t;
> memcpy(, y.b, sizeof T2);  // 3
> t;   // 4
> y.a; // 5
> 
> We have said that (2) is valid, obviously (3) in isolation is valid as well,

N.B. (3) is not valid if T2 isn't trivially copyable. But for the cases we care
about, it is (the code takes a completely different path otherwise).

> but it influences the validity of (4).  (5) is invalid as it's not the
> active member of the union y (which is instead b).
> 
> (4) is valid if y.b contained a T2, which is only the case if (2) transferred
> the dynamic type from x.b _and_ (1) was valid to start with and dynamically
> typed x.b to be of type T2.
> 
> So, it all boils down to if (1) is valid and types x.b to T2, even though it
> has a different declared type.

I don't think it changes the type of x.b, but it does begin the lifetime of an
object of type T2 at that location. 

As I see it, the question is whether copying the object representation of that
object to another location means we have another object at the second location.
We all seem to agree that copying the object representation using memcpy *does*
do that. We don't agree whether copying the object representation via an
implicit union copy does that.

I don't see a distinction between copying the object representation via memcpy
or the union copy.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[matz at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #70 from Michael Matz  ---
(In reply to rguent...@suse.de from comment #69)
> As I noted elsewhere union members in C++ seem to be pure convenience and a
> union contains implicit members of all types (well, somehow factor in
> alignment).

Well, the whole introduction of "object representation" is clearly a very bad
idea if you want to avoid the above, but let's assume that the above is really
not intended.  So let's wait for answers to the questions and not see darkness
wherever we go :)

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #69 from rguenther at suse dot de  ---
On March 27, 2017 8:11:10 PM GMT+02:00, "matz at gcc dot gnu.org"
 wrote:
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
>
>--- Comment #68 from Michael Matz  ---
>(In reply to Jonathan Wakely from comment #65)
>> It accesses b, but it doesn't access the object stored in b's char[N]
>member
>> via placement new.
>
>Okay, let's go with this.  So the copying of the union is then defined
>(as a memcpy equivalent).  Then there's still the question if the
>following
>sequences are valid:
>
>// assume T1 and T2 are some types and new is trivial placement new
>union U {T1 a; char b[sizeof T2];} x,y;
>new (x.b) T2();  // 1
>y = x;   // 2
>T2 t;
>memcpy(, y.b, sizeof T2);  // 3
>t;   // 4
>y.a; // 5
>
>We have said that (2) is valid, obviously (3) in isolation is valid as
>well,
>but it influences the validity of (4).  (5) is invalid as it's not the
>active member of the union y (which is instead b).
>
>(4) is valid if y.b contained a T2, which is only the case if (2)
>transferred
>the dynamic type from x.b _and_ (1) was valid to start with and
>dynamically
>typed x.b to be of type T2.
>
>So, it all boils down to if (1) is valid and types x.b to T2, even
>though it
>has a different declared type.  For C we say it's not, because char[]
>is
>asymmetric: you can access all types via a char*, but you can't change
>the
>dynamic type of a declared char array to contain arbitrary other things
>(well,
>the ME memory model does cater for this and makes it valid, even though
>it's
>invalid in C).
>
>I guess you're arguing that (1) is valid in C++ and that then due to
>3.9/3
>and 12.8/29 also (2) and (4) are.  I guess it can be defined to be so,
>but
>I wonder what the type of 'x' is after (1)?  It can't be T2, because
>clearly
>x.b is valid even if T2 doesn't contain a member 'b'.  So it must stay
>a union,
>but in order to transfer the type T2 in (2) it must also contain T2, so
>is it
>the type 'union {T1 a; char b[sizeof T2]; T2 ;}' then
>(conceptually)?
>Messy :)

As I noted elsewhere union members in C++ seem to be pure convenience and a
union contains implicit members of all types (well, somehow factor in
alignment).  Of course Jason argues char[] is special and introduces this but I
can't find text anywhere to support that or require char[] and not, say int[].

Richard.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[matz at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #68 from Michael Matz  ---
(In reply to Jonathan Wakely from comment #65)
> It accesses b, but it doesn't access the object stored in b's char[N] member
> via placement new.

Okay, let's go with this.  So the copying of the union is then defined
(as a memcpy equivalent).  Then there's still the question if the following
sequences are valid:

// assume T1 and T2 are some types and new is trivial placement new
union U {T1 a; char b[sizeof T2];} x,y;
new (x.b) T2();  // 1
y = x;   // 2
T2 t;
memcpy(, y.b, sizeof T2);  // 3
t;   // 4
y.a; // 5

We have said that (2) is valid, obviously (3) in isolation is valid as well,
but it influences the validity of (4).  (5) is invalid as it's not the
active member of the union y (which is instead b).

(4) is valid if y.b contained a T2, which is only the case if (2) transferred
the dynamic type from x.b _and_ (1) was valid to start with and dynamically
typed x.b to be of type T2.

So, it all boils down to if (1) is valid and types x.b to T2, even though it
has a different declared type.  For C we say it's not, because char[] is
asymmetric: you can access all types via a char*, but you can't change the
dynamic type of a declared char array to contain arbitrary other things (well,
the ME memory model does cater for this and makes it valid, even though it's
invalid in C).

I guess you're arguing that (1) is valid in C++ and that then due to 3.9/3
and 12.8/29 also (2) and (4) are.  I guess it can be defined to be so, but
I wonder what the type of 'x' is after (1)?  It can't be T2, because clearly
x.b is valid even if T2 doesn't contain a member 'b'.  So it must stay a union,
but in order to transfer the type T2 in (2) it must also contain T2, so is it
the type 'union {T1 a; char b[sizeof T2]; T2 ;}' then (conceptually)?
Messy :)

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[matz at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #67 from Michael Matz  ---
(In reply to Jason Merrill from comment #66)
> The operator semantics described in clause 5 [expr] apply to the built-in
> operators, not any overloaded operators.  Assignment of classes is always
> done by way of an assignment operator function, even if it happens to be
> trivial and therefore open-coded as a block copy, so the above doesn't apply
> to classes.

I think that's at least slippery.  If none of 5.18 would apply to class types
at all there would be no need for 5.18/3 (if non-class type) 5.18/4 (if
class-type) 5.18/5 (class objects are special), or 5.18/9.2 (braced-inits for
class objects).  So if /2 would have been conditional on "non-class type" as
well, like /3, it wouldn't matter, but so ...

(This is of course only a side-track, I used clause 5 merely because like you
I have difficulties to find a real definition of what constitutes an access
to the value of an object :) )

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #66 from Jason Merrill  ---
(In reply to Michael Matz from comment #64)
> I would find it extremely surprising if in
> 
>   a = b;
> 
> the RHS doesn't constitute an access to the value of object 'b' (even
> depending on the type of b).  Are you really saying this Jason? (just trying
> to make extra sure)

Well, there's a kind of access involved in forming a reference to each of the
members of b.  I'm having trouble finding text pertaining to this; the closest
I'm coming up with is in 12.7 [class.cdtor]:

To form a pointer to (or access the value of) a direct non-static member of an
object obj, the construction of obj shall have started and its destruction
shall not have completed, otherwise the computation of the pointer value (or
accessing
the member value) results in undefined behavior.

> (e.g. 5.17/2 is saying about the assignment operator, before any
> differentiation between class and non-class types:
>   "In simple assignment (=), the value of the expression replaces that of the
>object referred to by the left operand."
> How could it talk about the value of the expression if the RHS doesn't
> constitute an access to the value of that expression?  While /4 specifies that
> the actual assignment is carried out by the copy/move assignment operator
> and hence via object representation for unions when implicit (12.8/29), we
> cannot simply ignore the above sentence, can we?)

The operator semantics described in clause 5 [expr] apply to the built-in
operators, not any overloaded operators.  Assignment of classes is always done
by way of an assignment operator function, even if it happens to be trivial and
therefore open-coded as a block copy, so the above doesn't apply to classes.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #65 from Jonathan Wakely  ---
(In reply to Michael Matz from comment #64)
> I would find it extremely surprising if in
> 
>   a = b;
> 
> the RHS doesn't constitute an access to the value of object 'b' (even
> depending on the type of b).  Are you really saying this Jason? (just trying
> to make extra
> sure)

It accesses b, but it doesn't access the object stored in b's char[N] member
via placement new.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[matz at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

Michael Matz  changed:

   What|Removed |Added

 CC||matz at gcc dot gnu.org

--- Comment #64 from Michael Matz  ---
I would find it extremely surprising if in

  a = b;

the RHS doesn't constitute an access to the value of object 'b' (even depending
on the type of b).  Are you really saying this Jason? (just trying to make
extra
sure)

(e.g. 5.17/2 is saying about the assignment operator, before any
differentiation
between class and non-class types:
  "In simple assignment (=), the value of the expression replaces that of the
   object referred to by the left operand."
How could it talk about the value of the expression if the RHS doesn't
constitute an access to the value of that expression?  While /4 specifies that
the actual assignment is carried out by the copy/move assignment operator and
hence via object representation for unions when implicit (12.8/29), we cannot
simply ignore the above sentence, can we?)

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #63 from Jason Merrill  ---
(In reply to rguent...@suse.de from comment #54)
> > > > --- Comment #51 from Bernd Edlinger  
> > > > ---
> > > > Doesn't 3.10/10 explicitly say that it is undefined to use a union to
> > > > to move an object representation that is not a member of the union?

> But still the other clause says the storage representation is transfered
> and so you could read into that that no "access" happens and thus
> 3.10/10 doesn't apply.

Right, union copy copies the bytes of the object representation, i.e. memcpy.

(In reply to Richard Biener from comment #50)
> Note that what changed with GCC 7 is only that unions with char members
> no longer behave as alias-set zero but 12.8/16 talks about all unions,
> not just unions with char members.
> 
> Now I read comment#14 as that _only_ char[] members (of structs or unions)
> may ever "contain" different dynamic types.  Any pointer to a part of
> the standard that singles out char[] that way?

char is special that way because it can be used to access the stored value of
an object of any type (3.10/10.8).

Now that C++17 introduces std::byte for this sort of thing, I hope to
transition away from the permissive aliasing behavior of char.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #62 from Bernd Edlinger  ---
(In reply to Jason Merrill from comment #44)
> Created attachment 41048 [details]
> trial patch
> 
> Does this fix the issue?  I don't have an ARM setup handy for testing.

Yes. I just tried, it and the crash is gone.

However if 3.10/10 does not apply here, then it would be
good to remove the special handling of the char type,
and just look for a union and/or maybe a may_alias
attribute.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #61 from Jonathan Wakely  ---
(In reply to rguent...@suse.de from comment #59)
> On Mon, 27 Mar 2017, redi at gcc dot gnu.org wrote:
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
> > 
> > --- Comment #58 from Jonathan Wakely  ---
> > But this seemingly equivalent code doesn't work:
> > 
> >   this->functor = static_cast(f.functor);
> 
> Is function_buffer may_alias?

Yes. Here's what I tested:


union function_buffer_members {
  void* p;
  void(*fp)();
};

union function_buffer {
  function_buffer_members members;
  char data[sizeof(function_buffer_members)];
} __attribute__((may_alias));

struct function_base {
  mutable function_buffer functor;
};

struct function : function_base {
  void func(const function& f) {
const function_buffer& aliasing_ref = f.functor;
this->functor = aliasing_ref;
  }
};

void blah(function& f1, function& f2)
{
  f1.func(f2);
}

The -fdump-tree-optimized dump is:


;; Function void blah(function&, function&) (_Z4blahR8functionS0_,
funcdef_no=1, decl_uid=2315, cgraph_uid=1, symbol_order=1)

void blah(function&, function&) (struct function & f1, struct function & f2)
{
   [100.00%]:
  # DEBUG this => f1_2(D)
  # DEBUG f => f2_3(D)
  # DEBUG D#1 => [(const struct function *)f2_3(D)].D.2297.functor
  # DEBUG aliasing_ref => D#1
  f1_2(D)->D.2297.functor = MEM[(const union function_buffer &
{ref-all})f2_3(D)];
  # DEBUG this => NULL
  # DEBUG f => NULL
  return;

}

This has {ref-all}.

If I use static_cast:

union function_buffer_members {
  void* p;
  void(*fp)();
};

union function_buffer {
  function_buffer_members members;
  char data[sizeof(function_buffer_members)];
} __attribute__((may_alias));

struct function_base {
  mutable function_buffer functor;
};

struct function : function_base {
  void func(const function& f) {
this->functor = static_cast(f.functor);
  }
};

void blah(function& f1, function& f2)
{
  f1.func(f2);
}

Then the dump doesn't have {ref-all}:


;; Function void blah(function&, function&) (_Z4blahR8functionS0_,
funcdef_no=1, decl_uid=2314, cgraph_uid=1, symbol_order=1)

void blah(function&, function&) (struct function & f1, struct function & f2)
{
   [100.00%]:
  # DEBUG this => f1_2(D)
  # DEBUG f => f2_3(D)
  f1_2(D)->D.2297.functor = MEM[(const struct function
*)f2_3(D)].D.2297.functor;
  # DEBUG this => NULL
  # DEBUG f => NULL
  return;

}

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #60 from Richard Biener  ---
-> PR80222

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #59 from rguenther at suse dot de  ---
On Mon, 27 Mar 2017, redi at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
> 
> --- Comment #58 from Jonathan Wakely  ---
> But this seemingly equivalent code doesn't work:
> 
>   this->functor = static_cast(f.functor);

Is function_buffer may_alias?  Then it should work unless the
FE messes up via folding.

struct C { int i; }__attribute__((may_alias)) ;

C a, b;

void foo()
{
  a = static_cast  (b);
}

in .original (correct):

<;

in .gimple (wrong-code):

void foo() ()
{
  a = b;
}

caused by gimple_fold_indirect_ref_rhs doing

tree
gimple_fold_indirect_ref (tree t)
{
  tree ptype = TREE_TYPE (t), type = TREE_TYPE (ptype);
  tree sub = t;
  tree subtype;

  STRIP_NOPS (sub);
  subtype = TREE_TYPE (sub);
  if (!POINTER_TYPE_P (subtype))
return NULL_TREE;

  if (TREE_CODE (sub) == ADDR_EXPR)
{
  tree op = TREE_OPERAND (sub, 0);
  tree optype = TREE_TYPE (op);
  /* * => p */
  if (useless_type_conversion_p (type, optype))
return op;

some of the transforms properly preserve ptype semantics but most don't

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #58 from Jonathan Wakely  ---
But this seemingly equivalent code doesn't work:

  this->functor = static_cast(f.functor);

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #57 from Jonathan Wakely  ---
OK, so this appears to work (assuming may_alias on function_buffer):

  function_buffer& aliasing_ref = f.functor;
  this->functor = aliasing_ref;

It changes the GIMPLE from:

  f1_2(D)->D.2297.functor = MEM[(const struct function
*)f2_3(D)].D.2297.functor;

to:

  f1_2(D)->D.2297.functor = MEM[(const union function_buffer &
{ref-all})f2_3(D)];

Can I rely on that continuing to work? Is it likely to perform better than just
doing memcpy?

  __builtin_memcpy(>functor, , sizeof(f.functor));

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #56 from Jakub Jelinek  ---
I think may_alias attribute only affects pointers/references (pointer/reference
to may_alias type is considered to reference any type), but in this->functor =
f.functor the only pointer is this and *this likely isn't may_alias, and the
bug is about f.functor access anyway.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #55 from Jonathan Wakely  ---
(In reply to Bernd Edlinger from comment #53)
> The boost code was: "this->functor = f.functor;"

And I still don't understand why may_alias doesn't help on the type of
this->functor.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #54 from rguenther at suse dot de  ---
On Mon, 27 Mar 2017, bernd.edlinger at hotmail dot de wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
> 
> --- Comment #53 from Bernd Edlinger  ---
> (In reply to rguent...@suse.de from comment #52)
> > On Mon, 27 Mar 2017, bernd.edlinger at hotmail dot de wrote:
> > 
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
> > > 
> > > --- Comment #51 from Bernd Edlinger  ---
> > > Doesn't 3.10/10 explicitly say that it is undefined to use a union to
> > > to move an object representation that is not a member of the union?
> > 
> > That was my reading...  but 3.10/10 talks about "attempts to access
> > the stored value of an object" and Jason says that this doesn't apply
> > to  d = *p but the result of the decomposition to memberwise copy
> > plus union special handling (where it wouldn't apply at all)
> > 
> 
> The boost code was: "this->functor = f.functor;"
> thus directly accessing the union, so there was no decomposition
> to memberwise copy, right?

Right.

But still the other clause says the storage representation is transfered
and so you could read into that that no "access" happens and thus
3.10/10 doesn't apply.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #53 from Bernd Edlinger  ---
(In reply to rguent...@suse.de from comment #52)
> On Mon, 27 Mar 2017, bernd.edlinger at hotmail dot de wrote:
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
> > 
> > --- Comment #51 from Bernd Edlinger  ---
> > Doesn't 3.10/10 explicitly say that it is undefined to use a union to
> > to move an object representation that is not a member of the union?
> 
> That was my reading...  but 3.10/10 talks about "attempts to access
> the stored value of an object" and Jason says that this doesn't apply
> to  d = *p but the result of the decomposition to memberwise copy
> plus union special handling (where it wouldn't apply at all)
> 

The boost code was: "this->functor = f.functor;"
thus directly accessing the union, so there was no decomposition
to memberwise copy, right?

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #52 from rguenther at suse dot de  ---
On Mon, 27 Mar 2017, bernd.edlinger at hotmail dot de wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
> 
> --- Comment #51 from Bernd Edlinger  ---
> Doesn't 3.10/10 explicitly say that it is undefined to use a union to
> to move an object representation that is not a member of the union?

That was my reading...  but 3.10/10 talks about "attempts to access
the stored value of an object" and Jason says that this doesn't apply
to  d = *p but the result of the decomposition to memberwise copy
plus union special handling (where it wouldn't apply at all)

So the question (DR?) is whether 3.10/10 applies to the access as
written in the source or to it after the decomposition happened.

> "If a program attempts to access the stored value of an object through a
> glvalue of other than one of the
> following types the behavior is undefined:52
> — the dynamic type of the object,
> — a cv-qualified version of the dynamic type of the object,
> — a type similar (as defined in 4.4) to the dynamic type of the object,
> — a type that is the signed or unsigned type corresponding to the dynamic type
> of the object,
> — a type that is the signed or unsigned type corresponding to a cv-qualified
> version of the dynamic type
> of the object,
> — an aggregate or union type that includes one of the aforementioned types
> among its elements or non-
> static data members (including, recursively, an element or non-static data
> member of a subaggregate
> or contained union),
> — a type that is a (possibly cv-qualified) base class type of the dynamic type
> of the object,
> — a char or unsigned char type."

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #51 from Bernd Edlinger  ---
Doesn't 3.10/10 explicitly say that it is undefined to use a union to
to move an object representation that is not a member of the union?

"If a program attempts to access the stored value of an object through a
glvalue of other than one of the
following types the behavior is undefined:52
— the dynamic type of the object,
— a cv-qualified version of the dynamic type of the object,
— a type similar (as defined in 4.4) to the dynamic type of the object,
— a type that is the signed or unsigned type corresponding to the dynamic type
of the object,
— a type that is the signed or unsigned type corresponding to a cv-qualified
version of the dynamic type
of the object,
— an aggregate or union type that includes one of the aforementioned types
among its elements or non-
static data members (including, recursively, an element or non-static data
member of a subaggregate
or contained union),
— a type that is a (possibly cv-qualified) base class type of the dynamic type
of the object,
— a char or unsigned char type."

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #50 from Richard Biener  ---
(In reply to Jason Merrill from comment #44)
> Created attachment 41048 [details]
> trial patch
> 
> Does this fix the issue?  I don't have an ARM setup handy for testing.

I should.

Note that what changed with GCC 7 is only that unions with char members
no longer behave as alias-set zero but 12.8/16 talks about all unions,
not just unions with char members.

Now I read comment#14 as that _only_ char[] members (of structs or unions)
may ever "contain" different dynamic types.  Any pointer to a part of
the standard that singles out char[] that way?

Note your proposed patch will weaken TBAA for all structs/unions with a char
(array) member for the task of aggregate copying.  Might not be too bad
as open-coding memberwise copy would expose alias-set zero writes as well.

The change that exposed char[] union handling differences is

r223126 | rguenth | 2015-05-13 12:53:42 +0200 (Wed, 13 May 2015) | 10 lines

2015-05-13  Richard Biener  

PR middle-end/66110
* alias.c (alias_sets_conflict_p): Do not treat has_zero_child
specially.

which is to avoid having aggregates with char members aliasing with everything,
instead they only alias with alias subsets (including explicit zero).

So if you take the original reduced testcase in this PR and exchange the
char[] array inside the union with a int[] array then the "bug" should trigger
with older GCC as well.  That is, GCC never implemented 12.8/16.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #49 from Bernd Edlinger  ---
While I think that adding explicit copy/assignment constructors
as in comment #43 should work and looks like the most portable
solution for boost, I am unsure if may_alias shouldn't really
work different.

I thought of another use of may_alias that also applies to C:
Isn't the plan to add may_alias to the struct sockaddr_storage ?

And wouldn't you then expect to add a sockaddr_storage
to a structure like:

struct A
{
   struct sockaddr_storage s;
};

does'nt that mean that

A a, b;

a.s = b.s;

will also ignore the may_alias ?

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #48 from Bernd Edlinger  ---
(In reply to Jonathan Wakely from comment #47)
> (In reply to Bernd Edlinger from comment #46)
> > Yes, it seems, the __attribute__((may_alias)) does not propagate from
> > structure members to enclosing structure:
> 
> What enclosing structure? You're apparently agreeing with me, but saying
> something different. In the real code, and the example above, _there_ _is_
> _no_ _enclosing_ _structure_. I understand that it doesn't propagate to
> enclosing structures, that's fine. But if a type X has the attribute then it
> doesn't need to propagate anywhere, it should affect X. But it doesn't fix
> the original bug that started all this.

I think I agree with you, that is surprising that the may_alias
does not do what we need.
I think the enclosing structure is:

class function_base
{
public:
  function_base() : vtable(0) { }
[...]
public:
  detail::function::vtable_base* get_vtable() const {
return reinterpret_cast(
 reinterpret_cast(vtable) &
~static_cast(0x01));
  }

  bool has_trivial_copy_and_destroy() const {
return reinterpret_cast(vtable) & 0x01;
  }

  detail::function::vtable_base* vtable;
  mutable detail::function::function_buffer functor;
};

well, in fact even further down the class hierarchy.
the offending statement is:
this->functor = f.functor;

I think, if a functor may in general alias to anything,
it is possible that the outer object does not alias.
Does that make sense?

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #47 from Jonathan Wakely  ---
(In reply to Bernd Edlinger from comment #46)
> Yes, it seems, the __attribute__((may_alias)) does not propagate from
> structure members to enclosing structure:

What enclosing structure? You're apparently agreeing with me, but saying
something different. In the real code, and the example above, _there_ _is_ _no_
_enclosing_ _structure_. I understand that it doesn't propagate to enclosing
structures, that's fine. But if a type X has the attribute then it doesn't need
to propagate anywhere, it should affect X. But it doesn't fix the original bug
that started all this.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #46 from Bernd Edlinger  ---
(In reply to Jonathan Wakely from comment #45)
> (In reply to rguent...@suse.de from comment #32)
> > So you need to place may-alias at a point to make the following
> > stmt safe:
> > 
> > >   c = *p;
> > 
> > which means placing it on B, not only on the union (p is a B).
> > 
> > Thus do
> > 
> > struct B
> > {
> >   int x;
> >   union U
> > {
> >   int a;
> >   char b[sizeof (float)];
> > } u;
> >   int y;
> > } __attribute__((may_alias));
> 
> In the real code there is no B, there is just the union, and it is assigned
> directly, so it's more like:
> 
> union function_buffer_members {
>   void* p;
>   void(*fp)();
> };
> 
> union function_buffer {
>   function_buffer_members members;
>   char data[sizeof(function_buffer_members)];
> };
> 
> struct function_base {
>   mutable function_buffer functor;
> };
> 
> struct function : function_base {
>   void func(const function& f) {
> this->functor = f.functor;
>   }
> };
> 
> So it should only be necessary to put __attribute__((may_alias)) on union
> function_buffer, right? That doesn't fix the problem though.

Yes, it seems, the __attribute__((may_alias)) does not propagate from
structure members to enclosing structure:

If B has the may_alias, but it is a member of C
then the test case fails again:

inline void* operator new(__SIZE_TYPE__, void *p) { return p; }
struct A { A (float x) : f (x) {} float f; };
struct B
{
  int x;
  union U
  {
int a;
char b[sizeof (float)];
  } u;
  int y;
} __attribute__((may_alias));

struct C
{
  struct B b;
};

__attribute__((noinline, noclone)) void
bar (B , B )
{
  if (x.x != 0 || x.y != 3 || y.x != 0 || y.y != 3)
__builtin_abort ();
  float f;
  __builtin_memcpy (, x.u.b, sizeof (float));
  if (f != 3.5f)
__builtin_abort ();
  __builtin_memcpy (, y.u.b, sizeof (float));
  if (f != 3.5f)
__builtin_abort ();
}

__attribute__((noinline, noclone))
C *
baz (C )
{
  return 
}

__attribute__((noinline, noclone)) void
foo (float x)
{
  C b { 0, {}, 3 }, c;
  C *p = baz (b);
  new (b.b.u.b) A (x);
  c.b = p->b;
  bar (p->b, c.b);
}

int
main ()
{
  foo (3.5f);
}

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #45 from Jonathan Wakely  ---
(In reply to rguent...@suse.de from comment #32)
> So you need to place may-alias at a point to make the following
> stmt safe:
> 
> >   c = *p;
> 
> which means placing it on B, not only on the union (p is a B).
> 
> Thus do
> 
> struct B
> {
>   int x;
>   union U
> {
>   int a;
>   char b[sizeof (float)];
> } u;
>   int y;
> } __attribute__((may_alias));

In the real code there is no B, there is just the union, and it is assigned
directly, so it's more like:

union function_buffer_members {
  void* p;
  void(*fp)();
};

union function_buffer {
  function_buffer_members members;
  char data[sizeof(function_buffer_members)];
};

struct function_base {
  mutable function_buffer functor;
};

struct function : function_base {
  void func(const function& f) {
this->functor = f.functor;
  }
};

So it should only be necessary to put __attribute__((may_alias)) on union
function_buffer, right? That doesn't fix the problem though.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #44 from Jason Merrill  ---
Created attachment 41048
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=41048=edit
trial patch

Does this fix the issue?  I don't have an ARM setup handy for testing.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #43 from Bernd Edlinger  ---
Would something like this also work?

  union function_buffer
  {
[...]
mutable char data;

function_buffer() {}
function_buffer(const function_buffer )
{
   __builtin_memcpy(this, , sizeof(*this));
}
function_buffer& operator = (const function_buffer )
{
   __builtin_memcpy(this, , sizeof(*this));
   return *this;
}
  };

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #42 from Jonathan Wakely  ---
(In reply to Richard Biener from comment #14)
> Seems to be
> 
> void move_assign(function10& f)
> {
>   if ( == this)
> return;
> 
>   { try {
> if (!f.empty()) {
>   this->vtable = f.vtable;
>   if (this->has_trivial_copy_and_destroy())
> this->functor = f.functor;
> ^^^
> 
> for the aggregate copy but lineno info looks confused for the aliasing store.

Changing this aggregate copy to:

__builtin_memcpy(>functor, , sizeof(f.functor));

fixes the crash on armv7hl.

No amount of may_alias attributes on the definition of the union
detail::function::function_buffer type helped.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #41 from Jason Merrill  ---
(In reply to Richard Biener from comment #38)
> Simplified testcase for discussion (is not "miscompiled"):
> 
> struct S {
>   union { int i; } u;
> };
> 
> int main()
> {
>   S s;
>   new () float (2.0);
>   S q = s;
>   if (*reinterpret_cast() != 2.0)
> abort ();
> }
> 
> so you say q = s is valid because the object representation of the union
> is copied.  I say after storing 2.0 to s.u.i the access 's' in q = s is
> invalid as you are accessing the stored value (a float) via a glvalue of
> inappropriate type.

I say what's wrong with this testcase is that storing a float in an int field
is undefined.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #40 from Bernd Edlinger  ---
Yikes, I need an assignement operator:

  B(const B )
  {
__builtin_memcpy(this, , sizeof(B));
  }
  B& operator = (const B )
  {
 __builtin_memcpy(this, , sizeof(B));
 return *this;
  }

now it seems to work...

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #39 from Bernd Edlinger  ---
one thing I do not understand is this:

if I do this in Jakub's test case:

  new (b.u.b) A (x);
  //c = *p;
  __builtin_memcpy(, p, sizeof(B));
  bar (*p, c);

the mis-compilation goes away,
if I add this to Jakub's test case:

struct B
{
  int x;
  union U
  {
int a;
char b[sizeof (float)];
  } u;
  int y;
  B() {}
  B(int xx, U uu, int yy):x(xx), u(uu), y(yy) {}
  B(B )
  {
__asm("":::"memory");
__builtin_memcpy(this, , sizeof(B));
__asm("":::"memory");
  }
};

The mis-compilation gets not fixed.
Why?

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #38 from Richard Biener  ---
Simplified testcase for discussion (is not "miscompiled"):

struct S {
  union { int i; } u;
};

int main()
{
  S s;
  new () float (2.0);
  S q = s;
  if (*reinterpret_cast() != 2.0)
abort ();
}

so you say q = s is valid because the object representation of the union
is copied.  I say after storing 2.0 to s.u.i the access 's' in q = s is
invalid as you are accessing the stored value (a float) via a glvalue of
inappropriate type.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #37 from rguenther at suse dot de  ---
On Fri, 24 Mar 2017, jason at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
> 
> --- Comment #36 from Jason Merrill  ---
> (In reply to Richard Biener from comment #34)
> > C++14 12.8/16 says
> > 
> > "The implicitly-defined copy/move constructor for a union X copies the
> > object representation (3.9) of X."
> > 
> > 3.9/4 says
> > 
> > "The object representation of an object of type T is the sequence of N
> > unsigned char objects...  For trivially copyable types, the value
> > representation is a set of bits in the object representation that determines
> > a value,..."
> > 
> > this suggests that the copying should work but the C++ FE may not simply
> > elide the copy construction by emitting
> > 
> >   c = *p;
> > 
> > because that does _not_ implement memcpy semantics for the union member.
> 
> Nothing in the standard makes memcpy magic; 3.9/3 says,
> 
> For any trivially copyable type T, if two pointers to T point to distinct T
> objects obj1 and obj2, where neither obj1 nor obj2 is a base-class subobject,
> if the underlying bytes (4.4) making up obj1 are copied into obj2, obj2 shall
> subsequently hold the same value as obj1.
> 
> It just talks about copying the underlying bytes, not necessarily using memcpy
> to do so.  And a copy of a B is a memberwise copy, which implies copying the U
> member, which as you quote means copying the object representation, which is
> the underlying bytes.

Yes.  But then the act of copying, the

  c = *p;

stmt needs to still follow 3.10/10, *p is not a valid glvalue to access
the object live in p->u.b which is of type A.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #36 from Jason Merrill  ---
(In reply to Richard Biener from comment #34)
> C++14 12.8/16 says
> 
> "The implicitly-defined copy/move constructor for a union X copies the
> object representation (3.9) of X."
> 
> 3.9/4 says
> 
> "The object representation of an object of type T is the sequence of N
> unsigned char objects...  For trivially copyable types, the value
> representation is a set of bits in the object representation that determines
> a value,..."
> 
> this suggests that the copying should work but the C++ FE may not simply
> elide the copy construction by emitting
> 
>   c = *p;
> 
> because that does _not_ implement memcpy semantics for the union member.

Nothing in the standard makes memcpy magic; 3.9/3 says,

For any trivially copyable type T, if two pointers to T point to distinct T
objects obj1 and obj2, where neither obj1 nor obj2 is a base-class subobject,
if the underlying bytes (4.4) making up obj1 are copied into obj2, obj2 shall
subsequently hold the same value as obj1.

It just talks about copying the underlying bytes, not necessarily using memcpy
to do so.  And a copy of a B is a memberwise copy, which implies copying the U
member, which as you quote means copying the object representation, which is
the underlying bytes.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #35 from Richard Biener  ---
(In reply to Richard Biener from comment #34)
> C++14 12.8/16 says
> 
> "The implicitly-defined copy/move constructor for a union X copies the
> object representation (3.9) of X."
> 
> 3.9/4 says
> 
> "The object representation of an object of type T is the sequence of N
> unsigned char objects...  For trivially copyable types, the value
> representation is a set of bits in the object representation that determines
> a value,..."
> 
> this suggests that the copying should work but the C++ FE may not simply
> elide the copy construction by emitting
> 
>   c = *p;
> 
> because that does _not_ implement memcpy semantics for the union member.
> 
> Note the above may not apply at all here if B is POD and thus the assignment
> is an assignment of PODs (I don't know all of the standard).

It would still conflict with the wording of 3.10/10 (that is, if 3.10/10
is fulfilled the middle-end handles the copying above correct).

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #34 from Richard Biener  ---
C++14 12.8/16 says

"The implicitly-defined copy/move constructor for a union X copies the object
representation (3.9) of X."

3.9/4 says

"The object representation of an object of type T is the sequence of N unsigned
char objects...  For trivially copyable types, the value representation is a
set of bits in the object representation that determines a value,..."

this suggests that the copying should work but the C++ FE may not simply
elide the copy construction by emitting

  c = *p;

because that does _not_ implement memcpy semantics for the union member.

Note the above may not apply at all here if B is POD and thus the assignment
is an assignment of PODs (I don't know all of the standard).

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

Jakub Jelinek  changed:

   What|Removed |Added

 CC||jason at gcc dot gnu.org

--- Comment #33 from Jakub Jelinek  ---
From IRC discussion today about this:
 so, for PR79671 would you be ok with say new may_alias_field attribute
allowed on FIELD_DECLs only that forces has_zero_child on the containing type?
 or may_alias_fields type attribute that would force has_zero_child on
that type
 I have no idea what else boost/libstdc++ could use for the mess they
want/need to do
 didn't I provide a solution for them?
 IMHO the bug is still INVALID
 not sure if adding has_zero_child will have any effect - the same issue
exists for B with char[] array member you placement new into
 the problem with that solution is that you need to move it to the
outermost type, say if somebody puts that weirdo boost class into
some other class, then assignments will not work again
 iff the FE decides that copying PODs is semantically equivalent to
memcpy (alias-wise) then it has to emit it that way (but as said in comment #28
there is no evidence the standard suggests that)
 yes, because C++ doesnt' work that way
 it's very simple...
 and I bet adding ->has_zero_child = 1 to B doesn't actually help in
general
 my understanding was that jason disagrees with that
 I see no comment in the bug from jason
 let's discuss it later today when jason/jwakely are both around
 the C++ FE can simply use alias-set zero for POD assignments then
 thus change c = *p; to MEM[, alias-set-zero] = MEM[p,
alias-set-zero];
 the important thing to notice is that it is 'c = *p' that is undefined

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #32 from rguenther at suse dot de  ---
On Tue, 7 Mar 2017, redi at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
> 
> Jonathan Wakely  changed:
> 
>What|Removed |Added
> 
>  Resolution|INVALID |FIXED
> 
> --- Comment #30 from Jonathan Wakely  ---
> The code in comment 20 still fails even with attribute((may_alias)), so I'm 
> not
> sure how the boost code can be fixed:
> 
> inline void* operator new(__SIZE_TYPE__, void *p) { return p; }
> struct A { A (float x) : f (x) {} float f; };
> static_assert(sizeof(A) == sizeof(float), "");
> struct B
> {
>   int x;
>   union __attribute__((__may_alias__)) U
>   {
> int a;
> char b[sizeof (float)];
>   } u;
>   int y;
> };
> 
> __attribute__((noinline, noclone)) void
> bar (B , B )
> {
>   if (x.x != 0)
> __builtin_abort ();
>   if (x.y != 3)
> __builtin_abort ();
>   if (y.x != 0)
> __builtin_abort ();
>   if (y.y != 3)
> __builtin_abort ();
>   float f;
>   __builtin_memcpy (, x.u.b, sizeof (float));
>   if (f != 3.5f)
> __builtin_abort ();
>   __builtin_memcpy (, y.u.b, sizeof (float));
>   if (f != 3.5f)
> __builtin_abort ();
> }
> 
> __attribute__((noinline, noclone)) 
> B *
> baz (B )
> {
>   return 
> }
> 
> __attribute__((noinline, noclone)) void
> foo (float x)
> {
>   B b { 0, {}, 3 }, c;
>   B *p = baz (b);
>   new (b.u.b) A (x);

The issue is that while operator new takes a ref-all pointer here
it just returns void * which is then passed as this to A::A and thus
construction happens via a non-ref-all pointer.

That is, C++ abstraction comes in the way of may_alias (basically
casting that away again).

So you need to place may-alias at a point to make the following
stmt safe:

>   c = *p;

which means placing it on B, not only on the union (p is a B).

Thus do

struct B
{
  int x;
  union U
{
  int a;
  char b[sizeof (float)];
} u;
  int y;
} __attribute__((may_alias));

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

Jakub Jelinek  changed:

   What|Removed |Added

 Status|RESOLVED|REOPENED
 Resolution|FIXED   |---

--- Comment #31 from Jakub Jelinek  ---
Thus reopening.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

Jonathan Wakely  changed:

   What|Removed |Added

 Resolution|INVALID |FIXED

--- Comment #30 from Jonathan Wakely  ---
The code in comment 20 still fails even with attribute((may_alias)), so I'm not
sure how the boost code can be fixed:

inline void* operator new(__SIZE_TYPE__, void *p) { return p; }
struct A { A (float x) : f (x) {} float f; };
static_assert(sizeof(A) == sizeof(float), "");
struct B
{
  int x;
  union __attribute__((__may_alias__)) U
  {
int a;
char b[sizeof (float)];
  } u;
  int y;
};

__attribute__((noinline, noclone)) void
bar (B , B )
{
  if (x.x != 0)
__builtin_abort ();
  if (x.y != 3)
__builtin_abort ();
  if (y.x != 0)
__builtin_abort ();
  if (y.y != 3)
__builtin_abort ();
  float f;
  __builtin_memcpy (, x.u.b, sizeof (float));
  if (f != 3.5f)
__builtin_abort ();
  __builtin_memcpy (, y.u.b, sizeof (float));
  if (f != 3.5f)
__builtin_abort ();
}

__attribute__((noinline, noclone)) 
B *
baz (B )
{
  return 
}

__attribute__((noinline, noclone)) void
foo (float x)
{
  B b { 0, {}, 3 }, c;
  B *p = baz (b);
  new (b.u.b) A (x);
  c = *p;
  bar (*p, c);
}

int
main ()
{
  foo (3.5f);
}

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

Jakub Jelinek  changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |INVALID

--- Comment #29 from Jakub Jelinek  ---
Jonathan said he'll file upstream boost bugreport about this.
So closing.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #28 from Richard Biener  ---
To quote the only eventually relevant part(s) of 3.10/10:

 - an aggregate or union type that includes one of the aforementioned types
   among its elements or non-static data members (...)
...
 - a char or unsigned char type

note the order.  For union U { char c[4]; float f; } the stored value of type
'int' is not accessed through any of those when accessing it as U.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #27 from rguenther at suse dot de  ---
On Tue, 28 Feb 2017, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
> 
> --- Comment #26 from Jakub Jelinek  ---
> What about PODs or say int?  Let's say I have a class with union in it and in
> that union say float and char array, then use placement new an int with some
> value into the char array in the union.  What will then happen if I default
> copy the class (default assignment operator or copy constructor)?  The
> following doesn't fail, but I think true_dependence still returns 0 for
> (mem/c:SI (plus:DI (reg/f:DI 7 sp)
> (const_int 4 [0x4])) [4 MEM[(int *) + 4B]+0 S4 A32])
> and
> (mem:DI (reg/v/f:DI 0 ax [orig:87 p ] [87]) [1 MEM[(const struct B &)p_4]+0 S8
> A32])
> where the former is from the placement new store of int and the load is from
> the structure assignment.
> 
> inline void* operator new(__SIZE_TYPE__, void *p) { return p; }
> struct B
> {
>   float x;
>   union U
>   {
> float a;
> char b[sizeof (int)];
>   } u;
>   float y;
> };
> 
> __attribute__((noinline, noclone)) void
> bar (B , B )
> {
>   if (x.x != 0 || x.y != 3 || y.x != 0 || y.y != 3)
> __builtin_abort ();
>   int f;
>   __builtin_memcpy (, x.u.b, sizeof (int));
>   if (f != 81)
> __builtin_abort ();
>   __builtin_memcpy (, y.u.b, sizeof (int));
>   if (f != 81)
> __builtin_abort ();
> }
> 
> __attribute__((noinline, noclone)) 
> B *
> baz (B )
> {
>   return 
> }
> 
> __attribute__((noinline, noclone)) void
> foo (int x)
> {
>   B b { 0.0f, {}, 3.0f }, c;
>   B *p = baz (b);
>   new (b.u.b) int (x);
>   c = *p;

I don't think *p is of any type allowed to access the object of type int.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #26 from Jakub Jelinek  ---
What about PODs or say int?  Let's say I have a class with union in it and in
that union say float and char array, then use placement new an int with some
value into the char array in the union.  What will then happen if I default
copy the class (default assignment operator or copy constructor)?  The
following doesn't fail, but I think true_dependence still returns 0 for
(mem/c:SI (plus:DI (reg/f:DI 7 sp)
(const_int 4 [0x4])) [4 MEM[(int *) + 4B]+0 S4 A32])
and
(mem:DI (reg/v/f:DI 0 ax [orig:87 p ] [87]) [1 MEM[(const struct B &)p_4]+0 S8
A32])
where the former is from the placement new store of int and the load is from
the structure assignment.

inline void* operator new(__SIZE_TYPE__, void *p) { return p; }
struct B
{
  float x;
  union U
  {
float a;
char b[sizeof (int)];
  } u;
  float y;
};

__attribute__((noinline, noclone)) void
bar (B , B )
{
  if (x.x != 0 || x.y != 3 || y.x != 0 || y.y != 3)
__builtin_abort ();
  int f;
  __builtin_memcpy (, x.u.b, sizeof (int));
  if (f != 81)
__builtin_abort ();
  __builtin_memcpy (, y.u.b, sizeof (int));
  if (f != 81)
__builtin_abort ();
}

__attribute__((noinline, noclone)) 
B *
baz (B )
{
  return 
}

__attribute__((noinline, noclone)) void
foo (int x)
{
  B b { 0.0f, {}, 3.0f }, c;
  B *p = baz (b);
  new (b.u.b) int (x);
  c = *p;
  bar (*p, c);
}

int
main ()
{
  foo (81);
}

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #25 from rguenther at suse dot de  ---
On Tue, 28 Feb 2017, bernd.edlinger at hotmail dot de wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
> 
> --- Comment #24 from Bernd Edlinger  ---
> (In reply to Richard Biener from comment #23)
> > (In reply to Bernd Edlinger from comment #22)
> > > (In reply to Jakub Jelinek from comment #20)
> > > > which fails also on x86_64-linux at -O2.  And that testcase regressed 
> > > > with
> > > > r223126.  Now whether this is valid C++, no idea, placement new is 
> > > > messy.
> > > 
> > > This test case can't be valid, suppose the A has a copy constructor
> > > that that is also not called when B is moved around.
> > 
> > The canonical fix is to put the type you placement new into the union 
> > storage
> > into the union as regular member rather than having a char[] member in the
> > union.
> 
> Yes. Of course you cannot put a non-POD type in a union,
> but maybe a pointer to A, that is probably what boost should
> do in their functor class.

You can put a non-POD into a union but then you need to provide
copy/move/etc. constructors as they are otherwise default deleted.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #24 from Bernd Edlinger  ---
(In reply to Richard Biener from comment #23)
> (In reply to Bernd Edlinger from comment #22)
> > (In reply to Jakub Jelinek from comment #20)
> > > which fails also on x86_64-linux at -O2.  And that testcase regressed with
> > > r223126.  Now whether this is valid C++, no idea, placement new is messy.
> > 
> > This test case can't be valid, suppose the A has a copy constructor
> > that that is also not called when B is moved around.
> 
> The canonical fix is to put the type you placement new into the union storage
> into the union as regular member rather than having a char[] member in the
> union.

Yes. Of course you cannot put a non-POD type in a union,
but maybe a pointer to A, that is probably what boost should
do in their functor class.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #23 from Richard Biener  ---
(In reply to Bernd Edlinger from comment #22)
> (In reply to Jakub Jelinek from comment #20)
> > which fails also on x86_64-linux at -O2.  And that testcase regressed with
> > r223126.  Now whether this is valid C++, no idea, placement new is messy.
> 
> This test case can't be valid, suppose the A has a copy constructor
> that that is also not called when B is moved around.

The canonical fix is to put the type you placement new into the union storage
into the union as regular member rather than having a char[] member in the
union.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #22 from Bernd Edlinger  ---
(In reply to Jakub Jelinek from comment #20)
> which fails also on x86_64-linux at -O2.  And that testcase regressed with
> r223126.  Now whether this is valid C++, no idea, placement new is messy.

This test case can't be valid, suppose the A has a copy constructor
that that is also not called when B is moved around.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #21 from rguenther at suse dot de  ---
On February 27, 2017 5:46:44 PM GMT+01:00, "jakub at gcc dot gnu.org"
 wrote:
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671
>
>Jakub Jelinek  changed:
>
>   What|Removed |Added
>
>CC||redi at gcc dot gnu.org
>
>--- Comment #20 from Jakub Jelinek  ---
>I believe this all boils down to:
>inline void* operator new(__SIZE_TYPE__, void *p) { return p; }
>struct A { A (float x) : f (x) {} float f; };
>struct B
>{
>  int x;
>  union U
>  {
>int a;
>char b[sizeof (float)];
>  } u;
>  int y;
>};
>
>__attribute__((noinline, noclone)) void
>bar (B , B )
>{
>  if (x.x != 0 || x.y != 3 || y.x != 0 || y.y != 3)
>__builtin_abort ();
>  float f;
>  __builtin_memcpy (, x.u.b, sizeof (float));
>  if (f != 3.5f)
>__builtin_abort ();
>  __builtin_memcpy (, y.u.b, sizeof (float));
>  if (f != 3.5f)
>__builtin_abort ();
>}
>
>__attribute__((noinline, noclone)) 
>B *
>baz (B )
>{
>  return 
>}
>
>__attribute__((noinline, noclone)) void
>foo (float x)
>{
>  B b { 0, {}, 3 }, c;
>  B *p = baz (b);
>  new (b.u.b) A (x);
>  c = *p;
>  bar (*p, c);
>}
>
>int
>main ()
>{
>  foo (3.5f);
>}
>
>which fails also on x86_64-linux at -O2.  And that testcase regressed
>with
>r223126.  Now whether this is valid C++, no idea, placement new is
>messy.

We have one or two duplicates that were resolved invalid.  We can't reasonably
support this in the ME anyway and the lvalue used to access this is not one of
those listed compatible with A.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

Jakub Jelinek  changed:

   What|Removed |Added

 CC||redi at gcc dot gnu.org

--- Comment #20 from Jakub Jelinek  ---
I believe this all boils down to:
inline void* operator new(__SIZE_TYPE__, void *p) { return p; }
struct A { A (float x) : f (x) {} float f; };
struct B
{
  int x;
  union U
  {
int a;
char b[sizeof (float)];
  } u;
  int y;
};

__attribute__((noinline, noclone)) void
bar (B , B )
{
  if (x.x != 0 || x.y != 3 || y.x != 0 || y.y != 3)
__builtin_abort ();
  float f;
  __builtin_memcpy (, x.u.b, sizeof (float));
  if (f != 3.5f)
__builtin_abort ();
  __builtin_memcpy (, y.u.b, sizeof (float));
  if (f != 3.5f)
__builtin_abort ();
}

__attribute__((noinline, noclone)) 
B *
baz (B )
{
  return 
}

__attribute__((noinline, noclone)) void
foo (float x)
{
  B b { 0, {}, 3 }, c;
  B *p = baz (b);
  new (b.u.b) A (x);
  c = *p;
  bar (*p, c);
}

int
main ()
{
  foo (3.5f);
}

which fails also on x86_64-linux at -O2.  And that testcase regressed with
r223126.  Now whether this is valid C++, no idea, placement new is messy.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #19 from Jakub Jelinek  ---
The MEM[(struct parser_binder *) + 4B] = f;'s ultimate origin is:
if (D.589607 != 0B)
  {
iftmp.77 = try
  {
MEM[(struct parser_binder *)D.589607] = f;
^^ This stmt
  }
catch
  {
operator delete (D.589607, NON_LVALUE_EXPR );
  }, (struct parser_binder *) D.589607;;
  }
else
  {
iftmp.77 = (struct parser_binder *) D.589607;
  }

from:
template
void
assign_functor(FunctionObj f, function_buffer& functor, mpl::true_)
const
{
  new (reinterpret_cast()) FunctionObj(f);
}
which looks like a placement new into that mutable char data; field of
boost::detail::function::function_buffer, and then the whole union is copied
including the placement new created data in there.  No idea if that is valid
C++.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #18 from Jakub Jelinek  ---
(In reply to Richard Biener from comment #14)
> Seems to be
> 
> void move_assign(function10& f)
> {
>   if ( == this)
> return;
> 
>   { try {
> if (!f.empty()) {
>   this->vtable = f.vtable;
>   if (this->has_trivial_copy_and_destroy())
> this->functor = f.functor;
> ^^^

Indeed.

> for the aggregate copy but lineno info looks confused for the aliasing store.
> It points at
> 
>   operator=(Functor f)
>   {
> self_type(f).swap(*this);
> return *this;
>   }

The inline location of the aliasing store is:

In function ‘bool boost::detail::function::basic_vtable4::assign_to(FunctionObj, boost::detail::function::function_buffer&,
boost::detail::function::function_obj_tag) const [with FunctionObj =
boost::spirit::qi::detail::parser_binder >,
boost::spirit::qi::literal_char > >, mpl_::bool_ >; R = bool; T0 =
__gnu_cxx::__normal_iterator >&;
T1 = const __gnu_cxx::__normal_iterator >&; T2 =
boost::spirit::context, boost::fusion::vector<> >&; T3 = const
boost::spirit::qi::char_class >&]’,
inlined from ‘void boost::function4::assign_to(Functor)
[with Functor =
boost::spirit::qi::detail::parser_binder >,
boost::spirit::qi::literal_char > >, mpl_::bool_ >; R = bool; T0 =
__gnu_cxx::__normal_iterator >&;
T1 = const __gnu_cxx::__normal_iterator >&; T2 =
boost::spirit::context, boost::fusion::vector<> >&; T3 = const
boost::spirit::qi::char_class >&]’ at
/usr/include/boost/function/function_template.hpp:498:45,
inlined from ‘boost::function4::function4(Functor,
typename boost::enable_if_c<(! boost::is_integral::value), int>::type)
[with Functor =
boost::spirit::qi::detail::parser_binder >,
boost::spirit::qi::literal_char > >, mpl_::bool_ >; R = bool; T0 =
__gnu_cxx::__normal_iterator >&;
T1 = const __gnu_cxx::__normal_iterator >&; T2 =
boost::spirit::context, boost::fusion::vector<> >&; T3 = const
boost::spirit::qi::char_class >&]’ at
/usr/include/boost/function/function_template.hpp:727:7,
inlined from ‘boost::function::function(Functor,
typename boost::enable_if_c<(! boost::is_integral::value), int>::type)
[with Functor =
boost::spirit::qi::detail::parser_binder >,
boost::spirit::qi::literal_char > >, mpl_::bool_ >; R = bool; T0 =
__gnu_cxx::__normal_iterator >&;
T1 = const __gnu_cxx::__normal_iterator >&; T2 =
boost::spirit::context, boost::fusion::vector<> >&; T3 = const
boost::spirit::qi::char_class >&]’ at
/usr/include/boost/function/function_template.hpp:1073:16,
inlined from ‘boost::spirit::qi::rule&
boost::spirit::qi::operator%=(boost::spirit::qi::rule&, Expr&&) [with Expr = const
boost::proto::exprns_::expr&,
boost::proto::exprns_::expr >, 2>&>, 1>; Iterator =
__gnu_cxx::__normal_iterator >;
T1 = std::__cxx11::basic_string(); T2 =
boost::proto::exprns_::expr >, 0>; T3 =
boost::spirit::unused_type; T4 

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #17 from Jakub Jelinek  ---
But if I change:
-mutable char data;
+mutable char data[sizeof (void *) + 2 * sizeof (bool)];
so that the union field occupies the size of some other union members, the
warning is gone, but I still see:
add r5, sp, #292
...
ldm r5, {r0, r1, r2}
...
strbr6, [sp, #293]

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #16 from Jakub Jelinek  ---
With -Wsystem-headers there are various warnings, e.g.
/usr/include/boost/function/function_template.hpp:572:11: warning: placement
new constructing an object of type
‘boost::spirit::qi::detail::parser_binder >,
boost::spirit::qi::literal_char > >, mpl_::bool_ >’ and size ‘2’ in a region of type ‘char’ and
size ‘1’ [-Wplacement-new=]
   new (reinterpret_cast()) FunctionObj(f);
   ^~~
/usr/include/boost/function/function_base.hpp:308:13: warning: placement new
constructing an object of type
‘boost::detail::function::functor_manager_common >,
boost::spirit::qi::literal_char > >, mpl_::bool_ > >::functor_type {aka
boost::spirit::qi::detail::parser_binder >,
boost::spirit::qi::literal_char > >, mpl_::bool_ >}’ and size ‘2’ in a region of type ‘char’ and
size ‘1’ [-Wplacement-new=]
 new (reinterpret_cast(_buffer.data))
functor_type(*in_functor);

^
etc.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #15 from Richard Biener  ---
w/o SRA the cases look like

   [48.23%]:
  <&0x7f3d13ec30f0> f = f;
  <&0x7f3d13ec3140> MEM[(struct parser_binder *) + 4B] = f;
  <&0x7f3d13ec3190> value_405 = (size_t) _vtable.base;
  <&0x7f3d1f70cdc0> value_406 = value_405 | 1;
  <&0x7f3d13ec31e0> value.90_407 = (struct vtable_base *) value_406;
  <&0x7f3d13ec3230> MEM[(struct function4 *)].D.547000.vtable =
value.90_407;
  <&0x7f3d13ebd9b0> MEM[(struct  &)] ={v} {CLOBBER};
  <&0x7f3d13ec3370> tmp.D.547000.vtable = value.90_407;
  <&0x7f3d13ec3460> tmp.D.547000.functor = MEM[(struct function4
*)].D.547000.functor;
  <&0x7f3d13ec35f0> MEM[(struct function4 *)].D.547000.vtable = 0B;
  <&0x7f3d13f06d70> _135 = MEM[(struct vtable_base * *)this_10(D) + 144B];
  <&0x7f3d13f06dc0> if (_135 != 0B)

which look similar from TBAA perspective (parser_binder vs. function4).  Might
not miscompile because of pure luck of course.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #14 from Richard Biener  ---
Seems to be

void move_assign(function10& f)
{
  if ( == this)
return;

  { try {
if (!f.empty()) {
  this->vtable = f.vtable;
  if (this->has_trivial_copy_and_destroy())
this->functor = f.functor;
^^^

for the aggregate copy but lineno info looks confused for the aliasing store.
It points at

  operator=(Functor f)
  {
self_type(f).swap(*this);
return *this;
  }

the boost code for function looks quite convoluted with its vtable handling...

It looks like ESRA creates that piecewise store, but it's hard to track down
exactly where it comes from.  Example transform looks like

+  char f$p$subject$subject$right$ch;
   struct parser_binder f;
   bool D.611696;
   struct parser_binder f;
@@ -15343,7 +11698,8 @@

[100.00%]:
   <&0x7f50d29d3d70> f = f;
-  <&0x7f50d29d3dc0> f = f;
+  <&0x7f50d2a16050> f$p$subject$subject$right$ch_16 = MEM[(struct
parser_binder *) + 1B];
+  <&0x7f50d2a160a0> MEM[(struct parser_binder *) + 1B] =
f$p$subject$subject$right$ch_16;
   <&0x7f50d2a14240> _13 = boost::detail::function::has_empty_target ();

there's no offset 5 one at this point so inlining eventually comes up with

   [100.00%]:
  <&0x7f50d2a16b40> f = f;
  <&0x7f50d2a314b0> f$1_39 = MEM[(struct parser_binder *) + 1B];
  <&0x7f50d2a16b90> f$1_11 = f$1_39;
  <&0x7f50d2a16be0> MEM[(struct  &)] ={v} {CLOBBER};
  <&0x7f50d2a16c30> MEM[(struct function_base *)].vtable = 0B;
  <&0x7f50d2a16c80> MEM[(struct parser_binder *) + 1B] = f$1_11;
  <&0x7f50d2a28bd0> _12 = boost::detail::function::has_empty_target ();
  <&0x7f50d2a16cd0> if (_12 != 0)
goto ; [46.00%]
  else
goto ; [54.00%]

   [54.00%]:
  <&0x7f50d2a16d20> MEM[(struct parser_binder *) + 5B] = f$1_11;

note that .original shows

  size_t value = (size_t) _vtable.base;

  <>;
  <;
  ;

and thus questionable casts.

The question ultimatively boils down to validity of the testcase.

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #13 from Jakub Jelinek  ---
(In reply to Bernd Edlinger from comment #12)
> Hi,
> 
> I don't know if it helps, but on the assembler level there are only two
> instructions that need to be moved to make the test case pass:

That of course helps a lot, that is what I was trying to narrow through the
hacks.
Before sched1 there is:
(insn 598 595 1707 67 (set (mem/c:QI (plus:SI (reg/f:SI 102 sfp)
(const_int -19 [0xffed])) [98 MEM[(struct
parser_binder *) + 5B]+0 S1 A8])
(subreg:QI (reg:SI 505) 0)) 192 {*arm_movqi_insn}
 (expr_list:REG_DEAD (reg:SI 505)
(nil)))
...
(insn 604 603 605 67 (set (reg/f:SI 692)
(plus:SI (reg/f:SI 102 sfp)
(const_int -20 [0xffec])))
"/usr/include/boost/function/function_template.hpp":998 4 {*arm_addsi3}
 (nil))
(insn 605 604 606 67 (parallel [
(set (reg:SI 0 r0)
(mem/c:SI (reg/f:SI 692) [19 MEM[(struct function4
*)].D.542442.functor+0 S4 A32]))
(set (reg:SI 1 r1)
(mem/c:SI (plus:SI (reg/f:SI 692)
(const_int 4 [0x4])) [19 MEM[(struct function4
*)].D.542442.functor+4 S4 A32]))
(set (reg:SI 2 r2)
(mem/c:SI (plus:SI (reg/f:SI 692)
(const_int 8 [0x8])) [19 MEM[(struct function4
*)].D.542442.functor+8 S4 A32]))
]) "/usr/include/boost/function/function_template.hpp":998 364 {*ldm3_}
 (nil))
(current trunk, the above command line options, no patches), so ignoring strict
aliasing, there is a must alias in between the middle mem in insn 605 and the
mem insn 598 stores to.
Now if I do:
break sched_analyze_2 if x->code == MEM && insn->u2.insn_uid == 605
run
continue
then x is:
(mem/c:SI (plus:SI (reg/f:SI 692)
(const_int 4 [0x4])) [19 MEM[(struct function4
*)].D.542442.functor+4 S4 A32])
and deps->pending_write_mems is:
(expr_list:REG_DEP_TRUE (mem/f/c:SI (plus:SI (reg/f:SI 102 sfp)
(const_int -264 [0xfef8])) [18 tmp.D.542442.vtable+0 S4
A64])
(expr_list:REG_DEP_TRUE (mem/c:QI (plus:SI (reg/f:SI 102 sfp)
(const_int -19 [0xffed])) [98 MEM[(struct
parser_binder *) + 5B]+0 S1 A8])
(nil)))
This then calls true_dependence with mem:
(mem/c:QI (plus:SI (reg/f:SI 102 sfp)
(const_int -19 [0xffed])) [98 MEM[(struct parser_binder
*) + 5B]+0 S1 A8])
x:
(mem/c:SI (plus:SI (reg/f:SI 692)
(const_int 4 [0x4])) [19 MEM[(struct function4
*)].D.542442.functor+4 S4 A32])
and we return 0 because of:
2931  if (mems_in_disjoint_alias_sets_p (x, mem))
2932return 0;

In *.optimized dump I believe this is:
  MEM[(struct parser_binder *) + 5B] = 93;
  value_283 = (size_t) _vtable.base;
  value_284 = value_283 | 1;
  value.75_285 = (struct vtable_base *) value_284;
  MEM[(struct function4 *)].D.542442.vtable = value.75_285;
  MEM[(struct  &)] ={v} {CLOBBER};
  tmp.D.542442.vtable = value.75_285;
  tmp.D.542442.functor = MEM[(struct function4 *)].D.542442.functor;
insn 598 is that MEM[(struct parser_binder *) + 5B] = 93; and insn 605
(ldm3_) is the load for the structure assignment
tmp.D.542442.functor = MEM[(struct function4 *)].D.542442.functor;

Richard, could you please have a look from the alias oracle POV?

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

Bernd Edlinger  changed:

   What|Removed |Added

 CC||bernd.edlinger at hotmail dot 
de

--- Comment #12 from Bernd Edlinger  ---
Hi,

I don't know if it helps, but on the assembler level there are only two
instructions that need to be moved to make the test case pass:

diff -u rh1422456-orig.s rh1422456.s
--- rh1422456-orig.s2017-02-26 22:46:30.068919262 +0100
+++ rh1422456.s 2017-02-26 22:46:56.445920712 +0100
@@ -2046,10 +2046,10 @@
ldr ip, [r4, #100]
add lr, sp, #52
movwr3,
#:lower16:_ZZN5boost9function4IbRN9__gnu_cxx17__normal_iteratorIPKcNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcERKSB_RNS_6spirit7contextINS_6fusion4consIRSA_NSH_4nil_EEENSH_6vectorIJERKNSF_2qi10char_classINSF_3tag9char_codeINSS_5spaceENSF_13char_encoding13standard_wideEE9assign_toINSQ_6detail13parser_binderINSQ_4plusINSQ_10differenceINSR_INST_INSS_5char_ESW_NSQ_12literal_charINSV_8standardELb1ELb0EEEN4mpl_5bool_ILb1EEEvT_E13stored_vtable
+   strbr6, [sp, #293]
ldm r5, {r0, r1, r2}
cmp ip, #0
movtr3,
#:upper16:_ZZN5boost9function4IbRN9__gnu_cxx17__normal_iteratorIPKcNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcERKSB_RNS_6spirit7contextINS_6fusion4consIRSA_NSH_4nil_EEENSH_6vectorIJERKNSF_2qi10char_classINSF_3tag9char_codeINSS_5spaceENSF_13char_encoding13standard_wideEE9assign_toINSQ_6detail13parser_binderINSQ_4plusINSQ_10differenceINSR_INST_INSS_5char_ESW_NSQ_12literal_charINSV_8standardELb1ELb0EEEN4mpl_5bool_ILb1EEEvT_E13stored_vtable
-   strbr6, [sp, #293]
orr r3, r3, #1
str r7, [sp, #288]
str r3, [sp, #48]
@@ -2062,10 +2062,10 @@
ldr ip, [r4, #144]
add lr, sp, #68
movwr3,
#:lower16:_ZZN5boost9function4IbRN9__gnu_cxx17__normal_iteratorIPKcNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcERKSB_RNS_6spirit7contextINS_6fusion4consIRSA_NSH_4nil_EEENSH_6vectorIJERKNSF_11unused_typeEE9assign_toINSF_2qi6detail13parser_binderINSV_16lexeme_directiveINSV_4plusINSV_10differenceINSV_10char_classINSF_3tag9char_codeINS12_5char_ENSF_13char_encoding13standard_wideENSV_12literal_charINS15_8standardELb1ELb0EN4mpl_5bool_ILb1EEEvT_E13stored_vtable
+   strbr6, [sp, #293]
ldm r5, {r0, r1, r2}
cmp ip, #0
movtr3,
#:upper16:_ZZN5boost9function4IbRN9__gnu_cxx17__normal_iteratorIPKcNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcERKSB_RNS_6spirit7contextINS_6fusion4consIRSA_NSH_4nil_EEENSH_6vectorIJERKNSF_11unused_typeEE9assign_toINSF_2qi6detail13parser_binderINSV_16lexeme_directiveINSV_4plusINSV_10differenceINSV_10char_classINSF_3tag9char_codeINS12_5char_ENSF_13char_encoding13standard_wideENSV_12literal_charINS15_8standardELb1ELb0EN4mpl_5bool_ILb1EEEvT_E13stored_vtable
-   strbr6, [sp, #293]
orr r3, r3, #1
str r7, [sp, #288]
str r3, [sp, #64]

=> r5 = sp+292, so strb and ldm overlap.
In the reload pass I can find the sp+293 mem rtx:

(insn 593 1693 598 67 (parallel [
(set (reg:SI 0 r0)
(mem/c:SI (reg/f:SI 5 r5 [680]) [19 MEM[(struct
function4D.541978 *)].D.542200.functorD.466600+0 S4 A32]))
(set (reg:SI 1 r1)
(mem/c:SI (plus:SI (reg/f:SI 5 r5 [680])
(const_int 4 [0x4])) [19 MEM[(struct function4D.541978
*)].D.542200.functorD.466600+4 S4 A32]))
(set (reg:SI 2 r2)
(mem/c:SI (plus:SI (reg/f:SI 5 r5 [680])
(const_int 8 [0x8])) [19 MEM[(struct function4D.541978
*)].D.542200.functorD.466600+8 S4 A32]))
]) "rh1422456.cc":151826 364 {*ldm3_}
 (nil))
(insn 598 593 586 67 (set (reg:CC 100 cc)
(compare:CC (reg/f:SI 12 ip [orig:181 _251 ] [181])
(const_int 0 [0]))) "rh1422456.cc":151823 196 {*arm_cmpsi_insn}
 (nil))
(insn 586 598 591 67 (set (mem/c:QI (plus:SI (reg/f:SI 13 sp)
(const_int 293 [0x125])) [101 MEM[(struct parser_binderD.572000
*) + 5B]+0 S1 A8])
(reg:QI 6 r6 [493])) 192 {*arm_movqi_insn}
 (nil))

... and: 

(insn 878 1691 883 99 (parallel [
(set (reg:SI 0 r0)
(mem/c:SI (reg/f:SI 5 r5 [680]) [19 MEM[(struct
function4D.546780 *)].D.547000.functorD.466600+0 S4 A32]))
(set (reg:SI 1 r1)
(mem/c:SI (plus:SI (reg/f:SI 5 r5 [680])
(const_int 4 [0x4])) [19 MEM[(struct function4D.546780
*)].D.547000.functorD.466600+4 S4 A32]))
(set (reg:SI 2 r2)
(mem/c:SI (plus:SI (reg/f:SI 5 r5 [680])
(const_int 8 [0x8])) [19 MEM[(struct function4D.546780
*)].D.547000.functorD.466600+8 S4 A32]))
]) 

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[ktkachov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

--- Comment #11 from ktkachov at gcc dot gnu.org ---
I'll have a look but -fno-schedule-insns "fixing things" has been known to
uncover alias analysis bugs from the GIMPLE level due to the scheduler using
the alias information to reorder memory ops

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

Jakub Jelinek  changed:

   What|Removed |Added

   Priority|P3  |P1

[Bug target/79671] [7 Regression] mapnik miscompilation on armv7hl since r235622

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671

Jakub Jelinek  changed:

   What|Removed |Added

 Status|UNCONFIRMED |NEW
   Last reconfirmed||2017-02-24
 CC||ktkachov at gcc dot gnu.org
  Component|tree-optimization   |target
 Ever confirmed|0   |1

--- Comment #10 from Jakub Jelinek  ---
-fno-schedule-insns fixes it even with PTRS_COMPARE_UNEQUAL=286 (while
-fno-schedule-insns2 still aborts).
Let's consider this a target bug for now (especially when it doesn't fail on
any other arch).