Bare bones of type verifier

Bare bones of type verifier

[Jan Hubicka]

Hi,
this patch adds bare bones of type checker.  You can call verify_type on any
type in the IL and see compiler bomb if some of invariants are broken.  So far
it only verify tests I already tested in last stage1 with my reverted variant
streaming patch https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02454.html

The checks found interesting problems that was fixed. I have other tests and
fixes but would like to go incrementally.  Some of tests already broke again
with recent C++ align attribute changes (I think), see FIXME comments.
I plan to proceed with small steps becuase all the checks seems to trigger
fun issues.

The patch still fix on bug in ipa-chkp.c that is obvious enough - it is
cut&pasted from old code in cgraphunit.c that was updated same way.

I hope with early debug we are on the track of getting simplified gimple types,
but to make this without hitting too many surprises I think we first want to
document and verify our FE type representation and also verify what we need
in middle-end and drop the rest (in free lang data).

Placement of verify_type calls may seem bit random. Basic idea is to verify that
bad types do not hit LTO and are not produced by LTO type merging. In non-LTO path
we verify types at dwarf2out that is major consumer of the type variants.
I would be happy to place more calls/relocate existing to better places.

LTO-Boostrapped/regtested x86_64-linux without Ada. I am doing full run on
PPC64, but it fails on unrelated libgmp miscomplation I proably need to track down
first.

OK if testing passes?

Honza

	* tree.c: Include print-tree.h
	(verify_type_variant): New function.
	(verify_type): New function.
	* tree.h (verify_type): Declare.
	* tree-streamer-out.c (write_ts_type_common_tree_pointers): Verify type.
	* dwarf2out.c (dwarf2out_decl): Verify type.
	* ipa-chkp.c (chkp_copy_function_type_adding_bounds): Do not consider
	updated type to be variant.

	* lto.c (lto_fixup_state): Verify types.
Index: tree.c
===================================================================
--- tree.c	(revision 222391)
+++ tree.c	(working copy)
@@ -102,6 +102,7 @@ along with GCC; see the file COPYING3.
 #include "debug.h"
 #include "intl.h"
 #include "builtins.h"
+#include "print-tree.h"
 
 /* Tree code classes.  */
 
@@ -12425,4 +12437,124 @@ element_mode (const_tree t)
   return TYPE_MODE (t);
 }
 
+/* Veirfy that basic properties of T match TV and thus T can be a variant of
+   TV.  TV should be the more specified variant (i.e. the main variant).  */
+
+static bool
+verify_type_variant (const_tree t, tree tv)
+{
+  if (TREE_CODE (t) != TREE_CODE (tv))
+    {
+      error ("type variant has different TREE_CODE");
+      debug_tree (tv);
+      return false;
+    }
+  if (COMPLETE_TYPE_P (t) && TYPE_SIZE (t) != TYPE_SIZE (tv))
+    {
+      error ("type variant has different TYPE_SIZE");
+      debug_tree (tv);
+      return false;
+    }
+  if (COMPLETE_TYPE_P (t) && TYPE_SIZE_UNIT (t) != TYPE_SIZE_UNIT (tv))
+    {
+      error ("type variant has different TYPE_SIZE_UNIT");
+      debug_tree (tv);
+      return false;
+    }
+  if (RECORD_OR_UNION_TYPE_P (t) && TYPE_VFIELD (t) != TYPE_VFIELD (tv))
+    {
+      error ("type variant has different TYPE_VFIELD");
+      debug_tree (tv);
+      return false;
+    }
+  if (((TREE_CODE (t) == ENUMERAL_TYPE && COMPLETE_TYPE_P (t))
+	|| TREE_CODE (t) == INTEGER_TYPE
+	|| TREE_CODE (t) == BOOLEAN_TYPE
+	|| TREE_CODE (t) == REAL_TYPE
+	|| TREE_CODE (t) == FIXED_POINT_TYPE)
+       && (TYPE_MAX_VALUE (t) != TYPE_MAX_VALUE (tv)
+	   || TYPE_MAX_VALUE (t) != TYPE_MAX_VALUE (tv)))
+    {
+      error ("type variant has different TYPE_MIN_VALUE");
+      debug_tree (tv);
+      return false;
+    }
+  if (TREE_CODE (t) == METHOD_TYPE
+      && TYPE_METHOD_BASETYPE (t) != TYPE_METHOD_BASETYPE (tv))
+    {
+      error ("type variant has different TYPE_METHOD_BASETYPE");
+      debug_tree (tv);
+      return false;
+    }
+  /* FIXME: Be lax and allow TYPE_METHODS to be NULL.  This is a bug
+     but affecting only the debug output.  */
+  if (RECORD_OR_UNION_TYPE_P (t) && COMPLETE_TYPE_P (t)
+      && TYPE_METHODS (t) && TYPE_METHODS (tv)
+      && TYPE_METHODS (t) != TYPE_METHODS (tv))
+    {
+      error ("type variant has different TYPE_METHODS");
+      debug_tree (tv);
+      return false;
+    }
+  if (TREE_CODE (t) == OFFSET_TYPE
+      && TYPE_OFFSET_BASETYPE (t) != TYPE_OFFSET_BASETYPE (tv))
+    {
+      error ("type variant has different TYPE_OFFSET_BASETYPE");
+      debug_tree (tv);
+      return false;
+    }
+  if (TREE_CODE (t) == ARRAY_TYPE
+      && TYPE_ARRAY_MAX_SIZE (t) != TYPE_ARRAY_MAX_SIZE (tv))
+    {
+      error ("type variant has different TYPE_ARRAY_MAX_SIZE");
+      debug_tree (tv);
+      return false;
+    }
+  /* FIXME: Be lax and allow TYPE_BINFO to be missing in variant types
+     or even type's main variant.  This is needed to make bootstrap pass
+     and the bug seems new in GCC 5.
+     C++ FE should be updated to make this consistent and we should check
+     that TYPE_BINFO is always NULL for !COMPLETE_TYPE_P and otherwise there
+     is a match with main variant.  */
+  if (RECORD_OR_UNION_TYPE_P (t) && TYPE_BINFO (t) && TYPE_BINFO (tv)
+      && TYPE_BINFO (t) != TYPE_BINFO (tv))
+    {
+      error ("type variant has different TYPE_BINFO");
+      debug_tree (tv);
+      error ("type variant's TYPE_BINFO");
+      debug_tree (TYPE_BINFO (tv));
+      error ("type's TYPE_BINFO");
+      debug_tree (TYPE_BINFO (tv));
+      return false;
+    }
+  return true;
+}
+
+/* Verify type T.  */
+
+void
+verify_type (const_tree t)
+{
+  bool error_found = false;
+  tree mv = TYPE_MAIN_VARIANT (t);
+  if (!mv)
+    {
+      error ("Main variant is not defined");
+      error_found = true;
+    }
+  else if (mv != TYPE_MAIN_VARIANT (mv))
+    {
+      error ("TYPE_MAIN_VARAINT has different TYPE_MAIN_VARIANT");
+      debug_tree (mv);
+      error_found = true;
+    }
+  else if (t != mv && !verify_type_variant (t, mv))
+    error_found = true;
+  if (error_found)
+    {
+      debug_tree (const_cast <tree> (t));
+      internal_error ("verify_type failed");
+    }
+}
+
 #include "gt-tree.h"
