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 5D7FE3951822 for ; Wed, 2 Dec 2020 00:09:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 5D7FE3951822 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-95-wKI-qFcMP_icFaLCgZSZmg-1; Tue, 01 Dec 2020 19:09:40 -0500 X-MC-Unique: wKI-qFcMP_icFaLCgZSZmg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 7585D9CC2A; Wed, 2 Dec 2020 00:09:39 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-145.phx2.redhat.com [10.3.112.145]) by smtp.corp.redhat.com (Postfix) with ESMTP id 26AAB5D705; Wed, 2 Dec 2020 00:09:39 +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> <2fff96f2-5e6d-03b5-3a18-3a89afa7fa14@redhat.com> <585abfb6-d9a8-339d-2cca-7e95a96c82f3@gmail.com> From: Jeff Law Message-ID: Date: Tue, 1 Dec 2020 17:09:38 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0 MIME-Version: 1.0 In-Reply-To: <585abfb6-d9a8-339d-2cca-7e95a96c82f3@gmail.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Content-Language: en-US X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, 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 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: Wed, 02 Dec 2020 00:09:46 -0000 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 ‘’ 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