[patch,fortran] Bug 69497 - ICE in gfc_free_namespace

[Jerry DeLisle]

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