[Bug ipa/61190] New: [4.8/4.9/4.10 Regression] g++.old-deja/g++.mike/p4736b.C FAILs at -O2/-Os/-O3

[Bug ipa/61190] New: [4.8/4.9/4.10 Regression] g++.old-deja/g++.mike/p4736b.C FAILs at -O2/-Os/-O3

[zsojka at seznam dot cz]

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

            Bug ID: 61190
           Summary: [4.8/4.9/4.10 Regression]
                    g++.old-deja/g++.mike/p4736b.C FAILs at -O2/-Os/-O3
           Product: gcc
           Version: 4.10.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: ipa
          Assignee: unassigned at gcc dot gnu.org
          Reporter: zsojka at seznam dot cz

Created attachment 32798
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=32798&action=edit
prep

Valgrind output with various GCC versions:

$ g++-trunk-r210412 -O2 p4736b.ii
$ valgrind -q --track-origins=yes ./a.out
==11986== Use of uninitialised value of size 8
==11986==    at 0x4006F3: virtual thunk to Main::foo() (in
/home/smatz/gcc-bug/1256/a.out)
==11986==    by 0x400517: main (in /home/smatz/gcc-bug/1256/a.out)
==11986==  Uninitialised value was created by a stack allocation
==11986==    at 0x400500: main (in /home/smatz/gcc-bug/1256/a.out)
==11986== 
==11986== Invalid read of size 8
==11986==    at 0x4006F3: virtual thunk to Main::foo() (in
/home/smatz/gcc-bug/1256/a.out)
==11986==    by 0x400517: main (in /home/smatz/gcc-bug/1256/a.out)
==11986==  Address 0xffffffffffffffe8 is not stack'd, malloc'd or (recently)
free'd
==11986== 
==11986== 
==11986== Process terminating with default action of signal 11 (SIGSEGV)
==11986==  Access not within mapped region at address 0xFFFFFFFFFFFFFFE8
==11986==    at 0x4006F3: virtual thunk to Main::foo() (in
/home/smatz/gcc-bug/1256/a.out)
==11986==    by 0x400517: main (in /home/smatz/gcc-bug/1256/a.out)


$ g++-4_9-r210307 -O2 p4736b.ii
$ valgrind -q --track-origins=yes ./a.out
==12013== Use of uninitialised value of size 8
==12013==    at 0x4006F3: virtual thunk to Main::foo() (in
/home/smatz/gcc-bug/1256/a.out)
==12013==    by 0x400517: main (in /home/smatz/gcc-bug/1256/a.out)
==12013==  Uninitialised value was created by a stack allocation
==12013==    at 0x400500: main (in /home/smatz/gcc-bug/1256/a.out)
==12013== 
==12013== Invalid read of size 8
==12013==    at 0x4006F3: virtual thunk to Main::foo() (in
/home/smatz/gcc-bug/1256/a.out)
==12013==    by 0x400517: main (in /home/smatz/gcc-bug/1256/a.out)
==12013==  Address 0xffffffffffffffe8 is not stack'd, malloc'd or (recently)
free'd
==12013== 
==12013== 
==12013== Process terminating with default action of signal 11 (SIGSEGV)
==12013==  Access not within mapped region at address 0xFFFFFFFFFFFFFFE8
==12013==    at 0x4006F3: virtual thunk to Main::foo() (in
/home/smatz/gcc-bug/1256/a.out)
==12013==    by 0x400517: main (in /home/smatz/gcc-bug/1256/a.out)


