public inbox for fortran@gcc.gnu.org
 help / color / mirror / Atom feed
* [patch,fortran] Bug 69497 - ICE in gfc_free_namespace
@ 2018-03-24 21:25 Jerry DeLisle
  2018-03-24 21:56 ` Steve Kargl
  0 siblings, 1 reply; 9+ messages in thread
From: Jerry DeLisle @ 2018-03-24 21:25 UTC (permalink / raw)
  To: fortran; +Cc: GCC Patches

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.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch,fortran] Bug 69497 - ICE in gfc_free_namespace
  2018-03-24 21:25 [patch,fortran] Bug 69497 - ICE in gfc_free_namespace Jerry DeLisle
@ 2018-03-24 21:56 ` Steve Kargl
  2018-03-24 23:25   ` Jerry DeLisle
  0 siblings, 1 reply; 9+ messages in thread
From: Steve Kargl @ 2018-03-24 21:56 UTC (permalink / raw)
  To: Jerry DeLisle; +Cc: fortran, GCC Patches

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch,fortran] Bug 69497 - ICE in gfc_free_namespace
  2018-03-24 21:56 ` Steve Kargl
@ 2018-03-24 23:25   ` Jerry DeLisle
  2018-03-25 17:49     ` Mikael Morin
  0 siblings, 1 reply; 9+ messages in thread
From: Jerry DeLisle @ 2018-03-24 23:25 UTC (permalink / raw)
  To: sgk; +Cc: fortran, GCC Patches

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch,fortran] Bug 69497 - ICE in gfc_free_namespace
  2018-03-24 23:25   ` Jerry DeLisle
@ 2018-03-25 17:49     ` Mikael Morin
  2018-03-25 19:28       ` Jerry DeLisle
  0 siblings, 1 reply; 9+ messages in thread
From: Mikael Morin @ 2018-03-25 17:49 UTC (permalink / raw)
  To: fortran

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.
A fatal error would do as well, and would probably be nicer to users, 
but I think the compiler should be stopped when it reaches the point 
where the number of references is negative.


Mikael

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch,fortran] Bug 69497 - ICE in gfc_free_namespace
  2018-03-25 17:49     ` Mikael Morin
@ 2018-03-25 19:28       ` Jerry DeLisle
  2018-03-25 21:11         ` Mikael Morin
  0 siblings, 1 reply; 9+ messages in thread
From: Jerry DeLisle @ 2018-03-25 19:28 UTC (permalink / raw)
  To: fortran

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. With the patch, which I committed already after OK from 
Steve on IRC, the resulting error messages are reasonable. For example:

program p
    block
    do
    end block
end

Gives:

$ gfc pr69497.f90
pr69497.f90:6:6:

     end block
       1
Error: Expecting END DO statement at (1)
pr69497.f90:7:3:

  end
    1
Error: END DO statement expected at (1)
f951: Error: Unexpected end of file in ‘pr69497.f90’

This is a lot more useful then a fatal error.  All of the 30 cases I 
tested gave similar reasonable errors.

Jerry

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch,fortran] Bug 69497 - ICE in gfc_free_namespace
  2018-03-25 19:28       ` Jerry DeLisle
@ 2018-03-25 21:11         ` Mikael Morin
  2018-03-26  1:53           ` Jerry DeLisle
  0 siblings, 1 reply; 9+ messages in thread
From: Mikael Morin @ 2018-03-25 21:11 UTC (permalink / raw)
  To: fortran

[-- Attachment #1: Type: text/plain, Size: 2500 bytes --]

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.

> 
> 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.
That patch, however doesn’t fix the underlying memory management problem 
(which is, well, less trivial to fix).

Mikael


[-- Attachment #2: fatal_error.diff --]
[-- Type: text/x-patch, Size: 629 bytes --]

diff --git a/gcc/fortran/symbol.c b/gcc/fortran/symbol.c
index 4c109fdfbad..00ab7cd1a9e 100644
--- a/gcc/fortran/symbol.c
+++ b/gcc/fortran/symbol.c
@@ -3938,12 +3938,19 @@ gfc_free_namespace (gfc_namespace *ns)
   if (ns == NULL)
     return;
 
+  if (ns->refs <= 0)
+    {
+      int errors;
+      gfc_error_check();
+      gfc_get_errors(NULL, &errors);
+      gcc_assert(errors > 0);
+      gfc_fatal_error ("Unable to recover from previous errors, aborting");
+    }
+
   ns->refs--;
   if (ns->refs > 0)
     return;
 
-  gcc_assert (ns->refs == 0);
-
   gfc_free_statements (ns->code);
 
   free_sym_tree (ns->sym_root);

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch,fortran] Bug 69497 - ICE in gfc_free_namespace
  2018-03-25 21:11         ` Mikael Morin
@ 2018-03-26  1:53           ` Jerry DeLisle
  2018-03-27 20:53             ` Mikael Morin
  0 siblings, 1 reply; 9+ messages in thread
From: Jerry DeLisle @ 2018-03-26  1:53 UTC (permalink / raw)
  To: fortran

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.

> That patch, however doesn’t fix the underlying memory management problem 
> (which is, well, less trivial to fix).

So true.

Jerry

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch,fortran] Bug 69497 - ICE in gfc_free_namespace
  2018-03-26  1:53           ` Jerry DeLisle
@ 2018-03-27 20:53             ` Mikael Morin
  2018-03-28  1:03               ` Jerry DeLisle
  0 siblings, 1 reply; 9+ messages in thread
From: Mikael Morin @ 2018-03-27 20:53 UTC (permalink / raw)
  To: Jerry DeLisle, fortran; +Cc: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 2927 bytes --]

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


[-- Attachment #2: pr69497.CL --]
[-- Type: text/x-opencl-src, Size: 198 bytes --]

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


[-- Attachment #3: pr69497.diff --]
[-- Type: text/x-patch, Size: 871 bytes --]

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch,fortran] Bug 69497 - ICE in gfc_free_namespace
  2018-03-27 20:53             ` Mikael Morin
@ 2018-03-28  1:03               ` Jerry DeLisle
  0 siblings, 0 replies; 9+ messages in thread
From: Jerry DeLisle @ 2018-03-28  1:03 UTC (permalink / raw)
  To: Mikael Morin, fortran; +Cc: gcc-patches

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-03-28  1:03 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-24 21:25 [patch,fortran] Bug 69497 - ICE in gfc_free_namespace Jerry DeLisle
2018-03-24 21:56 ` Steve Kargl
2018-03-24 23:25   ` Jerry DeLisle
2018-03-25 17:49     ` Mikael Morin
2018-03-25 19:28       ` Jerry DeLisle
2018-03-25 21:11         ` Mikael Morin
2018-03-26  1:53           ` Jerry DeLisle
2018-03-27 20:53             ` Mikael Morin
2018-03-28  1:03               ` Jerry DeLisle

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).