Fix ICE on VLA in LTO mode
diff mbox series

Message ID 14329479.qMsKKfkivG@polaris
State New
Headers show
Series
  • Fix ICE on VLA in LTO mode
Related show

Commit Message

Eric Botcazou Dec. 9, 2019, 12:22 p.m. UTC
Hi,

this is a regression present on the mainline and 9 branch: the compiler gives 
an ICE for the attached Ada testcase on the following assertion:

      if (DECL_P (ref))
	{
	  /* We shouldn't have true variables here.  */
	  gcc_assert (TREE_READONLY (ref));
	  subst = ref;
	}

in self_referential_size because the size function machinery is invoked again 
by the *free_lang_data pass, more precisely from fld_process_array_type:

  if (!existed)
    {
      array
	= build_array_type_1 (t2, TYPE_DOMAIN (t), TYPE_TYPELESS_STORAGE (t),
			      false, false);
      TYPE_CANONICAL (array) = TYPE_CANONICAL (t);
      if (!fld->pset.add (array))
	add_tree_to_fld_list (array, fld);
    }

through the call to build_array_type_1, and more precisely through the 
recursive call made for computing TYPE_CANONICAL:

  if (TYPE_CANONICAL (t) == t)
    {
      if (TYPE_STRUCTURAL_EQUALITY_P (elt_type)
	  || (index_type && TYPE_STRUCTURAL_EQUALITY_P (index_type))
	  || in_lto_p)
	SET_TYPE_STRUCTURAL_EQUALITY (t);
      else if (TYPE_CANONICAL (elt_type) != elt_type
	       || (index_type && TYPE_CANONICAL (index_type) != index_type))
	TYPE_CANONICAL (t)
	  = build_array_type_1 (TYPE_CANONICAL (elt_type),
				index_type
				? TYPE_CANONICAL (index_type) : NULL_TREE,
				typeless_storage, shared, set_canonical);
    }


That's a bit surprising because t2 is an incomplete type and we have these 
lines in build_array_type_1 just before:

  /* If the element type is incomplete at this point we get marked for
     structural equality.  Do not record these types in the canonical
     type hashtable.  */
  if (TYPE_STRUCTURAL_EQUALITY_P (t))
    return t;

so the computation of TYPE_CANONICAL should be skipped.  But it turns out that 
these lines from 2009 are obsolete because layout_type no longer forces the 
TYPE_STRUCTURAL_EQUALITY_P on the array when the element type is incomplete.


Since fld_process_array_type overwrites TYPE_CANONICAL just after the call to 
build_array_type_1, there is no point for the latter in computing it so the 
proposed fix is to add a new SET_CANONICAL parameter to build_array_type_1.

Tested on x86_64-suse-linux, OK for mainline and 9 branch?


2019-12-09  Eric Botcazou  <ebotcazou@adacore.com>

	* tree.c (build_array_type_1): Add SET_CANONICAL parameter and compute
	TYPE_CANONICAL from the element type only if it is set.  Remove obsolete
	lines and adjust recursive call.
	(fld_process_array_type): Adjust call to build_array_type_1.
	(build_array_type): Likewise.
	(build_nonshared_array_type): Likewise.


2019-12-09  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/lto23.adb: New test.

Comments

Richard Biener Dec. 9, 2019, 2:14 p.m. UTC | #1
On Mon, Dec 9, 2019 at 1:23 PM Eric Botcazou <ebotcazou@adacore.com> wrote:
>
> Hi,
>
> this is a regression present on the mainline and 9 branch: the compiler gives
> an ICE for the attached Ada testcase on the following assertion:
>
>       if (DECL_P (ref))
>         {
>           /* We shouldn't have true variables here.  */
>           gcc_assert (TREE_READONLY (ref));
>           subst = ref;
>         }
>
> in self_referential_size because the size function machinery is invoked again
> by the *free_lang_data pass, more precisely from fld_process_array_type:
>
>   if (!existed)
>     {
>       array
>         = build_array_type_1 (t2, TYPE_DOMAIN (t), TYPE_TYPELESS_STORAGE (t),
>                               false, false);
>       TYPE_CANONICAL (array) = TYPE_CANONICAL (t);
>       if (!fld->pset.add (array))
>         add_tree_to_fld_list (array, fld);
>     }
>
> through the call to build_array_type_1, and more precisely through the
> recursive call made for computing TYPE_CANONICAL:
>
>   if (TYPE_CANONICAL (t) == t)
>     {
>       if (TYPE_STRUCTURAL_EQUALITY_P (elt_type)
>           || (index_type && TYPE_STRUCTURAL_EQUALITY_P (index_type))
>           || in_lto_p)
>         SET_TYPE_STRUCTURAL_EQUALITY (t);
>       else if (TYPE_CANONICAL (elt_type) != elt_type
>                || (index_type && TYPE_CANONICAL (index_type) != index_type))
>         TYPE_CANONICAL (t)
>           = build_array_type_1 (TYPE_CANONICAL (elt_type),
>                                 index_type
>                                 ? TYPE_CANONICAL (index_type) : NULL_TREE,
>                                 typeless_storage, shared, set_canonical);
>     }
>
>
> That's a bit surprising because t2 is an incomplete type and we have these
> lines in build_array_type_1 just before:
>
>   /* If the element type is incomplete at this point we get marked for
>      structural equality.  Do not record these types in the canonical
>      type hashtable.  */
>   if (TYPE_STRUCTURAL_EQUALITY_P (t))
>     return t;
>
> so the computation of TYPE_CANONICAL should be skipped.  But it turns out that
> these lines from 2009 are obsolete because layout_type no longer forces the
> TYPE_STRUCTURAL_EQUALITY_P on the array when the element type is incomplete.
>
>
> Since fld_process_array_type overwrites TYPE_CANONICAL just after the call to
> build_array_type_1, there is no point for the latter in computing it so the
> proposed fix is to add a new SET_CANONICAL parameter to build_array_type_1.
>
> Tested on x86_64-suse-linux, OK for mainline and 9 branch?

