[Bug c++/94314] New: [10 Regression] Optimizing mismatched new/delete pairs

[Bug c++/94314] New: [10 Regression] Optimizing mismatched new/delete pairs

[glisse at gcc dot gnu.org]

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

            Bug ID: 94314
           Summary: [10 Regression] Optimizing mismatched new/delete pairs
           Product: gcc
           Version: 10.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: glisse at gcc dot gnu.org
  Target Milestone: ---

(originally posted at
https://gcc.gnu.org/legacy-ml/gcc-patches/2019-08/msg00276.html , I don't know
if we will do something about it, but it seems worth documenting it in
bugzilla)

Now that we optimize class-specific operator new/delete pairs (but you could do
the same with the global replacable ones as well):

#include <stdio.h>
int count = 0;
struct A {
  __attribute__((malloc,noinline))
  static void* operator new(unsigned long sz){++count;return ::operator
new(sz);}
  static void operator delete(void* ptr){--count;::operator delete(ptr);}
};
int main(){
  delete new A;
  printf("%d\n",count); // Should print 0.
}

If we do not inline anything, we can remove the pair and nothing touches count.
If we inline both new and delete, we can then remove the inner pair instead,
count increases and decreases, fine. If we inline only one of them, and DCE the
mismatched pair new/delete, we get something inconsistent (count is -1).

This seems to indicate we should check that the new and delete match somehow...

[Bug c++/94314] [10 Regression] Optimizing mismatched new/delete pairs

[jason at gcc dot gnu.org]

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

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

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

--- Comment #1 from Jason Merrill <jason at gcc dot gnu.org> ---
Martin?

[Bug c++/94314] [10 Regression] Optimizing mismatched new/delete pairs

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org
   Target Milestone|---                         |10.0

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Uh.  The frontend would need to help us here, no?

[Bug c++/94314] [10 Regression] Optimizing mismatched new/delete pairs since r10-2106-g6343b6bf3bb83c87

