[patch,fortran] Bug 69497 - ICE in gfc_free_namespace

[Jerry DeLisle]

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