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); }
Started with r11-4745-g58c9de46541ade79, -fno-strict-aliasing is not needed.
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; }
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)
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.
> 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
(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.
(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.
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.
(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?
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.
(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.
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.
wrong-code should be now fixed, keeping open if Richard or Honza don't want to improve handling of non-replaceable delete operators.
The gcc patch also fixes original liferea+webkit-gtk-2.28.4 crash. Thank you!
(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.
Fixed. Any improvement shouldn't be tracked in this PR.