[marxin at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
            Summary|[10 Regression] Optimizing  |[10 Regression] Optimizing
                   |mismatched new/delete pairs |mismatched new/delete pairs
                   |                            |since
                   |                            |r10-2106-g6343b6bf3bb83c87
                 CC|                            |redi at gcc dot gnu.org
      Known to work|                            |9.3.0
   Last reconfirmed|                            |2020-03-25
           Assignee|unassigned at gcc dot gnu.org      |marxin at gcc dot gnu.org
             Status|UNCONFIRMED                 |ASSIGNED
      Known to fail|                            |10.0

--- Comment #3 from Martin Liška <marxin at gcc dot gnu.org> ---
Yes, I remember we discussed the topic about the user-provided new/delete
implementations. Can please Jason or Jonathan comment about the test-case?

[Bug c++/94314] [10 Regression] Optimizing mismatched new/delete pairs since r10-2106-g6343b6bf3bb83c87

[rguenth at gcc dot gnu.org]

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

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Martin Liška from comment #3)
> Yes, I remember we discussed the topic about the user-provided new/delete
> implementations. Can please Jason or Jonathan comment about the test-case?

The testcase is certainly valid.  The issue is we're matching new/delete
pairs by means of dataflow (the new resulting pointer is fed to the delete)
and identify new/delete by the decls flag.  But that doesn't catch the case
in this PR where there is a mismatch between the new/delete calls.

Now - the question is if whether class-specific operator new/delete even
have to "match" in this sense or how it's possible to "match" at all.

I think the frontend has to provide some "link" between the new/delete
decls to make this work (which is then quite heavy - an extra pointer in
function decls) :/

[Bug c++/94314] [10 Regression] Optimizing mismatched new/delete pairs since r10-2106-g6343b6bf3bb83c87

[glisse at gcc dot gnu.org]

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

--- Comment #5 from Marc Glisse <glisse at gcc dot gnu.org> ---
I don't think we need heavy machinery linking new and delete (and if we did I'd
be tempted to store it in some global table rather than in the nodes). The most
important case is the global replacable functions, for which we have a finite
list, and for those a few checks like not matching array with non-array
versions should do. For user overloads with attribute malloc (a gcc extension),
I would go with heuristics like both/neither being class members, being members
of the same class, etc. Although I am not quite sure how doable that is from
the middle-end, how much of that information is still available (I think it is
available in the mangled name, but demangling doesn't seem like a great idea).

[Bug c++/94314] [10 Regression] Optimizing mismatched new/delete pairs since r10-2106-g6343b6bf3bb83c87

[jason at gcc dot gnu.org]

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

--- Comment #6 from Jason Merrill <jason at gcc dot gnu.org> ---
(In reply to Marc Glisse from comment #5)
> I don't think we need heavy machinery linking new and delete (and if we did
> I'd be tempted to store it in some global table rather than in the nodes).
> The most important case is the global replacable functions, for which we
> have a finite list, and for those a few checks like not matching array with
> non-array versions should do. For user overloads with attribute malloc (a
> gcc extension), I would go with heuristics like both/neither being class
> members, being members of the same class, etc. Although I am not quite sure
> how doable that is from the middle-end, how much of that information is
> still available (I think it is available in the mangled name, but demangling
> doesn't seem like a great idea).

It should be sufficient to check whether they have the same DECL_CONTEXT.

[Bug c++/94314] [10 Regression] Optimizing mismatched new/delete pairs since r10-2106-g6343b6bf3bb83c87

[marxin at gcc dot gnu.org]

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

--- Comment #7 from Martin Liška <marxin at gcc dot gnu.org> ---
> It should be sufficient to check whether they have the same DECL_CONTEXT.

This seems to work. I'm testing a patch candidate.

[Bug c++/94314] [10 Regression] Optimizing mismatched new/delete pairs since r10-2106-g6343b6bf3bb83c87

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1

[Bug c++/94314] [10 Regression] Optimizing mismatched new/delete pairs since r10-2106-g6343b6bf3bb83c87

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Martin Liska <marxin@gcc.gnu.org>:

https://gcc.gnu.org/g:70df40cab6f268ba7f05c6d1421928cca0834ee3

commit r10-7628-g70df40cab6f268ba7f05c6d1421928cca0834ee3
Author: Martin Liska <mliska@suse.cz>
Date:   Wed Apr 8 17:16:55 2020 +0200

    Allow new/delete operator deletion only for replaceable.

            PR c++/94314
            * gimple.c (gimple_call_operator_delete_p): Rename to...
            (gimple_call_replaceable_operator_delete_p): ... this.
            Use DECL_IS_REPLACEABLE_OPERATOR_DELETE_P.
            * gimple.h (gimple_call_operator_delete_p): Rename to ...
            (gimple_call_replaceable_operator_delete_p): ... this.
            * tree-core.h (tree_function_decl): Add replaceable_operator
            flag.
            * tree-ssa-dce.c (mark_all_reaching_defs_necessary_1):
            Use DECL_IS_REPLACEABLE_OPERATOR_DELETE_P.
            (propagate_necessity): Use
gimple_call_replaceable_operator_delete_p.
            (eliminate_unnecessary_stmts): Likewise.
            * tree-streamer-in.c (unpack_ts_function_decl_value_fields):
            Pack DECL_IS_REPLACEABLE_OPERATOR.
            * tree-streamer-out.c (pack_ts_function_decl_value_fields):
            Unpack the field here.
            * tree.h (DECL_IS_REPLACEABLE_OPERATOR): New.
            (DECL_IS_REPLACEABLE_OPERATOR_NEW_P): New.
            (DECL_IS_REPLACEABLE_OPERATOR_DELETE_P): New.
            * cgraph.c (cgraph_node::dump): Dump if an operator is replaceable.
            * ipa-icf.c (sem_item::compare_referenced_symbol_properties):
Compare
            replaceable operator flags.
            PR c++/94314
            * decl.c (duplicate_decls): Duplicate also
DECL_IS_REPLACEABLE_OPERATOR.
            (cxx_init_decl_processing): Mark replaceable all implicitly defined
            operators.
            PR c++/94314
            * lto-common.c (compare_tree_sccs_1): Compare also
            DECL_IS_REPLACEABLE_OPERATOR.
            PR c++/94314
            * g++.dg/pr94314-2.C: New test.
            * g++.dg/pr94314-3.C: New test.
            * g++.dg/pr94314.C: New test.

[Bug c++/94314] [10 Regression] Optimizing mismatched new/delete pairs since r10-2106-g6343b6bf3bb83c87

[marxin at gcc dot gnu.org]

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

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

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

--- Comment #9 from Martin Liška <marxin at gcc dot gnu.org> ---
Fixed.

[Bug c++/94314] [10 Regression] Optimizing mismatched new/delete pairs since r10-2106-g6343b6bf3bb83c87

[glisse at gcc dot gnu.org]

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

--- Comment #10 from Marc Glisse <glisse at gcc dot gnu.org> ---
I am still getting -1 at -O2 for

#include <stdio.h>
#include <new>
int count = 0;
__attribute__((malloc,noinline))
void* operator new[](unsigned long sz){++count;return ::operator new(sz);}
void operator delete[](void* ptr)noexcept{--count;::operator delete(ptr);}
void operator delete[](void* ptr, std::size_t sz)noexcept{--count;::operator
delete(ptr, sz);}
int main(){
  delete[] new int[1];
  printf("%d\n",count); // Should print 0.
}

I am not aware of any code that breaks in practice, but it still looks strange.

[Bug c++/94314] [10 Regression] Optimizing mismatched new/delete pairs since r10-2106-g6343b6bf3bb83c87

[cvs-commit at gcc dot gnu.org]

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

--- Comment #11 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:6c9a711575d8fdf9e75f01d7a0e84d558600df40

commit r10-7638-g6c9a711575d8fdf9e75f01d7a0e84d558600df40
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Apr 8 20:04:01 2020 +0200

    testsuite: Fix up pr94314*.C tests [PR94314]

    The test FAIL everywhere where size_t is not unsigned long.  Fixed by
    using __SIZE_TYPE__ instead.

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

            PR c++/94314
            * g++.dg/pr94314.C (A::operator new, B::operator new, C::operator
new):
            Use __SIZE_TYPE__ instead of unsigned long.
            * g++.dg/pr94314-3.C (base::operator new, B::operator new):
Likewise.

[Bug c++/94314] [10 Regression] Optimizing mismatched new/delete pairs since r10-2106-g6343b6bf3bb83c87

[marxin at gcc dot gnu.org]

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

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

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

--- Comment #12 from Martin Liška <marxin at gcc dot gnu.org> ---
Let's reopen it again..

[Bug c++/94314] [10 Regression] Optimizing mismatched new/delete pairs since r10-2106-g6343b6bf3bb83c87

[cvs-commit at gcc dot gnu.org]

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

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Martin Liska <marxin@gcc.gnu.org>:

https://gcc.gnu.org/g:d7a65edb629a010f7ef907d457343abcb569fab7

commit r10-7758-gd7a65edb629a010f7ef907d457343abcb569fab7
Author: Martin Liska <mliska@suse.cz>
Date:   Thu Apr 16 15:39:22 2020 +0200

    List valid pairs for new and delete operators.

            PR c++/94314
            * cgraphclones.c (set_new_clone_decl_and_node_flags): Drop
            DECL_IS_REPLACEABLE_OPERATOR during cloning.
            * tree-ssa-dce.c (valid_new_delete_pair_p): New function.
            (propagate_necessity): Check operator names.

            PR c++/94314
            * g++.dg/pr94314.C: Do not use dg-additional-options
            and remove not needed stdio.h include.
            * g++.dg/pr94314-2.C: Likewise.
            * g++.dg/pr94314-3.C: Likewise.
            * g++.dg/pr94314-4.C: New test.

    Co-Authored-By: Jakub Jelinek <jakub@redhat.com>

[Bug c++/94314] [10 Regression] Optimizing mismatched new/delete pairs since r10-2106-g6343b6bf3bb83c87

[marxin at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
      Known to work|                            |10.0
      Known to fail|10.0                        |
             Status|REOPENED                    |RESOLVED

--- Comment #14 from Martin Liška <marxin at gcc dot gnu.org> ---
Now it should be fixed.

[Bug c++/94314] [10 Regression] Optimizing mismatched new/delete pairs since r10-2106-g6343b6bf3bb83c87

[cvs-commit at gcc dot gnu.org]

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

--- Comment #15 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:61b58e7fa5aea3ef0c7db2d9f75d17d65dff56a3

commit r10-7766-g61b58e7fa5aea3ef0c7db2d9f75d17d65dff56a3
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Apr 17 09:01:08 2020 +0200

    testsuite: Fix up test language requirements [PR94314]

    With c++11 one gets:
    Excess errors:
    .../testsuite/g++.dg/pr94314-4.C:19:28: error: too many arguments to
function 'void operator delete(void*)'
    because C++ sized deallocation is a C++14 feature.

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

            PR c++/94314
            * g++.dg/pr94314-4.C: Require c++14 rather than c++11.