Re: [PATCH] add -Wmismatched-new-delete to middle end (PR 90629)

[Jeff Law <law@redhat.com>]

On 11/17/20 11:02 AM, Martin Sebor wrote:
>
>>
>> If you're interested
>> torsion.usersys.redhat.com:/opt/notnfs/law/WARNINGS contains all the
>> lines with "warning:" from all the Fedora test builds. Warning (pun
>> intended), it's big...  10G, so don't try to download it :-)  But it
>> is faster than find | xargs zgrep across all the build logs :-)
>
> There are quite a few (411 instances of -Wmismatched-new-delete to
> be exact) but without more context (at least the informational notes)
> they're hard to analyze.  I looked at just the first one and it points
> to this bug:
>
> ./gengetopt/builds/80/log.gz:gengetopt.cc:581:12: warning: 'free'
> called on pointer returned from a mismatched allocation function
> [-Wmismatched-new-delete]
>
> gengetopt_create_option (gengetopt_option *&n, const char * long_opt,
> char short_opt,
>                       const char * desc,
>                       int type, int flagstat, int required,
>                       const char * default_value,
>                       const char * group_value,
>                       const char * mode_value,
>                       const char * type_str,
>                       const AcceptedValues *acceptedvalues,
>                       int multiple,
>                       int argoptional)
> {
>   if ((long_opt == NULL) ||
>       (long_opt[0] == 0) ||
>       (desc == NULL))
>     return FOUND_BUG;
>
>   n = new gengetopt_option;         <<< allocate by new
>   if (n == NULL)
>     return NOT_ENOUGH_MEMORY;
>
>   // here we will set required anyway
>   n->required_set = true;
>
>   n->long_opt = strdup (long_opt);
>   if (n->long_opt == NULL)
>     {
>       free (n);                     <<< deallocate by free
>       return NOT_ENOUGH_MEMORY;
>     }
>
> Based on what others have said about what some static analyzers
> report I expect this to be a common bug (the "operator delete"
> message accounts for 336 instances out of the 411).
Yea, I suspect there's a lot of these lying around and probably will
continue to do so as long as folks aren't using -Werror.  Hence my
desire to have an opt-in mechanism in Fedora that turns on -Werror
within redhat-rpm-config for opted-in packages.

>
>>> diff --git a/gcc/builtins.c b/gcc/builtins.c
>>> index 5d60eab6ba2..1b8a5b82dac 100644
>>> --- a/gcc/builtins.c
>>> +++ b/gcc/builtins.c
>>> @@ -12589,30 +12594,336 @@ maybe_emit_sprintf_chk_warning (tree exp,
>>> enum built_in_function fcode)
>>>           access_write_only);
>>>   }
>>>   -/* Emit warning if a free is called with address of a variable.  */
>>> +/* Return the location of the assignment STMT if it has one, or
>>> another
>>> +   assignment on the chain that has one.  Used to improve the location
>>> +   of informational notes.  */
>>>   -static void
>>> +static location_t
>>> +find_assignment_location (tree var)
>>> +{
>>> +  gimple *stmt = SSA_NAME_DEF_STMT (var);
>>> +
>>> +  for (gimple *asgn = stmt; ; )
>>> +    {
>>> +      if (gimple_has_location (asgn))
>>> +    return gimple_location (asgn);
>>> +
>>> +      if (!is_gimple_assign (asgn))
>>> +    break;
>>> +
>>> +      tree rhs = gimple_assign_rhs1 (asgn);
>>> +      if (TREE_CODE (rhs) != SSA_NAME)
>>> +    break;
>>> +
>>> +      asgn = SSA_NAME_DEF_STMT (rhs);
>>> +    }
>>
>> What is this code for ^^^
>>
>>
>> Under what conditions does the assignment not have a location?
>> Perhaps if it's a default definition?
>
> I've seen all sorts of assignments with no location, including
> ASSERT_EXPRs, BIT_AND_EXPRs, as well as MIN/MAX_EXPs, clobber
> statements and others.  I didn't try to understand the pattern
> behind them.
I'd rather not include that hunk and instead xfail any tests affected by
the missing locations until such time as we fix them rather than just
papering over the problem.  Fixing locations helps diagnostics, our
ability to suppress them via pragmas, etc.    So it's in our best
interest to fix these issues.

>
>> All the nonsense walking up the use-def chain seems unnecessary and
>> probably misleading when we actually issue the diagnostic. Why are
>> you going through the trouble to do that?
>
> The "nonsense" is me doing my best to provide at least some context
> for warnings that might otherwise not have any and be harder to track
> down to the root cause.  A message like:
>
>   warning: ‘free’ called on pointer ‘<unknown>’ with nonzero offset
>
> with nothing else for a function hundreds of lines long makes it
> harder to track down the cause of the problem than with a note
> pointing to at least a location of the closest assignment to
> the unknown pointer.
>
> That said, in this patch the function is only used for constants
> and AFAICS, doesn't get exercised by any tests so it can probably
> be removed with no adverse effect.  I do expect it to need to come
> back in some form because of the missing location problem.  (It
> comes from a bigger patch in progress where it's used more
> extensively.)
ACK.  So let's avoid it for now and try to tackle the missing location
problems as we trip over them.

OK with those bits removed.

jeff