[Bug c++/98130] [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98130 Richard Biener changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #16 from Richard Biener --- Fixed. Any improvement shouldn't be tracked in this PR.
[Bug c++/98130] [11 regression] placement new fails on webkit-gtk-2.28.4 since r11-4745-g58c9de46541ade79
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98130 Jason Merrill changed: What|Removed |Added CC||jason at gcc dot gnu.org --- Comment #15 from Jason Merrill --- (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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98130 --- Comment #14 from Sergei Trofimovich --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98130 --- Comment #13 from Jakub Jelinek --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98130 --- Comment #12 from CVS Commits --- The master branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:78c4a9feceaccf487516aa1eff417e0741556e10 commit r11-5748-g78c4a9feceaccf487516aa1eff417e0741556e10 Author: Jakub Jelinek 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 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98130 --- Comment #11 from Richard Biener --- (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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98130 --- Comment #10 from Jakub Jelinek --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98130 --- Comment #9 from Martin Liška --- (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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98130 --- Comment #8 from Jakub Jelinek --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98130
--- Comment #7 from Richard Biener ---
(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.C2020-12-04 12:33:05.663028984
> +0100
> @@ -0,0 +1,25 @@
> +// PR c++/98130
> +// { dg-do run { target c++11 } }
> +// { dg-options "-O2" }
> +
> +#include
> +
> +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 ();
> + 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98130 --- Comment #6 from Richard Biener --- (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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98130 --- Comment #5 from Jan Hubicka --- > 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98130
--- Comment #4 from Jakub Jelinek ---
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
+
+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 ();
+ if (!*p)
+__builtin_abort ();
+}
--- gcc/gimple.c.jj 2020-11-26 01:14:47.528081989 +0100
+++ gcc/gimple.c2020-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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98130 Jakub Jelinek changed: What|Removed |Added CC||jakub at gcc dot gnu.org --- Comment #3 from Jakub Jelinek --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98130
--- Comment #2 from Richard Biener ---
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;
:
_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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98130 Martin Liška 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 --- Started with r11-4745-g58c9de46541ade79, -fno-strict-aliasing is not needed.
