| Message ID | 2a78763f-b5e3-2696-78d0-8c808720ba1a@charter.net |
|---|---|
| State | New |
| Headers | show |
| Series | [fortran] Bug 69497 - ICE in gfc_free_namespace | expand |
On Sat, Mar 24, 2018 at 02:25:36PM -0700, Jerry DeLisle wrote: > > 2018-03-24 Jerry DeLisle <jvdelisle@gcc.gnu.org> > Dominique d'Humieres <dominiq@gcc.gnu.org> > > PR fortran/84506 > * symbol.c (gfc_free_namespace): Delete the assert and if refs > count is less than zero, free the namespece. Something is > halfway and other errors will resound. > > diff --git a/gcc/fortran/symbol.c b/gcc/fortran/symbol.c > index ce6b1e93644..997d90b00fd 100644 > --- a/gcc/fortran/symbol.c > +++ b/gcc/fortran/symbol.c > @@ -4037,10 +4037,9 @@ gfc_free_namespace (gfc_namespace *ns) > return; > > ns->refs--; > - if (ns->refs > 0) > - return; > > - gcc_assert (ns->refs == 0); > + if (ns->refs != 0) > + return; > > gfc_free_statements (ns->code); The ChangeLog doesn't seem to match the patch. If ns->refs==0, you free the namespace. If ns->refs!=0, you return. So, if ns->refs<0, the namespace is not freed.
On 03/24/2018 02:56 PM, Steve Kargl wrote: > On Sat, Mar 24, 2018 at 02:25:36PM -0700, Jerry DeLisle wrote: >> >> 2018-03-24 Jerry DeLisle <jvdelisle@gcc.gnu.org> >> Dominique d'Humieres <dominiq@gcc.gnu.org> >> >> PR fortran/84506 >> * symbol.c (gfc_free_namespace): Delete the assert and if refs >> count is less than zero, free the namespece. Something is >> halfway and other errors will resound. >> >> diff --git a/gcc/fortran/symbol.c b/gcc/fortran/symbol.c >> index ce6b1e93644..997d90b00fd 100644 >> --- a/gcc/fortran/symbol.c >> +++ b/gcc/fortran/symbol.c >> @@ -4037,10 +4037,9 @@ gfc_free_namespace (gfc_namespace *ns) >> return; >> >> ns->refs--; >> - if (ns->refs > 0) >> - return; >> >> - gcc_assert (ns->refs == 0); >> + if (ns->refs != 0) >> + return; >> >> gfc_free_statements (ns->code); > > The ChangeLog doesn't seem to match the patch. > > If ns->refs==0, you free the namespace. > If ns->refs!=0, you return. > So, if ns->refs<0, the namespace is not freed. > That is what I get when I am in hurry. Try this: PR fortran/84506 * symbol.c (gfc_free_namespace): Delete the assert and only if refs count equals zero, free the namespece. Otherwise, something is halfway and other errors will resound.
Le 26/03/2018 à 03:53, Jerry DeLisle a écrit : > On 03/25/2018 02:11 PM, Mikael Morin wrote: >> Le 25/03/2018 à 21:27, Jerry DeLisle a écrit : >>> On 03/25/2018 10:49 AM, Mikael Morin wrote: >>>> Le 25/03/2018 à 00:25, Jerry DeLisle a écrit : >>>>> On 03/24/2018 02:56 PM, Steve Kargl wrote: >>>>>> On Sat, Mar 24, 2018 at 02:25:36PM -0700, Jerry DeLisle wrote: >>>>>>> >>>>>>> diff --git a/gcc/fortran/symbol.c b/gcc/fortran/symbol.c >>>>>>> index ce6b1e93644..997d90b00fd 100644 >>>>>>> --- a/gcc/fortran/symbol.c >>>>>>> +++ b/gcc/fortran/symbol.c >>>>>>> @@ -4037,10 +4037,9 @@ gfc_free_namespace (gfc_namespace *ns) >>>>>>> return; >>>>>>> >>>>>>> ns->refs--; >>>>>>> - if (ns->refs > 0) >>>>>>> - return; >>>>>>> >>>>>>> - gcc_assert (ns->refs == 0); >>>>>>> + if (ns->refs != 0) >>>>>>> + return; >>>>>>> >>>>>>> gfc_free_statements (ns->code); >>>>>> >>>>>> The ChangeLog doesn't seem to match the patch. >>>>>> >>>>>> If ns->refs==0, you free the namespace. >>>>>> If ns->refs!=0, you return. >>>>>> So, if ns->refs<0, the namespace is not freed. >>>>>> >>>>> >>>>> That is what I get when I am in hurry. Try this: >>>>> >>>>> PR fortran/84506 >>>>> * symbol.c (gfc_free_namespace): Delete the assert and only if >>>>> refs count equals zero, free the namespece. Otherwise, >>>>> something is halfway and other errors will resound. >>>>> >>>> Hello, >>>> >>>> The assert was put in place to exhibit memory management issues, and >>>> that’s what it does. >>>> If ns->refs < 0, then it was 0 on the previous call, and ns should >>>> have been freed at that time. >>>> So if you read ns->refs you are reading garbage, and if you decrease >>>> it you are writing to memory that you don’t own any more. >>>> I think ICEing at this point is good enough, instead of going >>>> further down the road. >>> >>> The problem with ICEing is that it tells the users to report it as a >>> bug in the compiler. >> >> It is a bug in the compiler, albeit one of little concern to us (at >> least when dealing with invalid code): the memory is incorrectly managed. > > No argument there, just saying in the cases of the PR, it is not useful > to the user. > >> >>> >>> This is a lot more useful then a fatal error. All of the 30 cases I >>> tested gave similar reasonable errors. >>> >> >> A fatal error doesn’t actually remove previously emitted (reasonable) >> errors, it just doesn’t let the compiler continue. I can propose the >> attached patch to convince you. > > No need to convince. If you prefer your patch, its OK with me. > I have tried to restore the assert instead. With the attached patch, freshly regression tested. I have also checked the 29 cases from the PR. OK? Mikael 2018-03-27 Mikael Morin <mikael@gcc.gnu.org> PR fortran/69497 * symbol.c (gfc_symbol_done_2): Start freeing namespaces from the root. (gfc_free_namespace): Restore assert (revert r258839). diff --git a/gcc/fortran/symbol.c b/gcc/fortran/symbol.c index 997d90b00fd..546a4fae0a8 100644 --- a/gcc/fortran/symbol.c +++ b/gcc/fortran/symbol.c @@ -4037,10 +4037,11 @@ gfc_free_namespace (gfc_namespace *ns) return; ns->refs--; - - if (ns->refs != 0) + if (ns->refs > 0) return; + gcc_assert (ns->refs == 0); + gfc_free_statements (ns->code); free_sym_tree (ns->sym_root); @@ -4087,8 +4088,14 @@ gfc_symbol_init_2 (void) void gfc_symbol_done_2 (void) { - gfc_free_namespace (gfc_current_ns); - gfc_current_ns = NULL; + if (gfc_current_ns != NULL) + { + /* free everything from the root. */ + while (gfc_current_ns->parent != NULL) + gfc_current_ns = gfc_current_ns->parent; + gfc_free_namespace (gfc_current_ns); + gfc_current_ns = NULL; + } gfc_free_dt_list (); enforce_single_undo_checkpoint ();
On 03/27/2018 01:53 PM, Mikael Morin wrote: > Le 26/03/2018 à 03:53, Jerry DeLisle a écrit : >> On 03/25/2018 02:11 PM, Mikael Morin wrote: >>> Le 25/03/2018 à 21:27, Jerry DeLisle a écrit : >>>> On 03/25/2018 10:49 AM, Mikael Morin wrote: >>>>> Le 25/03/2018 à 00:25, Jerry DeLisle a écrit : >>>>>> On 03/24/2018 02:56 PM, Steve Kargl wrote: >>>>>>> On Sat, Mar 24, 2018 at 02:25:36PM -0700, Jerry DeLisle wrote: >>>>>>>> >>>>>>>> diff --git a/gcc/fortran/symbol.c b/gcc/fortran/symbol.c >>>>>>>> index ce6b1e93644..997d90b00fd 100644 >>>>>>>> --- a/gcc/fortran/symbol.c >>>>>>>> +++ b/gcc/fortran/symbol.c >>>>>>>> @@ -4037,10 +4037,9 @@ gfc_free_namespace (gfc_namespace *ns) >>>>>>>> return; >>>>>>>> >>>>>>>> ns->refs--; >>>>>>>> - if (ns->refs > 0) >>>>>>>> - return; >>>>>>>> >>>>>>>> - gcc_assert (ns->refs == 0); >>>>>>>> + if (ns->refs != 0) >>>>>>>> + return; >>>>>>>> >>>>>>>> gfc_free_statements (ns->code); >>>>>>> >>>>>>> The ChangeLog doesn't seem to match the patch. >>>>>>> >>>>>>> If ns->refs==0, you free the namespace. >>>>>>> If ns->refs!=0, you return. >>>>>>> So, if ns->refs<0, the namespace is not freed. >>>>>>> >>>>>> >>>>>> That is what I get when I am in hurry. Try this: >>>>>> >>>>>> PR fortran/84506 >>>>>> * symbol.c (gfc_free_namespace): Delete the assert and only if >>>>>> refs count equals zero, free the namespece. Otherwise, >>>>>> something is halfway and other errors will resound. >>>>>> >>>>> Hello, >>>>> >>>>> The assert was put in place to exhibit memory management issues, >>>>> and that’s what it does. >>>>> If ns->refs < 0, then it was 0 on the previous call, and ns should >>>>> have been freed at that time. >>>>> So if you read ns->refs you are reading garbage, and if you >>>>> decrease it you are writing to memory that you don’t own any more. >>>>> I think ICEing at this point is good enough, instead of going >>>>> further down the road. >>>> >>>> The problem with ICEing is that it tells the users to report it as a >>>> bug in the compiler. >>> >>> It is a bug in the compiler, albeit one of little concern to us (at >>> least when dealing with invalid code): the memory is incorrectly >>> managed. >> >> No argument there, just saying in the cases of the PR, it is not >> useful to the user. >> >>> >>>> >>>> This is a lot more useful then a fatal error. All of the 30 cases I >>>> tested gave similar reasonable errors. >>>> >>> >>> A fatal error doesn’t actually remove previously emitted (reasonable) >>> errors, it just doesn’t let the compiler continue. I can propose the >>> attached patch to convince you. >> >> No need to convince. If you prefer your patch, its OK with me. >> > I have tried to restore the assert instead. > With the attached patch, freshly regression tested. > I have also checked the 29 cases from the PR. > OK? > > Mikael > Good to go Mikael, thanks. Jerry
diff --git a/gcc/fortran/symbol.c b/gcc/fortran/symbol.c index ce6b1e93644..997d90b00fd 100644 --- a/gcc/fortran/symbol.c +++ b/gcc/fortran/symbol.c @@ -4037,10 +4037,9 @@ gfc_free_namespace (gfc_namespace *ns) return; ns->refs--; - if (ns->refs > 0) - return; - gcc_assert (ns->refs == 0); + if (ns->refs != 0) + return; gfc_free_statements (ns->code);