[Bug ipa/60306] New: Incorrect devirtualization "pure virtual method called"

[Bug ipa/60306] New: Incorrect devirtualization "pure virtual method called"

[bredelin at ucla dot edu]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60306

            Bug ID: 60306
           Summary: Incorrect devirtualization "pure virtual method
                    called"
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: ipa
          Assignee: unassigned at gcc dot gnu.org
          Reporter: bredelin at ucla dot edu

Created attachment 32192
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=32192&action=edit
File that exhibits the compilation error.

When compiled with GCC 4.9, my C++ code gives:

pure virtual method called
terminate called without an active exception
Aborted

This goes away if I compile with -fno-devirtualize, or if I compile with GCC
4.8.2.

I compiled with:

% g++-4.9 -O3 main.C -std=c++11 -g
% ./a.out

[Bug ipa/60306] [4.9 Regression] Incorrect devirtualization "pure virtual method called"

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60306

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at gcc dot gnu.org
   Target Milestone|---                         |4.9.0
            Summary|Incorrect devirtualization  |[4.9 Regression] Incorrect
                   |"pure virtual method        |devirtualization "pure
                   |called"                     |virtual method called"

[Bug ipa/60306] [4.9 Regression] Incorrect devirtualization "pure virtual method called"

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60306

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
           Priority|P3                          |P1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2014-02-27
     Ever confirmed|0                           |1

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Confirmed.

[Bug ipa/60306] [4.9 Regression] Incorrect devirtualization "pure virtual method called"

