diff mbox series

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

Message ID 20230402150515.40826-4-rep.dot.nop@gmail.com
State Accepted
Headers
Series [1/3] go: Fix memory leak in Integer_expression |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Bernhard Reutner-Fischer April 2, 2023, 3:05 p.m. UTC 
  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(-)

Comments

Harald Anlauf April 3, 2023, 7:50 p.m. UTC | #1 
Hi Bernhard,

there is neither context nor a related PR with a testcase showing
that this patch fixes issues seen there.

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?

> diff --git a/gcc/fortran/expr.cc b/gcc/fortran/expr.cc
> index 7fb33f81788..b4736804eda 100644
> --- a/gcc/fortran/expr.cc
> +++ b/gcc/fortran/expr.cc
> @@ -1539,6 +1539,7 @@ find_array_section (gfc_expr *expr, gfc_ref *ref)
>     mpz_init_set_ui (delta_mpz, one);
>     mpz_init_set_ui (nelts, one);
>     mpz_init (tmp_mpz);
> +  mpz_init (ptr);
>
>     /* Do the initialization now, so that we can cleanup without
>        keeping track of where we were.  */
> @@ -1682,7 +1683,6 @@ find_array_section (gfc_expr *expr, gfc_ref *ref)
>         mpz_mul (delta_mpz, delta_mpz, tmp_mpz);
>       }
>
> -  mpz_init (ptr);
>     cons = gfc_constructor_first (base);
>
>     /* Now clock through the array reference, calculating the index in
> @@ -1735,7 +1735,8 @@ find_array_section (gfc_expr *expr, gfc_ref *ref)
>   		     "at %L requires an increase of the allowed %d "
>   		     "upper limit.  See %<-fmax-array-constructor%> "
>   		     "option", &expr->where, flag_max_array_constructor);
> -	  return false;
> +	  t = false;
> +	  goto cleanup;
>   	}
>
>         cons = gfc_constructor_lookup (base, limit);
> @@ -1750,8 +1751,6 @@ find_array_section (gfc_expr *expr, gfc_ref *ref)
>   				   gfc_copy_expr (cons->expr), NULL);
>       }
>
> -  mpz_clear (ptr);
> -
>   cleanup:
>
>     mpz_clear (delta_mpz);
> @@ -1765,6 +1764,7 @@ cleanup:
>         mpz_clear (ctr[d]);
>         mpz_clear (stride[d]);
>       }
> +  mpz_clear (ptr);
>     gfc_constructor_free (base);
>     return t;
>   }
> diff --git a/gcc/fortran/simplify.cc b/gcc/fortran/simplify.cc
> index ecf0e3558df..d1f06335e79 100644
> --- a/gcc/fortran/simplify.cc
> +++ b/gcc/fortran/simplify.cc
> @@ -6866,6 +6866,7 @@ gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
>   	  gfc_error ("The SHAPE array for the RESHAPE intrinsic at %L has a "
>   		     "negative value %d for dimension %d",
>   		     &shape_exp->where, shape[rank], rank+1);
> +	  mpz_clear (index);
>   	  return &gfc_bad_expr;
>   	}
>
> @@ -6889,6 +6890,7 @@ gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
>   	{
>   	  gfc_error ("Shapes of ORDER at %L and SHAPE at %L are different",
>   		     &order_exp->where, &shape_exp->where);
> +	  mpz_clear (index);
>   	  return &gfc_bad_expr;
>   	}
>
> @@ -6902,6 +6904,7 @@ gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
>   	{
>   	  gfc_error ("Sizes of ORDER at %L and SHAPE at %L are different",
>   		     &order_exp->where, &shape_exp->where);
> +	  mpz_clear (index);
>   	  return &gfc_bad_expr;
>   	}
>
> @@ -6918,6 +6921,7 @@ gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
>   			 "in the range [1, ..., %d] for the RESHAPE intrinsic "
>   			 "near %L", order[i], &order_exp->where, rank,
>   			 &shape_exp->where);
> +	      mpz_clear (index);
>   	      return &gfc_bad_expr;
>   	    }
>
> @@ -6926,6 +6930,7 @@ gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
>   	    {
>   	      gfc_error ("ORDER at %L is not a permutation of the size of "
>   			 "SHAPE at %L", &order_exp->where, &shape_exp->where);
> +	      mpz_clear (index);
>   	      return &gfc_bad_expr;
>   	    }
>   	  x[order[i]] = 1;
> @@ -7408,7 +7413,7 @@ gfc_simplify_set_exponent (gfc_expr *x, gfc_expr *i)
>     exp2 = (unsigned long) mpz_get_d (i->value.integer);
>     mpfr_mul_2exp (result->value.real, frac, exp2, GFC_RND_MODE);
>
> -  mpfr_clears (absv, log2, pow2, frac, NULL);
> +  mpfr_clears (exp, absv, log2, pow2, frac, NULL);
>
>     return range_check (result, "SET_EXPONENT");
>   }
Bernhard Reutner-Fischer April 3, 2023, 9:42 p.m. UTC | #2 
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.

thanks,
>
>> diff --git a/gcc/fortran/expr.cc b/gcc/fortran/expr.cc
>> index 7fb33f81788..b4736804eda 100644
>> --- a/gcc/fortran/expr.cc
>> +++ b/gcc/fortran/expr.cc
>> @@ -1539,6 +1539,7 @@ find_array_section (gfc_expr *expr, gfc_ref *ref)
>>     mpz_init_set_ui (delta_mpz, one);
>>     mpz_init_set_ui (nelts, one);
>>     mpz_init (tmp_mpz);
>> +  mpz_init (ptr);
>> 
>>     /* Do the initialization now, so that we can cleanup without
>>        keeping track of where we were.  */
>> @@ -1682,7 +1683,6 @@ find_array_section (gfc_expr *expr, gfc_ref *ref)
>>         mpz_mul (delta_mpz, delta_mpz, tmp_mpz);
>>       }
>> 
>> -  mpz_init (ptr);
>>     cons = gfc_constructor_first (base);
>> 
>>     /* Now clock through the array reference, calculating the index in
>> @@ -1735,7 +1735,8 @@ find_array_section (gfc_expr *expr, gfc_ref *ref)
>>   		     "at %L requires an increase of the allowed %d "
>>   		     "upper limit.  See %<-fmax-array-constructor%> "
>>   		     "option", &expr->where, flag_max_array_constructor);
>> -	  return false;
>> +	  t = false;
>> +	  goto cleanup;
>>   	}
>> 
>>         cons = gfc_constructor_lookup (base, limit);
>> @@ -1750,8 +1751,6 @@ find_array_section (gfc_expr *expr, gfc_ref *ref)
>>   				   gfc_copy_expr (cons->expr), NULL);
>>       }
>> 
>> -  mpz_clear (ptr);
>> -
>>   cleanup:
>> 
>>     mpz_clear (delta_mpz);
>> @@ -1765,6 +1764,7 @@ cleanup:
>>         mpz_clear (ctr[d]);
>>         mpz_clear (stride[d]);
>>       }
>> +  mpz_clear (ptr);
>>     gfc_constructor_free (base);
>>     return t;
>>   }
>> diff --git a/gcc/fortran/simplify.cc b/gcc/fortran/simplify.cc
>> index ecf0e3558df..d1f06335e79 100644
>> --- a/gcc/fortran/simplify.cc
>> +++ b/gcc/fortran/simplify.cc
>> @@ -6866,6 +6866,7 @@ gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
>>   	  gfc_error ("The SHAPE array for the RESHAPE intrinsic at %L has a "
>>   		     "negative value %d for dimension %d",
>>   		     &shape_exp->where, shape[rank], rank+1);
>> +	  mpz_clear (index);
>>   	  return &gfc_bad_expr;
>>   	}
>> 
>> @@ -6889,6 +6890,7 @@ gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
>>   	{
>>   	  gfc_error ("Shapes of ORDER at %L and SHAPE at %L are different",
>>   		     &order_exp->where, &shape_exp->where);
>> +	  mpz_clear (index);
>>   	  return &gfc_bad_expr;
>>   	}
>> 
>> @@ -6902,6 +6904,7 @@ gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
>>   	{
>>   	  gfc_error ("Sizes of ORDER at %L and SHAPE at %L are different",
>>   		     &order_exp->where, &shape_exp->where);
>> +	  mpz_clear (index);
>>   	  return &gfc_bad_expr;
>>   	}
>> 
>> @@ -6918,6 +6921,7 @@ gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
>>   			 "in the range [1, ..., %d] for the RESHAPE intrinsic "
>>   			 "near %L", order[i], &order_exp->where, rank,
>>   			 &shape_exp->where);
>> +	      mpz_clear (index);
>>   	      return &gfc_bad_expr;
>>   	    }
>> 
>> @@ -6926,6 +6930,7 @@ gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
>>   	    {
>>   	      gfc_error ("ORDER at %L is not a permutation of the size of "
>>   			 "SHAPE at %L", &order_exp->where, &shape_exp->where);
>> +	      mpz_clear (index);
>>   	      return &gfc_bad_expr;
>>   	    }
>>   	  x[order[i]] = 1;
>> @@ -7408,7 +7413,7 @@ gfc_simplify_set_exponent (gfc_expr *x, gfc_expr *i)
>>     exp2 = (unsigned long) mpz_get_d (i->value.integer);
>>     mpfr_mul_2exp (result->value.real, frac, exp2, GFC_RND_MODE);
>> 
>> -  mpfr_clears (absv, log2, pow2, frac, NULL);
>> +  mpfr_clears (exp, absv, log2, pow2, frac, NULL);
>> 
>>     return range_check (result, "SET_EXPONENT");
>>   }
>
Bernhard Reutner-Fischer April 17, 2023, 7:47 p.m. UTC | #3 
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?

thanks,

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.
> 
> thanks,
> >  
> >> diff --git a/gcc/fortran/expr.cc b/gcc/fortran/expr.cc
> >> index 7fb33f81788..b4736804eda 100644
> >> --- a/gcc/fortran/expr.cc
> >> +++ b/gcc/fortran/expr.cc
> >> @@ -1539,6 +1539,7 @@ find_array_section (gfc_expr *expr, gfc_ref *ref)
> >>     mpz_init_set_ui (delta_mpz, one);
> >>     mpz_init_set_ui (nelts, one);
> >>     mpz_init (tmp_mpz);
> >> +  mpz_init (ptr);
> >> 
> >>     /* Do the initialization now, so that we can cleanup without
> >>        keeping track of where we were.  */
> >> @@ -1682,7 +1683,6 @@ find_array_section (gfc_expr *expr, gfc_ref *ref)
> >>         mpz_mul (delta_mpz, delta_mpz, tmp_mpz);
> >>       }
> >> 
> >> -  mpz_init (ptr);
> >>     cons = gfc_constructor_first (base);
> >> 
> >>     /* Now clock through the array reference, calculating the index in
> >> @@ -1735,7 +1735,8 @@ find_array_section (gfc_expr *expr, gfc_ref *ref)
> >>   		     "at %L requires an increase of the allowed %d "
> >>   		     "upper limit.  See %<-fmax-array-constructor%> "
> >>   		     "option", &expr->where, flag_max_array_constructor);
> >> -	  return false;
> >> +	  t = false;
> >> +	  goto cleanup;
> >>   	}
> >> 
> >>         cons = gfc_constructor_lookup (base, limit);
> >> @@ -1750,8 +1751,6 @@ find_array_section (gfc_expr *expr, gfc_ref *ref)
> >>   				   gfc_copy_expr (cons->expr), NULL);
> >>       }
> >> 
> >> -  mpz_clear (ptr);
> >> -
> >>   cleanup:
> >> 
> >>     mpz_clear (delta_mpz);
> >> @@ -1765,6 +1764,7 @@ cleanup:
> >>         mpz_clear (ctr[d]);
> >>         mpz_clear (stride[d]);
> >>       }
> >> +  mpz_clear (ptr);
> >>     gfc_constructor_free (base);
> >>     return t;
> >>   }
> >> diff --git a/gcc/fortran/simplify.cc b/gcc/fortran/simplify.cc
> >> index ecf0e3558df..d1f06335e79 100644
> >> --- a/gcc/fortran/simplify.cc
> >> +++ b/gcc/fortran/simplify.cc
> >> @@ -6866,6 +6866,7 @@ gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
> >>   	  gfc_error ("The SHAPE array for the RESHAPE intrinsic at %L has a "
> >>   		     "negative value %d for dimension %d",
> >>   		     &shape_exp->where, shape[rank], rank+1);
> >> +	  mpz_clear (index);
> >>   	  return &gfc_bad_expr;
> >>   	}
> >> 
> >> @@ -6889,6 +6890,7 @@ gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
> >>   	{
> >>   	  gfc_error ("Shapes of ORDER at %L and SHAPE at %L are different",
> >>   		     &order_exp->where, &shape_exp->where);
> >> +	  mpz_clear (index);
> >>   	  return &gfc_bad_expr;
> >>   	}
> >> 
> >> @@ -6902,6 +6904,7 @@ gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
> >>   	{
> >>   	  gfc_error ("Sizes of ORDER at %L and SHAPE at %L are different",
> >>   		     &order_exp->where, &shape_exp->where);
> >> +	  mpz_clear (index);
> >>   	  return &gfc_bad_expr;
> >>   	}
> >> 
> >> @@ -6918,6 +6921,7 @@ gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
> >>   			 "in the range [1, ..., %d] for the RESHAPE intrinsic "
> >>   			 "near %L", order[i], &order_exp->where, rank,
> >>   			 &shape_exp->where);
> >> +	      mpz_clear (index);
> >>   	      return &gfc_bad_expr;
> >>   	    }
> >> 
> >> @@ -6926,6 +6930,7 @@ gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
> >>   	    {
> >>   	      gfc_error ("ORDER at %L is not a permutation of the size of "
> >>   			 "SHAPE at %L", &order_exp->where, &shape_exp->where);
> >> +	      mpz_clear (index);
> >>   	      return &gfc_bad_expr;
> >>   	    }
> >>   	  x[order[i]] = 1;
> >> @@ -7408,7 +7413,7 @@ gfc_simplify_set_exponent (gfc_expr *x, gfc_expr *i)
> >>     exp2 = (unsigned long) mpz_get_d (i->value.integer);
> >>     mpfr_mul_2exp (result->value.real, frac, exp2, GFC_RND_MODE);
> >> 
> >> -  mpfr_clears (absv, log2, pow2, frac, NULL);
> >> +  mpfr_clears (exp, absv, log2, pow2, frac, NULL);
> >> 
> >>     return range_check (result, "SET_EXPONENT");
> >>   }  
> >  
>
Li, Pan2 via Gcc-patches April 17, 2023, 10:18 p.m. UTC | #4 
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.
Bernhard Reutner-Fischer May 8, 2023, 6 a.m. UTC | #5 
On Mon, 17 Apr 2023 15:18:27 -0700
Steve Kargl <sgk@troutmask.apl.washington.edu> wrote:
> On Mon, Apr 17, 2023 at 09:47:50PM +0200, Bernhard Reutner-Fischer via Fortran wrote:
> > 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.

I've dropped this hunk and pushed the rest as r14-567-g2521390dd2f8e5
Harald fixed the leak of expr in gfc_simplify_set_exponent in the
meantime.
thanks,
diff mbox series

Patch

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");
     }
 
diff --git a/gcc/fortran/expr.cc b/gcc/fortran/expr.cc
index 7fb33f81788..b4736804eda 100644
--- a/gcc/fortran/expr.cc
+++ b/gcc/fortran/expr.cc
@@ -1539,6 +1539,7 @@  find_array_section (gfc_expr *expr, gfc_ref *ref)
   mpz_init_set_ui (delta_mpz, one);
   mpz_init_set_ui (nelts, one);
   mpz_init (tmp_mpz);
+  mpz_init (ptr);
 
   /* Do the initialization now, so that we can cleanup without
      keeping track of where we were.  */
