Patchwork patch fortran, pr 59746, internal compiler error : segmentation fault

login
register
mail settings
Submitter jimmie.davis@l-3com.com
Date March 9, 2014, 7:47 p.m.
Message ID <FFF95760268D324AB6DD9426E83C8DF70B2E0FB9@ARLEXCHMBX01.lst.link.l-3com.com>
Download mbox | patch
Permalink /patch/328389/
State New
Headers show

Comments

jimmie.davis@l-3com.com - March 9, 2014, 7:47 p.m.
Comments from Mikael:
 
#1.  Please merge the two ifs; it seems they have exactly the same scope
after the patch.
 
#2.  This comment applies to the TOUPPER thing below...
 .. so it should be put here. Also the indentation is wrong.
 
#3.This is unnecessary.
Mikael Morin - March 9, 2014, 8:40 p.m.
Le 09/03/2014 20:47, jimmie.davis@l-3com.com a écrit :
> Comments from Mikael:
>  
> #1.  Please merge the two ifs; it seems they have exactly the same scope
> after the patch.
>  
> #2.  This comment applies to the TOUPPER thing below...
>  .. so it should be put here. Also the indentation is wrong.
>  
> #3.This is unnecessary.
>  
> ===========================
> 
> All corrected in the patch below.  
> This patch is not very important.   If we are in 'regressions fix only' mode, then this needs to sit until stage 1 happens....It is not worth any kind of breakage.
Alright, don't forget to ping the patch during next stage1 then.

Next review iteration:
> Index: gcc/gcc/fortran/symbol.c
> ===================================================================
> --- gcc/gcc/fortran/symbol.c	(revision 208437)
> +++ gcc/gcc/fortran/symbol.c	(working copy)
> @@ -3069,65 +3069,65 @@
>  
>    FOR_EACH_VEC_ELT (latest_undo_chgset->syms, i, p)
>      {
> -      if (p->gfc_new)
> -	{
> -	  /* Symbol was new.  */
> -	  if (p->attr.in_common && p->common_block && p->common_block->head)
> -	    {
> -	      /* If the symbol was added to any common block, it
> -		 needs to be removed to stop the resolver looking
> -		 for a (possibly) dead symbol.  */
> +      /* Symbol was new. Or was old and just put in common */
> +      if ((p->gfc_new || 
> +          (p->attr.in_common && !p->old_symbol->attr.in_common )) && 
> +           p->attr.in_common && p->common_block && p->common_block->head)
write this instead:
if (p->attr.in_common && p->common_block && p->common_block->head
    && (p->gfc_new || !p->old_symbol->attr.in_common))

[p->attr.in_common is less likely than p->gfc_new which happens all the
time; we may as well short-circuit the latter.]


For the rest of the patch, there are indentation issues, any leading 8
spaces block should be replaced with a tab.
You can use "contrib/check_GNU_style.sh your_patch" to check common
style issues)

> +           )
> +        {
> +          /* If the symbol was added to any common block, it
> +             needs to be removed to stop the resolver looking
> +             for a (possibly) dead symbol.  */
>  
> -	      if (p->common_block->head == p && !p->common_next)
> -		{
> -		  gfc_symtree st, *st0;
> -		  st0 = find_common_symtree (p->ns->common_root,
> -					     p->common_block);
> -		  if (st0)
> -		    {
> -		      st.name = st0->name;
> -		      gfc_delete_bbt (&p->ns->common_root, &st, compare_symtree);
> -		      free (st0);
> -		    }
> -		}
> +          if (p->common_block->head == p && !p->common_next)
> +            {
> +              gfc_symtree st, *st0;
> +              st0 = find_common_symtree (p->ns->common_root,
> +                                         p->common_block);
> +              if (st0)
> +                {
> +                  st.name = st0->name;
> +		  gfc_delete_bbt (&p->ns->common_root, &st, compare_symtree);
> +		  free (st0);
> +                }
> +            }
>  
> -	      if (p->common_block->head == p)
> -	        p->common_block->head = p->common_next;
> -	      else
> -		{
> -		  gfc_symbol *cparent, *csym;


> +	    if (p->common_block->head == p)
I think the code block starting here is indented too much.

> +	      p->common_block->head = p->common_next;
> +	    else
> +              {
> +                gfc_symbol *cparent, *csym;
>  
> -		  cparent = p->common_block->head;
> -		  csym = cparent->common_next;
> +                cparent = p->common_block->head;
> +                csym = cparent->common_next;
>  
> -		  while (csym != p)
> -		    {
> -		      cparent = csym;
> -		      csym = csym->common_next;
> -		    }
> +                while (csym != p)
> +                  {
> +                    cparent = csym;
> +                    csym = csym->common_next;
> +                  }
>  
> -		  gcc_assert(cparent->common_next == p);
> +                  gcc_assert(cparent->common_next == p);
> +                  cparent->common_next = csym->common_next;
> +              }
> +        }


> +        if (p->gfc_new) 
Same here, the code after this shouldn't need reindenting.

> +          {
> +            /* The derived type is saved in the symtree with the first
> +               letter capitalized; the all lower-case version to the
> +               derived type contains its associated generic function.  */
>  
> -		  cparent->common_next = csym->common_next;
> -		}
> -	    }
> +            if (p->attr.flavor == FL_DERIVED)
> +              gfc_delete_symtree (&p->ns->sym_root, gfc_get_string ("%c%s",
> +                                  (char) TOUPPER ((unsigned char) p->name[0]),
> +                                  &p->name[1]));
> +            else
> +	      gfc_delete_symtree (&p->ns->sym_root, p->name);
>  
> -	  /* The derived type is saved in the symtree with the first
> -	     letter capitalized; the all lower-case version to the
> -	     derived type contains its associated generic function.  */
> -	  if (p->attr.flavor == FL_DERIVED)
> -	    gfc_delete_symtree (&p->ns->sym_root, gfc_get_string ("%c%s",
> -                        (char) TOUPPER ((unsigned char) p->name[0]),
> -                        &p->name[1]));
> -	  else
> -	    gfc_delete_symtree (&p->ns->sym_root, p->name);
> -
> -	  gfc_release_symbol (p);
> -	}
> -      else
> -	restore_old_symbol (p);
> +	    gfc_release_symbol (p);
> +	  }
> +        else
> +          restore_old_symbol (p);
>      }
> -
>    latest_undo_chgset->syms.truncate (0);
>    latest_undo_chgset->tbps.truncate (0);
>  
> 

