From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from troutmask.apl.washington.edu (troutmask.apl.washington.edu [128.95.76.21]) by sourceware.org (Postfix) with ESMTPS id 9BA84385702E; Mon, 17 Apr 2023 22:18:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9BA84385702E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=troutmask.apl.washington.edu Authentication-Results: sourceware.org; spf=none smtp.mailfrom=troutmask.apl.washington.edu Received: from troutmask.apl.washington.edu (localhost [127.0.0.1]) by troutmask.apl.washington.edu (8.17.1/8.17.1) with ESMTPS id 33HMISCs091102 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Mon, 17 Apr 2023 15:18:28 -0700 (PDT) (envelope-from sgk@troutmask.apl.washington.edu) Received: (from sgk@localhost) by troutmask.apl.washington.edu (8.17.1/8.17.1/Submit) id 33HMIRC6091101; Mon, 17 Apr 2023 15:18:27 -0700 (PDT) (envelope-from sgk) Date: Mon, 17 Apr 2023 15:18:27 -0700 From: Steve Kargl To: Bernhard Reutner-Fischer via Fortran Cc: gcc-patches@gcc.gnu.org, rep.dot.nop@gmail.com, Harald Anlauf , Bernhard Reutner-Fischer Subject: Re: ping Re: [PATCH 3/3] Fortran: Fix mpz and mpfr memory leaks Message-ID: Reply-To: sgk@troutmask.apl.washington.edu References: <20230402150515.40826-4-rep.dot.nop@gmail.com> <57162b6a-95e0-35c6-386a-3687908fce05@gmx.de> <20230417214750.539e74dc@nbbrfq> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230417214750.539e74dc@nbbrfq> X-Spam-Status: No, score=-7.6 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Mon, Apr 17, 2023 at 09:47:50PM +0200, Bernhard Reutner-Fischer via Fortran wrote: > Ping! > > Harald fixed the leak in set_exponent in the meantime. > As stated in the cover-letter, it was bootstrapped and regtested > without regression on x86_64-foo-linux. > > I consider it obvious, but never the less, OK for trunk (as in gcc-14) > so far? See below. > > On Mon, 03 Apr 2023 23:42:06 +0200 > Bernhard Reutner-Fischer wrote: > > > On 3 April 2023 21:50:49 CEST, Harald Anlauf wrote: > > >Hi Bernhard, > > > > > >there is neither context nor a related PR with a testcase showing > > >that this patch fixes issues seen there. > > > > Yes, i forgot to mention the PR: > > > > PR fortran/68800 > > > > I did not construct individual test cases but it should be obvious that we should not leak these. > > > > > > > >On 4/2/23 17:05, Bernhard Reutner-Fischer via Gcc-patches wrote: > > >> From: Bernhard Reutner-Fischer > > >> > > >> Cc: fortran@gcc.gnu.org > > >> > > >> gcc/fortran/ChangeLog: > > >> > > >> * array.cc (gfc_ref_dimen_size): Free mpz memory before ICEing. > > >> * expr.cc (find_array_section): Fix mpz memory leak. > > >> * simplify.cc (gfc_simplify_reshape): Fix mpz memory leaks in > > >> error paths. > > >> (gfc_simplify_set_exponent): Fix mpfr memory leak. > > >> --- > > >> gcc/fortran/array.cc | 3 +++ > > >> gcc/fortran/expr.cc | 8 ++++---- > > >> gcc/fortran/simplify.cc | 7 ++++++- > > >> 3 files changed, 13 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/gcc/fortran/array.cc b/gcc/fortran/array.cc > > >> index be5eb8b6a0f..8b1e816a859 100644 > > >> --- a/gcc/fortran/array.cc > > >> +++ b/gcc/fortran/array.cc > > >> @@ -2541,6 +2541,9 @@ gfc_ref_dimen_size (gfc_array_ref *ar, int dimen, mpz_t *result, mpz_t *end) > > >> return t; > > >> > > >> default: > > >> + mpz_clear (lower); > > >> + mpz_clear (stride); > > >> + mpz_clear (upper); > > >> gfc_internal_error ("gfc_ref_dimen_size(): Bad dimen_type"); > > >> } > > > > > > What is the point of clearing variables before issuing > > > a gfc_internal_error? > > > > To make it obvious that we are aware that we allocated these. I must admit I agree with Harald on this portion of the diff. What's the point? There is alot more allocated than just those 3 mzp_t variables when the internal error occurs. For example, gfortran does not walk the namespaces and free those before the ICE. I suppose silencing valgrind might be sufficient reason for the clutter. So, ok. > > >> &shape_exp->where, shape[rank], rank+1); > > >> + mpz_clear (index); > > >> return &gfc_bad_expr; > > >> } These types of changes are 'ok'. IIRC, gfortran will queue an error, and then forge on trying to match the code with other matchers. If successful, the error is tossed, so this would be a memory leak. -- Steve