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