[Bug c++/98130] New: [11 regression] placement new fails on webkit-gtk-2.28.4

[Bug c++/98130] New: [11 regression] placement new fails on webkit-gtk-2.28.4

[slyfox at gcc dot gnu.org]

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

            Bug ID: 98130
           Summary: [11 regression] placement new fails on
                    webkit-gtk-2.28.4
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: slyfox at gcc dot gnu.org
  Target Milestone: ---

Initial problem is observed on liferea crash at startup against
webkit-gtk-2.28.4 built with gcc-11.

If I reduced original source correctly here is minimized version of the crash:

//$ cat a.cc
/*

  $ g++-10.2.0 -O1 -fno-strict-aliasing a.cc -o a-10 && ./a-10
  &a=0x7ffc83475894
  $ g++-11.0.0 -O1 -fno-strict-aliasing a.cc -o a-11 && ./a-11
  Illegal instruction     (core dumped) ./a-11

*/

#if 0
#  include <memory>
#else
inline void* operator new(unsigned long, void* __p) { return __p; }
#endif

#include <stdio.h>

typedef int* T;

static T storage;
static T* p = &storage;

// '__attribute__((__always_inline__))' seems to be the trigger
static inline __attribute__((__always_inline__)) void append(T value)
{
    new (p) T(value);
}

int main() {
    int a;
    append(&a);

    if (!*p)
        __builtin_trap();
    fprintf(stderr, "&a=%p\n", *p);
}

