From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 95FE23851C0B for ; Tue, 17 Nov 2020 00:54:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 95FE23851C0B Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-37-cLLy7IY0Pr2cMHvTknxJuQ-1; Mon, 16 Nov 2020 19:54:42 -0500 X-MC-Unique: cLLy7IY0Pr2cMHvTknxJuQ-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 17723186841F; Tue, 17 Nov 2020 00:54:41 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-176.phx2.redhat.com [10.3.112.176]) by smtp.corp.redhat.com (Postfix) with ESMTP id BA02610013C4; Tue, 17 Nov 2020 00:54:40 +0000 (UTC) Subject: Re: [PATCH] add -Wmismatched-new-delete to middle end (PR 90629) To: Martin Sebor , gcc-patches References: <9cf539a3-2cf8-96df-9747-4c536fc524df@gmail.com> From: Jeff Law Message-ID: <2fff96f2-5e6d-03b5-3a18-3a89afa7fa14@redhat.com> Date: Mon, 16 Nov 2020 17:54:40 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.1 MIME-Version: 1.0 In-Reply-To: <9cf539a3-2cf8-96df-9747-4c536fc524df@gmail.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, HTML_MESSAGE, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Nov 2020 00:54:48 -0000 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