Re: Bare bones of type verifier

[Richard Biener <rguenther at suse dot de>]

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;
>  }