@@ -1682,7 +1683,6 @@  find_array_section (gfc_expr *expr, gfc_ref *ref)
       mpz_mul (delta_mpz, delta_mpz, tmp_mpz);
     }
 
-  mpz_init (ptr);
   cons = gfc_constructor_first (base);
 
   /* Now clock through the array reference, calculating the index in
@@ -1735,7 +1735,8 @@  find_array_section (gfc_expr *expr, gfc_ref *ref)
 		     "at %L requires an increase of the allowed %d "
 		     "upper limit.  See %<-fmax-array-constructor%> "
 		     "option", &expr->where, flag_max_array_constructor);
-	  return false;
+	  t = false;
+	  goto cleanup;
 	}
 
       cons = gfc_constructor_lookup (base, limit);
@@ -1750,8 +1751,6 @@  find_array_section (gfc_expr *expr, gfc_ref *ref)
 				   gfc_copy_expr (cons->expr), NULL);
     }
 
-  mpz_clear (ptr);
-
 cleanup:
 
   mpz_clear (delta_mpz);
@@ -1765,6 +1764,7 @@  cleanup:
       mpz_clear (ctr[d]);
       mpz_clear (stride[d]);
     }
+  mpz_clear (ptr);
   gfc_constructor_free (base);
   return t;
 }
diff --git a/gcc/fortran/simplify.cc b/gcc/fortran/simplify.cc
index ecf0e3558df..d1f06335e79 100644
--- a/gcc/fortran/simplify.cc
+++ b/gcc/fortran/simplify.cc
@@ -6866,6 +6866,7 @@  gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
 	  gfc_error ("The SHAPE array for the RESHAPE intrinsic at %L has a "
 		     "negative value %d for dimension %d",
 		     &shape_exp->where, shape[rank], rank+1);
+	  mpz_clear (index);
 	  return &gfc_bad_expr;
 	}
 
@@ -6889,6 +6890,7 @@  gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
 	{
 	  gfc_error ("Shapes of ORDER at %L and SHAPE at %L are different",
 		     &order_exp->where, &shape_exp->where);
+	  mpz_clear (index);
 	  return &gfc_bad_expr;
 	}
 
@@ -6902,6 +6904,7 @@  gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
 	{
 	  gfc_error ("Sizes of ORDER at %L and SHAPE at %L are different",
 		     &order_exp->where, &shape_exp->where);
+	  mpz_clear (index);
 	  return &gfc_bad_expr;
 	}
 
@@ -6918,6 +6921,7 @@  gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
 			 "in the range [1, ..., %d] for the RESHAPE intrinsic "
 			 "near %L", order[i], &order_exp->where, rank,
 			 &shape_exp->where);
+	      mpz_clear (index);
 	      return &gfc_bad_expr;
 	    }
 
@@ -6926,6 +6930,7 @@  gfc_simplify_reshape (gfc_expr *source, gfc_expr *shape_exp,
 	    {
 	      gfc_error ("ORDER at %L is not a permutation of the size of "
 			 "SHAPE at %L", &order_exp->where, &shape_exp->where);
+	      mpz_clear (index);
 	      return &gfc_bad_expr;
 	    }
 	  x[order[i]] = 1;
@@ -7408,7 +7413,7 @@  gfc_simplify_set_exponent (gfc_expr *x, gfc_expr *i)
   exp2 = (unsigned long) mpz_get_d (i->value.integer);
   mpfr_mul_2exp (result->value.real, frac, exp2, GFC_RND_MODE);
 
-  mpfr_clears (absv, log2, pow2, frac, NULL);
+  mpfr_clears (exp, absv, log2, pow2, frac, NULL);
 
   return range_check (result, "SET_EXPONENT");
 }