[hubicka at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60306

Jan Hubicka <hubicka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |hubicka at gcc dot gnu.org

--- Comment #2 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Mine. It seems that the cxa_pure_virtual actually happens as a result of fre
propagation, possible_polymorphic_call_targets always returns 2 possible
targets. I wonder how we trigger this.

[Bug ipa/60306] [4.9 Regression] Incorrect devirtualization "pure virtual method called"

[hubicka at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60306

Jan Hubicka <hubicka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mjambor at suse dot cz

--- Comment #3 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
OK, it happens by ipa_intraprocedural_devirtualization, but since I was nasty
enough to assign the other bug to Martin today, I will look into this one.

[Bug ipa/60306] [4.9 Regression] Incorrect devirtualization "pure virtual method called"

[hubicka at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60306

--- Comment #4 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
OK, I am re-considering my decision to not assign this to Martin.
The problem is the following.  We have call:

  struct Box x;
...
  x = edges_connecting_to_node (1); [return slot optimization]
...
  _19 = OBJ_TYPE_REF(_18;(const struct Object)&x.D.8084->0) (&x.D.8084);

The dynamic type of x at that point is Box. We however get it wrong as Object.
This is what come from detect_type_change.

The reason is that detect_type_change actually ignores
  x = edges_connecting_to_node (1); [return slot optimization]
which it should not, since it gives it an useful information that x is fully
constructed when the return value happens.

But it considers other statement:
  MEM[(struct new_allocator *)&x + 8B] ={v} {CLOBBER};
  MEM[(struct allocator *)&x + 8B] ={v} {CLOBBER};
  MEM[(struct _Vector_impl *)&x + 8B] ={v} {CLOBBER};
  MEM[(struct _Vector_base *)&x + 8B] ={v} {CLOBBER};
  MEM[(struct vector *)&x + 8B] ={v} {CLOBBER};
  MEM[(struct Object *)&x]._vptr.Object = &MEM[(void *)&_ZTV6Object + 16B];
  MEM[(struct Object *)&x] ={v} {CLOBBER};
  x ={v} {CLOBBER};

which is end of the loop the whole thing is contained in.  The dead store to
._vptr.Object come from inlined destructor and it makes detect_type_change to
believe that the dynamic type is Object.  That is true if you manage to ignore
the initialization.

Now I wonder how to fix this; simple fix is to make detect_type_change to
notice the call and constructors, that is useful by itself.
But I believe there is deeper problem, we need to prove that on _all_ paths to
the statement the dynamic type was changed in known way, not that on all paths
where we can understand the dynamic change the type changed same way.

Can alias oracle walker tell us when it runs into default def?

[Bug ipa/60306] [4.9 Regression] Incorrect devirtualization "pure virtual method called"

[hubicka at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60306

--- Comment #5 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Created attachment 32231
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=32231&action=edit
WIP patch

Hi,
I actually had code around to detect type known from function call, so it is
easy to plug it in and solve the testcase.

I am not convinced this is safe - I think we may miss the dynamic type setting
statement and then we may give wrong answer. Basically we want to know if on
_all_ or none paths we encoutered the type setting statemnt.  I do not think
the alias oracle walker can tell me if it reached top of function?

Honza

[Bug ipa/60306] [4.9 Regression] Incorrect devirtualization "pure virtual method called"

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60306

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jan Hubicka from comment #4)
> OK, I am re-considering my decision to not assign this to Martin.
> The problem is the following.  We have call:
> 
>   struct Box x;
> ...
>   x = edges_connecting_to_node (1); [return slot optimization]
> ...
>   _19 = OBJ_TYPE_REF(_18;(const struct Object)&x.D.8084->0) (&x.D.8084);
> 
> The dynamic type of x at that point is Box. We however get it wrong as
> Object. This is what come from detect_type_change.
> 
> The reason is that detect_type_change actually ignores
>   x = edges_connecting_to_node (1); [return slot optimization]
> which it should not, since it gives it an useful information that x is fully
> constructed when the return value happens.
> 
> But it considers other statement:
>   MEM[(struct new_allocator *)&x + 8B] ={v} {CLOBBER};
>   MEM[(struct allocator *)&x + 8B] ={v} {CLOBBER};
>   MEM[(struct _Vector_impl *)&x + 8B] ={v} {CLOBBER};
>   MEM[(struct _Vector_base *)&x + 8B] ={v} {CLOBBER};
>   MEM[(struct vector *)&x + 8B] ={v} {CLOBBER};
>   MEM[(struct Object *)&x]._vptr.Object = &MEM[(void *)&_ZTV6Object + 16B];
>   MEM[(struct Object *)&x] ={v} {CLOBBER};
>   x ={v} {CLOBBER};
> 
> which is end of the loop the whole thing is contained in.  The dead store to
> ._vptr.Object come from inlined destructor and it makes detect_type_change
> to believe that the dynamic type is Object.  That is true if you manage to
> ignore the initialization.
> 
> Now I wonder how to fix this; simple fix is to make detect_type_change to
> notice the call and constructors, that is useful by itself.
> But I believe there is deeper problem, we need to prove that on _all_ paths
> to the statement the dynamic type was changed in known way, not that on all
> paths where we can understand the dynamic change the type changed same way.

It seems that the current code doesn't properly perform that "merging".  The
walker will happily walks all incoming edges of PHIs.

> Can alias oracle walker tell us when it runs into default def?

Well, it simply stops walking, so no, it doesn't return whether the
callback returned always false (I didn't need it so I didn't implement it ...).

[Bug ipa/60306] [4.9 Regression] Incorrect devirtualization "pure virtual method called"

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60306

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
Of course the bug seems to be

static bool
stmt_may_be_vtbl_ptr_store (gimple stmt)
{
  if (is_gimple_call (stmt))
    return false;
^^^

this.  I remember being very curious when seeing this ;)

Please at this point only fix that bug, not introduce more fancy stuff (we're
long past a stage where that is appropriate).

[Bug ipa/60306] [4.9 Regression] Incorrect devirtualization "pure virtual method called"

[hubicka at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60306

--- Comment #8 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Created attachment 32235
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=32235&action=edit
Better WIP patch

Yep, ignoring the calls also surprised me. I spent some time trying to think of
and produce more evil testcases to this issue, but rest of Martin's reasoning
seems right. The code really just mixes up the case where it knows something on
all understood places with the case it knows something on all possible paths
through the code. It implements the first but wants the second.

I also tried to give up on all calls first. That makes the intraprocedural and
part of inter-procedural devirt to give up almost always. Except for trivial
cases that are handled by SCCVN you almost always have a call in a way, so
testsuite is not really happy about it.

Other option is to simply give up on recording anything when type change is
seen. This also fixes the bug, perhaps with less fallout.

Third option is to record if walk ever reached top of function and if it did
give up when type change is seen. This seems to be simple addition to ao
walker, we just need some interface for it.

Fourth option is to be smarter about the calls defining type that is something
I would like to do for next stage1, patch attached. It is ortoghonal to 1/2/3,
if we have it we will just understand more paths (and currently I punt if I see
some not understood path)

I can get some data on difference in between options 1,2,3 for mainline, or do
we just want to give up?

[Bug ipa/60306] [4.9 Regression] Incorrect devirtualization "pure virtual method called"

[hubicka at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60306

--- Comment #9 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
OK, this is what I am testing for mainline now:
Index: ipa-prop.c
===================================================================
--- ipa-prop.c  (revision 208247)
+++ ipa-prop.c  (working copy)
@@ -572,7 +572,12 @@ static bool
 stmt_may_be_vtbl_ptr_store (gimple stmt)
 {
   if (is_gimple_call (stmt))
-    return false;
+    {
+      return ((gimple_call_lhs (stmt)
+              && AGGREGATE_TYPE_P (TREE_TYPE (gimple_call_lhs (stmt))))
+             || (gimple_call_fndecl (stmt)
+                 && DECL_CXX_CONSTRUCTOR_P (gimple_call_fndecl (stmt))));
+    }
   else if (gimple_clobber_p (stmt))
     return false;
   else if (is_gimple_assign (stmt))

This will make us to punt on about every ctor sequence except case where
everything got early inlined.  I hope to get some firefox numbers - I don't
expecct it to be that bad. ipa-devirt made us less dependent on type change
detection and type change detection was never too strong anyway.

other option I did not mention is to simply revert:

2013-12-14   Jan Hubicka  <jh@suse.cz>

        PR middle-end/58477
        * ipa-prop.c (stmt_may_be_vtbl_ptr_store): Skip clobbers.

this change made the bug to trigger: by punting on all clobbers one can not mix
construction and destruction within the loop for automatic vars.

I spent some time playing with this and I think those are only ones that
matters. It seems that most of checks for dynamic type change for function
parameters are not that useful based on Jason's comment that once you get a
non-pod built you can not in-place new it to something else. Because we don't
track heap allocated objects the type is given by the type of decl the instance
lives in.

Reverting that patch seems however rather symptomatic fix.  This code is on my
TODO to rework and integrate into ipa-devirt for next stage1 anyway.

[Bug ipa/60306] [4.9 Regression] Incorrect devirtualization "pure virtual method called"

[hubicka at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60306

--- Comment #10 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Author: hubicka
Date: Sun Mar  2 20:51:48 2014
New Revision: 208261

URL: http://gcc.gnu.org/viewcvs?rev=208261&root=gcc&view=rev
Log:

    PR ipa/60306

    Revert:
    2013-12-14   Jan Hubicka  <jh@suse.cz>
        PR middle-end/58477
        * ipa-prop.c (stmt_may_be_vtbl_ptr_store): Skip clobbers.

    * testsuite/g++.dg/ipa/devirt-29.C: New testcase

Added:
    trunk/gcc/testsuite/g++.dg/ipa/devirt-29.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-prop.c
    trunk/gcc/testsuite/ChangeLog

[Bug ipa/60306] [4.9 Regression] Incorrect devirtualization "pure virtual method called"

[hubicka at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60306

Jan Hubicka <hubicka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #11 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Fixed, even though for next stage1 we need more work.