Bug 98130 - [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79
Summary: [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 11.0
: P1 normal
Target Milestone: 11.0
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2020-12-03 20:45 UTC by Sergei Trofimovich
Modified: 2021-01-19 11:59 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work: 10.2.0
Known to fail: 11.0
Last reconfirmed: 2020-12-04 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sergei Trofimovich 2020-12-03 20:45:13 UTC
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);
}
Comment 1 Martin Liška 2020-12-04 06:54:54 UTC
Started with r11-4745-g58c9de46541ade79, -fno-strict-aliasing is not needed.
Comment 2 Richard Biener 2020-12-04 07:19:26 UTC
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;

}
Comment 3 Jakub Jelinek 2020-12-04 11:27:41 UTC
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)
Comment 4 Jakub Jelinek 2020-12-04 11:37:39 UTC
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.
Comment 5 Jan Hubicka 2020-12-04 11:59:40 UTC
> 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
Comment 6 Richard Biener 2020-12-04 12:21:10 UTC
(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.
Comment 7 Richard Biener 2020-12-04 12:25:03 UTC
(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.
Comment 8 Jakub Jelinek 2020-12-04 12:33:50 UTC
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.
Comment 9 Martin Liška 2020-12-04 12:37:37 UTC
(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?
Comment 10 Jakub Jelinek 2020-12-04 12:48:33 UTC
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.
Comment 11 Richard Biener 2020-12-04 13:33:38 UTC
(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.
Comment 12 GCC Commits 2020-12-04 18:11:51 UTC
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.
Comment 13 Jakub Jelinek 2020-12-04 18:12:43 UTC
wrong-code should be now fixed, keeping open if Richard or Honza don't want to improve handling of non-replaceable delete operators.
Comment 14 Sergei Trofimovich 2020-12-04 19:49:59 UTC
The gcc patch also fixes original liferea+webkit-gtk-2.28.4 crash. Thank you!
Comment 15 Jason Merrill 2020-12-22 19:09:41 UTC
(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.
Comment 16 Richard Biener 2021-01-19 11:59:40 UTC
Fixed.  Any improvement shouldn't be tracked in this PR.