[Bug ipa/101839] New: [9/10/11/12 Regression] Hand in C++ code with -fdevirtualize

[Bug ipa/101839] New: [9/10/11/12 Regression] Hand in C++ code with -fdevirtualize

[marxin at gcc dot gnu.org]

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

            Bug ID: 101839
           Summary: [9/10/11/12 Regression] Hand in C++ code with
                    -fdevirtualize
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: ipa
          Assignee: unassigned at gcc dot gnu.org
          Reporter: marxin at gcc dot gnu.org
                CC: hubicka at gcc dot gnu.org, marxin at gcc dot gnu.org
  Target Milestone: ---

Reported by leon zadorin <leonleon77@gmail.com> via email:

The following produces hanged application (busy-polling by what appears to be
calling self-method in a loop indefinitely) when, essentially, "-fdevirtualize"
is brought into play (albeit with other options possibly).

The reproduction was achieved on
Linux: gcc version 9.2.1 20191008 (Ubuntu 9.2.1-9ubuntu2), x86_64-linux-gnu
and Windows: gcc version 11.1.0 (GCC), x86_64-w64-mingw32

Code compiled as:
c++ -O3 -fprofile-generate ./devirtualize.cpp

Whilst consistently reproducible ("done writing" is never printed out), the
buggy behavior is rather strange and any of the following would change/"fix"
the situation:

Taking out the anonymous namespace;
Marking BA::type() as non-final;
Taking out std::map from CA;
Adding -fno-devirtualize flag to GCC compilation flags.

Note: taking out -fprofile-generate from GCC compilation flags results in
different busy-print loop (potentially followed by segfault) i.e. different,
yet still buggy behaviour.

The repro code could be simplified even further (e.g. by taking out Buf class,
reducing number of calls to 'add', etc) and  still reproduce in gcc-9.2 on
Linux. However on gcc-11.1 on Windows it would then no longer reproduce.
So I've left the following code in its current state so as to hopefully induce
more consistent reproduction across different versions.

Here it is, devirtualize.cpp (also attached):

#include <string.h>
#include <iostream>
#include <map>
namespace {
  struct Buf {
    char * buf; int a{0}; int b{0};
    Buf(char * b) : buf(b) { }
    void add(int v) {
      ::memcpy(buf, &v, sizeof(v));
      a += sizeof(v);
      b += sizeof(v);
    }
  };
  struct A {
    virtual void fill(Buf &buf) {
      buf.add(type());
      buf.add(type());
    }
    virtual ~A() {}
    virtual int type() = 0;
  };
  struct BA : A {
    void fill(Buf &buf) {
      A::fill(buf);
      buf.add(type());
      buf.add(type());
    }
    int type() final {
      return 1;
    }
  };
  struct CBA final : BA {
  };
  struct CA final : A {
    ::std::map<int, int> m;
    int type() final {
      return 2;
    }
  };
}
int main(int argc, char ** ) {
  char d[1024];
  CBA cba;
  ::std::cerr << "writing \n"; ::std::cerr.flush();
  Buf buf(d);
  cba.fill(buf);
  ::std::cerr << "done writing \n"; ::std::cerr.flush();
  CA ca;
}

Richi replied:

The issue also reproduces with GCC 7.5
(on Leap 15.3), -O -fprofile-generate -fdevirtualize is enough
to trigger it there.  Confirmed also with GCC 9, 10, 11 and trunk
and those options.

We hang in

Program received signal SIGINT, Interrupt.
0x00000000004017d5 in (anonymous namespace)::A::fill (buf=..., 
    this=<optimized out>) at t.C:16
16            buf.add(type());
Missing separate debuginfos, use: zypper install 
libgcc_s1-debuginfo-10.3.0+git1587-1.6.4.x86_64 
libstdc++6-debuginfo-10.3.0+git1587-1.6.4.x86_64
(gdb) bt
#0  0x00000000004017d5 in (anonymous namespace)::A::fill (buf=..., 
    this=<optimized out>) at t.C:16
#1  (anonymous namespace)::BA::fill (this=this@entry=0x7fffffffde08, 
buf=...)
    at t.C:24
#2  0x0000000000401860 in main (argc=<optimized out>) at t.C:46
(gdb) s        
(anonymous namespace)::BA::fill (this=this@entry=0x7fffffffde08, buf=...)
    at t.C:24