Index: tree.h
===================================================================
--- tree.h	(revision 222391)
+++ tree.h	(working copy)
@@ -4495,6 +4495,7 @@ extern tree drop_tree_overflow (tree);
 extern int tree_map_base_eq (const void *, const void *);
 extern unsigned int tree_map_base_hash (const void *);
 extern int tree_map_base_marked_p (const void *);
+extern void DEBUG_FUNCTION verify_type (const_tree t);
 
 #define tree_map_eq tree_map_base_eq
 extern unsigned int tree_map_hash (const void *);
Index: lto/lto.c
===================================================================
--- lto/lto.c	(revision 222391)
+++ lto/lto.c	(working copy)
@@ -2844,6 +2844,10 @@ lto_fixup_state (struct lto_in_decl_stat
       for (i = 0; i < vec_safe_length (trees); i++)
 	{
 	  tree t = (*trees)[i];
+#ifdef ENABLE_CHECKING
+	  if (TYPE_P (t))
+	    verify_type (t);
+#endif
 	  if (VAR_OR_FUNCTION_DECL_P (t)
 	      && (TREE_PUBLIC (t) || DECL_EXTERNAL (t)))
 	    (*trees)[i] = lto_symtab_prevailing_decl (t);
Index: tree-streamer-out.c
===================================================================
--- tree-streamer-out.c	(revision 222391)
+++ tree-streamer-out.c	(working copy)
@@ -721,6 +721,9 @@ static void
 write_ts_type_common_tree_pointers (struct output_block *ob, tree expr,
 				    bool ref_p)
 {
+#ifdef ENABLE_CHECKING
+  verify_type (expr);
+#endif
   stream_write_tree (ob, TYPE_SIZE (expr), ref_p);
   stream_write_tree (ob, TYPE_SIZE_UNIT (expr), ref_p);
   stream_write_tree (ob, TYPE_ATTRIBUTES (expr), ref_p);
Index: dwarf2out.c
===================================================================
--- dwarf2out.c	(revision 222391)
+++ dwarf2out.c	(working copy)
@@ -21264,6 +21264,11 @@ dwarf2out_decl (tree decl)
 {
   dw_die_ref context_die = comp_unit_die ();
 
+#ifdef ENABLE_CHECKING
+  if (TREE_TYPE (decl))
+     verify_type (TREE_TYPE (decl));
+#endif
+
   switch (TREE_CODE (decl))
     {
     case ERROR_MARK:
Index: ipa-chkp.c
===================================================================
--- ipa-chkp.c	(revision 222391)
+++ ipa-chkp.c	(working copy)
@@ -244,7 +244,7 @@ tree
 chkp_copy_function_type_adding_bounds (tree orig_type)
 {
   tree type;
-  tree arg_type, attrs, t;
+  tree arg_type, attrs;
   unsigned len = list_length (TYPE_ARG_TYPES (orig_type));
   unsigned *indexes = XALLOCAVEC (unsigned, len);
   unsigned idx = 0, new_idx = 0;
@@ -327,20 +327,6 @@ chkp_copy_function_type_adding_bounds (t
       TYPE_ATTRIBUTES (type) = attrs;
     }
 
-  t = TYPE_MAIN_VARIANT (orig_type);
-  if (orig_type != t)
-    {
-      TYPE_MAIN_VARIANT (type) = t;
-      TYPE_NEXT_VARIANT (type) = TYPE_NEXT_VARIANT (t);
-      TYPE_NEXT_VARIANT (t) = type;
-    }
-  else
-    {
-      TYPE_MAIN_VARIANT (type) = type;
-      TYPE_NEXT_VARIANT (type) = NULL;
-    }
-
-
   return type;
 }
 

Re: Bare bones of type verifier

[Richard Biener]

On Tue, 28 Apr 2015, Jan Hubicka wrote:

> Hi,
> this patch adds bare bones of type checker.  You can call verify_type on any
> type in the IL and see compiler bomb if some of invariants are broken.  So far
> it only verify tests I already tested in last stage1 with my reverted variant
> streaming patch https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02454.html
> 
> The checks found interesting problems that was fixed. I have other tests and
> fixes but would like to go incrementally.  Some of tests already broke again
> with recent C++ align attribute changes (I think), see FIXME comments.
> I plan to proceed with small steps becuase all the checks seems to trigger
> fun issues.
> 
> The patch still fix on bug in ipa-chkp.c that is obvious enough - it is
> cut&pasted from old code in cgraphunit.c that was updated same way.
> 
> I hope with early debug we are on the track of getting simplified gimple types,
> but to make this without hitting too many surprises I think we first want to
> document and verify our FE type representation and also verify what we need
> in middle-end and drop the rest (in free lang data).
> 
> Placement of verify_type calls may seem bit random. Basic idea is to verify that
> bad types do not hit LTO and are not produced by LTO type merging. In non-LTO path
> we verify types at dwarf2out that is major consumer of the type variants.
> I would be happy to place more calls/relocate existing to better places.

(early) dwarf2out is a good place, likewise verifying right after 
free-lang-data.  I agree that LTO type merging is also a good place.

I hope we get early dwarf2out merged soon and can enable free-lang-data
also for non-LTO compilation.

> LTO-Boostrapped/regtested x86_64-linux without Ada. I am doing full run on
> PPC64, but it fails on unrelated libgmp miscomplation I proably need to track down
> first.
> 
> OK if testing passes?

Please make sure to test _all_ languages (all,ada,obj-c++,go) and also
include multilibs in testing (thus -m64/-m32).

You don't verify that TYPE_CANONICAL is consistent nor that TYPE_CANONICAL
doesn't form a tree (TYPE_CANONICAL (t) == TYPE_CANONICAL (TYPE_CANONICAL 
(t))).

More comments below


> Honza
> 
> 	* tree.c: Include print-tree.h
> 	(verify_type_variant): New function.
> 	(verify_type): New function.
> 	* tree.h (verify_type): Declare.
> 	* tree-streamer-out.c (write_ts_type_common_tree_pointers): Verify type.
> 	* dwarf2out.c (dwarf2out_decl): Verify type.
> 	* ipa-chkp.c (chkp_copy_function_type_adding_bounds): Do not consider
> 	updated type to be variant.
> 
> 	* lto.c (lto_fixup_state): Verify types.
> Index: tree.c
> ===================================================================
> --- tree.c	(revision 222391)
> +++ tree.c	(working copy)
> @@ -102,6 +102,7 @@ along with GCC; see the file COPYING3.
>  #include "debug.h"
>  #include "intl.h"
>  #include "builtins.h"
> +#include "print-tree.h"
>  
>  /* Tree code classes.  */
>  
> @@ -12425,4 +12437,124 @@ element_mode (const_tree t)
>    return TYPE_MODE (t);
>  }
>  
> +/* Veirfy that basic properties of T match TV and thus T can be a variant of
> +   TV.  TV should be the more specified variant (i.e. the main variant).  */
> +
> +static bool
> +verify_type_variant (const_tree t, tree tv)
> +{
> +  if (TREE_CODE (t) != TREE_CODE (tv))
> +    {
> +      error ("type variant has different TREE_CODE");
> +      debug_tree (tv);
> +      return false;
> +    }
> +  if (COMPLETE_TYPE_P (t) && TYPE_SIZE (t) != TYPE_SIZE (tv))
> +    {
> +      error ("type variant has different TYPE_SIZE");
> +      debug_tree (tv);
> +      return false;
> +    }
> +  if (COMPLETE_TYPE_P (t) && TYPE_SIZE_UNIT (t) != TYPE_SIZE_UNIT (tv))
> +    {
> +      error ("type variant has different TYPE_SIZE_UNIT");
> +      debug_tree (tv);
> +      return false;
> +    }
> +  if (RECORD_OR_UNION_TYPE_P (t) && TYPE_VFIELD (t) != TYPE_VFIELD (tv))
> +    {
> +      error ("type variant has different TYPE_VFIELD");
> +      debug_tree (tv);
> +      return false;
> +    }
> +  if (((TREE_CODE (t) == ENUMERAL_TYPE && COMPLETE_TYPE_P (t))
> +	|| TREE_CODE (t) == INTEGER_TYPE
> +	|| TREE_CODE (t) == BOOLEAN_TYPE
> +	|| TREE_CODE (t) == REAL_TYPE
> +	|| TREE_CODE (t) == FIXED_POINT_TYPE)
> +       && (TYPE_MAX_VALUE (t) != TYPE_MAX_VALUE (tv)
> +	   || TYPE_MAX_VALUE (t) != TYPE_MAX_VALUE (tv)))
> +    {
> +      error ("type variant has different TYPE_MIN_VALUE");
> +      debug_tree (tv);
> +      return false;
> +    }
> +  if (TREE_CODE (t) == METHOD_TYPE
> +      && TYPE_METHOD_BASETYPE (t) != TYPE_METHOD_BASETYPE (tv))
> +    {
> +      error ("type variant has different TYPE_METHOD_BASETYPE");
> +      debug_tree (tv);
> +      return false;
> +    }
> +  /* FIXME: Be lax and allow TYPE_METHODS to be NULL.  This is a bug
> +     but affecting only the debug output.  */
> +  if (RECORD_OR_UNION_TYPE_P (t) && COMPLETE_TYPE_P (t)
> +      && TYPE_METHODS (t) && TYPE_METHODS (tv)
> +      && TYPE_METHODS (t) != TYPE_METHODS (tv))
> +    {
> +      error ("type variant has different TYPE_METHODS");
> +      debug_tree (tv);
> +      return false;
> +    }
> +  if (TREE_CODE (t) == OFFSET_TYPE
> +      && TYPE_OFFSET_BASETYPE (t) != TYPE_OFFSET_BASETYPE (tv))
> +    {
> +      error ("type variant has different TYPE_OFFSET_BASETYPE");
> +      debug_tree (tv);
> +      return false;
> +    }
> +  if (TREE_CODE (t) == ARRAY_TYPE
> +      && TYPE_ARRAY_MAX_SIZE (t) != TYPE_ARRAY_MAX_SIZE (tv))
> +    {
> +      error ("type variant has different TYPE_ARRAY_MAX_SIZE");
> +      debug_tree (tv);
> +      return false;
> +    }
> +  /* FIXME: Be lax and allow TYPE_BINFO to be missing in variant types
> +     or even type's main variant.  This is needed to make bootstrap pass
> +     and the bug seems new in GCC 5.
> +     C++ FE should be updated to make this consistent and we should check
> +     that TYPE_BINFO is always NULL for !COMPLETE_TYPE_P and otherwise there
> +     is a match with main variant.  */
> +  if (RECORD_OR_UNION_TYPE_P (t) && TYPE_BINFO (t) && TYPE_BINFO (tv)
> +      && TYPE_BINFO (t) != TYPE_BINFO (tv))
> +    {
> +      error ("type variant has different TYPE_BINFO");
> +      debug_tree (tv);
> +      error ("type variant's TYPE_BINFO");
> +      debug_tree (TYPE_BINFO (tv));
> +      error ("type's TYPE_BINFO");
> +      debug_tree (TYPE_BINFO (tv));
> +      return false;
> +    }
> +  return true;
> +}
> +
> +/* Verify type T.  */
> +
> +void
> +verify_type (const_tree t)
> +{
> +  bool error_found = false;
> +  tree mv = TYPE_MAIN_VARIANT (t);
> +  if (!mv)
> +    {
> +      error ("Main variant is not defined");
> +      error_found = true;
> +    }
> +  else if (mv != TYPE_MAIN_VARIANT (mv))
> +    {
> +      error ("TYPE_MAIN_VARAINT has different TYPE_MAIN_VARIANT");
> +      debug_tree (mv);
> +      error_found = true;
> +    }
> +  else if (t != mv && !verify_type_variant (t, mv))
> +    error_found = true;
> +  if (error_found)
> +    {
> +      debug_tree (const_cast <tree> (t));
> +      internal_error ("verify_type failed");
> +    }
> +}
> +
>  #include "gt-tree.h"
> Index: tree.h
> ===================================================================
> --- tree.h	(revision 222391)
> +++ tree.h	(working copy)
> @@ -4495,6 +4495,7 @@ extern tree drop_tree_overflow (tree);
>  extern int tree_map_base_eq (const void *, const void *);
>  extern unsigned int tree_map_base_hash (const void *);
>  extern int tree_map_base_marked_p (const void *);
> +extern void DEBUG_FUNCTION verify_type (const_tree t);
>  
>  #define tree_map_eq tree_map_base_eq
>  extern unsigned int tree_map_hash (const void *);
> Index: lto/lto.c
> ===================================================================
> --- lto/lto.c	(revision 222391)
> +++ lto/lto.c	(working copy)
> @@ -2844,6 +2844,10 @@ lto_fixup_state (struct lto_in_decl_stat
>        for (i = 0; i < vec_safe_length (trees); i++)
>  	{
>  	  tree t = (*trees)[i];
> +#ifdef ENABLE_CHECKING
> +	  if (TYPE_P (t))
> +	    verify_type (t);
> +#endif
>  	  if (VAR_OR_FUNCTION_DECL_P (t)
>  	      && (TREE_PUBLIC (t) || DECL_EXTERNAL (t)))
>  	    (*trees)[i] = lto_symtab_prevailing_decl (t);
> Index: tree-streamer-out.c
> ===================================================================
> --- tree-streamer-out.c	(revision 222391)
> +++ tree-streamer-out.c	(working copy)
> @@ -721,6 +721,9 @@ static void
>  write_ts_type_common_tree_pointers (struct output_block *ob, tree expr,
>  				    bool ref_p)
>  {
> +#ifdef ENABLE_CHECKING
> +  verify_type (expr);
> +#endif

As said I think that doing this after free-lang-data is better.  Like
here:

  /* Traverse every type found freeing its language data.  */
  FOR_EACH_VEC_ELT (fld.types, i, t)
    free_lang_data_in_type (t);

do another loop verifying types.

>    stream_write_tree (ob, TYPE_SIZE (expr), ref_p);
>    stream_write_tree (ob, TYPE_SIZE_UNIT (expr), ref_p);
>    stream_write_tree (ob, TYPE_ATTRIBUTES (expr), ref_p);
> Index: dwarf2out.c
> ===================================================================
> --- dwarf2out.c	(revision 222391)
> +++ dwarf2out.c	(working copy)
> @@ -21264,6 +21264,11 @@ dwarf2out_decl (tree decl)
>  {
>    dw_die_ref context_die = comp_unit_die ();
>  
> +#ifdef ENABLE_CHECKING
> +  if (TREE_TYPE (decl))
> +     verify_type (TREE_TYPE (decl));
> +#endif
> +

Hmm, this has the chance to verify types multiple times, no?  Wouldn't
gen_type_die_with_usage be a better place to verify the type we create
the DIE for?

So besides placing and doing more checking the patch looks ok to me.

Thanks,
Richard.

>    switch (TREE_CODE (decl))
>      {
>      case ERROR_MARK:
> Index: ipa-chkp.c
> ===================================================================
> --- ipa-chkp.c	(revision 222391)
> +++ ipa-chkp.c	(working copy)
> @@ -244,7 +244,7 @@ tree
>  chkp_copy_function_type_adding_bounds (tree orig_type)
>  {
>    tree type;
> -  tree arg_type, attrs, t;
> +  tree arg_type, attrs;
>    unsigned len = list_length (TYPE_ARG_TYPES (orig_type));
>    unsigned *indexes = XALLOCAVEC (unsigned, len);
>    unsigned idx = 0, new_idx = 0;
> @@ -327,20 +327,6 @@ chkp_copy_function_type_adding_bounds (t
>        TYPE_ATTRIBUTES (type) = attrs;
>      }
>  
> -  t = TYPE_MAIN_VARIANT (orig_type);
> -  if (orig_type != t)
> -    {
> -      TYPE_MAIN_VARIANT (type) = t;
> -      TYPE_NEXT_VARIANT (type) = TYPE_NEXT_VARIANT (t);
> -      TYPE_NEXT_VARIANT (t) = type;
> -    }
> -  else
> -    {
> -      TYPE_MAIN_VARIANT (type) = type;
> -      TYPE_NEXT_VARIANT (type) = NULL;
> -    }
> -
> -
>    return type;
>  }

Re: Bare bones of type verifier

[Jan Hubicka]

Hi,
I will fix the free count issue - my internet access in China was bit sporadic
and I hoped you will know why the count seem to disagree between PPC64 and Linux
builds. In meantime PPC64 stopped to build for me.
> 
> (early) dwarf2out is a good place, likewise verifying right after 
> free-lang-data.  I agree that LTO type merging is also a good place.
> 
> I hope we get early dwarf2out merged soon and can enable free-lang-data
> also for non-LTO compilation.
> 
> > LTO-Boostrapped/regtested x86_64-linux without Ada. I am doing full run on
> > PPC64, but it fails on unrelated libgmp miscomplation I proably need to track down
> > first.
> > 
> > OK if testing passes?
> 
> Please make sure to test _all_ languages (all,ada,obj-c++,go) and also
> include multilibs in testing (thus -m64/-m32).
> 
> You don't verify that TYPE_CANONICAL is consistent nor that TYPE_CANONICAL
> doesn't form a tree (TYPE_CANONICAL (t) == TYPE_CANONICAL (TYPE_CANONICAL 
> (t))).

Yep, I have this one in queue, but of course it fires, thus it is not in the initial patch.
> > +#ifdef ENABLE_CHECKING
> > +  verify_type (expr);
> > +#endif
> 
> As said I think that doing this after free-lang-data is better.  Like
> here:
> 
>   /* Traverse every type found freeing its language data.  */
>   FOR_EACH_VEC_ELT (fld.types, i, t)
>     free_lang_data_in_type (t);
> 
> do another loop verifying types.

OK, we however build new types in middle end (ipa-icf/ipa-split/ipa-sra),
so perhaps we can check on both places?
> 
> >    stream_write_tree (ob, TYPE_SIZE (expr), ref_p);
> >    stream_write_tree (ob, TYPE_SIZE_UNIT (expr), ref_p);
> >    stream_write_tree (ob, TYPE_ATTRIBUTES (expr), ref_p);
> > Index: dwarf2out.c
> > ===================================================================
> > --- dwarf2out.c	(revision 222391)
> > +++ dwarf2out.c	(working copy)
> > @@ -21264,6 +21264,11 @@ dwarf2out_decl (tree decl)
> >  {
> >    dw_die_ref context_die = comp_unit_die ();
> >  
> > +#ifdef ENABLE_CHECKING
> > +  if (TREE_TYPE (decl))
> > +     verify_type (TREE_TYPE (decl));
> > +#endif
> > +
> 
> Hmm, this has the chance to verify types multiple times, no?  Wouldn't
> gen_type_die_with_usage be a better place to verify the type we create
> the DIE for?

That looks better indeed.  The checks are not paritcularly expensive though.

Honza

Re: Bare bones of type verifier

[Bernhard Reutner-Fischer]

On April 28, 2015 11:05:44 AM GMT+02:00, Jan Hubicka <hubicka@ucw.cz> wrote:
>Hi,
>I will fix the free count issue - my internet access in China was bit
>sporadic
>and I hoped you will know why the count seem to disagree between PPC64
>and Linux
>builds. In meantime PPC64 stopped to build for me.
>> 
>> (early) dwarf2out is a good place, likewise verifying right after 
>> free-lang-data.  I agree that LTO type merging is also a good place.
>> 
>> I hope we get early dwarf2out merged soon and can enable
>free-lang-data
>> also for non-LTO compilation.
>> 
>> > LTO-Boostrapped/regtested x86_64-linux without Ada. I am doing full
>run on
>> > PPC64, but it fails on unrelated libgmp miscomplation I proably
>need to track down
>> > first.
>> > 
>> > OK if testing passes?
>> 
>> Please make sure to test _all_ languages (all,ada,obj-c++,go) and
>also
>> include multilibs in testing (thus -m64/-m32).
>> 
>> You don't verify that TYPE_CANONICAL is consistent nor that
>TYPE_CANONICAL
>> doesn't form a tree (TYPE_CANONICAL (t) == TYPE_CANONICAL
>(TYPE_CANONICAL 
>> (t))).
>
>Yep, I have this one in queue, but of course it fires, thus it is not
>in the initial patch.
>> > +#ifdef ENABLE_CHECKING
>> > +  verify_type (expr);
>> > +#endif
>> 
>> As said I think that doing this after free-lang-data is better.  Like
>> here:
>> 
>>   /* Traverse every type found freeing its language data.  */
>>   FOR_EACH_VEC_ELT (fld.types, i, t)
>>     free_lang_data_in_type (t);
>> 
>> do another loop verifying types.
>
>OK, we however build new types in middle end
>(ipa-icf/ipa-split/ipa-sra),
>so perhaps we can check on both places?
>> 
>> >    stream_write_tree (ob, TYPE_SIZE (expr), ref_p);
>> >    stream_write_tree (ob, TYPE_SIZE_UNIT (expr), ref_p);
>> >    stream_write_tree (ob, TYPE_ATTRIBUTES (expr), ref_p);
>> > Index: dwarf2out.c
>> > ===================================================================
>> > --- dwarf2out.c	(revision 222391)
>> > +++ dwarf2out.c	(working copy)
>> > @@ -21264,6 +21264,11 @@ dwarf2out_decl (tree decl)
>> >  {
>> >    dw_die_ref context_die = comp_unit_die ();
>> >  
>> > +#ifdef ENABLE_CHECKING
>> > +  if (TREE_TYPE (decl))
>> > +     verify_type (TREE_TYPE (decl));
>> > +#endif
>> > +
>> 
>> Hmm, this has the chance to verify types multiple times, no? 
>Wouldn't
>> gen_type_die_with_usage be a better place to verify the type we
>create
>> the DIE for?
>
>That looks better indeed.  The checks are not paritcularly expensive
>though.

+ if (((TREE_CODE (t) == ENUMERAL_TYPE && COMPLETE_TYPE_P (t))
+	|| TREE_CODE (t) == INTEGER_TYPE
+	|| TREE_CODE (t) == BOOLEAN_TYPE
+	|| TREE_CODE (t) == REAL_TYPE
+	|| TREE_CODE (t) == FIXED_POINT_TYPE)
+ && (TYPE_MAX_VALUE (t) != TYPE_MAX_VALUE (tv)
+	 || TYPE_MAX_VALUE (t) != TYPE_MAX_VALUE (tv)))
+ {
+ error ("type variant has different TYPE_MIN_VALUE");
+ debug_tree (tv);
+ return false;
+ }

The second || check of TYPE_MAX_VALUE should probably be something else, maybe TYPE_MIN_VALUE ?

Why don't we warn about such useless || where both hands are identical? :)

Thanks,
>
>Honza

Re: Bare bones of type verifier

[Bernhard Reutner-Fischer]

On April 28, 2015 6:01:24 PM GMT+02:00, Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:
>On April 28, 2015 11:05:44 AM GMT+02:00, Jan Hubicka <hubicka@ucw.cz>
>wrote:
>>Hi,
>>I will fix the free count issue - my internet access in China was bit
>>sporadic
>>and I hoped you will know why the count seem to disagree between PPC64
>>and Linux
>>builds. In meantime PPC64 stopped to build for me.
>>> 
>>> (early) dwarf2out is a good place, likewise verifying right after 
>>> free-lang-data.  I agree that LTO type merging is also a good place.
>>> 
>>> I hope we get early dwarf2out merged soon and can enable
>>free-lang-data
>>> also for non-LTO compilation.
>>> 
>>> > LTO-Boostrapped/regtested x86_64-linux without Ada. I am doing
>full
>>run on
>>> > PPC64, but it fails on unrelated libgmp miscomplation I proably
>>need to track down
>>> > first.
>>> > 
>>> > OK if testing passes?
>>> 
>>> Please make sure to test _all_ languages (all,ada,obj-c++,go) and
>>also
>>> include multilibs in testing (thus -m64/-m32).
>>> 
>>> You don't verify that TYPE_CANONICAL is consistent nor that
>>TYPE_CANONICAL
>>> doesn't form a tree (TYPE_CANONICAL (t) == TYPE_CANONICAL
>>(TYPE_CANONICAL 
>>> (t))).
>>
>>Yep, I have this one in queue, but of course it fires, thus it is not
>>in the initial patch.
>>> > +#ifdef ENABLE_CHECKING
>>> > +  verify_type (expr);
>>> > +#endif
>>> 
>>> As said I think that doing this after free-lang-data is better. 
>Like
>>> here:
>>> 
>>>   /* Traverse every type found freeing its language data.  */
>>>   FOR_EACH_VEC_ELT (fld.types, i, t)
>>>     free_lang_data_in_type (t);
>>> 
>>> do another loop verifying types.
>>
>>OK, we however build new types in middle end
>>(ipa-icf/ipa-split/ipa-sra),
>>so perhaps we can check on both places?
>>> 
>>> >    stream_write_tree (ob, TYPE_SIZE (expr), ref_p);
>>> >    stream_write_tree (ob, TYPE_SIZE_UNIT (expr), ref_p);
>>> >    stream_write_tree (ob, TYPE_ATTRIBUTES (expr), ref_p);
>>> > Index: dwarf2out.c
>>> >
>===================================================================
>>> > --- dwarf2out.c	(revision 222391)
>>> > +++ dwarf2out.c	(working copy)
>>> > @@ -21264,6 +21264,11 @@ dwarf2out_decl (tree decl)
>>> >  {
>>> >    dw_die_ref context_die = comp_unit_die ();
>>> >  
>>> > +#ifdef ENABLE_CHECKING
>>> > +  if (TREE_TYPE (decl))
>>> > +     verify_type (TREE_TYPE (decl));
>>> > +#endif
>>> > +
>>> 
>>> Hmm, this has the chance to verify types multiple times, no? 
>>Wouldn't
>>> gen_type_die_with_usage be a better place to verify the type we
>>create
>>> the DIE for?
>>
>>That looks better indeed.  The checks are not paritcularly expensive
>>though.
>
>+ if (((TREE_CODE (t) == ENUMERAL_TYPE && COMPLETE_TYPE_P (t))
>+	|| TREE_CODE (t) == INTEGER_TYPE
>+	|| TREE_CODE (t) == BOOLEAN_TYPE
>+	|| TREE_CODE (t) == REAL_TYPE
>+	|| TREE_CODE (t) == FIXED_POINT_TYPE)
>+ && (TYPE_MAX_VALUE (t) != TYPE_MAX_VALUE (tv)
>+	 || TYPE_MAX_VALUE (t) != TYPE_MAX_VALUE (tv)))
>+ {
>+ error ("type variant has different TYPE_MIN_VALUE");
>+ debug_tree (tv);
>+ return false;
>+ }
>
>The second || check of TYPE_MAX_VALUE should probably be something
>else, maybe TYPE_MIN_VALUE ?
>
>Why don't we warn about such useless || where both hands are identical?
>:)

Also: s/TYPE_MAIN_VARAINT/TYPE_MAIN_VARIANT/g
Thanks,

Re: Bare bones of type verifier

[Jan Hubicka]

> >though.
> 
> + if (((TREE_CODE (t) == ENUMERAL_TYPE && COMPLETE_TYPE_P (t))
> +	|| TREE_CODE (t) == INTEGER_TYPE
> +	|| TREE_CODE (t) == BOOLEAN_TYPE
> +	|| TREE_CODE (t) == REAL_TYPE
> +	|| TREE_CODE (t) == FIXED_POINT_TYPE)
> + && (TYPE_MAX_VALUE (t) != TYPE_MAX_VALUE (tv)
> +	 || TYPE_MAX_VALUE (t) != TYPE_MAX_VALUE (tv)))
> + {
> + error ("type variant has different TYPE_MIN_VALUE");
> + debug_tree (tv);
> + return false;
> + }
> 
> The second || check of TYPE_MAX_VALUE should probably be something else, maybe TYPE_MIN_VALUE ?
> 
> Why don't we warn about such useless || where both hands are identical? :)

Thanks for both corrections! Something I crept in at last moment when restructuring the code
(I want to re-use the variant checking for ODR varaints and also in limited form for canonical
types - combined with existing code to verify canonical type sanity in C++ FE)

Yep, warning would be cute, though it may be bit hard to interpret when the tests are produced
by different maco expansions/inlines.

Honza
> 
> Thanks,
> >
> >Honza
> 

Re: Bare bones of type verifier

[Jan Hubicka]

Hi,
this is updated patch that passes all languages LTO and non-LTO testing on ppc64.
I found few issues
 - C++ FE messes up METHOD pointers becuase sometimes it duplicated a type while
   METHOD list is incomplete, so there are type variants that points to the middle
   of the final list. (I suppose we want to track this only on main variant)
 - C FE messes TYPE_VFIELD in libgo build because it uses it to link
   C_TYPE_INCOMPLETE_VARS. Sometimes it produce type variant and forgets to clear
   the pointer and sometimes it never walks the lists and never clears it.
   I added code to free_lang_data to clear it since it is not quite clear
   how to track this in FE.  
 - Java FE messes up with TYPE_BINFO because it produces dummy BINFO by:
    /* Unfortunately we must create the binfo here, so that class
       loading works.  */
    TYPE_BINFO (type) = make_tree_binfo (0);
   and in some variants it forgets to replace it by real thing later.
   I just relaxed the check for the moment and will look into it incrementally.
   Again I think we may want TYPE_BINFO on main variants only or Java may just
   propagate proper info to all vairant copies. Both would work.
 - Java FE sometimes unnecesarily recomputes TYPE_SIZE_UNIT in different type
   so pointer compare fails even when it could pass (TYPE_SIZE is fine)
   Again I relaxed the sanity check to require sizes to be equal but not pointer
   equivalent and will look into it.

This only covers the few checks implemented in verify_type_variant.  There are
clearly many issues to look into. I will commit this version of patch tomorrow
if there are no complains and will start chasing issues in small stemps.

Honza

	* dwarf2out.c (gen_type_die_with_usage): Call verify_type.
	* ipa-chkp.c (chkp_copy_function_type_adding_bounds): Do not produce
	bugus variants.
	* tree.c: Include print-tree.h and ipa-utils.h
	(free_lang_data_in_type): Clear TYPE_VFIELD leaked by C FE.
	(free_lang_data_in_cgraph): Call verify_type.
	(verify_type_variant): New function.
	(verify_type): New function.
	* tree.h (verify_type): Declare.

	* lto.c (lto_fixup_state): Call verify_type.
Index: dwarf2out.c
===================================================================
--- dwarf2out.c	(revision 222526)
+++ dwarf2out.c	(working copy)
@@ -20238,6 +20238,11 @@ gen_type_die_with_usage (tree type, dw_d
   if (type == NULL_TREE || type == error_mark_node)
     return;
 
+#ifdef ENABLE_CHECKING
+  if (type)
+     verify_type (type);
+#endif
+
   if (TYPE_NAME (type) != NULL_TREE
       && TREE_CODE (TYPE_NAME (type)) == TYPE_DECL
       && is_redundant_typedef (TYPE_NAME (type))
Index: ipa-chkp.c
===================================================================
--- ipa-chkp.c	(revision 222526)
+++ ipa-chkp.c	(working copy)
@@ -244,7 +244,7 @@ tree
 chkp_copy_function_type_adding_bounds (tree orig_type)
 {
   tree type;
-  tree arg_type, attrs, t;
+  tree arg_type, attrs;
   unsigned len = list_length (TYPE_ARG_TYPES (orig_type));
   unsigned *indexes = XALLOCAVEC (unsigned, len);
   unsigned idx = 0, new_idx = 0;
@@ -327,20 +327,6 @@ chkp_copy_function_type_adding_bounds (t
       TYPE_ATTRIBUTES (type) = attrs;
     }
 
-  t = TYPE_MAIN_VARIANT (orig_type);
-  if (orig_type != t)
-    {
-      TYPE_MAIN_VARIANT (type) = t;
-      TYPE_NEXT_VARIANT (type) = TYPE_NEXT_VARIANT (t);
-      TYPE_NEXT_VARIANT (t) = type;
-    }
-  else
-    {
-      TYPE_MAIN_VARIANT (type) = type;
-      TYPE_NEXT_VARIANT (type) = NULL;
-    }
-
-
   return type;
 }
 
Index: lto/lto.c
===================================================================
--- lto/lto.c	(revision 222526)
+++ lto/lto.c	(working copy)
@@ -2844,6 +2844,10 @@ lto_fixup_state (struct lto_in_decl_stat
       for (i = 0; i < vec_safe_length (trees); i++)
 	{
 	  tree t = (*trees)[i];
+#ifdef ENABLE_CHECKING
+	  if (TYPE_P (t))
+	    verify_type (t);
+#endif
 	  if (VAR_OR_FUNCTION_DECL_P (t)
 	      && (TREE_PUBLIC (t) || DECL_EXTERNAL (t)))
 	    (*trees)[i] = lto_symtab_prevailing_decl (t);
Index: tree.c
===================================================================
--- tree.c	(revision 222526)
+++ tree.c	(working copy)
@@ -102,6 +102,8 @@ along with GCC; see the file COPYING3.
 #include "debug.h"
 #include "intl.h"
 #include "builtins.h"
+#include "print-tree.h"
+#include "ipa-utils.h"
 
 /* Tree code classes.  */
 
@@ -5077,6 +5079,11 @@ free_lang_data_in_type (tree type)
       else
 	TYPE_FIELDS (type) = NULL_TREE;
 
+      /* FIXME: C FE uses TYPE_VFIELD to record C_TYPE_INCOMPLETE_VARS
+ 	 and danagle the pointer from time to time.  */
+      if (TYPE_VFIELD (type) && TREE_CODE (TYPE_VFIELD (type)) != FIELD_DECL)
+        TYPE_VFIELD (type) = NULL_TREE;
+
       TYPE_METHODS (type) = NULL_TREE;
       if (TYPE_BINFO (type))
 	{
@@ -5784,6 +5791,10 @@ free_lang_data_in_cgraph (void)
   /* Traverse every type found freeing its language data.  */
   FOR_EACH_VEC_ELT (fld.types, i, t)
     free_lang_data_in_type (t);
+#ifdef ENABLE_CHECKING
+  FOR_EACH_VEC_ELT (fld.types, i, t)
+    verify_type (t);
+#endif
 
   delete fld.pset;
   fld.worklist.release ();
@@ -12425,4 +12436,157 @@ element_mode (const_tree t)
   return TYPE_MODE (t);
 }
 
+/* Veirfy that basic properties of T match TV and thus T can be a variant of
+   TV.  TV should be the more specified variant (i.e. the main variant).  */
+
+static bool
+verify_type_variant (const_tree t, tree tv)
+{
+  if (TREE_CODE (t) != TREE_CODE (tv))
+    {
+      error ("type variant has different TREE_CODE");
+      debug_tree (tv);
+      return false;
+    }
+  if (COMPLETE_TYPE_P (t) && TYPE_SIZE (t) != TYPE_SIZE (tv))
+    {
+      error ("type variant has different TYPE_SIZE");
+      debug_tree (tv);
+      error ("type variant's TYPE_SIZE");
+      debug_tree (TYPE_SIZE (tv));
+      error ("type's TYPE_SIZE");
+      debug_tree (TYPE_SIZE (t));
+      return false;
+    }
+  if (COMPLETE_TYPE_P (t)
+      && TYPE_SIZE_UNIT (t) != TYPE_SIZE_UNIT (tv)
+      /* FIXME: ideally we should compare pointer equality, but java FE produce
+ 	 variants where size is INTEGER_CST of different type (int wrt size_type)
+	 during libjava biuld.  */
+      && !operand_equal_p (TYPE_SIZE_UNIT (t), TYPE_SIZE_UNIT (tv), 0))
+    {
+      error ("type variant has different TYPE_SIZE_UNIT");
+      debug_tree (tv);
+      error ("type variant's TYPE_SIZE_UNIT");
+      debug_tree (TYPE_SIZE_UNIT (tv));
+      error ("type's TYPE_SIZE_UNIT");
+      debug_tree (TYPE_SIZE_UNIT (t));
+      return false;
+    }
+  /* FIXME: C FE uses TYPE_VFIELD to record C_TYPE_INCOMPLETE_VARS
+     and danagle the pointer from time to time.  */
+  if (RECORD_OR_UNION_TYPE_P (t) && TYPE_VFIELD (t) != TYPE_VFIELD (tv)
+      && (!TYPE_VFIELD (tv) || TREE_CODE (TYPE_VFIELD (tv)) != TREE_LIST))
+    {
+      error ("type variant has different TYPE_VFIELD");
+      debug_tree (tv);
+      return false;
+    }
+  if (((TREE_CODE (t) == ENUMERAL_TYPE && COMPLETE_TYPE_P (t))
+	|| TREE_CODE (t) == INTEGER_TYPE
+	|| TREE_CODE (t) == BOOLEAN_TYPE
+	|| TREE_CODE (t) == REAL_TYPE
+	|| TREE_CODE (t) == FIXED_POINT_TYPE)
+       && (TYPE_MAX_VALUE (t) != TYPE_MAX_VALUE (tv)
+	   || TYPE_MIN_VALUE (t) != TYPE_MIN_VALUE (tv)))
+    {
+      error ("type variant has different TYPE_MAX_VALUE or TYPE_MIN_VALUE");
+      debug_tree (tv);
+      return false;
+    }
+  if (TREE_CODE (t) == METHOD_TYPE
+      && TYPE_METHOD_BASETYPE (t) != TYPE_METHOD_BASETYPE (tv))
+    {
+      error ("type variant has different TYPE_METHOD_BASETYPE");
+      debug_tree (tv);
+      return false;
+    }
+  /* FIXME: this check triggers during libstdc++ build that is a bug.
+     It affects non-LTO debug output only, because free_lang_data clears
+     this anyway.  */
+  if (RECORD_OR_UNION_TYPE_P (t) && COMPLETE_TYPE_P (t) && 0
+      && TYPE_METHODS (t) != TYPE_METHODS (tv))
+    {
+      error ("type variant has different TYPE_METHODS");
+      debug_tree (tv);
+      return false;
+    }
+  if (TREE_CODE (t) == OFFSET_TYPE
+      && TYPE_OFFSET_BASETYPE (t) != TYPE_OFFSET_BASETYPE (tv))
+    {
+      error ("type variant has different TYPE_OFFSET_BASETYPE");
+      debug_tree (tv);
+      return false;
+    }
+  if (TREE_CODE (t) == ARRAY_TYPE
+      && TYPE_ARRAY_MAX_SIZE (t) != TYPE_ARRAY_MAX_SIZE (tv))
+    {
+      error ("type variant has different TYPE_ARRAY_MAX_SIZE");
+      debug_tree (tv);
+      return false;
+    }
+  /* FIXME: Be lax and allow TYPE_BINFO to be missing in variant types
+     or even type's main variant.  This is needed to make bootstrap pass
+     and the bug seems new in GCC 5.
+     C++ FE should be updated to make this consistent and we should check
+     that TYPE_BINFO is always NULL for !COMPLETE_TYPE_P and otherwise there
+     is a match with main variant.
+
+     Also disable the check for Java for now because of parser hack that builds
+     first an dummy BINFO and then sometimes replace it by real BINFO in some
+     of the copies.  */
+  if (RECORD_OR_UNION_TYPE_P (t) && TYPE_BINFO (t) && TYPE_BINFO (tv)
+      && TYPE_BINFO (t) != TYPE_BINFO (tv)
+      /* FIXME: Java sometimes keep dump TYPE_BINFOs on variant types.
+	 Since there is no cheap way to tell C++/Java type w/o LTO, do checking
+	 at LTO time only.  */
+      && (in_lto_p && odr_type_p (t)))
+    {
+      error ("type variant has different TYPE_BINFO");
+      debug_tree (tv);
+      error ("type variant's TYPE_BINFO");
+      debug_tree (TYPE_BINFO (tv));
+      error ("type's TYPE_BINFO");
+      debug_tree (TYPE_BINFO (t));
+      return false;
+    }
+  return true;
+}
+
+/* Verify type T.  */
+
+void
+verify_type (const_tree t)
+{
+  bool error_found = false;
+  tree mv = TYPE_MAIN_VARIANT (t);
+  if (!mv)
+    {
+      error ("Main variant is not defined");
+      error_found = true;
+    }
+  else if (mv != TYPE_MAIN_VARIANT (mv))
+    {
+      error ("TYPE_MAIN_VARIANT has different TYPE_MAIN_VARIANT");
+      debug_tree (mv);
+      error_found = true;
+    }
+  else if (t != mv && !verify_type_variant (t, mv))
+    error_found = true;
+  /* FIXME: C FE uses TYPE_VFIELD to record C_TYPE_INCOMPLETE_VARS
+     and danagle the pointer from time to time.  */
+  if (RECORD_OR_UNION_TYPE_P (t) && TYPE_VFIELD (t)
+      && TREE_CODE (TYPE_VFIELD (t)) != FIELD_DECL
+      && TREE_CODE (TYPE_VFIELD (t)) != TREE_LIST)
+    {
+      error ("TYPE_VFIELD is not FIELD_DECL nor TREE_LIST");
+      debug_tree (TYPE_VFIELD (t));
+    }
+  if (error_found)
+    {
+      debug_tree (const_cast <tree> (t));
+      internal_error ("verify_type failed");
+    }
+}
+
 #include "gt-tree.h"
Index: tree.h
===================================================================
--- tree.h	(revision 222526)
+++ tree.h	(working copy)
@@ -4495,6 +4495,7 @@ extern tree drop_tree_overflow (tree);
 extern int tree_map_base_eq (const void *, const void *);
 extern unsigned int tree_map_base_hash (const void *);
 extern int tree_map_base_marked_p (const void *);
+extern void DEBUG_FUNCTION verify_type (const_tree t);
 
 #define tree_map_eq tree_map_base_eq
 extern unsigned int tree_map_hash (const void *);