Re: [PATCH] teach compute_objsize about placement new (PR 100876)

[Martin Sebor <msebor@gmail.com>]

On 6/13/21 5:45 PM, Jeff Law wrote:
> 
> 
> On 6/2/2021 3:40 PM, Martin Sebor via Gcc-patches wrote:
>> The two forms of placement operator new defined in <new> return their
>> pointer argument and may not be displaced by user-defined functions.
>> But because they are ordinary (not built-in) functions this property
>> isn't reflected in their declarations alone, and there's no user-
>> level attribute to annotate them with.  When they are inlined
>> the property is transparent in the IL but when they are not (without
>> inlining such as -O0), calls to the operators appear in the IL and
>> cause -Wmismatched-new-delete to try to match them with the functions
>> called to deallocate memory.  When the pointer to the memory was
>> obtained from a function that matches the deallocator but not
>> the placement new, the warning falsely triggers.
>>
>> The attached patch solves this by detecting calls to placement new
>> and treating them the same as those to other pass-through calls (such
>> as memset).  In addition, it also teaches -Wfree-nonheap-object about
>> placement delete, for a similar reason as above.  Finally, it also
>> adds a test for attribute fn spec indicating a function returns its
>> argument.  It's not necessary for the fix (I had initially though
>> placement new might have the attribute) but it seems appropriate
>> to check.
>>
>> Tested on x86_64-linux.
>>
>> Martin
>>
>> gcc-100876.diff
>>
>> PR c++/100876 - -Wmismatched-new-delete should understand placement new when it's not inlined
>>
>> gcc/ChangeLog:
>>
>> 	PR c++/100876
>> 	* builtins.c (gimple_call_return_array): Check for attribute fn spec.
>> 	Handle calls to placement new.
>> 	(ndecl_dealloc_argno): Avoid placement delete.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR c++/100876
>> 	* g++.dg/warn/Wmismatched-new-delete-4.C: New test.
>> 	* g++.dg/warn/Wmismatched-new-delete-5.C: New test.
>> 	* g++.dg/warn/Wstringop-overflow-7.C: New test.
>> 	* g++.dg/warn/Wfree-nonheap-object-6.C: New test.
>> 	* g++.dg/analyzer/placement-new.C: Prune out expected warning.
>>
>> diff --git a/gcc/builtins.c b/gcc/builtins.c
>> index af1fe49bb48..fb0717a0248 100644
>> --- a/gcc/builtins.c
>> +++ b/gcc/builtins.c
>> @@ -5159,11 +5159,43 @@ static tree
>>   gimple_call_return_array (gimple *stmt, offset_int offrng[2],
>>   			  range_query *rvals)
>>   {
>> -  if (!gimple_call_builtin_p (stmt, BUILT_IN_NORMAL)
>> -      || gimple_call_num_args (stmt) < 1)
>> +  {
>> +    /* Check for attribute fn spec to see if the function returns one
>> +       of its arguments.  */
>> +    attr_fnspec fnspec = gimple_call_fnspec (as_a <gcall *>(stmt));
>> +    unsigned int argno;
>> +    if (fnspec.returns_arg (&argno))
>> +      {
>> +	offrng[0] = offrng[1] = 0;
>> +	return gimple_call_arg (stmt, argno);
>> +      }
>> +  }
>> +
>> +  if (gimple_call_num_args (stmt) < 1)
>>       return NULL_TREE;
> Nit.  You've got an unnecessary {} at the outer level of this hunk.
> 
> OK with the nit fixed.

That's intentional, to minimize the scope of of the two new local
variables.

Martin

> 
> THanks,
> Jeff
>