24            A::fill(buf);
(gdb) s
(anonymous namespace)::A::fill (buf=..., this=0x7fffffffddd8) at t.C:15
15          virtual void fill(Buf &buf) {
(gdb) s
16            buf.add(type());
(gdb) s
(anonymous namespace)::BA::fill (this=this@entry=0x7fffffffde08, buf=...)
    at t.C:24
24            A::fill(buf);
(gdb) s
(anonymous namespace)::A::fill (buf=..., this=0x7fffffffddd8) at t.C:15
15          virtual void fill(Buf &buf) {
(gdb) s
16            buf.add(type());


there is one thing that I'm not sure is allowed.  You do:

  struct A {
    virtual void fill(Buf &buf) {
      buf.add(type());
      buf.add(type());
    }
    virtual ~A() {}
    virtual int type() = 0;
  };
  struct BA : A {
    void fill(Buf &buf) {
      A::fill(buf);
      buf.add(type());
      buf.add(type());
    }

thus override virtual A::fill with non-virtual BA::fill.  But this
might be where the devirtualization code (or the indirect call/devirt
profiling code?) is confused.

[Bug ipa/101839] [9/10/11/12 Regression] Hand in C++ code with -fdevirtualize

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2021-08-10

--- Comment #1 from Martin Liška <marxin at gcc dot gnu.org> ---
With, -O2 -fprofile-generate -fdevirtualize, it started first with
r6-168-g5a33401eabe208e6 which might be a non-culprit commit.

[Bug ipa/101839] [9/10/11/12 Regression] Hand in C++ code with -fdevirtualize

[marxin at gcc dot gnu.org]

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

--- Comment #2 from Martin Liška <marxin at gcc dot gnu.org> ---
Apparently, -fprofile-generate is not needed, w/o it it started with the same
revision.

[Bug ipa/101839] [9/10/11/12 Regression] Hang in C++ code with -fdevirtualize

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[9/10/11/12 Regression]     |[9/10/11/12 Regression]
                   |Hand in C++ code with       |Hang in C++ code with
                   |-fdevirtualize              |-fdevirtualize
      Known to fail|                            |10.3.0, 11.2.1, 7.5.0,
                   |                            |9.4.0
   Target Milestone|---                         |9.5
           Priority|P3                          |P2

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
BA::fill can be declared virtual without any change (it is implicitely so as
stated by the C++ standard it seems).

With GCC 11 and -O -fdevirtualize the program crashes and we see

void {anonymous}::BA::fill (struct BA * const this, struct Buf & buf)
{
  <bb 2> [count: 0]:
  __builtin_unreachable ();

which is from

a-t.C.083i.inline:t.C:16:14: optimized: folding virtual function call to 
__builtin_unreachable
a-t.C.083i.inline:Introduced new external node (void 
__builtin_unreachable()/1356).
a-t.C.083i.inline:t.C:17:14: optimized: folding virtual function call to 
__builtin_unreachable

[Bug ipa/101839] [9/10/11/12 Regression] Hang in C++ code with -fdevirtualize

[redi at gcc dot gnu.org]

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

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #3)
> BA::fill can be declared virtual without any change (it is implicitely so as
> stated by the C++ standard it seems).

Correct.

[Bug ipa/101839] [10/11/12/13 Regression] Hang in C++ code with -fdevirtualize

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|9.5                         |10.4

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 9 branch is being closed

[Bug ipa/101839] [10/11/12/13 Regression] Hang in C++ code with -fdevirtualize

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.4                        |10.5

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 10.4 is being released, retargeting bugs to GCC 10.5.

[Bug ipa/101839] [10/11/12/13 Regression] Hang in C++ code with -fdevirtualize

[rguenth at gcc dot gnu.org]

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

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
Honza?

[Bug ipa/101839] [10/11/12/13 Regression] Hang in C++ code with -fdevirtualize

[yinyuefengyi at gmail dot com]

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

--- Comment #8 from Xionghu Luo (luoxhu at gcc dot gnu.org) <yinyuefengyi at gmail dot com> ---
The relationship is:

A  A::type
| ------------
|             |
BA BA::type   CA    CA::type
|
CBA CBA::type

class CA and CBA are final, also function CA::type and BA::type are final, then
in function possible_polymorphic_call_targets for "target" BA::type, the
"DECL_FINAL_P (target)" check is not accurate enough, as there may be classes
like CBA derived from BA and have instance that need continue walk recursively
in possible_polymorphic_call_targets_1 to record_target_from_binfo.

      if (target)
        {
          /* In the case we get complete method, we don't need 
             to walk derivations.  */
          if (DECL_FINAL_P (target))
            context.maybe_derived_type = false;
        }

So fix this by belong change only stop walk derivations when target is final
and it's class outer_type->type is also final?

diff --git a/gcc/ipa-devirt.cc b/gcc/ipa-devirt.cc
index 412ca14f66b..77f9b268e86 100644
--- a/gcc/ipa-devirt.cc
+++ b/gcc/ipa-devirt.cc
@@ -3188,7 +3188,9 @@ possible_polymorphic_call_targets (tree otr_type,

       /* In the case we get complete method, we don't need
         to walk derivations.  */
-      if (target && DECL_FINAL_P (target))
+      if (target && TREE_CODE (target) == FUNCTION_DECL && DECL_FINAL_P
(target)
+         && RECORD_OR_UNION_TYPE_P (out er_type->type)
+         && TYPE_FINAL_P (outer_type->type))
        context.speculative_maybe_derived_type = false;
       if (type_possibly_instantiated_p (speculative_outer_type->type))
        maybe_record_node (nodes, target, &inserted, can_refer,
&speculation_complete);
@@ -3233,7 +3235,9 @@ possible_polymorphic_call_targets (tree otr_type,
        {
          /* In the case we get complete method, we don't need
             to walk derivations.  */
-         if (DECL_FINAL_P (target))
+         if (TREE_CODE (target) == FUNCTION_DECL && DECL_FINAL_P (target)
+             && RECORD_OR_UNION_TYPE_P (outer_type->type)
+             && TYPE_FINAL_P (outer_type->type))
            context.maybe_derived_type = false;
        }

[Bug ipa/101839] [10/11/12/13 Regression] Hang in C++ code with -fdevirtualize

[hubicka at gcc dot gnu.org]

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

--- Comment #9 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Thanks for looking into this.  
What happens here is that we start working from a call where we know that
outer_type is BA. We correctly find the BA::type and notice that it is final
and thus we do not need to look for sucessors to find something.

However we later decide to discard it here:

3241          if (type_possibly_instantiated_p (outer_type->type))
3244            skipped = true;

This is trying to check, for anonymous types, the possibility that a given
target is never going to be taken since type was never instantiated. It is true
that BA is not instantiated however its successor is.  So we need to keep track
that we saw FINAL and in that case extend type_possibly_instantiated_p to also
look for all derived types. This is quite ugly mistake I got in as the
possibly_instantiated code was implemented before we got DECL_FINAl flag.

I will prepare patch.

[Bug ipa/101839] [10/11/12/13 Regression] Hang in C++ code with -fdevirtualize

[hubicka at gcc dot gnu.org]

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

--- Comment #10 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Created attachment 53430
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53430&action=edit
Patch I am testing

[Bug ipa/101839] [10/11/12/13 Regression] Hang in C++ code with -fdevirtualize

[hubicka at gcc dot gnu.org]

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

--- Comment #11 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Fixed on mainline with r:0f2c7ccd14a29a8af8318f50b8296098fb0ab218

[Bug ipa/101839] [10/11/12 Regression] Hang in C++ code with -fdevirtualize

[marxin at gcc dot gnu.org]

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

--- Comment #12 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to Jan Hubicka from comment #11)
> Fixed on mainline with r:0f2c7ccd14a29a8af8318f50b8296098fb0ab218

g:0f2c7ccd14a29a8af8318f50b8296098fb0ab218

[Bug ipa/101839] [10/11 Regression] Hang in C++ code with -fdevirtualize

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |12.2.0
      Known to work|                            |12.2.1
            Summary|[10/11/12 Regression] Hang  |[10/11 Regression] Hang in
                   |in C++ code with            |C++ code with
                   |-fdevirtualize              |-fdevirtualize

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
And on the 12 branch with r12-9478-gea162107bb376f

[Bug ipa/101839] [11 Regression] Hang in C++ code with -fdevirtualize

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.5                        |11.5

--- Comment #14 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10 branch is being closed.