OK.

>
> 2019-12-09  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * tree.c (build_array_type_1): Add SET_CANONICAL parameter and compute
>         TYPE_CANONICAL from the element type only if it is set.  Remove obsolete
>         lines and adjust recursive call.
>         (fld_process_array_type): Adjust call to build_array_type_1.
>         (build_array_type): Likewise.
>         (build_nonshared_array_type): Likewise.
>
>
> 2019-12-09  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gnat.dg/lto23.adb: New test.
>
> --
> Eric Botcazou

Patch
diff mbox series

Index: tree.c
===================================================================
--- tree.c	(revision 278938)
+++ tree.c	(working copy)
@@ -266,7 +266,7 @@  static void print_type_hash_statistics (
 static void print_debug_expr_statistics (void);
 static void print_value_expr_statistics (void);
 
-static tree build_array_type_1 (tree, tree, bool, bool);
+static tree build_array_type_1 (tree, tree, bool, bool, bool);
 
 tree global_trees[TI_MAX];
 tree integer_types[itk_none];
@@ -5303,8 +5303,9 @@  fld_process_array_type (tree t, tree t2,
      = map->get_or_insert (t, &existed);
   if (!existed)
     {
-      array = build_array_type_1 (t2, TYPE_DOMAIN (t),
-				  TYPE_TYPELESS_STORAGE (t), false);
+      array
+	= build_array_type_1 (t2, TYPE_DOMAIN (t), TYPE_TYPELESS_STORAGE (t),
+			      false, false);
       TYPE_CANONICAL (array) = TYPE_CANONICAL (t);
       if (!fld->pset.add (array))
 	add_tree_to_fld_list (array, fld);
@@ -8155,11 +8156,12 @@  subrange_type_for_debug_p (const_tree ty
 /* Construct, lay out and return the type of arrays of elements with ELT_TYPE
    and number of elements specified by the range of values of INDEX_TYPE.
    If TYPELESS_STORAGE is true, TYPE_TYPELESS_STORAGE flag is set on the type.
-   If SHARED is true, reuse such a type that has already been constructed.  */
+   If SHARED is true, reuse such a type that has already been constructed.
+   If SET_CANONICAL is true, compute TYPE_CANONICAL from the element type.  */
 
 static tree
 build_array_type_1 (tree elt_type, tree index_type, bool typeless_storage,
-		    bool shared)
+		    bool shared, bool set_canonical)
 {
   tree t;
 
@@ -8176,19 +8178,13 @@  build_array_type_1 (tree elt_type, tree
   TYPE_TYPELESS_STORAGE (t) = typeless_storage;
   layout_type (t);
 
-  /* If the element type is incomplete at this point we get marked for
-     structural equality.  Do not record these types in the canonical
-     type hashtable.  */
-  if (TYPE_STRUCTURAL_EQUALITY_P (t))
-    return t;
-
   if (shared)
     {
       hashval_t hash = type_hash_canon_hash (t);
       t = type_hash_canon (hash, t);
     }
 
-  if (TYPE_CANONICAL (t) == t)
+  if (TYPE_CANONICAL (t) == t && set_canonical)
     {
       if (TYPE_STRUCTURAL_EQUALITY_P (elt_type)
 	  || (index_type && TYPE_STRUCTURAL_EQUALITY_P (index_type))
@@ -8200,7 +8196,7 @@  build_array_type_1 (tree elt_type, tree
 	  = build_array_type_1 (TYPE_CANONICAL (elt_type),
 				index_type
 				? TYPE_CANONICAL (index_type) : NULL_TREE,
-				typeless_storage, shared);
+				typeless_storage, shared, set_canonical);
     }
 
   return t;
@@ -8211,7 +8207,8 @@  build_array_type_1 (tree elt_type, tree
 tree
 build_array_type (tree elt_type, tree index_type, bool typeless_storage)
 {
-  return build_array_type_1 (elt_type, index_type, typeless_storage, true);
+  return
+    build_array_type_1 (elt_type, index_type, typeless_storage, true, true);
 }
 
 /* Wrapper around build_array_type_1 with SHARED set to false.  */
@@ -8219,7 +8216,7 @@  build_array_type (tree elt_type, tree in
 tree
 build_nonshared_array_type (tree elt_type, tree index_type)
 {
-  return build_array_type_1 (elt_type, index_type, false, false);
+  return build_array_type_1 (elt_type, index_type, false, false, true);
 }
 
 /* Return a representation of ELT_TYPE[NELTS], using indices of type