[Bug c++/98130] [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79

[marxin at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
            Summary|[11 regression] placement   |[11 regression] placement
                   |new fails on                |new fails on
                   |webkit-gtk-2.28.4           |webkit-gtk-2.28.4 since
                   |                            |r11-4745-g58c9de46541ade79
   Target Milestone|---                         |11.0
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |marxin at gcc dot gnu.org
           Keywords|                            |wrong-code
             Status|UNCONFIRMED                 |NEW
           Priority|P3                          |P1
   Last reconfirmed|                            |2020-12-04

--- Comment #1 from Martin Liška <marxin at gcc dot gnu.org> ---
Started with r11-4745-g58c9de46541ade79, -fno-strict-aliasing is not needed.

[Bug c++/98130] [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79

[rguenth at gcc dot gnu.org]

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

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Confirmed.  The issue is that placement new is _not_ __attribute__((malloc)),
it makes PTA consider the object not escaping and then we have DSE do

;; Function append (_ZL6appendPi, funcdef_no=1, decl_uid=2355, cgraph_uid=2,
symbol_order=3)

  Deleted dead store: MEM[(int * *)_4] = value_5(D);

__attribute__((always_inline))
void append (int * value)
{
  void * _2;
  void * _4;

  <bb 2> :
  _2 = p;
  _4 = operator new (8, _2);
  return;

}

[Bug c++/98130] [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79

[jakub at gcc dot gnu.org]

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

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> ---
So, shouldn't the code match what the comment says?
  /* If the call is to a replaceable operator delete and results
     from a delete expression as opposed to a direct call to
     such operator, then we can treat it as free.  */
There is no check that it is a replaceable operator, that would mean
testing also
  && DECL_IS_REPLACEABLE_OPERATOR (fndecl)

[Bug c++/98130] [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
That would mean:

--- gcc/testsuite/g++.dg/opt/pr98130.C.jj       2020-12-04 12:30:11.510988404
+0100
+++ gcc/testsuite/g++.dg/opt/pr98130.C  2020-12-04 12:33:05.663028984 +0100
@@ -0,0 +1,25 @@
+// PR c++/98130
+// { dg-do run { target c++11 } }
+// { dg-options "-O2" }
+
+#include <new>
+
+typedef int *T;
+
+static unsigned char storage[sizeof (T)] alignas (T);
+static T *p = (T *) storage;
+
+static inline __attribute__((__always_inline__)) void
+foo (T value)
+{
+  new (p) T(value);
+}
+
+int
+main ()
+{
+  int a;
+  foo (&a);
+  if (!*p)
+    __builtin_abort ();
+}
--- gcc/gimple.c.jj     2020-11-26 01:14:47.528081989 +0100
+++ gcc/gimple.c        2020-12-04 12:34:44.865911907 +0100
@@ -1514,11 +1514,12 @@ gimple_call_fnspec (const gcall *stmt)
      such operator, then we can treat it as free.  */
   if (fndecl
       && DECL_IS_OPERATOR_DELETE_P (fndecl)
+      && DECL_IS_REPLACEABLE_OPERATOR (fndecl)
       && gimple_call_from_new_or_delete (stmt))
     return ".co ";
   /* Similarly operator new can be treated as malloc.  */
   if (fndecl
-      && DECL_IS_OPERATOR_NEW_P (fndecl)
+      && DECL_IS_REPLACEABLE_OPERATOR_NEW_P (fndecl)
       && gimple_call_from_new_or_delete (stmt))
     return "mC";
   return "";

but the testcase is miscompiled even with that change, and we don't really
return ".co " nor "mC" on the testcase.

[Bug c++/98130] [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79

[hubicka at ucw dot cz]

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

--- Comment #5 from Jan Hubicka <hubicka at ucw dot cz> ---
> So, shouldn't the code match what the comment says?
>   /* If the call is to a replaceable operator delete and results
>      from a delete expression as opposed to a direct call to
>      such operator, then we can treat it as free.  */
> There is no check that it is a replaceable operator, that would mean
> testing also
>   && DECL_IS_REPLACEABLE_OPERATOR (fndecl)

I copied the test from find_func_aliases_for_call (including the
comment).  I will look into what is happening here.

Honza

[Bug c++/98130] [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79

[rguenth at gcc dot gnu.org]

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

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jan Hubicka from comment #5)
> > So, shouldn't the code match what the comment says?
> >   /* If the call is to a replaceable operator delete and results
> >      from a delete expression as opposed to a direct call to
> >      such operator, then we can treat it as free.  */
> > There is no check that it is a replaceable operator, that would mean
> > testing also
> >   && DECL_IS_REPLACEABLE_OPERATOR (fndecl)
> 
> I copied the test from find_func_aliases_for_call (including the
> comment).  I will look into what is happening here.
> 
> Honza

But it's only used for delete there.

[Bug c++/98130] [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79

[rguenth at gcc dot gnu.org]

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

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #4)
> That would mean:
> 
> --- gcc/testsuite/g++.dg/opt/pr98130.C.jj	2020-12-04 12:30:11.510988404 +0100
> +++ gcc/testsuite/g++.dg/opt/pr98130.C	2020-12-04 12:33:05.663028984 +0100
> @@ -0,0 +1,25 @@
> +// PR c++/98130
> +// { dg-do run { target c++11 } }
> +// { dg-options "-O2" }
> +
> +#include <new>
> +
> +typedef int *T;
> +
> +static unsigned char storage[sizeof (T)] alignas (T);
> +static T *p = (T *) storage;
> +
> +static inline __attribute__((__always_inline__)) void
> +foo (T value)
> +{
> +  new (p) T(value);
> +}
> +
> +int
> +main ()
> +{
> +  int a;
> +  foo (&a);
> +  if (!*p)
> +    __builtin_abort ();
> +}
> --- gcc/gimple.c.jj	2020-11-26 01:14:47.528081989 +0100
> +++ gcc/gimple.c	2020-12-04 12:34:44.865911907 +0100
> @@ -1514,11 +1514,12 @@ gimple_call_fnspec (const gcall *stmt)
>       such operator, then we can treat it as free.  */
>    if (fndecl
>        && DECL_IS_OPERATOR_DELETE_P (fndecl)
> +      && DECL_IS_REPLACEABLE_OPERATOR (fndecl)
>        && gimple_call_from_new_or_delete (stmt))
>      return ".co ";
>    /* Similarly operator new can be treated as malloc.  */
>    if (fndecl
> -      && DECL_IS_OPERATOR_NEW_P (fndecl)
> +      && DECL_IS_REPLACEABLE_OPERATOR_NEW_P (fndecl)
>        && gimple_call_from_new_or_delete (stmt))
>      return "mC";
>    return "";
> 
> but the testcase is miscompiled even with that change, and we don't really
> return ".co " nor "mC" on the testcase.

The fix works for me?  Note I think we don't need the extra check on
the delete case.

[Bug c++/98130] [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79

[jakub at gcc dot gnu.org]

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Oops, yes, dunno why it didn't work for me before, confirmed now that it works
with the patch and fails without.

I think we want it even for the operator delete case, I believe the C++
standard only constraints how the replaceable operators work, not arbitrary
user operator new/delete/new[]/delete[] operators.

[Bug c++/98130] [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79

[marxin at gcc dot gnu.org]

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

--- Comment #9 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #8)
> Oops, yes, dunno why it didn't work for me before, confirmed now that it
> works with the patch and fails without.
> 
> I think we want it even for the operator delete case, I believe the C++
> standard only constraints how the replaceable operators work, not arbitrary
> user operator new/delete/new[]/delete[] operators.

I bet it's pretty similar to what we have for -fallocation-dce:
valid_new_delete_pair_p?

[Bug c++/98130] [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79

[jakub at gcc dot gnu.org]

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

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
valid_new_delete_pair_p checks the extra constraints C++ has, like that if you
allocate with a particular replaceable operator new, you can free it only with
those and those replaceable operator delete and not others.

[Bug c++/98130] [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79

[rguenth at gcc dot gnu.org]

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

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #8)
> Oops, yes, dunno why it didn't work for me before, confirmed now that it
> works with the patch and fails without.
> 
> I think we want it even for the operator delete case, I believe the C++
> standard only constraints how the replaceable operators work, not arbitrary
> user operator new/delete/new[]/delete[] operators.

Note we already require to see a new/delete _expression_ and IIRC any delete
expression will make the contents undefined (see my discussion with Jason on
this topic).  But yes, we have to preserve other side-effects so the ".c"
part is probably bogus, the PTA code treats it as "..X ", "..o " would
still make 'this' receive pointers.  So we probably cannot model 'delete'
beavior exactly but "..o " is probably good enough.  Attempts to break it
welcome, of course.

[Bug c++/98130] [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79

[cvs-commit at gcc dot gnu.org]

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

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:78c4a9feceaccf487516aa1eff417e0741556e10

commit r11-5748-g78c4a9feceaccf487516aa1eff417e0741556e10
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Dec 4 19:10:56 2020 +0100

    gimple: Return fnspec only for replaceable new/delete operators called from
new/delete [PR98130]

    As mentioned in the PR, we shouldn't treat non-replaceable operator
    new/delete (e.g. with the placement new) as replaceable ones.

    There is some pending discussion that perhaps operator delete called from
    delete if not replaceable should return some other fnspec, but can we
handle
    that incrementally, fix this wrong-code and then deal with a missed
    optimization?  I really don't know what exactly should be returned.

    2020-12-04  Jakub Jelinek  <jakub@redhat.com>

            PR c++/98130
            * gimple.c (gimple_call_fnspec): Only return ".co " for replaceable
            operator delete or ".mC" for replaceable operator new called from
            new/delete.

            * g++.dg/opt/pr98130.C: New test.

[Bug c++/98130] [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79

[jakub at gcc dot gnu.org]

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

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
wrong-code should be now fixed, keeping open if Richard or Honza don't want to
improve handling of non-replaceable delete operators.

[Bug c++/98130] [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79

[slyfox at gcc dot gnu.org]

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

--- Comment #14 from Sergei Trofimovich <slyfox at gcc dot gnu.org> ---
The gcc patch also fixes original liferea+webkit-gtk-2.28.4 crash. Thank you!

[Bug c++/98130] [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

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

--- Comment #15 from Jason Merrill <jason at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #11)

Yes, any delete-expression ends the lifetime of the pointed-to object before
calling the delete operator, so it seems appropriate to say that it doesn't
escape.

[Bug c++/98130] [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #16 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed.  Any improvement shouldn't be tracked in this PR.