[patch, fortran] PR18923 segfault after subroutine name confusion

[patch, fortran] PR18923 segfault after subroutine name confusion

[Jerry DeLisle]

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

:ADDPATCH fortran:

This patch completes the fix for this PR.  The scrambled symbols were coming
from improperly allocating namespace pointers after an error with a CONTAINS block.

The key is to do the right thing when rejecting a statement.  The tmp namespace
pointer is needed to make sure we tie up the loose ends correctly. The test for 
tmp->refs is to avoid an assert later for keyword_symbol_1.f90 in the testsuite.

This patch partially fixes pr25252 which suffers the CONTAINS problem fixed here
as well as something else.  I am on that trail now.

Regression tested on x86-64-gnu-linux.  I will add the two cases from the PR now 
that the errors have been stabilized.

OK for trunk?

Regards,

Jerry

2007-06-02  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR fortran/18923
	* parse.c (decode_statement): Don't call gfc_undo_symbols on MATCH_ERROR
	for ST_FUNCTION since it is called in reject_statement.
	(parse_contained): If error, loop back after reject_statement and try
	again.  Free the namespace if an error occured.



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

Index: parse.c
===================================================================
*** parse.c	(revision 125258)
--- parse.c	(working copy)
*************** decode_statement (void)
*** 117,124 ****
  	return ST_FUNCTION;
        else if (m == MATCH_ERROR)
  	reject_statement ();
! 
!       gfc_undo_symbols ();
        gfc_current_locus = old_locus;
      }
  
--- 117,124 ----
  	return ST_FUNCTION;
        else if (m == MATCH_ERROR)
  	reject_statement ();
!       else 
! 	gfc_undo_symbols ();
        gfc_current_locus = old_locus;
      }
  
