Am Freitag, dem 03.05.2024 um 14:13 +0200 schrieb Richard Biener:
> TYPE_STRUCTURAL_EQUALITY_P is part of our type system so we have
> to make sure to include that into the type unification done via
> type_hash_canon. This requires the flag to be set before querying
> the hash which is the biggest part of the patch.
I assume this does not affect structs / unions because they
do not make this mechanism of type unification (each tagged type
is a unique type), but only derived types that end up having
TYPE_STRUCTURAL_EQUALITY_P because they are constructed from
incomplete structs / unions before TYPE_CANONICAL is set.
I do not yet understand why this change is needed. Type
identity should not be affected by setting TYPE_CANONICAL, so
why do we need to keep such types separate? I understand that we
created some inconsistencies, but I do not see why this change
is needed to fix it. But I also haven't understood how we ended
up with a TYPE_CANONICAL having TYPE_STRUCTURAL_EQUALITY_P in
PR 114931 ...
Martin
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu for all languages.
>
> As said in the PR this merely makes sure to keep individual types
> consistent with themselves. We still will have a set of types
> with TYPE_STRUCTURAL_EQUALITY_P and a set without that might be
> otherwise identical. That could be only avoided with changes in
> the frontend.
>
> OK for trunk?
>
> Thanks,
> Richard.
>
> PR middle-end/114931
> gcc/
> * tree.cc (type_hash_canon_hash): Hash TYPE_STRUCTURAL_EQUALITY_P.
> (type_cache_hasher::equal): Compare TYPE_STRUCTURAL_EQUALITY_P.
> (build_array_type_1): Set TYPE_STRUCTURAL_EQUALITY_P before
> probing with type_hash_canon.
> (build_function_type): Likewise.
> (build_method_type_directly): Likewise.
> (build_offset_type): Likewise.
> (build_complex_type): Likewise.
> * attribs.cc (build_type_attribute_qual_variant): Likewise.
>
> gcc/c-family/
> * c-common.cc (complete_array_type): Set TYPE_STRUCTURAL_EQUALITY_P
> before probing with type_hash_canon.
>
> gcc/testsuite/
> * gcc.dg/pr114931.c: New testcase.
> ---
> gcc/attribs.cc | 20 +++++-----
> gcc/c-family/c-common.cc | 11 ++++--
> gcc/testsuite/gcc.dg/pr114931.c | 10 +++++
> gcc/tree.cc | 65 +++++++++++++++++++++++----------
> 4 files changed, 74 insertions(+), 32 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/pr114931.c
>
> diff --git a/gcc/attribs.cc b/gcc/attribs.cc
> index 12ffc5f170a..3ab0b0fd87a 100644
> --- a/gcc/attribs.cc
> +++ b/gcc/attribs.cc
> @@ -1336,6 +1336,16 @@ build_type_attribute_qual_variant (tree otype, tree
> attribute, int quals)
> tree dtype = ntype = build_distinct_type_copy (ttype);
>
> TYPE_ATTRIBUTES (ntype) = attribute;
> + /* If the target-dependent attributes make NTYPE different from
> + its canonical type, we will need to use structural equality
> + checks for this type.
> +
> + We shouldn't get here for stripping attributes from a type;
> + the no-attribute type might not need structural comparison. But
> + we can if was discarded from type_hash_table. */
> + if (TYPE_STRUCTURAL_EQUALITY_P (ttype)
> + || !comp_type_attributes (ntype, ttype))
> + SET_TYPE_STRUCTURAL_EQUALITY (ntype);
>
> hashval_t hash = type_hash_canon_hash (ntype);
> ntype = type_hash_canon (hash, ntype);
> @@ -1343,16 +1353,6 @@ build_type_attribute_qual_variant (tree otype, tree
> attribute, int quals)
> if (ntype != dtype)
> /* This variant was already in the hash table, don't mess with
> TYPE_CANONICAL. */;
> - else if (TYPE_STRUCTURAL_EQUALITY_P (ttype)
> - || !comp_type_attributes (ntype, ttype))
> - /* If the target-dependent attributes make NTYPE different from
> - its canonical type, we will need to use structural equality
> - checks for this type.
> -
> - We shouldn't get here for stripping attributes from a type;
> - the no-attribute type might not need structural comparison. But
> - we can if was discarded from type_hash_table. */
> - SET_TYPE_STRUCTURAL_EQUALITY (ntype);
> else if (TYPE_CANONICAL (ntype) == ntype)
> TYPE_CANONICAL (ntype) = TYPE_CANONICAL (ttype);
>
> diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc
> index 01e3d247fc2..032dcb4b41d 100644
> --- a/gcc/c-family/c-common.cc
> +++ b/gcc/c-family/c-common.cc
> @@ -7115,6 +7115,13 @@ complete_array_type (tree *ptype, tree initial_value,
> bool do_default)
> TYPE_TYPELESS_STORAGE (main_type) = TYPE_TYPELESS_STORAGE (type);
> layout_type (main_type);
>
> + /* Set TYPE_STRUCTURAL_EQUALITY_P early. */
> + if (TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (main_type))
> + || TYPE_STRUCTURAL_EQUALITY_P (TYPE_DOMAIN (main_type)))
> + SET_TYPE_STRUCTURAL_EQUALITY (main_type);
> + else
> + TYPE_CANONICAL (main_type) = main_type;
> +
> /* Make sure we have the canonical MAIN_TYPE. */
> hashval_t hashcode = type_hash_canon_hash (main_type);
> main_type = type_hash_canon (hashcode, main_type);
> @@ -7122,7 +7129,7 @@ complete_array_type (tree *ptype, tree initial_value,
> bool do_default)
> /* Fix the canonical type. */
> if (TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (main_type))
> || TYPE_STRUCTURAL_EQUALITY_P (TYPE_DOMAIN (main_type)))
> - SET_TYPE_STRUCTURAL_EQUALITY (main_type);
> + gcc_assert (TYPE_STRUCTURAL_EQUALITY_P (main_type));
> else if (TYPE_CANONICAL (TREE_TYPE (main_type)) != TREE_TYPE (main_type)
> || (TYPE_CANONICAL (TYPE_DOMAIN (main_type))
> != TYPE_DOMAIN (main_type)))
> @@ -7130,8 +7137,6 @@ complete_array_type (tree *ptype, tree initial_value,
> bool do_default)
> = build_array_type (TYPE_CANONICAL (TREE_TYPE (main_type)),
> TYPE_CANONICAL (TYPE_DOMAIN (main_type)),
> TYPE_TYPELESS_STORAGE (main_type));
> - else
> - TYPE_CANONICAL (main_type) = main_type;
>
> if (quals == 0)
> type = main_type;
> diff --git a/gcc/testsuite/gcc.dg/pr114931.c b/gcc/testsuite/gcc.dg/pr114931.c
> new file mode 100644
> index 00000000000..d690ed70e52
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr114931.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile } */
> +/* { dg-options "-std=c23" } */
> +
> +struct Tcl_Obj;
> +void(Tcl_FreeInternalRepProc)(struct Tcl_Obj *);
> +typedef struct Tcl_Obj {
> +} Tcl_Obj;
> +struct {
> + void (*tclFreeObj)(Tcl_Obj *);
> +} Tcl_InitStubs;
> diff --git a/gcc/tree.cc b/gcc/tree.cc
> index 780662549fe..6564b002dc1 100644
> --- a/gcc/tree.cc
> +++ b/gcc/tree.cc
> @@ -6012,6 +6012,8 @@ type_hash_canon_hash (tree type)
>
> hstate.add_int (TREE_CODE (type));
>
> + hstate.add_flag (TYPE_STRUCTURAL_EQUALITY_P (type));
> +
> if (TREE_TYPE (type))
> hstate.add_object (TYPE_HASH (TREE_TYPE (type)));
>
> @@ -6109,6 +6111,10 @@ type_cache_hasher::equal (type_hash *a, type_hash *b)
> || TYPE_MODE (a->type) != TYPE_MODE (b->type)))
> return false;
>
> + if (TYPE_STRUCTURAL_EQUALITY_P (a->type)
> + != TYPE_STRUCTURAL_EQUALITY_P (b->type))
> + return false;
> +
> switch (TREE_CODE (a->type))
> {
> case VOID_TYPE:
> @@ -7347,6 +7353,14 @@ build_array_type_1 (tree elt_type, tree index_type,
> bool typeless_storage,
> TYPE_DOMAIN (t) = index_type;
> TYPE_ADDR_SPACE (t) = TYPE_ADDR_SPACE (elt_type);
> TYPE_TYPELESS_STORAGE (t) = typeless_storage;
> +
> + /* Set TYPE_STRUCTURAL_EQUALITY_P. */
> + if (set_canonical
> + && (TYPE_STRUCTURAL_EQUALITY_P (elt_type)
> + || (index_type && TYPE_STRUCTURAL_EQUALITY_P (index_type))
> + || in_lto_p))
> + SET_TYPE_STRUCTURAL_EQUALITY (t);
> +
> layout_type (t);
>
> if (shared)
> @@ -7363,7 +7377,7 @@ build_array_type_1 (tree elt_type, tree index_type,
> bool typeless_storage,
> if (TYPE_STRUCTURAL_EQUALITY_P (elt_type)
> || (index_type && TYPE_STRUCTURAL_EQUALITY_P (index_type))
> || in_lto_p)
> - SET_TYPE_STRUCTURAL_EQUALITY (t);
> + gcc_unreachable ();
> else if (TYPE_CANONICAL (elt_type) != elt_type
> || (index_type && TYPE_CANONICAL (index_type) != index_type))
> TYPE_CANONICAL (t)
> @@ -7510,21 +7524,25 @@ build_function_type (tree value_type, tree arg_types,
> TYPE_NO_NAMED_ARGS_STDARG_P (t) = 1;
> }
>
> - /* If we already have such a type, use the old one. */
> - hashval_t hash = type_hash_canon_hash (t);
> - tree probe_type = t;
> - t = type_hash_canon (hash, t);
> - if (t != probe_type)
> - return t;
> -
> /* Set up the canonical type. */
> any_structural_p = TYPE_STRUCTURAL_EQUALITY_P (value_type);
> any_noncanonical_p = TYPE_CANONICAL (value_type) != value_type;
> canon_argtypes = maybe_canonicalize_argtypes (arg_types,
> &any_structural_p,
> &any_noncanonical_p);
> + /* Set TYPE_STRUCTURAL_EQUALITY_P early. */
> if (any_structural_p)
> SET_TYPE_STRUCTURAL_EQUALITY (t);
> +
> + /* If we already have such a type, use the old one. */
> + hashval_t hash = type_hash_canon_hash (t);
> + tree probe_type = t;
> + t = type_hash_canon (hash, t);
> + if (t != probe_type)
> + return t;
> +
> + if (any_structural_p)
> + gcc_assert (TYPE_STRUCTURAL_EQUALITY_P (t));
> else if (any_noncanonical_p)
> TYPE_CANONICAL (t) = build_function_type (TYPE_CANONICAL (value_type),
> canon_argtypes);
> @@ -7667,13 +7685,6 @@ build_method_type_directly (tree basetype,
> argtypes = tree_cons (NULL_TREE, ptype, argtypes);
> TYPE_ARG_TYPES (t) = argtypes;
>
> - /* If we already have such a type, use the old one. */
> - hashval_t hash = type_hash_canon_hash (t);
> - tree probe_type = t;
> - t = type_hash_canon (hash, t);
> - if (t != probe_type)
> - return t;
> -
> /* Set up the canonical type. */
> any_structural_p
> = (TYPE_STRUCTURAL_EQUALITY_P (basetype)
> @@ -7684,8 +7695,20 @@ build_method_type_directly (tree basetype,
> canon_argtypes = maybe_canonicalize_argtypes (TREE_CHAIN (argtypes),
> &any_structural_p,
> &any_noncanonical_p);
> +
> + /* Set TYPE_STRUCTURAL_EQUALITY_P early. */
> if (any_structural_p)
> SET_TYPE_STRUCTURAL_EQUALITY (t);
> +
> + /* If we already have such a type, use the old one. */
> + hashval_t hash = type_hash_canon_hash (t);
> + tree probe_type = t;
> + t = type_hash_canon (hash, t);
> + if (t != probe_type)
> + return t;
> +
> + if (any_structural_p)
> + gcc_assert (TYPE_STRUCTURAL_EQUALITY_P (t));
> else if (any_noncanonical_p)
> TYPE_CANONICAL (t)
> = build_method_type_directly (TYPE_CANONICAL (basetype),
> @@ -7726,6 +7749,9 @@ build_offset_type (tree basetype, tree type)
>
> TYPE_OFFSET_BASETYPE (t) = TYPE_MAIN_VARIANT (basetype);
> TREE_TYPE (t) = type;
> + if (TYPE_STRUCTURAL_EQUALITY_P (basetype)
> + || TYPE_STRUCTURAL_EQUALITY_P (type))
> + SET_TYPE_STRUCTURAL_EQUALITY (t);
>
> /* If we already have such a type, use the old one. */
> hashval_t hash = type_hash_canon_hash (t);
> @@ -7741,7 +7767,7 @@ build_offset_type (tree basetype, tree type)
> {
> if (TYPE_STRUCTURAL_EQUALITY_P (basetype)
> || TYPE_STRUCTURAL_EQUALITY_P (type))
> - SET_TYPE_STRUCTURAL_EQUALITY (t);
> + gcc_unreachable ();
> else if (TYPE_CANONICAL (TYPE_MAIN_VARIANT (basetype)) != basetype
> || TYPE_CANONICAL (type) != type)
> TYPE_CANONICAL (t)
> @@ -7770,6 +7796,8 @@ build_complex_type (tree component_type, bool named)
> tree probe = make_node (COMPLEX_TYPE);
>
> TREE_TYPE (probe) = TYPE_MAIN_VARIANT (component_type);
> + if (TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (probe)))
> + SET_TYPE_STRUCTURAL_EQUALITY (probe);
>
> /* If we already have such a type, use the old one. */
> hashval_t hash = type_hash_canon_hash (probe);
> @@ -7781,11 +7809,10 @@ build_complex_type (tree component_type, bool named)
> out the type. We need to check the canonicalization and
> maybe set the name. */
> gcc_checking_assert (COMPLETE_TYPE_P (t)
> - && !TYPE_NAME (t)
> - && TYPE_CANONICAL (t) == t);
> + && !TYPE_NAME (t));
>
> if (TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (t)))
> - SET_TYPE_STRUCTURAL_EQUALITY (t);
> + ;
> else if (TYPE_CANONICAL (TREE_TYPE (t)) != TREE_TYPE (t))
> TYPE_CANONICAL (t)
> = build_complex_type (TYPE_CANONICAL (TREE_TYPE (t)), named);
