Re: ping Re: [PATCH 3/3] Fortran: Fix mpz and mpfr memory leaks

[Steve Kargl <sgk@troutmask.apl.washington.edu>]

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 <rep.dot.nop@gmail.com> wrote:
> 
> > On 3 April 2023 21:50:49 CEST, Harald Anlauf <anlauf@gmx.de> 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 <aldot@gcc.gnu.org>
> > >> 
> > >> 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