$ g++-4_8-r210303 -O2 p4736b.ii
$ valgrind -q --track-origins=yes ./a.out
==12047== Use of uninitialised value of size 8
==12047==    at 0x4006C3: virtual thunk to Main::foo() (in
/home/smatz/gcc-bug/1256/a.out)
==12047==    by 0x400517: main (in /home/smatz/gcc-bug/1256/a.out)
==12047==  Uninitialised value was created by a stack allocation
==12047==    at 0x400500: main (in /home/smatz/gcc-bug/1256/a.out)
==12047== 
==12047== Invalid read of size 8
==12047==    at 0x4006C3: virtual thunk to Main::foo() (in
/home/smatz/gcc-bug/1256/a.out)
==12047==    by 0x400517: main (in /home/smatz/gcc-bug/1256/a.out)
==12047==  Address 0xffffffffffffffe8 is not stack'd, malloc'd or (recently)
free'd
==12047== 
==12047== 
==12047== Process terminating with default action of signal 11 (SIGSEGV)
==12047==  Access not within mapped region at address 0xFFFFFFFFFFFFFFE8
==12047==    at 0x4006C3: virtual thunk to Main::foo() (in
/home/smatz/gcc-bug/1256/a.out)
==12047==    by 0x400517: main (in /home/smatz/gcc-bug/1256/a.out)


$ g++-4_7-r210302 -O2 p4736b.ii
$ valgrind -q --track-origins=yes ./a.out
==12072== Use of uninitialised value of size 8
==12072==    at 0x4006C3: virtual thunk to Main::foo() (in
/home/smatz/gcc-bug/1256/a.out)
==12072==    by 0x400511: main (in /home/smatz/gcc-bug/1256/a.out)
==12072==  Uninitialised value was created by a stack allocation
==12072==    at 0x400500: main (in /home/smatz/gcc-bug/1256/a.out)
==12072== 
==12072== Use of uninitialised value of size 8
==12072==    at 0x4006C3: virtual thunk to Main::foo() (in
/home/smatz/gcc-bug/1256/a.out)
==12072==    by 0x40052B: main (in /home/smatz/gcc-bug/1256/a.out)
==12072==  Uninitialised value was created by a stack allocation
==12072==    at 0x400500: main (in /home/smatz/gcc-bug/1256/a.out)
==12072== 
==12072== Use of uninitialised value of size 8
==12072==    at 0x4006D7: virtual thunk to Main::foo() (in
/home/smatz/gcc-bug/1256/a.out)
==12072==    by 0x40053F: main (in /home/smatz/gcc-bug/1256/a.out)
==12072==  Uninitialised value was created by a stack allocation
==12072==    at 0x400500: main (in /home/smatz/gcc-bug/1256/a.out)
==12072== 
==12072== Use of uninitialised value of size 8
==12072==    at 0x4006E7: virtual thunk to Main::foo() (in
/home/smatz/gcc-bug/1256/a.out)
==12072==    by 0x400553: main (in /home/smatz/gcc-bug/1256/a.out)
==12072==  Uninitialised value was created by a stack allocation
==12072==    at 0x400500: main (in /home/smatz/gcc-bug/1256/a.out)
==12072== 


$ g++-4_6-r197894 -O2 p4736b.ii
$ valgrind -q --track-origins=yes ./a.out
(no output)


The code seems valid and defined to me, but somebody else might know otherwise.

I haven't analysed whether messages output by valgrind for gcc 4.7 are valid or
just false positives.


Tested revisions:
trunk r210412 - SIGSEGV
4_9 r210307 - SIGSEGV
4_8 r210303 - SIGSEGV
4_7 r210302 - valgrind warnings, but no crash
4_6 r197894 - OK

[Bug ipa/61190] [4.8/4.9/4.10 Regression] g++.old-deja/g++.mike/p4736b.C FAILs at -O2/-Os/-O3

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.8.4

[Bug ipa/61190] [4.8/4.9/4.10 Regression] g++.old-deja/g++.mike/p4736b.C FAILs at -O2/-Os/-O3

[bernd.edlinger at hotmail dot de]

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

Bernd Edlinger <bernd.edlinger at hotmail dot de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bernd.edlinger at hotmail dot de

--- Comment #1 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
Created attachment 32879
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=32879&action=edit
slightly reduced test case

Acutally also -O1 produces wrong code, and happens to not crash by chance.
Only -O0 and -Og produce correct code.

The constructor Main::Main() first inlined and then completely optimized
away in the dce1 pass.

But even with -fno-tree-dce the constructor seems to be removed in the rtl
passes.
However when I use -fno-ipa-pure-const the sample works at all optimization
levels.

So should we blame "ipa-pure-const"?