*************** gfc_fixup_sibling_symbols (gfc_symbol *s
*** 2775,2786 ****
  static void
  parse_contained (int module)
  {
!   gfc_namespace *ns, *parent_ns;
    gfc_state_data s1, s2;
    gfc_statement st;
    gfc_symbol *sym;
    gfc_entry_list *el;
    int contains_statements = 0;
  
    push_state (&s1, COMP_CONTAINS, NULL);
    parent_ns = gfc_current_ns;
--- 2775,2787 ----
  static void
  parse_contained (int module)
  {
!   gfc_namespace *ns, *parent_ns, *tmp;
    gfc_state_data s1, s2;
    gfc_statement st;
    gfc_symbol *sym;
    gfc_entry_list *el;
    int contains_statements = 0;
+   int seen_error = 0;
  
    push_state (&s1, COMP_CONTAINS, NULL);
    parent_ns = gfc_current_ns;
*************** parse_contained (int module)
*** 2792,2797 ****
--- 2793,2801 ----
        gfc_current_ns->sibling = parent_ns->contained;
        parent_ns->contained = gfc_current_ns;
  
+  next:
+       /* Process the next available statement.  We come here if we got an error
+ 	 and rejected the last statement.  */
        st = next_statement ();
  
        switch (st)
*************** parse_contained (int module)
*** 2867,2872 ****
--- 2871,2878 ----
  	  gfc_error ("Unexpected %s statement in CONTAINS section at %C",
  		     gfc_ascii_statement (st));
  	  reject_statement ();
+ 	  seen_error = 1;
+ 	  goto next;
  	  break;
  	}
      }
*************** parse_contained (int module)
*** 2875,2882 ****
  
    /* The first namespace in the list is guaranteed to not have
       anything (worthwhile) in it.  */
! 
    gfc_current_ns = parent_ns;
  
    ns = gfc_current_ns->contained;
    gfc_current_ns->contained = ns->sibling;
--- 2881,2890 ----
  
    /* The first namespace in the list is guaranteed to not have
       anything (worthwhile) in it.  */
!   tmp = gfc_current_ns;
    gfc_current_ns = parent_ns;
+   if (seen_error && tmp->refs > 1)
+     gfc_free_namespace (tmp);
  
    ns = gfc_current_ns->contained;
    gfc_current_ns->contained = ns->sibling;

Re: [patch, fortran] PR18923 segfault after subroutine name confusion

[FX Coudert]

> 2007-06-02  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
>
> 	PR fortran/18923
> 	* parse.c (decode_statement): Don't call gfc_undo_symbols on  
> MATCH_ERROR
> 	for ST_FUNCTION since it is called in reject_statement.
> 	(parse_contained): If error, loop back after reject_statement and try
> 	again.  Free the namespace if an error occured.

OK

Re: [patch, fortran] PR18923 segfault after subroutine name confusion

[Bernhard Fischer]

On Tue, May 22, 2007 at 08:31:28PM -0700, Jerry DeLisle wrote:
>Tobias Burnus wrote:
>>Jerry DeLisle wrote:
>>>I would like to add this one additional change.  We need to give it up
>>>at some point and one of these test cases triggers this.  It seems to
>>>make sense to me.  Let me know if this is OK and I will include it. 
>>>Regression tested of course and it obviously passes.
>>>Index: symbol.c
>>>-     gfc_internal_error ("gfc_get_default_type(): Bad symbol");
>>>+     gfc_fatal_error ("Symbol name confusion at %C");
>>
>>Hmm, I like that it shows the position, but it only shows "Fatal Error:"
>>instead of "Internal Compiler Error:". Internal errors should never
>>happen (they are by definition a bug in the compiler) and should be
>>reported.
>>
>>"Fatal Error:" on the other hand sound more like user errors which
>>should not be reported.
>>
>>I think invalid user code should and are rejected earlier, namely in
>>gfc_match ("Invalid character in name at %C"). Therefore, I would prefer
>>an gfc_internal_error. Couldn't you use something like:
>>
>>     gfc_internal_error ("gfc_get_default_type(): Bad symbol at %C");
>>
>>this gives more debugging information but still implies the ICE. If I
>>read error.c correctly, this should work.
>>
>>Tobias
>>
>This is one of those invalids where we ought to quit after the syntax error 
>is found.  Instead we move on into resolve and get additional nonsense 
>messages before finally hitting the internal error.  I have an upcoming 
>patch, still being developed and tested to attempt to intercept these and 
>bail out earlier.

Would calling gfc_check_error() there be sufficient to bail right at
that spot?
>
>I will commit the segfault fixes, then work on this new patch to catch the 
>syntax and quit.  After that I will get the test cases in place.

Re: [patch, fortran] PR18923 segfault after subroutine name confusion

[Jerry DeLisle]

Tobias Burnus wrote:
> Jerry DeLisle wrote:
>> I would like to add this one additional change.  We need to give it up
>> at some point and one of these test cases triggers this.  It seems to
>> make sense to me.  Let me know if this is OK and I will include it. 
>> Regression tested of course and it obviously passes.
>> Index: symbol.c
>> -     gfc_internal_error ("gfc_get_default_type(): Bad symbol");
>> +     gfc_fatal_error ("Symbol name confusion at %C");
> 
> Hmm, I like that it shows the position, but it only shows "Fatal Error:"
> instead of "Internal Compiler Error:". Internal errors should never
> happen (they are by definition a bug in the compiler) and should be
> reported.
> 
> "Fatal Error:" on the other hand sound more like user errors which
> should not be reported.
> 
> I think invalid user code should and are rejected earlier, namely in
> gfc_match ("Invalid character in name at %C"). Therefore, I would prefer
> an gfc_internal_error. Couldn't you use something like:
> 
>      gfc_internal_error ("gfc_get_default_type(): Bad symbol at %C");
> 
> this gives more debugging information but still implies the ICE. If I
> read error.c correctly, this should work.
> 
> Tobias
> 
This is one of those invalids where we ought to quit after the syntax error is 
found.  Instead we move on into resolve and get additional nonsense messages 
before finally hitting the internal error.  I have an upcoming patch, still 
being developed and tested to attempt to intercept these and bail out earlier.

I will commit the segfault fixes, then work on this new patch to catch the 
syntax and quit.  After that I will get the test cases in place.

Jerry

Re: [patch, fortran] PR18923 segfault after subroutine name confusion

[Tobias Burnus]

Jerry DeLisle wrote:
> I would like to add this one additional change.  We need to give it up
> at some point and one of these test cases triggers this.  It seems to
> make sense to me.  Let me know if this is OK and I will include it. 
> Regression tested of course and it obviously passes.
> Index: symbol.c
> -     gfc_internal_error ("gfc_get_default_type(): Bad symbol");
> +     gfc_fatal_error ("Symbol name confusion at %C");

Hmm, I like that it shows the position, but it only shows "Fatal Error:"
instead of "Internal Compiler Error:". Internal errors should never
happen (they are by definition a bug in the compiler) and should be
reported.

"Fatal Error:" on the other hand sound more like user errors which
should not be reported.

I think invalid user code should and are rejected earlier, namely in
gfc_match ("Invalid character in name at %C"). Therefore, I would prefer
an gfc_internal_error. Couldn't you use something like:

     gfc_internal_error ("gfc_get_default_type(): Bad symbol at %C");

this gives more debugging information but still implies the ICE. If I
read error.c correctly, this should work.

Tobias

Re: [patch, fortran] PR18923 segfault after subroutine name confusion

[Jerry DeLisle]

Tobias Burnus wrote:
> :REVIEWMAIL:
> 
> Jerry DeLisle wrote:
>> This patch is self explanatory and eliminates the segfault for two
>> test cases given.  Regression tested on x86-64-gnu-linux.
>> OK for trunk and later 4.2.1
> Ok, but could you also commit the two testcases of PR18923, just to make
> sure the bug does not creep in again.
> 
> 
> Tobias
> 
I would like to add this one additional change.  We need to give it up at some 
point and one of these test cases triggers this.  It seems to make sense to me. 
  Let me know if this is OK and I will include it.  Regression tested of course 
and it obviously passes.

Index: symbol.c
===================================================================
*** symbol.c    (revision 124927)
--- symbol.c    (working copy)
*************** gfc_get_default_type (gfc_symbol * sym,
*** 211,217 ****
                         "implicitly typed variables");

     if (letter < 'a' || letter > 'z')
!     gfc_internal_error ("gfc_get_default_type(): Bad symbol");

     if (ns == NULL)
       ns = gfc_current_ns;
--- 211,217 ----
                         "implicitly typed variables");

     if (letter < 'a' || letter > 'z')
