From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x841.google.com (mail-qt1-x841.google.com [IPv6:2607:f8b0:4864:20::841]) by sourceware.org (Postfix) with ESMTPS id E8D973896826 for ; Tue, 17 Nov 2020 18:02:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E8D973896826 Received: by mail-qt1-x841.google.com with SMTP id g17so16371374qts.5 for ; Tue, 17 Nov 2020 10:02:17 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=5J5vCdI97Gk4ynIbkih0NJTVfj9QPetwfGm5BR4SCm0=; b=NXM0W4/BbrAHR7zBWiNPdEPfHcoiVeGJAOubbw2xE7PAKgl2V5lJVitQkSQxk/Gl8N cdSKAVoLeUOjRsvvkdkV6brP62gzVQNes5RZqw8qHiMq5gTITNwfq7/LqTieil/6kFXp 48sZlvkezkeW9VCts6S6reqQKeJfKL5ecVDO0B5mlnX9NMaoWUmZEFIrbzbvw7uWIS79 zsBi44y8q7M8eiiFClBF6k09Xspd6eXbCw3c+bMGf9qqYNN7GNxoTQTvVf1kjOH/I+0z VlCN0Yx1m957Kbz9pzAOzkCdm6BhrsGiYCQRG4BWsCuW4Sc7/Q1xIKS/Om7Ly2zznmrt cGkQ== X-Gm-Message-State: AOAM5329yJXDPUJkK/J8r83+zO6zbv0gUqhjEYNJSqkmXR6Zg2X6y6Qg tTnnQmECxmXgyftGEBclMImmJ0Ogy2M= X-Google-Smtp-Source: ABdhPJz3Oe2YeedmejHdtKkQ56I7p8csiXY+knsctrFH+y9aaOkhS0Lx9wmJ7w9fDymvoQKhk/RV8w== X-Received: by 2002:a05:622a:1c9:: with SMTP id t9mr812977qtw.289.1605636136745; Tue, 17 Nov 2020 10:02:16 -0800 (PST) Received: from [192.168.0.41] (174-16-106-146.hlrn.qwest.net. [174.16.106.146]) by smtp.gmail.com with ESMTPSA id z6sm13566247qti.88.2020.11.17.10.02.14 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 17 Nov 2020 10:02:15 -0800 (PST) Subject: Re: [PATCH] add -Wmismatched-new-delete to middle end (PR 90629) To: Jeff Law , gcc-patches References: <9cf539a3-2cf8-96df-9747-4c536fc524df@gmail.com> <2fff96f2-5e6d-03b5-3a18-3a89afa7fa14@redhat.com> From: Martin Sebor Message-ID: <585abfb6-d9a8-339d-2cca-7e95a96c82f3@gmail.com> Date: Tue, 17 Nov 2020 11:02:13 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <2fff96f2-5e6d-03b5-3a18-3a89afa7fa14@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, 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 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 18:02:20 -0000 On 11/16/20 5:54 PM, Jeff Law wrote: > > 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? Our emails have crossed mid air. I replied to your comments on David's patch here: https://gcc.gnu.org/pipermail/gcc-patches/2020-November/559309.html The work I point to there extends this warning to user-defined functions. I also gave some preliminary comments on David's initial RFC. It has some more background: https://gcc.gnu.org/pipermail/gcc-patches/2020-October/555667.html > 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 :-) 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). >> 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. > 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 ‘’ 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.) >> + 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? I'm not sure what you mean but as I said above, the function doesn't appear to be essential to this patch and can be removed. Martin