Mikael

Patch

===========================

All corrected in the patch below.  
This patch is not very important.   If we are in 'regressions fix only' mode, then this needs to sit until stage 1 happens....It is not worth any kind of breakage.

Reran the testsuite.  No new failures.


regards,
Bud Davis


Index: gcc/gcc/fortran/symbol.c
===================================================================
--- gcc/gcc/fortran/symbol.c	(revision 208437)
+++ gcc/gcc/fortran/symbol.c	(working copy)
@@ -3069,65 +3069,65 @@ 
 
   FOR_EACH_VEC_ELT (latest_undo_chgset->syms, i, p)
     {
-      if (p->gfc_new)
-	{
-	  /* Symbol was new.  */
-	  if (p->attr.in_common && p->common_block && p->common_block->head)
-	    {
-	      /* If the symbol was added to any common block, it
-		 needs to be removed to stop the resolver looking
-		 for a (possibly) dead symbol.  */
+      /* Symbol was new. Or was old and just put in common */
+      if ((p->gfc_new || 
+          (p->attr.in_common && !p->old_symbol->attr.in_common )) && 
+           p->attr.in_common && p->common_block && p->common_block->head)
+        {
+          /* If the symbol was added to any common block, it
+             needs to be removed to stop the resolver looking
+             for a (possibly) dead symbol.  */
 
-	      if (p->common_block->head == p && !p->common_next)
-		{
-		  gfc_symtree st, *st0;
-		  st0 = find_common_symtree (p->ns->common_root,
-					     p->common_block);
-		  if (st0)
-		    {
-		      st.name = st0->name;
-		      gfc_delete_bbt (&p->ns->common_root, &st, compare_symtree);
-		      free (st0);
-		    }
-		}
+          if (p->common_block->head == p && !p->common_next)
+            {
+              gfc_symtree st, *st0;
+              st0 = find_common_symtree (p->ns->common_root,
+                                         p->common_block);
+              if (st0)
+                {
+                  st.name = st0->name;
+		  gfc_delete_bbt (&p->ns->common_root, &st, compare_symtree);
+		  free (st0);
+                }
+            }
 
-	      if (p->common_block->head == p)
-	        p->common_block->head = p->common_next;
-	      else
-		{
-		  gfc_symbol *cparent, *csym;
+	    if (p->common_block->head == p)
+	      p->common_block->head = p->common_next;
+	    else
+              {
+                gfc_symbol *cparent, *csym;
 
-		  cparent = p->common_block->head;
-		  csym = cparent->common_next;
+                cparent = p->common_block->head;
+                csym = cparent->common_next;
 
-		  while (csym != p)
-		    {
-		      cparent = csym;
-		      csym = csym->common_next;
-		    }
+                while (csym != p)
+                  {
+                    cparent = csym;
+                    csym = csym->common_next;
+                  }
 
-		  gcc_assert(cparent->common_next == p);
+                  gcc_assert(cparent->common_next == p);
+                  cparent->common_next = csym->common_next;
+              }
+        }
+        if (p->gfc_new) 
+          {
+            /* The derived type is saved in the symtree with the first
+               letter capitalized; the all lower-case version to the
+               derived type contains its associated generic function.  */
 
-		  cparent->common_next = csym->common_next;
-		}
-	    }
+            if (p->attr.flavor == FL_DERIVED)
+              gfc_delete_symtree (&p->ns->sym_root, gfc_get_string ("%c%s",
+                                  (char) TOUPPER ((unsigned char) p->name[0]),
+                                  &p->name[1]));
+            else
+	      gfc_delete_symtree (&p->ns->sym_root, p->name);
 
-	  /* The derived type is saved in the symtree with the first
-	     letter capitalized; the all lower-case version to the
-	     derived type contains its associated generic function.  */
-	  if (p->attr.flavor == FL_DERIVED)
-	    gfc_delete_symtree (&p->ns->sym_root, gfc_get_string ("%c%s",
-                        (char) TOUPPER ((unsigned char) p->name[0]),
-                        &p->name[1]));
-	  else
-	    gfc_delete_symtree (&p->ns->sym_root, p->name);
-
-	  gfc_release_symbol (p);
-	}
-      else
-	restore_old_symbol (p);
+	    gfc_release_symbol (p);
+	  }
+        else
+          restore_old_symbol (p);
     }
-
   latest_undo_chgset->syms.truncate (0);
   latest_undo_chgset->tbps.truncate (0);