[Bug ipa/61190] [4.8/4.9/4.10 Regression] g++.old-deja/g++.mike/p4736b.C FAILs at -O2/-Os/-O3

[bernd.edlinger at hotmail dot de]

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

--- Comment #2 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
Created attachment 32883
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=32883&action=edit
possible fix

Well, the problem seems to be, that the thunk code is
not emitted in the normal way using gimple code,
but instead by the i386 back-end with a callback.

This makes the thunk invisible to the ipa-pure-const pass,
which assumes that all values just flow straight thu the thunk.

See ipa-pure-const.c (analyze_function):

  if (fn->thunk.thunk_p || fn->alias)
    {
      /* Thunk gets propagated through, so nothing interesting happens.  */
      gcc_assert (ipa);
      return l;
    }

But this is not true for a virtual base class:
The thunk itself references the vtable, so if nothing else might need
the vtable, the optimizer may remove the initalization of the vtable,
which happened in this example.

The most simple work around would be the attached patch, which
simply falls back to emitting gimple code, if virtual base classes
are used.

[Bug ipa/61190] [4.8/4.9/4.10 Regression] g++.old-deja/g++.mike/p4736b.C FAILs at -O2/-Os/-O3

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
That looks like a too ugly hack.  Much better is just to teach the
ipa-pure-const pass about thunks IMHO, what they can and can't access.

[Bug ipa/61190] [4.8/4.9/4.10 Regression] g++.old-deja/g++.mike/p4736b.C FAILs at -O2/-Os/-O3

[bernd.edlinger at hotmail dot de]

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

Bernd Edlinger <bernd.edlinger at hotmail dot de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at gcc dot gnu.org

