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

[Jeff Law <law@redhat.com>]

On 11/3/20 4:56 PM, Martin Sebor via Gcc-patches wrote:
> Attached is a simple middle end implementation of detection of
> mismatched pairs of calls to C++ new and delete, along with
> a substantially enhanced implementation of -Wfree-nonheap-object.
> The latter option has been in place since 2011 but detected only
> the most trivial bugs.
>
> Unlike the Clang -Wmismatched-new-delete which diagnoses
> declarations of "overloaded operator new() and operator delete()
> functions that do not have a corresponding free store function
> defined within the same scope", this patch detects mismatches
> between calls to allocation and deallocation functions, such as
> calling free() on the result of new, of delete on the result of
> array new.  The functionality provided by Clang can be added on
> top of what this feature does and since they are so close I think
> it's fine to have both under the same option (a new level could
> be introduced to distinguish the two).
>
> The -Wfree-nonheap-object enhancement lets the warning detect all
> calls to free, realloc, or C++ delete, with pointers that can be
> proven not to point to the first byte of an allocated object.
>
> The patch relies on the well-tested compute_objsize() function
> for the determination of pointer provenance and makes use of
> the changes in the following patch submitted for review just
> yesterday:
> https://gcc.gnu.org/pipermail/gcc-patches/2020-November/557807.html
>
> As usual, I tested on x86_64-linux with Glibc & Binutils/GDB with
> no new false positives.
>
> Martin
>
> PS A few words on the implementation choices:
>
> The new code is in builtins.c only because -Wfree-nonheap-object
> is there.  I still plan to move all of the invalid access checking
> code into its own module or pass at some point but I didn't want
> to make this improvement contingent on that restructuring.
> Even though it's all in builtins.c, the code is called from calls.c.
> This is so that simple mismatches can be diagnosed even when free
> isn't handed in builtins.c (i.e., without optimization).
> The warning makes no attempt to analyze the CFG or handle
> conditional mismatches.  That will have to wait until the code
> is moved to a GIMPLE pass.
>
> gcc-90629.diff
>
> PR c++/90629 - Support for -Wmismatched-new-delete
>
> gcc/ChangeLog:
>
> 	PR c++/90629
> 	* builtins.c (access_ref::access_ref): Initialize new member.
> 	(compute_objsize): Use access_ref::deref.  Handle simple pointer
> 	assignment.
> 	(expand_builtin): Remove handling of the free built-in.
> 	(find_assignment_location): New function.
> 	(gimple_call_alloc_p): Same.
> 	(gimple_call_dealloc_argno): Same.
> 	(gimple_call_dealloc_p): Same.
> 	(matching_alloc_calls_p): Same.
> 	(warn_dealloc_offset): Same.
> 	* builtins.h (struct access_ref): Declare new member.
> 	(maybe_emit_free_warning): Make extern.  Make use of access_ref.
> 	Handle -Wmismatched-new-delete.
> 	* calls.c (initialize_argument_information): Call
> 	maybe_emit_free_warning.
> 	* doc/invoke.texi (-Wfree-nonheap-object): Expand documentation.
> 	(-Wmismatched-new-delete): Document new option.
>
> gcc/c-family/ChangeLog:
>
> 	PR c++/90629
> 	* c.opt (-Wmismatched-new-delete): New option.
>
> gcc/testsuite/ChangeLog:
>
> 	PR c++/90629
> 	* g++.dg/warn/delete-array-1.C: Add expected warning.
> 	* g++.old-deja/g++.other/delete2.C: Add expected warning.
> 	* g++.dg/warn/Wfree-nonheap-object-2.C: New test.
> 	* g++.dg/warn/Wfree-nonheap-object.C: New test.
> 	* g++.dg/warn/Wmismatched-new-delete.C: New test.
> 	* gcc.dg/Wfree-nonheap-object-2.c: New test.
> 	* gcc.dg/Wfree-nonheap-object-3.c: New test.
> 	* gcc.dg/Wfree-nonheap-object.c: New test.

So do we need to reconcile with David M's patch that adds the
"deallocated_by" attribute.  In that thread I raised the question about
using the same attribute to track both pointers as well as things like
file descriptors.  It looks like this patch ignores everything except
builtins/language intrinsics as allocation functions.  ISTM that would
need to be fixed for David's patch to be useful here, right?


As we discussed privately, this can trigger false positives,
particularly for unoptimized code  as we've seen in Fedora testing.  But
I'm not terribly worried about these.

I haven't seen anything failing with a valid error because of this, but
that's because the packages where it's triggering aren't compiling with
-Werror.  I did some quick grepping of the logs and I think I found
instances of each of the new warnings.


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 :-)






>
> 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?


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?



+
> +  gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
> +  gsi_next_nondebug (&gsi);
> +  if (gimple *next_stmt = gsi_stmt (gsi))
> +    {
> +      if (gimple_has_location (next_stmt))
> +	return gimple_location (next_stmt);
> +    }
> +  else
> +    {
> +      gsi = gsi_for_stmt (stmt);
> +      gsi_prev_nondebug (&gsi);
> +      if (gimple *prev_stmt = gsi_stmt (gsi))
> +	if (gimple_has_location (prev_stmt))
> +	  return gimple_location (prev_stmt);
> +    }
> +
> +  /* As a last resort fallback, return the location of the closing
> +     curly brace.  */
> +  return cfun->function_end_locus;
> +}

And if you clean up the code above, then all this becomes pointless, no?




Jeff