On 12/14/20 2:05 PM, Martin Sebor wrote:
> 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).
You're right. My bad. There are days when I hate unidiff. It's only
been 20 years of daily use and my brain still handles context diffs better.
jeff