--- Comment #4 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
(In reply to Jakub Jelinek from comment #3)
> That looks like a too ugly hack.  Much better is just to teach the
> ipa-pure-const pass about thunks IMHO, what they can and can't access.

yes, sigh, I know...

But OTOH this patch not only fixes the test case, but also
allows several optimizations on the thunk,
that were not possible before:

- inlining the function into the thunk
- inlining the thunk into the caller
- versioning on the thunk
- general optimizations on the thunk


this means, in this example:
 the thunk's access to the vtable is optimized away
 and then the constructor is also optimized away.
 => everything is just fine!

And, if inlining of the function into the thunk is
not possible for any reason, then I still see a
tail-call optimization.


Well I have still a few days off, to work on this.

What I tried so far is this:

Following Honzas advice, I tried this:

Index: ipa-pure-const.c
===================================================================
--- ipa-pure-const.c    (revision 212426)
+++ ipa-pure-const.c    (working copy)
@@ -735,6 +735,8 @@ analyze_function (struct cgraph_node *fn, bool ipa
   l->looping_previously_known = true;
   l->looping = false;
   l->can_throw = false;
+  if (fn->thunk.thunk_p && fn->thunk.virtual_offset_p)
+    l->pure_const_state = IPA_NEITHER;
   state_from_flags (&l->state_previously_known, &l->looping_previously_known,
             flags_from_decl_or_type (fn->decl),
             cgraph_node_cannot_return (fn));

BUT it does not fix the test case.  I frankly admit I do not fully
understand why...   Any insight here, might be helpful.



When I look at the code in ipa-pure-const.c,
I noticed, that cgraph_function_body_availability is called in many
places, and it may be able to influence lots of things.

So I tried this:

Index: cgraph.c
===================================================================
--- cgraph.c    (revision 212426)
+++ cgraph.c    (working copy)
@@ -2133,6 +2133,8 @@ cgraph_function_body_availability (struct cgraph_n
     avail = AVAIL_LOCAL;
   else if (node->alias && node->weakref)
     cgraph_function_or_thunk_node (node, &avail);
+  else if (node->thunk.thunk_p && node->thunk.virtual_offset_p)
+    avail = AVAIL_OVERWRITABLE;
   else if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (node->decl)))
     avail = AVAIL_OVERWRITABLE;
   else if (!node->externally_visible)


And yes, this fixes the test case!

Returning AVAIL_OVERWRITABLE in this case means in my own words:
"this thunk can do anything, and you can not assume anything,
it may be even replaced by someting completely different, by the back-end."

However I don't know if this is a better soulution at all...

The resulting code, at -O2 is worse than with my first hacky patch,
because the constructor is now fully expanded, and can no longer be
optimized away, but it seems to work.

IMHO this issue clearly needs to be fixed, one way or the other...
What would be your ideas, how to fix it?


Thanks
Bernd.

[Bug ipa/61190] [4.8/4.9/5 Regression] g++.old-deja/g++.mike/p4736b.C FAILs at -O2/-Os/-O3

[edlinger at gcc dot gnu.org]

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

--- Comment #5 from Bernd Edlinger <edlinger at gcc dot gnu.org> ---
Author: edlinger
Date: Wed Nov 26 18:10:29 2014
New Revision: 218091

URL: https://gcc.gnu.org/viewcvs?rev=218091&root=gcc&view=rev
Log:
2014-11-26  Bernd Edlinger  <bernd.edlinger@hotmail.de>

        PR ipa/61190
        * cgraph.h (symtab_node::call_for_symbol_and_aliases): Fix comment.
        (cgraph_node::function_or_virtual_thunk_symbol): New function.
        (cgraph_node::call_for_symbol_and_aliases): Fix comment.
        (cgraph_node::call_for_symbol_thunks_and_aliases): Adjust comment.
        Add new optional parameter exclude_virtual_thunks.
        * cgraph.c (cgraph_node::call_for_symbol_thunks_and_aliases): Add new
        optional parameter exclude_virtual_thunks.
        (cgraph_node::set_const_flag): Don't propagate to virtual thunks.
        (cgraph_node::set_pure_flag): Likewise.
        (cgraph_node::function_symbol): Simplified.
        (cgraph_node::function_or_virtual_thunk_symbol): New function.
        * ipa-pure-const.c (analyze_function): For virtual thunks set
        pure_const_state to IPA_NEITHER.
        (propagate_pure_const): Use function_or_virtual_thunk_symbol.

testsuite/ChangeLog:
2014-11-26  Bernd Edlinger  <bernd.edlinger@hotmail.de>

        PR ipa/61190
        * g++.old-deja/g++.mike/p4736b.C: Use -O2.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cgraph.c
    trunk/gcc/cgraph.h
    trunk/gcc/ipa-pure-const.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.old-deja/g++.mike/p4736b.C

[Bug ipa/61190] [4.8/4.9 Regression] g++.old-deja/g++.mike/p4736b.C FAILs at -O2/-Os/-O3

[edlinger at gcc dot gnu.org]

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

Bernd Edlinger <edlinger at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2014-11-26
      Known to work|                            |5.0
            Summary|[4.8/4.9/5 Regression]      |[4.8/4.9 Regression]
                   |g++.old-deja/g++.mike/p4736 |g++.old-deja/g++.mike/p4736
                   |b.C FAILs at -O2/-Os/-O3    |b.C FAILs at -O2/-Os/-O3
     Ever confirmed|0                           |1
      Known to fail|4.10.0                      |

[Bug ipa/61190] [4.8/4.9 Regression] g++.old-deja/g++.mike/p4736b.C FAILs at -O2/-Os/-O3

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

[Bug ipa/61190] [4.8/4.9 Regression] g++.old-deja/g++.mike/p4736b.C FAILs at -O2/-Os/-O3

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.8.4                       |4.8.5

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 4.8.4 has been released.

[Bug ipa/61190] [4.8/4.9 Regression] g++.old-deja/g++.mike/p4736b.C FAILs at -O2/-Os/-O3

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.8.5                       |4.9.3

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
The gcc-4_8-branch is being closed, re-targeting regressions to 4.9.3.

[Bug ipa/61190] [4.9 Regression] g++.old-deja/g++.mike/p4736b.C FAILs at -O2/-Os/-O3

[jakub at gcc dot gnu.org]

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 4.9.3 has been released.

[Bug ipa/61190] [4.9 Regression] g++.old-deja/g++.mike/p4736b.C FAILs at -O2/-Os/-O3

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.9.3                       |4.9.4