!     gfc_fatal_error ("Symbol name confusion at %C");

     if (ns == NULL)
       ns = gfc_current_ns;

Re: [patch, fortran] PR18923 segfault after subroutine name confusion

[Tobias Burnus]

:REVIEWMAIL:

Jerry DeLisle wrote:
> This patch is self explanatory and eliminates the segfault for two
> test cases given.  Regression tested on x86-64-gnu-linux.
> OK for trunk and later 4.2.1
Ok, but could you also commit the two testcases of PR18923, just to make
sure the bug does not creep in again.


Tobias

[patch, fortran] PR18923 segfault after subroutine name confusion

[Jerry DeLisle]

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

:ADDPATCH fortran:

This patch is self explanatory and eliminates the segfault for two test cases 
given.  Regression tested on x86-64-gnu-linux.

OK for trunk and later 4.2.1

Regards,

Jerry

2007-05-18  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR fortran/18923
	* resolve.c (resolve_function): Don't call resolve_global_procedure if
	there is no name. Delete duplicated statement in ELSE clause.

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

Index: resolve.c
===================================================================
*** resolve.c	(revision 124824)
--- resolve.c	(working copy)
*************** resolve_function (gfc_expr *expr)
*** 1560,1566 ****
       procedure,it must be external and should be checked for usage.  */
    if (sym && !sym->attr.dummy && !sym->attr.contained
        && sym->attr.proc != PROC_ST_FUNCTION
!       && !sym->attr.use_assoc)
      resolve_global_procedure (sym, &expr->where, 0);
  
    /* Switch off assumed size checking and do this again for certain kinds
--- 1560,1567 ----
       procedure,it must be external and should be checked for usage.  */
    if (sym && !sym->attr.dummy && !sym->attr.contained
        && sym->attr.proc != PROC_ST_FUNCTION
!       && !sym->attr.use_assoc
!       && sym->name  )
      resolve_global_procedure (sym, &expr->where, 0);
  
    /* Switch off assumed size checking and do this again for certain kinds
*************** resolve_function (gfc_expr *expr)
*** 1743,1750 ****
        if (expr->symtree->n.sym->result
  	    && expr->symtree->n.sym->result->ts.type != BT_UNKNOWN)
  	expr->ts = expr->symtree->n.sym->result->ts;
-       else
- 	expr->ts = expr->symtree->n.sym->result->ts;
      }
  
    return t;
--- 1744,1749 ----