* [PATCH] Fix TYPE_TYPELESS_STORAGE handling (PR middle-end/80423)
@ 2017-04-18 16:39 Jakub Jelinek
2017-04-19 5:45 ` Jeff Law
2017-04-19 5:58 ` Richard Biener
0 siblings, 2 replies; 8+ messages in thread
From: Jakub Jelinek @ 2017-04-18 16:39 UTC (permalink / raw)
To: Richard Biener, Jason Merrill; +Cc: gcc-patches
Hi!
As mentioned in the PR, we now use TYPE_TYPELESS_STORAGE flag on
ARRAY_TYPEs to denote types that need the special C++ alias handling.
The problem is how is that created, we just use build_array_type and
set TYPE_TYPELESS_STORAGE on the result, but build_array_type uses type
caching, so we might modify that way some other array type.
If all the array type creation goes through build_cplus_array_type, that
wouldn't be a problem, as that flag is dependent just on the element
type, but that is not the case, c-family as well as the middle-end has
lots of spots that also create array types. So in the end whether
one gets TYPE_TYPELESS_STORAGE flag or not is quite random, depends on
GC etc.
The following patch attempts to resolve this, by making the type hashing
take that flag into account. Bootstrapped/regtested on x86_64-linux and
i686-linux, ok for trunk?
2017-04-18 Jakub Jelinek <jakub@redhat.com>
PR middle-end/80423
* tree.h (build_array_type_1): New prototype.
* tree.c (type_cache_hasher::equal): Also compare
TYPE_TYPELESS_STORAGE flag for ARRAY_TYPEs.
(build_array_type_1): No longer static. Add typeless_storage
argument, set TYPE_TYPELESS_STORAGE to it, if shared also
hash it, and pass to recursive call.
(build_array_type, build_nonshared_array_type): Adjust
build_array_type_1 callers.
c-family/
* c-common.c (complete_array_type): Preserve TYPE_TYPELESS_STORAGE.
cp/
* tree.c (build_cplus_array_type): Call build_array_type_1
with the intended TYPE_TYPELESS_STORAGE flag value, instead
of calling build_array_type and modifying TYPE_TYPELESS_STORAGE
on the shared type.
testsuite/
* g++.dg/other/pr80423.C: New test.
--- gcc/tree.h.jj 2017-04-12 13:22:23.000000000 +0200
+++ gcc/tree.h 2017-04-18 12:38:02.981708334 +0200
@@ -4068,6 +4068,7 @@ extern tree build_truth_vector_type (uns
extern tree build_same_sized_truth_vector_type (tree vectype);
extern tree build_opaque_vector_type (tree innertype, int nunits);
extern tree build_index_type (tree);
+extern tree build_array_type_1 (tree, tree, bool, bool);
extern tree build_array_type (tree, tree);
extern tree build_nonshared_array_type (tree, tree);
extern tree build_array_type_nelts (tree, unsigned HOST_WIDE_INT);
--- gcc/tree.c.jj 2017-04-07 11:46:46.000000000 +0200
+++ gcc/tree.c 2017-04-18 12:37:38.765024350 +0200
@@ -7073,7 +7073,9 @@ type_cache_hasher::equal (type_hash *a,
break;
return 0;
case ARRAY_TYPE:
- return TYPE_DOMAIN (a->type) == TYPE_DOMAIN (b->type);
+ return (TYPE_TYPELESS_STORAGE (a->type)
+ == TYPE_TYPELESS_STORAGE (b->type)
+ && TYPE_DOMAIN (a->type) == TYPE_DOMAIN (b->type));
case RECORD_TYPE:
case UNION_TYPE:
@@ -8352,8 +8354,9 @@ subrange_type_for_debug_p (const_tree ty
and number of elements specified by the range of values of INDEX_TYPE.
If SHARED is true, reuse such a type that has already been constructed. */
-static tree
-build_array_type_1 (tree elt_type, tree index_type, bool shared)
+tree
+build_array_type_1 (tree elt_type, tree index_type, bool shared,
+ bool typeless_storage)
{
tree t;
@@ -8367,6 +8370,7 @@ build_array_type_1 (tree elt_type, tree
TREE_TYPE (t) = elt_type;
TYPE_DOMAIN (t) = index_type;
TYPE_ADDR_SPACE (t) = TYPE_ADDR_SPACE (elt_type);
+ TYPE_TYPELESS_STORAGE (t) = typeless_storage;
layout_type (t);
/* If the element type is incomplete at this point we get marked for
@@ -8381,6 +8385,7 @@ build_array_type_1 (tree elt_type, tree
hstate.add_object (TYPE_HASH (elt_type));
if (index_type)
hstate.add_object (TYPE_HASH (index_type));
+ hstate.add_flag (typeless_storage);
t = type_hash_canon (hstate.end (), t);
}
@@ -8396,7 +8401,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,
- shared);
+ shared, typeless_storage);
}
return t;
@@ -8407,7 +8412,7 @@ build_array_type_1 (tree elt_type, tree
tree
build_array_type (tree elt_type, tree index_type)
{
- return build_array_type_1 (elt_type, index_type, true);
+ return build_array_type_1 (elt_type, index_type, true, false);
}
/* Wrapper around build_array_type_1 with SHARED set to false. */
@@ -8415,7 +8420,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);
+ return build_array_type_1 (elt_type, index_type, false, false);
}
/* Return a representation of ELT_TYPE[NELTS], using indices of type
--- gcc/c-family/c-common.c.jj 2017-04-10 22:26:55.000000000 +0200
+++ gcc/c-family/c-common.c 2017-04-18 14:43:53.448092585 +0200
@@ -6397,7 +6397,6 @@ complete_array_type (tree *ptype, tree i
{
tree maxindex, type, main_type, elt, unqual_elt;
int failure = 0, quals;
- hashval_t hashcode = 0;
bool overflow_p = false;
maxindex = size_zero_node;
@@ -6491,13 +6490,15 @@ complete_array_type (tree *ptype, tree i
TYPE_DOMAIN (main_type)
= build_range_type (TREE_TYPE (maxindex),
build_int_cst (TREE_TYPE (maxindex), 0), maxindex);
+ TYPE_TYPELESS_STORAGE (main_type) = TYPE_TYPELESS_STORAGE (type);
layout_type (main_type);
/* Make sure we have the canonical MAIN_TYPE. */
- hashcode = iterative_hash_object (TYPE_HASH (unqual_elt), hashcode);
- hashcode = iterative_hash_object (TYPE_HASH (TYPE_DOMAIN (main_type)),
- hashcode);
- main_type = type_hash_canon (hashcode, main_type);
+ inchash::hash hstate;
+ hstate.add_object (TYPE_HASH (unqual_elt));
+ hstate.add_object (TYPE_HASH (TYPE_DOMAIN (main_type)));
+ hstate.add_flag (TYPE_TYPELESS_STORAGE (main_type));
+ main_type = type_hash_canon (hstate.end (), main_type);
/* Fix the canonical type. */
if (TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (main_type))
@@ -6507,8 +6508,9 @@ complete_array_type (tree *ptype, tree i
|| (TYPE_CANONICAL (TYPE_DOMAIN (main_type))
!= TYPE_DOMAIN (main_type)))
TYPE_CANONICAL (main_type)
- = build_array_type (TYPE_CANONICAL (TREE_TYPE (main_type)),
- TYPE_CANONICAL (TYPE_DOMAIN (main_type)));
+ = build_array_type_1 (TYPE_CANONICAL (TREE_TYPE (main_type)),
+ TYPE_CANONICAL (TYPE_DOMAIN (main_type)),
+ true, TYPE_TYPELESS_STORAGE (main_type));
else
TYPE_CANONICAL (main_type) = main_type;
--- gcc/cp/tree.c.jj 2017-04-18 09:11:54.000000000 +0200
+++ gcc/cp/tree.c 2017-04-18 14:42:28.911225896 +0200
@@ -949,14 +949,14 @@ build_cplus_array_type (tree elt_type, t
}
else
{
- t = build_array_type (elt_type, index_type);
- if (elt_type == unsigned_char_type_node
- || elt_type == signed_char_type_node
- || elt_type == char_type_node
- || (TREE_CODE (elt_type) == ENUMERAL_TYPE
- && TYPE_CONTEXT (elt_type) == std_node
- && !strcmp ("byte", TYPE_NAME_STRING (elt_type))))
- TYPE_TYPELESS_STORAGE (t) = 1;
+ bool typeless_storage
+ = (elt_type == unsigned_char_type_node
+ || elt_type == signed_char_type_node
+ || elt_type == char_type_node
+ || (TREE_CODE (elt_type) == ENUMERAL_TYPE
+ && TYPE_CONTEXT (elt_type) == std_node
+ && !strcmp ("byte", TYPE_NAME_STRING (elt_type))));
+ t = build_array_type_1 (elt_type, index_type, true, typeless_storage);
}
/* Now check whether we already have this array variant. */
--- gcc/testsuite/g++.dg/other/pr80423.C.jj 2017-04-18 14:48:21.098510615 +0200
+++ gcc/testsuite/g++.dg/other/pr80423.C 2017-04-18 14:49:02.049964200 +0200
@@ -0,0 +1,11 @@
+// PR middle-end/80423
+// { dg-do compile { target c++11 } }
+
+typedef unsigned char uint8_t;
+struct A {
+ template <int N> A(unsigned char (&)[N]);
+};
+void fn1(A) {
+ uint8_t a[]{0};
+ fn1(a);
+}
Jakub
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix TYPE_TYPELESS_STORAGE handling (PR middle-end/80423)
2017-04-18 16:39 [PATCH] Fix TYPE_TYPELESS_STORAGE handling (PR middle-end/80423) Jakub Jelinek
@ 2017-04-19 5:45 ` Jeff Law
2017-04-19 5:58 ` Richard Biener
1 sibling, 0 replies; 8+ messages in thread
From: Jeff Law @ 2017-04-19 5:45 UTC (permalink / raw)
To: Jakub Jelinek, Richard Biener, Jason Merrill; +Cc: gcc-patches
On 04/18/2017 09:14 AM, Jakub Jelinek wrote:
> Hi!
>
> As mentioned in the PR, we now use TYPE_TYPELESS_STORAGE flag on
> ARRAY_TYPEs to denote types that need the special C++ alias handling.
> The problem is how is that created, we just use build_array_type and
> set TYPE_TYPELESS_STORAGE on the result, but build_array_type uses type
> caching, so we might modify that way some other array type.
> If all the array type creation goes through build_cplus_array_type, that
> wouldn't be a problem, as that flag is dependent just on the element
> type, but that is not the case, c-family as well as the middle-end has
> lots of spots that also create array types. So in the end whether
> one gets TYPE_TYPELESS_STORAGE flag or not is quite random, depends on
> GC etc.
>
> The following patch attempts to resolve this, by making the type hashing
> take that flag into account. Bootstrapped/regtested on x86_64-linux and
> i686-linux, ok for trunk?
>
> 2017-04-18 Jakub Jelinek <jakub@redhat.com>
>
> PR middle-end/80423
> * tree.h (build_array_type_1): New prototype.
> * tree.c (type_cache_hasher::equal): Also compare
> TYPE_TYPELESS_STORAGE flag for ARRAY_TYPEs.
> (build_array_type_1): No longer static. Add typeless_storage
> argument, set TYPE_TYPELESS_STORAGE to it, if shared also
> hash it, and pass to recursive call.
> (build_array_type, build_nonshared_array_type): Adjust
> build_array_type_1 callers.
> c-family/
> * c-common.c (complete_array_type): Preserve TYPE_TYPELESS_STORAGE.
> cp/
> * tree.c (build_cplus_array_type): Call build_array_type_1
> with the intended TYPE_TYPELESS_STORAGE flag value, instead
> of calling build_array_type and modifying TYPE_TYPELESS_STORAGE
> on the shared type.
> testsuite/
> * g++.dg/other/pr80423.C: New test.
Rather than exporting build_array_type_1, which seems rather gross, why
not add a default argument for typeless storage to build_array_type
prototype in tree.h? That way existing users "just work", and in the
cases where we may want to specify typeless storage, we can pass it into
the build_array_type wrapper.
That seems marginally cleaner to me.
jeff
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix TYPE_TYPELESS_STORAGE handling (PR middle-end/80423)
2017-04-18 16:39 [PATCH] Fix TYPE_TYPELESS_STORAGE handling (PR middle-end/80423) Jakub Jelinek
2017-04-19 5:45 ` Jeff Law
@ 2017-04-19 5:58 ` Richard Biener
2017-04-19 6:20 ` Jakub Jelinek
1 sibling, 1 reply; 8+ messages in thread
From: Richard Biener @ 2017-04-19 5:58 UTC (permalink / raw)
To: Jakub Jelinek, Jason Merrill; +Cc: gcc-patches
On April 18, 2017 5:14:30 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>As mentioned in the PR, we now use TYPE_TYPELESS_STORAGE flag on
>ARRAY_TYPEs to denote types that need the special C++ alias handling.
>The problem is how is that created, we just use build_array_type and
>set TYPE_TYPELESS_STORAGE on the result, but build_array_type uses type
>caching, so we might modify that way some other array type.
>If all the array type creation goes through build_cplus_array_type,
>that
>wouldn't be a problem, as that flag is dependent just on the element
>type, but that is not the case, c-family as well as the middle-end has
>lots of spots that also create array types. So in the end whether
>one gets TYPE_TYPELESS_STORAGE flag or not is quite random, depends on
>GC etc.
>
>The following patch attempts to resolve this, by making the type
>hashing
>take that flag into account. Bootstrapped/regtested on x86_64-linux
>and
>i686-linux, ok for trunk?
When changing the C++ function I thought that calling build_array_type was wrong and it should instead do the same it does in the other places, use its raw creation routine and then the canonical type register stuff. But I was hesitant to change this at this point.
I'm on FTO today so can't experiment with that before torrow.
Richard.
>2017-04-18 Jakub Jelinek <jakub@redhat.com>
>
> PR middle-end/80423
> * tree.h (build_array_type_1): New prototype.
> * tree.c (type_cache_hasher::equal): Also compare
> TYPE_TYPELESS_STORAGE flag for ARRAY_TYPEs.
> (build_array_type_1): No longer static. Add typeless_storage
> argument, set TYPE_TYPELESS_STORAGE to it, if shared also
> hash it, and pass to recursive call.
> (build_array_type, build_nonshared_array_type): Adjust
> build_array_type_1 callers.
>c-family/
> * c-common.c (complete_array_type): Preserve TYPE_TYPELESS_STORAGE.
>cp/
> * tree.c (build_cplus_array_type): Call build_array_type_1
> with the intended TYPE_TYPELESS_STORAGE flag value, instead
> of calling build_array_type and modifying TYPE_TYPELESS_STORAGE
> on the shared type.
>testsuite/
> * g++.dg/other/pr80423.C: New test.
>
>--- gcc/tree.h.jj 2017-04-12 13:22:23.000000000 +0200
>+++ gcc/tree.h 2017-04-18 12:38:02.981708334 +0200
>@@ -4068,6 +4068,7 @@ extern tree build_truth_vector_type (uns
> extern tree build_same_sized_truth_vector_type (tree vectype);
> extern tree build_opaque_vector_type (tree innertype, int nunits);
> extern tree build_index_type (tree);
>+extern tree build_array_type_1 (tree, tree, bool, bool);
> extern tree build_array_type (tree, tree);
> extern tree build_nonshared_array_type (tree, tree);
> extern tree build_array_type_nelts (tree, unsigned HOST_WIDE_INT);
>--- gcc/tree.c.jj 2017-04-07 11:46:46.000000000 +0200
>+++ gcc/tree.c 2017-04-18 12:37:38.765024350 +0200
>@@ -7073,7 +7073,9 @@ type_cache_hasher::equal (type_hash *a,
> break;
> return 0;
> case ARRAY_TYPE:
>- return TYPE_DOMAIN (a->type) == TYPE_DOMAIN (b->type);
>+ return (TYPE_TYPELESS_STORAGE (a->type)
>+ == TYPE_TYPELESS_STORAGE (b->type)
>+ && TYPE_DOMAIN (a->type) == TYPE_DOMAIN (b->type));
>
> case RECORD_TYPE:
> case UNION_TYPE:
>@@ -8352,8 +8354,9 @@ subrange_type_for_debug_p (const_tree ty
> and number of elements specified by the range of values of INDEX_TYPE.
>If SHARED is true, reuse such a type that has already been constructed.
> */
>
>-static tree
>-build_array_type_1 (tree elt_type, tree index_type, bool shared)
>+tree
>+build_array_type_1 (tree elt_type, tree index_type, bool shared,
>+ bool typeless_storage)
> {
> tree t;
>
>@@ -8367,6 +8370,7 @@ build_array_type_1 (tree elt_type, tree
> TREE_TYPE (t) = elt_type;
> TYPE_DOMAIN (t) = index_type;
> TYPE_ADDR_SPACE (t) = TYPE_ADDR_SPACE (elt_type);
>+ TYPE_TYPELESS_STORAGE (t) = typeless_storage;
> layout_type (t);
>
> /* If the element type is incomplete at this point we get marked for
>@@ -8381,6 +8385,7 @@ build_array_type_1 (tree elt_type, tree
> hstate.add_object (TYPE_HASH (elt_type));
> if (index_type)
> hstate.add_object (TYPE_HASH (index_type));
>+ hstate.add_flag (typeless_storage);
> t = type_hash_canon (hstate.end (), t);
> }
>
>@@ -8396,7 +8401,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,
>- shared);
>+ shared, typeless_storage);
> }
>
> return t;
>@@ -8407,7 +8412,7 @@ build_array_type_1 (tree elt_type, tree
> tree
> build_array_type (tree elt_type, tree index_type)
> {
>- return build_array_type_1 (elt_type, index_type, true);
>+ return build_array_type_1 (elt_type, index_type, true, false);
> }
>
> /* Wrapper around build_array_type_1 with SHARED set to false. */
>@@ -8415,7 +8420,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);
>+ return build_array_type_1 (elt_type, index_type, false, false);
> }
>
> /* Return a representation of ELT_TYPE[NELTS], using indices of type
>--- gcc/c-family/c-common.c.jj 2017-04-10 22:26:55.000000000 +0200
>+++ gcc/c-family/c-common.c 2017-04-18 14:43:53.448092585 +0200
>@@ -6397,7 +6397,6 @@ complete_array_type (tree *ptype, tree i
> {
> tree maxindex, type, main_type, elt, unqual_elt;
> int failure = 0, quals;
>- hashval_t hashcode = 0;
> bool overflow_p = false;
>
> maxindex = size_zero_node;
>@@ -6491,13 +6490,15 @@ complete_array_type (tree *ptype, tree i
> TYPE_DOMAIN (main_type)
> = build_range_type (TREE_TYPE (maxindex),
> build_int_cst (TREE_TYPE (maxindex), 0), maxindex);
>+ TYPE_TYPELESS_STORAGE (main_type) = TYPE_TYPELESS_STORAGE (type);
> layout_type (main_type);
>
> /* Make sure we have the canonical MAIN_TYPE. */
>- hashcode = iterative_hash_object (TYPE_HASH (unqual_elt), hashcode);
>- hashcode = iterative_hash_object (TYPE_HASH (TYPE_DOMAIN
>(main_type)),
>- hashcode);
>- main_type = type_hash_canon (hashcode, main_type);
>+ inchash::hash hstate;
>+ hstate.add_object (TYPE_HASH (unqual_elt));
>+ hstate.add_object (TYPE_HASH (TYPE_DOMAIN (main_type)));
>+ hstate.add_flag (TYPE_TYPELESS_STORAGE (main_type));
>+ main_type = type_hash_canon (hstate.end (), main_type);
>
> /* Fix the canonical type. */
> if (TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (main_type))
>@@ -6507,8 +6508,9 @@ complete_array_type (tree *ptype, tree i
> || (TYPE_CANONICAL (TYPE_DOMAIN (main_type))
> != TYPE_DOMAIN (main_type)))
> TYPE_CANONICAL (main_type)
>- = build_array_type (TYPE_CANONICAL (TREE_TYPE (main_type)),
>- TYPE_CANONICAL (TYPE_DOMAIN (main_type)));
>+ = build_array_type_1 (TYPE_CANONICAL (TREE_TYPE (main_type)),
>+ TYPE_CANONICAL (TYPE_DOMAIN (main_type)),
>+ true, TYPE_TYPELESS_STORAGE (main_type));
> else
> TYPE_CANONICAL (main_type) = main_type;
>
>--- gcc/cp/tree.c.jj 2017-04-18 09:11:54.000000000 +0200
>+++ gcc/cp/tree.c 2017-04-18 14:42:28.911225896 +0200
>@@ -949,14 +949,14 @@ build_cplus_array_type (tree elt_type, t
> }
> else
> {
>- t = build_array_type (elt_type, index_type);
>- if (elt_type == unsigned_char_type_node
>- || elt_type == signed_char_type_node
>- || elt_type == char_type_node
>- || (TREE_CODE (elt_type) == ENUMERAL_TYPE
>- && TYPE_CONTEXT (elt_type) == std_node
>- && !strcmp ("byte", TYPE_NAME_STRING (elt_type))))
>- TYPE_TYPELESS_STORAGE (t) = 1;
>+ bool typeless_storage
>+ = (elt_type == unsigned_char_type_node
>+ || elt_type == signed_char_type_node
>+ || elt_type == char_type_node
>+ || (TREE_CODE (elt_type) == ENUMERAL_TYPE
>+ && TYPE_CONTEXT (elt_type) == std_node
>+ && !strcmp ("byte", TYPE_NAME_STRING (elt_type))));
>+ t = build_array_type_1 (elt_type, index_type, true,
>typeless_storage);
> }
>
> /* Now check whether we already have this array variant. */
>--- gcc/testsuite/g++.dg/other/pr80423.C.jj 2017-04-18
>14:48:21.098510615 +0200
>+++ gcc/testsuite/g++.dg/other/pr80423.C 2017-04-18 14:49:02.049964200
>+0200
>@@ -0,0 +1,11 @@
>+// PR middle-end/80423
>+// { dg-do compile { target c++11 } }
>+
>+typedef unsigned char uint8_t;
>
>+struct A {
>
>+ template <int N> A(unsigned char (&)[N]);
>
>+};
>
>+void fn1(A) {
>
>+ uint8_t a[]{0};
>
>+ fn1(a);
>
>+}
>
>
> Jakub
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix TYPE_TYPELESS_STORAGE handling (PR middle-end/80423)
2017-04-19 5:58 ` Richard Biener
@ 2017-04-19 6:20 ` Jakub Jelinek
2017-04-19 6:30 ` Jakub Jelinek
2017-04-19 8:06 ` Richard Biener
0 siblings, 2 replies; 8+ messages in thread
From: Jakub Jelinek @ 2017-04-19 6:20 UTC (permalink / raw)
To: Richard Biener; +Cc: Jason Merrill, gcc-patches
On Wed, Apr 19, 2017 at 07:45:36AM +0200, Richard Biener wrote:
> >As mentioned in the PR, we now use TYPE_TYPELESS_STORAGE flag on
> >ARRAY_TYPEs to denote types that need the special C++ alias handling.
> >The problem is how is that created, we just use build_array_type and
> >set TYPE_TYPELESS_STORAGE on the result, but build_array_type uses type
> >caching, so we might modify that way some other array type.
> >If all the array type creation goes through build_cplus_array_type,
> >that
> >wouldn't be a problem, as that flag is dependent just on the element
> >type, but that is not the case, c-family as well as the middle-end has
> >lots of spots that also create array types. So in the end whether
> >one gets TYPE_TYPELESS_STORAGE flag or not is quite random, depends on
> >GC etc.
> >
> >The following patch attempts to resolve this, by making the type
> >hashing
> >take that flag into account. Bootstrapped/regtested on x86_64-linux
> >and
> >i686-linux, ok for trunk?
>
> When changing the C++ function I thought that calling build_array_type was
> wrong and it should instead do the same it does in the other places, use
> its raw creation routine and then the canonical type register stuff. But
> I was hesitant to change this at this point.
The problem is that as the patch shows, we don't need it in a single place
(the C++ FE), but at least in two places (C++ FE and c-family), and it
wouldn't surprise me if we don't need it later on in further places
(e.g. in middle-end, if we have a TYPE_TYPELESS_STORAGE array and say DSE
wants to create a smaller one with the same property).
Using a default argument to build_array_type is likely cleaner indeed,
I'd just then also swap the arguments to build_array_type_1 (the shared
vs. typeless_storage).
Jakub
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix TYPE_TYPELESS_STORAGE handling (PR middle-end/80423)
2017-04-19 6:20 ` Jakub Jelinek
@ 2017-04-19 6:30 ` Jakub Jelinek
2017-04-20 7:46 ` Jakub Jelinek
2017-04-19 8:06 ` Richard Biener
1 sibling, 1 reply; 8+ messages in thread
From: Jakub Jelinek @ 2017-04-19 6:30 UTC (permalink / raw)
To: Richard Biener; +Cc: Jason Merrill, gcc-patches
On Wed, Apr 19, 2017 at 07:56:30AM +0200, Jakub Jelinek wrote:
> On Wed, Apr 19, 2017 at 07:45:36AM +0200, Richard Biener wrote:
> > >As mentioned in the PR, we now use TYPE_TYPELESS_STORAGE flag on
> > >ARRAY_TYPEs to denote types that need the special C++ alias handling.
> > >The problem is how is that created, we just use build_array_type and
> > >set TYPE_TYPELESS_STORAGE on the result, but build_array_type uses type
> > >caching, so we might modify that way some other array type.
> > >If all the array type creation goes through build_cplus_array_type,
> > >that
> > >wouldn't be a problem, as that flag is dependent just on the element
> > >type, but that is not the case, c-family as well as the middle-end has
> > >lots of spots that also create array types. So in the end whether
> > >one gets TYPE_TYPELESS_STORAGE flag or not is quite random, depends on
> > >GC etc.
> > >
> > >The following patch attempts to resolve this, by making the type
> > >hashing
> > >take that flag into account. Bootstrapped/regtested on x86_64-linux
> > >and
> > >i686-linux, ok for trunk?
> >
> > When changing the C++ function I thought that calling build_array_type was
> > wrong and it should instead do the same it does in the other places, use
> > its raw creation routine and then the canonical type register stuff. But
> > I was hesitant to change this at this point.
>
> The problem is that as the patch shows, we don't need it in a single place
> (the C++ FE), but at least in two places (C++ FE and c-family), and it
> wouldn't surprise me if we don't need it later on in further places
> (e.g. in middle-end, if we have a TYPE_TYPELESS_STORAGE array and say DSE
> wants to create a smaller one with the same property).
>
> Using a default argument to build_array_type is likely cleaner indeed,
> I'd just then also swap the arguments to build_array_type_1 (the shared
> vs. typeless_storage).
Here in (so far untested) patch form:
2017-04-19 Jakub Jelinek <jakub@redhat.com>
PR middle-end/80423
* tree.h (build_array_type): Add typeless_storage default argument.
* tree.c (type_cache_hasher::equal): Also compare
TYPE_TYPELESS_STORAGE flag for ARRAY_TYPEs.
(build_array_type): Add typeless_storage argument, set
TYPE_TYPELESS_STORAGE to it, if shared also hash it, and pass to
recursive call.
(build_nonshared_array_type): Adjust build_array_type_1 caller.
(build_array_type): Likewise. Add typeless_storage argument.
c-family/
* c-common.c (complete_array_type): Preserve TYPE_TYPELESS_STORAGE.
cp/
* tree.c (build_cplus_array_type): Call build_array_type
with the intended TYPE_TYPELESS_STORAGE flag value, instead
of calling build_array_type and modifying later TYPE_TYPELESS_STORAGE
on the shared type.
testsuite/
* g++.dg/other/pr80423.C: New test.
--- gcc/tree.h.jj 2017-04-18 15:13:25.180398014 +0200
+++ gcc/tree.h 2017-04-19 08:16:12.859844328 +0200
@@ -4068,7 +4068,7 @@ extern tree build_truth_vector_type (uns
extern tree build_same_sized_truth_vector_type (tree vectype);
extern tree build_opaque_vector_type (tree innertype, int nunits);
extern tree build_index_type (tree);
-extern tree build_array_type (tree, tree);
+extern tree build_array_type (tree, tree, bool = false);
extern tree build_nonshared_array_type (tree, tree);
extern tree build_array_type_nelts (tree, unsigned HOST_WIDE_INT);
extern tree build_function_type (tree, tree);
--- gcc/tree.c.jj 2017-04-18 15:13:25.158398308 +0200
+++ gcc/tree.c 2017-04-19 08:17:50.938542960 +0200
@@ -7073,7 +7073,9 @@ type_cache_hasher::equal (type_hash *a,
break;
return 0;
case ARRAY_TYPE:
- return TYPE_DOMAIN (a->type) == TYPE_DOMAIN (b->type);
+ return (TYPE_TYPELESS_STORAGE (a->type)
+ == TYPE_TYPELESS_STORAGE (b->type)
+ && TYPE_DOMAIN (a->type) == TYPE_DOMAIN (b->type));
case RECORD_TYPE:
case UNION_TYPE:
@@ -8350,10 +8352,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. */
static tree
-build_array_type_1 (tree elt_type, tree index_type, bool shared)
+build_array_type_1 (tree elt_type, tree index_type, bool typeless_storage,
+ bool shared)
{
tree t;
@@ -8367,6 +8371,7 @@ build_array_type_1 (tree elt_type, tree
TREE_TYPE (t) = elt_type;
TYPE_DOMAIN (t) = index_type;
TYPE_ADDR_SPACE (t) = TYPE_ADDR_SPACE (elt_type);
+ TYPE_TYPELESS_STORAGE (t) = typeless_storage;
layout_type (t);
/* If the element type is incomplete at this point we get marked for
@@ -8381,6 +8386,7 @@ build_array_type_1 (tree elt_type, tree
hstate.add_object (TYPE_HASH (elt_type));
if (index_type)
hstate.add_object (TYPE_HASH (index_type));
+ hstate.add_flag (typeless_storage);
t = type_hash_canon (hstate.end (), t);
}
@@ -8396,7 +8402,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,
- shared);
+ typeless_storage, shared);
}
return t;
@@ -8405,9 +8411,9 @@ build_array_type_1 (tree elt_type, tree
/* Wrapper around build_array_type_1 with SHARED set to true. */
tree
-build_array_type (tree elt_type, tree index_type)
+build_array_type (tree elt_type, tree index_type, bool typeless_storage)
{
- return build_array_type_1 (elt_type, index_type, true);
+ return build_array_type_1 (elt_type, index_type, typeless_storage, true);
}
/* Wrapper around build_array_type_1 with SHARED set to false. */
@@ -8415,7 +8421,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);
+ return build_array_type_1 (elt_type, index_type, false, false);
}
/* Return a representation of ELT_TYPE[NELTS], using indices of type
--- gcc/c-family/c-common.c.jj 2017-04-18 15:14:29.383541136 +0200
+++ gcc/c-family/c-common.c 2017-04-19 08:15:55.346076712 +0200
@@ -6386,7 +6386,6 @@ complete_array_type (tree *ptype, tree i
{
tree maxindex, type, main_type, elt, unqual_elt;
int failure = 0, quals;
- hashval_t hashcode = 0;
bool overflow_p = false;
maxindex = size_zero_node;
@@ -6480,13 +6479,15 @@ complete_array_type (tree *ptype, tree i
TYPE_DOMAIN (main_type)
= build_range_type (TREE_TYPE (maxindex),
build_int_cst (TREE_TYPE (maxindex), 0), maxindex);
+ TYPE_TYPELESS_STORAGE (main_type) = TYPE_TYPELESS_STORAGE (type);
layout_type (main_type);
/* Make sure we have the canonical MAIN_TYPE. */
- hashcode = iterative_hash_object (TYPE_HASH (unqual_elt), hashcode);
- hashcode = iterative_hash_object (TYPE_HASH (TYPE_DOMAIN (main_type)),
- hashcode);
- main_type = type_hash_canon (hashcode, main_type);
+ inchash::hash hstate;
+ hstate.add_object (TYPE_HASH (unqual_elt));
+ hstate.add_object (TYPE_HASH (TYPE_DOMAIN (main_type)));
+ hstate.add_flag (TYPE_TYPELESS_STORAGE (main_type));
+ main_type = type_hash_canon (hstate.end (), main_type);
/* Fix the canonical type. */
if (TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (main_type))
@@ -6497,7 +6498,8 @@ complete_array_type (tree *ptype, tree i
!= TYPE_DOMAIN (main_type)))
TYPE_CANONICAL (main_type)
= build_array_type (TYPE_CANONICAL (TREE_TYPE (main_type)),
- TYPE_CANONICAL (TYPE_DOMAIN (main_type)));
+ TYPE_CANONICAL (TYPE_DOMAIN (main_type)),
+ TYPE_TYPELESS_STORAGE (main_type));
else
TYPE_CANONICAL (main_type) = main_type;
--- gcc/cp/tree.c.jj 2017-04-18 15:13:27.136371909 +0200
+++ gcc/cp/tree.c 2017-04-19 08:15:39.191291063 +0200
@@ -949,14 +949,14 @@ build_cplus_array_type (tree elt_type, t
}
else
{
- t = build_array_type (elt_type, index_type);
- if (elt_type == unsigned_char_type_node
- || elt_type == signed_char_type_node
- || elt_type == char_type_node
- || (TREE_CODE (elt_type) == ENUMERAL_TYPE
- && TYPE_CONTEXT (elt_type) == std_node
- && !strcmp ("byte", TYPE_NAME_STRING (elt_type))))
- TYPE_TYPELESS_STORAGE (t) = 1;
+ bool typeless_storage
+ = (elt_type == unsigned_char_type_node
+ || elt_type == signed_char_type_node
+ || elt_type == char_type_node
+ || (TREE_CODE (elt_type) == ENUMERAL_TYPE
+ && TYPE_CONTEXT (elt_type) == std_node
+ && !strcmp ("byte", TYPE_NAME_STRING (elt_type))));
+ t = build_array_type (elt_type, index_type, typeless_storage);
}
/* Now check whether we already have this array variant. */
--- gcc/testsuite/g++.dg/other/pr80423.C.jj 2017-04-19 08:14:42.539042760 +0200
+++ gcc/testsuite/g++.dg/other/pr80423.C 2017-04-19 08:14:42.539042760 +0200
@@ -0,0 +1,11 @@
+// PR middle-end/80423
+// { dg-do compile { target c++11 } }
+
+typedef unsigned char uint8_t;
+struct A {
+ template <int N> A(unsigned char (&)[N]);
+};
+void fn1(A) {
+ uint8_t a[]{0};
+ fn1(a);
+}
Jakub
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix TYPE_TYPELESS_STORAGE handling (PR middle-end/80423)
2017-04-19 6:20 ` Jakub Jelinek
2017-04-19 6:30 ` Jakub Jelinek
@ 2017-04-19 8:06 ` Richard Biener
1 sibling, 0 replies; 8+ messages in thread
From: Richard Biener @ 2017-04-19 8:06 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Jason Merrill, gcc-patches
On April 19, 2017 7:56:30 AM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>On Wed, Apr 19, 2017 at 07:45:36AM +0200, Richard Biener wrote:
>> >As mentioned in the PR, we now use TYPE_TYPELESS_STORAGE flag on
>> >ARRAY_TYPEs to denote types that need the special C++ alias
>handling.
>> >The problem is how is that created, we just use build_array_type and
>> >set TYPE_TYPELESS_STORAGE on the result, but build_array_type uses
>type
>> >caching, so we might modify that way some other array type.
>> >If all the array type creation goes through build_cplus_array_type,
>> >that
>> >wouldn't be a problem, as that flag is dependent just on the element
>> >type, but that is not the case, c-family as well as the middle-end
>has
>> >lots of spots that also create array types. So in the end whether
>> >one gets TYPE_TYPELESS_STORAGE flag or not is quite random, depends
>on
>> >GC etc.
>> >
>> >The following patch attempts to resolve this, by making the type
>> >hashing
>> >take that flag into account. Bootstrapped/regtested on x86_64-linux
>> >and
>> >i686-linux, ok for trunk?
>>
>> When changing the C++ function I thought that calling
>build_array_type was
>> wrong and it should instead do the same it does in the other places,
>use
>> its raw creation routine and then the canonical type register stuff.
>But
>> I was hesitant to change this at this point.
>
>The problem is that as the patch shows, we don't need it in a single
>place
>(the C++ FE), but at least in two places (C++ FE and c-family), and it
>wouldn't surprise me if we don't need it later on in further places
>(e.g. in middle-end, if we have a TYPE_TYPELESS_STORAGE array and say
>DSE
>wants to create a smaller one with the same property).
Hmm, but the C++ FE largely avoids the type cache of the Me and uses its own. Apart from a single place in this function.
But as I said I'm AFK and can't really check.
>Using a default argument to build_array_type is likely cleaner indeed,
>I'd just then also swap the arguments to build_array_type_1 (the shared
>vs. typeless_storage).
Sure. With C++ there's no reason to export _1 variants. Either overloading or default args work for me here.
Richard.
> Jakub
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix TYPE_TYPELESS_STORAGE handling (PR middle-end/80423)
2017-04-19 6:30 ` Jakub Jelinek
@ 2017-04-20 7:46 ` Jakub Jelinek
2017-04-20 9:17 ` Richard Biener
0 siblings, 1 reply; 8+ messages in thread
From: Jakub Jelinek @ 2017-04-20 7:46 UTC (permalink / raw)
To: Richard Biener; +Cc: Jason Merrill, gcc-patches
On Wed, Apr 19, 2017 at 08:22:23AM +0200, Jakub Jelinek wrote:
> Here in (so far untested) patch form:
>
> 2017-04-19 Jakub Jelinek <jakub@redhat.com>
>
> PR middle-end/80423
> * tree.h (build_array_type): Add typeless_storage default argument.
> * tree.c (type_cache_hasher::equal): Also compare
> TYPE_TYPELESS_STORAGE flag for ARRAY_TYPEs.
> (build_array_type): Add typeless_storage argument, set
> TYPE_TYPELESS_STORAGE to it, if shared also hash it, and pass to
> recursive call.
> (build_nonshared_array_type): Adjust build_array_type_1 caller.
> (build_array_type): Likewise. Add typeless_storage argument.
> c-family/
> * c-common.c (complete_array_type): Preserve TYPE_TYPELESS_STORAGE.
> cp/
> * tree.c (build_cplus_array_type): Call build_array_type
> with the intended TYPE_TYPELESS_STORAGE flag value, instead
> of calling build_array_type and modifying later TYPE_TYPELESS_STORAGE
> on the shared type.
> testsuite/
> * g++.dg/other/pr80423.C: New test.
Just for completeness, it bootstrapped/regtested successfully on x86_64-linux
and i686-linux.
Jakub
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix TYPE_TYPELESS_STORAGE handling (PR middle-end/80423)
2017-04-20 7:46 ` Jakub Jelinek
@ 2017-04-20 9:17 ` Richard Biener
0 siblings, 0 replies; 8+ messages in thread
From: Richard Biener @ 2017-04-20 9:17 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Jason Merrill, gcc-patches
On Thu, 20 Apr 2017, Jakub Jelinek wrote:
> On Wed, Apr 19, 2017 at 08:22:23AM +0200, Jakub Jelinek wrote:
> > Here in (so far untested) patch form:
> >
> > 2017-04-19 Jakub Jelinek <jakub@redhat.com>
> >
> > PR middle-end/80423
> > * tree.h (build_array_type): Add typeless_storage default argument.
> > * tree.c (type_cache_hasher::equal): Also compare
> > TYPE_TYPELESS_STORAGE flag for ARRAY_TYPEs.
> > (build_array_type): Add typeless_storage argument, set
> > TYPE_TYPELESS_STORAGE to it, if shared also hash it, and pass to
> > recursive call.
> > (build_nonshared_array_type): Adjust build_array_type_1 caller.
> > (build_array_type): Likewise. Add typeless_storage argument.
> > c-family/
> > * c-common.c (complete_array_type): Preserve TYPE_TYPELESS_STORAGE.
> > cp/
> > * tree.c (build_cplus_array_type): Call build_array_type
> > with the intended TYPE_TYPELESS_STORAGE flag value, instead
> > of calling build_array_type and modifying later TYPE_TYPELESS_STORAGE
> > on the shared type.
> > testsuite/
> > * g++.dg/other/pr80423.C: New test.
>
> Just for completeness, it bootstrapped/regtested successfully on x86_64-linux
> and i686-linux.
Ok.
Richard.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-04-20 8:28 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-18 16:39 [PATCH] Fix TYPE_TYPELESS_STORAGE handling (PR middle-end/80423) Jakub Jelinek
2017-04-19 5:45 ` Jeff Law
2017-04-19 5:58 ` Richard Biener
2017-04-19 6:20 ` Jakub Jelinek
2017-04-19 6:30 ` Jakub Jelinek
2017-04-20 7:46 ` Jakub Jelinek
2017-04-20 9:17 ` Richard Biener
2017-04-19 8:06 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).