[Bug ipa/109770] New: wrong(?) devirtualization

[Bug ipa/109770] New: wrong(?) devirtualization

[rguenth at gcc dot gnu.org]

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

            Bug ID: 109770
           Summary: wrong(?) devirtualization
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: ipa
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rguenth at gcc dot gnu.org
                CC: marxin at gcc dot gnu.org
  Target Milestone: ---

#include <new>

struct Base
{
  virtual ~Base() {}
};
struct A : Base
{
  virtual ~A() {}
};
struct B : Base
{
  [[gnu::noinline]] B() { new (this) A; }
  virtual ~B() { __builtin_abort (); }
};
int main()
{
  Base *p = new B;
  delete p;
}

aborts when compiled with -O2 (with devirtualization enabled).  Neither
GCC nor clang diagnose the placement new in B::B() (but it looks fishy).

[Bug ipa/109770] wrong(?) devirtualization

[rguenth at gcc dot gnu.org]

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

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
I cannot find a clause in the C++ standard constraining the object returned by
a new expression (well, not clearly enough) nor whether re-using the object
storage during construction is allowed.

[Bug ipa/109770] wrong(?) devirtualization

[rguenth at gcc dot gnu.org]

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

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
A less questionable case would may be if A were derived from B instead of only
a common base?

[Bug ipa/109770] wrong(?) devirtualization

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
Only GCC seems to devirtualize here (even when hiding all definitions besides
main).

[Bug ipa/109770] [10/11/12/13 Regression] wrong(?) devirtualization

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |10.5
      Known to fail|                            |13.1.0, 7.5.0
            Summary|wrong(?) devirtualization   |[10/11/12/13 Regression]
                   |                            |wrong(?) devirtualization
      Known to work|                            |4.8.5

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 4.8 worked fine.

[Bug ipa/109770] [10/11/12/13/14 Regression] wrong(?) devirtualization

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
So we devirtualize during FRE1:

Determining dynamic type for call: OBJ_TYPE_REF(_3;(struct Base)_7->1B) (_7);
  Starting walk at: _1 = MEM[(struct Base *)_7]._vptr.Base;
  instance pointer: _7  Outer instance pointer: _7 offset: 0 (bits) vtbl
reference: MEM[(struct Base *)_7]._vptr.Base
  Checking constructor call: B::B (_7);
  Recording type: struct B at offset 0
  Determined dynamic type.

so eventually the CTOR call handling needs to be removed if we cannot
rely on that producing a specific dynamically typed object.

Any C++ experts?

[Bug ipa/109770] [10/11/12/13/14 Regression] wrong(?) devirtualization

[m.cencora at gmail dot com]

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

m.cencora at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |m.cencora at gmail dot com

--- Comment #6 from m.cencora at gmail dot com ---
(In reply to Richard Biener from comment #0)
> #include <new>
> 
> struct Base
> {
>   virtual ~Base() {}
> };
> struct A : Base
> {
>   virtual ~A() {}
> };
> struct B : Base
> {
>   [[gnu::noinline]] B() { new (this) A; }
>   virtual ~B() { __builtin_abort (); }
> };
> int main()
> {
>   Base *p = new B;
>   delete p;
> }
> 
> aborts when compiled with -O2 (with devirtualization enabled).  Neither
> GCC nor clang diagnose the placement new in B::B() (but it looks fishy).

I believe you need to std::launder the p pointer before calling delete:
https://en.cppreference.com/w/cpp/utility/launder

[Bug ipa/109770] [10/11/12/13/14 Regression] wrong(?) devirtualization

[rguenth at gcc dot gnu.org]

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

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to m.cencora from comment #6)
> (In reply to Richard Biener from comment #0)
> > #include <new>
> > 
> > struct Base
> > {
> >   virtual ~Base() {}
> > };
> > struct A : Base
> > {
> >   virtual ~A() {}
> > };
> > struct B : Base
> > {
> >   [[gnu::noinline]] B() { new (this) A; }
> >   virtual ~B() { __builtin_abort (); }
> > };
> > int main()
> > {
> >   Base *p = new B;
> >   delete p;
> > }
> > 
> > aborts when compiled with -O2 (with devirtualization enabled).  Neither
> > GCC nor clang diagnose the placement new in B::B() (but it looks fishy).
> 
> I believe you need to std::launder the p pointer before calling delete:
> https://en.cppreference.com/w/cpp/utility/launder

Ah, interesting.  I was looking for an answer whether

 new T

may produce anything other than an object with dynamic type T or if there
are any constraints on the object constructed.  In particular C++20 11.10.4
refered to by 6.7.3 doesn't mention whether re-using the storage is
permitted for an object under construction.  It says usage is limited
and doesn't explicitely allow re-use so that might mean re-use is not allowed.

Note std::launder is C++17 only, the original reporter was using C++98 / C++14

Reading C++20, 6.7.3/9, at least suggests that the placement new (this) A
in the CTOR invokes undefined behavior because the lifetime of B hasn't
ended (but 6.7.3/1.5 makes this ambiguous - we're re-using the storage here,
and also we're not talking about an object with automatic storage duration).

[Bug ipa/109770] [10/11/12/13/14 Regression] wrong(?) devirtualization

[m.cencora at gmail dot com]

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

--- Comment #8 from m.cencora at gmail dot com ---
(In reply to Richard Biener from comment #7)
> 
> Ah, interesting.  I was looking for an answer whether
> 
>  new T
> 
> may produce anything other than an object with dynamic type T or if there
> are any constraints on the object constructed.  In particular C++20 11.10.4
> refered to by 6.7.3 doesn't mention whether re-using the storage is
> permitted for an object under construction.  It says usage is limited
> and doesn't explicitely allow re-use so that might mean re-use is not
> allowed.
> 
> Note std::launder is C++17 only, the original reporter was using C++98 /
> C++14
> 
> Reading C++20, 6.7.3/9, at least suggests that the placement new (this) A
> in the CTOR invokes undefined behavior because the lifetime of B hasn't
> ended (but 6.7.3/1.5 makes this ambiguous - we're re-using the storage here,
> and also we're not talking about an object with automatic storage duration).

6.7.3/9 doesn't apply to your example - this is not an automatic storage
duration.

If 11.10.4 is suppose to be an exhaustive allow-list, then yeah ending the
complete object lifetime during its construction is UB, but I guess it would be
best to get a response from real C++ expert or file a C++ core issue.

[Bug ipa/109770] [10/11/12/13/14 Regression] wrong(?) devirtualization

[redi at gcc dot gnu.org]

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

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #0)
>   Base *p = new B;

I think if there isn't really a B at this address, then the derived-to-base
conversion would be undefined.

So on that basis, the compiler should be allowed to assume that there really is
a B there.

[Bug ipa/109770] [11/12/13/14 Regression] wrong(?) devirtualization

[rguenth at gcc dot gnu.org]

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

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

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

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

[Bug ipa/109770] [11/12/13/14 Regression] wrong(?) devirtualization

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #9)
> (In reply to Richard Biener from comment #0)
> >   Base *p = new B;
> 
> I think if there isn't really a B at this address, then the derived-to-base
> conversion would be undefined.
> 
> So on that basis, the compiler should be allowed to assume that there really
> is a B there.

OK, that makes sense.