Patchwork fix PR middle-end/48965, CASE_CHAIN fallout

login
register
mail settings
Submitter Nathan Froyd
Date May 11, 2011, 7:22 p.m.
Message ID <4DCAE1ED.6040203@codesourcery.com>
Download mbox | patch
Permalink /patch/95189/
State New
Headers show

Comments

Nathan Froyd - May 11, 2011, 7:22 p.m.
The comment for pointer_map_traverse says:

/* Pass each pointer in PMAP to the function in FN, together with the pointer
    to the value and the fixed parameter DATA.  If FN returns false, the
    iteration stops.  */

However, the code in tree-cfg:edge_to_cases_cleanup does:

static bool
edge_to_cases_cleanup (const void *key ATTRIBUTE_UNUSED, void **value,
		       void *data ATTRIBUTE_UNUSED)
{
   tree t, next;

   for (t = (tree) *value; t; t = next)
     {
       next = CASE_CHAIN (t);
       CASE_CHAIN (t) = NULL;
     }

   *value = NULL;
   return false;
}

...
   pointer_map_traverse (edge_to_cases, edge_to_cases_cleanup, NULL);

which means that we're only cleaning up one of the case chains stored in
EDGE_TO_CASES.  Since it's a pointer_map, we can potentially get
  /* Start recording information mapping edges to case labels.  */
@@ -2830,6 +2830,14 @@ verify_expr (tree *tp, int *walk_subtrees, void
*data ATTRIBUTE_UNUSED)
  	*walk_subtrees = 0;
        break;

+    case CASE_LABEL_EXPR:
+      if (CASE_CHAIN (t))
+	{
+	  error ("invalid CASE_CHAIN");
+	  return t;
+	}
+      break;
+
      default:
        break;
      }
Richard Guenther - May 12, 2011, 8:42 a.m.
On Wed, May 11, 2011 at 9:22 PM, Nathan Froyd <froydnj@codesourcery.com> wrote:
> The comment for pointer_map_traverse says:
>
> /* Pass each pointer in PMAP to the function in FN, together with the pointer
>    to the value and the fixed parameter DATA.  If FN returns false, the
>    iteration stops.  */
>
> However, the code in tree-cfg:edge_to_cases_cleanup does:
>
> static bool
> edge_to_cases_cleanup (const void *key ATTRIBUTE_UNUSED, void **value,
>                       void *data ATTRIBUTE_UNUSED)
> {
>   tree t, next;
>
>   for (t = (tree) *value; t; t = next)
>     {
>       next = CASE_CHAIN (t);
>       CASE_CHAIN (t) = NULL;
>     }
>
>   *value = NULL;
>   return false;
> }
>
> ...
>   pointer_map_traverse (edge_to_cases, edge_to_cases_cleanup, NULL);
>
> which means that we're only cleaning up one of the case chains stored in
> EDGE_TO_CASES.  Since it's a pointer_map, we can potentially get
> different chains selected each time.  Under -fcompare-debug, this leads
> to problems later on when we walk the function body and collect all the
> DECLs referenced therein; we might walk non-NULL CASE_CHAINs and reach
> more DECLs, depending on the memory layout.
>
> This wouldn't have shown up previously, since TREE_CHAIN was used, and
> we wouldn't walk TREE_CHAIN of expressions to find DECLs.
>
> The fix is simple: return true from the above function!  I've added
> logic to verify_expr to catch this sort of thing.
>
> Tested on x86_64-unknown-linux-gnu.  OK to commit?

Ok.

Thanks,
Richard.

> -Nathan
>
> gcc/
>        PR middle-end/48965
>        * tree-cfg.c (edge_to_cases_cleanup): Return true.
>        (verify_expr) [CASE_LABEL_EXPR]: Add checking.
>
> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> index e1f8707..e2e84a2 100644
> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -843,7 +843,7 @@ edge_to_cases_cleanup (const void *key
> ATTRIBUTE_UNUSED, void **value,
>      }
>
>    *value = NULL;
> -  return false;
> +  return true;
>  }
>
>  /* Start recording information mapping edges to case labels.  */
> @@ -2830,6 +2830,14 @@ verify_expr (tree *tp, int *walk_subtrees, void
> *data ATTRIBUTE_UNUSED)
>        *walk_subtrees = 0;
>        break;
>
> +    case CASE_LABEL_EXPR:
> +      if (CASE_CHAIN (t))
> +       {
> +         error ("invalid CASE_CHAIN");
> +         return t;
> +       }
> +      break;
> +
>      default:
>        break;
>      }
>

Patch

different chains selected each time.  Under -fcompare-debug, this leads
to problems later on when we walk the function body and collect all the
DECLs referenced therein; we might walk non-NULL CASE_CHAINs and reach
more DECLs, depending on the memory layout.

This wouldn't have shown up previously, since TREE_CHAIN was used, and
we wouldn't walk TREE_CHAIN of expressions to find DECLs.

The fix is simple: return true from the above function!  I've added
logic to verify_expr to catch this sort of thing.

Tested on x86_64-unknown-linux-gnu.  OK to commit?

-Nathan

gcc/
	PR middle-end/48965
	* tree-cfg.c (edge_to_cases_cleanup): Return true.
	(verify_expr) [CASE_LABEL_EXPR]: Add checking.

diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index e1f8707..e2e84a2 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -843,7 +843,7 @@  edge_to_cases_cleanup (const void *key
ATTRIBUTE_UNUSED, void **value,
      }

    *value = NULL;
-  return false;
+  return true;
  }