diff mbox

[PR69169] Fix infinite recursion in create_variable_info_for_1

Message ID alpine.LSU.2.11.1601130933530.31122@t29.fhfr.qr
State New
Headers show

Commit Message

Richard Biener Jan. 13, 2016, 8:40 a.m. UTC 
On Wed, 13 Jan 2016, Tom de Vries wrote:

> Hi,
> 
> Consider testcase test.c:
> ...
> struct pgm_slist_t
> {
>   struct pgm_slist_t *__restrict next;
> };
> 
> void
> fn1 (struct pgm_slist_t p1)
> {
> 
> }
> ...
> 
> The compilation of the testcase enters into infinite recursion, due to the
> more aggressive restrict support added recently.
> 
> The patch fixes this by:-
> - tracking which structs are being traversed when following restrict
>   pointers in create_variable_info_for_1, and
> - not following restrict pointers that point to already tracked structs.
> 
> Bootstrapped and reg-tested on x86_64.
> 
> OK for trunk?

Comments inline (unqouted)

Fix infinite recursion in create_variable_info_for_1

	PR tree-optimization/69169
	* tree-ssa-structalias.c (handled_struct_type): New hash set.
	(create_variable_info_for_1): Use handled_struct_type to keep track
	of structs recursed by following restrict pointers.
	(intra_create_variable_infos): Allocate and cleanup
	handled_struct_type.

	* gcc.dg/pr69169.c: New test.

---
 gcc/testsuite/gcc.dg/pr69169.c | 13 +++++++++++++
 gcc/tree-ssa-structalias.c     | 19 ++++++++++++++++---
 2 files changed, 29 insertions(+), 3 deletions(-)


Plase make the above static and GTY((skip)), no need to put it in
GC memory.

 /* Create a varinfo structure for NAME and DECL, and add it to VARMAP.
    This will also create any varinfo structures necessary for fields
    of DECL.  DECL is a function parameter if HANDLE_PARAM is set.  */
@@ -5851,7 +5857,8 @@ create_variable_info_for_1 (tree decl, const char *name, bool add_id,
 	vi->only_restrict_pointers = 1;
       if (vi->only_restrict_pointers
 	  && !type_contains_placeholder_p (TREE_TYPE (decl_type))
-	  && handle_param)
+	  && handle_param
+	  && !handled_struct_type->contains (TREE_TYPE (decl_type)))
 	{
 	  varinfo_t rvi;
 	  tree heapvar = build_fake_var_decl (TREE_TYPE (decl_type));

This path misses to add/remove decl_type to the set.  Thus
struct X { struct X * restrict p; } *; would still recurse infinitely?

@@ -5871,6 +5878,8 @@ create_variable_info_for_1 (tree decl, const char *name, bool add_id,
   vi->fullsize = tree_to_uhwi (declsize);
   if (fieldstack.length () == 1)
     vi->is_full_var = true;
+  if (handle_param)
+    handled_struct_type->add (decl_type);

while here we add it to the set even if in the end we won't create
a fake var decl (and recurse).  I think we should add to the set only
if we actually recurse.

   for (i = 0, newvi = vi;
        fieldstack.iterate (i, &fo);
        ++i, newvi = vi_next (newvi))
@@ -5902,7 +5911,8 @@ create_variable_info_for_1 (tree decl, const char *name, bool add_id,
       newvi->only_restrict_pointers = fo->only_restrict_pointers;
       if (handle_param
 	  && newvi->only_restrict_pointers
-	  && !type_contains_placeholder_p (fo->restrict_pointed_type))
+	  && !type_contains_placeholder_p (fo->restrict_pointed_type)
+	  && !handled_struct_type->contains (fo->restrict_pointed_type))
 	{
 	  varinfo_t rvi;
 	  tree heapvar = build_fake_var_decl (fo->restrict_pointed_type);
@@ -5921,6 +5931,8 @@ create_variable_info_for_1 (tree decl, const char *name, bool add_id,
 	  tem->head = vi->id;
 	}
     }
+  if (handle_param)
+    handled_struct_type->remove (decl_type);
 
   return vi;
 }
@@ -6065,10 +6077,11 @@ intra_create_variable_infos (struct function *fn)
      passed-by-reference argument.  */
   for (t = DECL_ARGUMENTS (fn->decl); t; t = DECL_CHAIN (t))
     {
+      handled_struct_type = hash_set<tree>::create_ggc (4);

As said, please put it in heap memory and eventually just pass it down
as argument to create_variable_info_for_1 avoiding the global variable
(it's just three callers as far as I can see).

       varinfo_t p
 	= create_variable_info_for_1 (t, alias_get_name (t), false, true);
+      handled_struct_type = NULL;
       insert_vi_for_tree (t, p);
-
       make_param_constraints (p);
     }
diff mbox

Patch

diff --git a/gcc/testsuite/gcc.dg/pr69169.c b/gcc/testsuite/gcc.dg/pr69169.c
new file mode 100644
index 0000000..ecf847c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr69169.c
@@ -0,0 +1,13 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+struct pgm_slist_t
+{
+  struct pgm_slist_t *__restrict next;
+};
+
+void
+fn1 (struct pgm_slist_t p1)
+{
+
+}
diff --git a/gcc/tree-ssa-structalias.c b/gcc/tree-ssa-structalias.c
index fd98352..72bef07 100644
--- a/gcc/tree-ssa-structalias.c
+++ b/gcc/tree-ssa-structalias.c
@@ -5767,6 +5767,12 @@  check_for_overlaps (vec<fieldoff_s> fieldstack)
   return false;
 }
 
+/* Hash set used to register structs traversed in create_variable_info_for_1
+   by following restrict pointers.  This is needed to prevent infinite
+   recursion.  */
+
+hash_set<tree> *handled_struct_type = NULL;
+

A bitmap indexed by TYPE_UID would be cheaper I guess?  Maybe not.
At least a hash_set<unsigned> would be ;)