On 12/13/20 10:23 AM, Jeff Law wrote:
On 12/8/20 1:46 PM, Martin Sebor wrote:
PR 98160 reports an ICE in pretty printer code called from the newly
added -Wmismatched-new-delete. The ICE is just a simple oversight,
but much more interesting is the warning issued for the test case.
It highlights an issue I didn't consider in the initial implementation:
that inlining one of a pair of allocation/deallocation functions but
not the other might lead to false positives when the inlined function
calls another allocator that the deallocator isn't associated with.
In addition, tests for the changes exposed the overly simplistic
nature of the detection of calls to mismatched forms of operator
new and delete which fails to consider member operators, also
resulting in false positives.
Finally, in a comment on the initial implementation Jonathan notes
that the -Wmismatched-new-delete warning should trigger not only
in user code but also in libstdc++ functions inlined into user code.
I thought I had done that but as it turns out, the "standard code
sequence" I put in place isn't sufficient to make this work.
The attached changes avoid the false positives a) by ignoring (with
a warning) the new form of the malloc attribute on inline functions,
and disabling the inlining of others by implicitly adding attribute
noinline to their declaration, and b) by making more robust
the detection of mismatched operators new and delete. Furthermore,
the patch also arranges for the warning to trigger even for inlined
calls to functions defined in system headers.
To make review a little (marginally) easier the change are two files:
1) gcc-98166-1.diff: introduces valid_new_delete_pair_p and
tree_inlined_location.
2) gcc-98166-2.diff: adjusts the atrribute/warning implementation .
Tested on x86_64-linux.
Martin
gcc-98166-1.diff
Introduce an overload of valid_new_delete_pair_p and tree_inlined_location.
gcc/ChangeLog:
* tree-ssa-dce.c (valid_new_delete_pair_p): Factor code out into
valid_new_delete_pair_p in tree.c.
* tree.c (tree_inlined_location): Define new function.
(valid_new_delete_pair_p): Define.
* tree.h (tree_inlined_location): Declare.
(valid_new_delete_pair_p): Declare.
OK
gcc-98166-2.diff
Correct/improve maybe_emit_free_warning (PR middle-end/98166, PR c++/57111, PR
middle-end/98160).
Resolves:
PR middle-end/98166 - bogus -Wmismatched-dealloc on user-defined allocator and
inlining
PR c++/57111 - 57111 - Generalize -Wfree-nonheap-object to delete
PR middle-end/98160 - ICE in default_tree_printer at gcc/tree-diagnostic.c:270
gcc/ChangeLog:
PR middle-end/98166
PR c++/57111
PR middle-end/98160
* builtins.c (call_dealloc_p): Remove unused function.
(new_delete_mismatch_p): Call valid_new_delete_pair_p and rework.
(matching_alloc_calls_p): Handle built-in deallocation functions.
(warn_dealloc_offset): Corrct the handling of user-defined operators
delete.
(maybe_emit_free_warning): Avoid assuming expression is a decl.
Simplify.
* doc/extend.texi (attribute malloc): Update.
gcc/c-family/ChangeLog:
PR middle-end/98166
PR c++/57111
PR middle-end/98160
* c-attribs.c (handle_malloc_attribute): Implicitly add attribute
noinline to functions not declared inline and warn on those.
libstdc++-v3/ChangeLog:
* testsuite/ext/vstring/requirements/exception/basic.cc: Suppress
a false positive warning.
*
testsuite/ext/vstring/requirements/exception/propagation_consistent.cc:
Same.
gcc/testsuite/ChangeLog:
PR middle-end/98166
PR c++/57111
PR middle-end/98160
* g++.dg/warn/Wmismatched-dealloc-2.C: Adjust test of expected warning.
* g++.dg/warn/Wmismatched-new-delete.C: Same.
* gcc.dg/Wmismatched-dealloc.c: Same.
* c-c++-common/Wfree-nonheap-object-2.c: New test.
* c-c++-common/Wfree-nonheap-object-3.c: New test.
* c-c++-common/Wfree-nonheap-object.c: New test.
* c-c++-common/Wmismatched-dealloc.c: New test.
* g++.dg/warn/Wfree-nonheap-object-3.C: New test.
* g++.dg/warn/Wfree-nonheap-object-4.C: New test.
* g++.dg/warn/Wmismatched-new-delete-2.C: New test.
OK, but there's a follow-up as noted below:
diff --git a/gcc/testsuite/g++.dg/warn/Wmismatched-dealloc-2.C b/gcc/testsuite/g++.dg/warn/Wmismatched-dealloc-2.C
index 7ecc99a325c..3aea02fa63d 100644
--- a/gcc/testsuite/g++.dg/warn/Wmismatched-dealloc-2.C
+++ b/gcc/testsuite/g++.dg/warn/Wmismatched-dealloc-2.C
Please fix the non-unique testnames in this file. This can be done as a
separate follow-up patch as they're pre-existing.
diff --git a/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete.C
b/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete.C
index ed1090be5c5..fc07149995d 100644
--- a/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete.C
+++ b/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete.C
And in this file.
I made an additional tweak to better handle Glibc headers and
committed r11-6028. I don't see any non-unique names in either
of the tests (or any others in the patch).
Martin
