[fortran] Bug 69497 - ICE in gfc_free_namespace

Message ID 2a78763f-b5e3-2696-78d0-8c808720ba1a@charter.net
State New
Headers show
Series
  • [fortran] Bug 69497 - ICE in gfc_free_namespace
Related show

Commit Message

Jerry March 24, 2018, 9:25 p.m.
This one has been hanging around for a while. Dominique found this fix.

I retested with the 30 cases provided in the PR. These are all invalid 
fortran. They give errors now and do not ICE.

Regression tested on trunk.

OK?

Jerry

PS I will use the first of the many test cases for the testsuite with 
appropriate ChangeLog, etc.

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.

Comments

Steve Kargl March 24, 2018, 9:56 p.m. | #1
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.

-- 
Steve
Jerry March 24, 2018, 11:25 p.m. | #2
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.
Mikael Morin March 27, 2018, 8:53 p.m. | #3
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 ();
Jerry March 28, 2018, 1:03 a.m. | #4
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

Patch

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);