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