On 13/09/16 03:36, Jason Merrill wrote:
On Wed, Sep 7, 2016 at 5:22 AM, Richard Biener
<richard.guent...@gmail.com> wrote:
On Mon, Sep 5, 2016 at 6:11 PM, Tom de Vries <tom_devr...@mentor.com> wrote:
On 05/09/16 09:49, Richard Biener wrote:
On Sun, Sep 4, 2016 at 11:30 PM, Tom de Vries <tom_devr...@mentor.com>
wrote:
On 04/09/16 16:08, Richard Biener wrote:
On September 4, 2016 12:33:02 PM GMT+02:00, Tom de Vries
<tom_devr...@mentor.com> wrote:
On 04/09/16 08:12, Richard Biener wrote:
On September 3, 2016 5:23:35 PM GMT+02:00, Tom de Vries
<tom_devr...@mentor.com> wrote:
Hi,
this patch fixes a c++ ICE, a p1 6/7 regression.
Consider test.C:
...
void bar (__builtin_va_list &);
struct c
{
operator const __builtin_va_list &();
operator __builtin_va_list &();
};
void
foo (void) {
struct c c1;
bar (c1);
}
...
The compiler ICEs as follows:
...
test.C: In function ‘void foo()’:
test.C:13:10: internal compiler error: canonical types differ
for
identical types __va_list_tag [1] and __va_list_tag [1]
bar (c1);
^
comptypes(tree_node*, tree_node*, int)
src/gcc/cp/typeck.c:1430
reference_related_p(tree_node*, tree_node*)
src/gcc/cp/call.c:1415
reference_binding
src/gcc/cp/call.c:1559
implicit_conversion
src/gcc/cp/call.c:1805
build_user_type_conversion_1
src/gcc/cp/call.c:3776
reference_binding
src/gcc/cp/call.c:1664
implicit_conversion
src/gcc/cp/call.c:1805
add_function_candidate
src/gcc/cp/call.c:2141
add_candidates
src/gcc/cp/call.c:5394
perform_overload_resolution
src/gcc/cp/call.c:4066
build_new_function_call(tree_node*, vec<tree_node*, va_gc,
vl_embed>**,
bool, int)
src/gcc/cp/call.c:4143
finish_call_expr(tree_node*, vec<tree_node*, va_gc,
vl_embed>**,
bool,
bool, int)
src/gcc/cp/semantics.c:2440
...
The regression is caused by the commit for PR70955, that adds
a
"sysv_abi va_list" attribute to the struct in the va_list
type
(which
is
an array of one of struct).
The ICE in comptypes happens as follows: we're comparing two
versions
of
va_list type (with identical array element type), each with
the
canonical type set to themselves. Since the types are
considered
identical, they're supposed to have identical canonical
types,
which is
Did you figure out why they are not assigned the same canonical
type?
When constructing the first type in ix86_build_builtin_va_list_64,
it's
cached.
When constructing the second type in build_array_type_1 (with call
stack: grokdeclarator -> cp_build_qualified_type_real ->
build_cplus_array_type -> build_cplus_array_type ->
build_array_type ->
build_array_type_1), we call type_hash_canon.
But the cached type has name __builtin_sysv_va_list, and the new
type
has no name, so we hit the clause 'TYPE_NAME (a->type) != TYPE_NAME
(b->type)' in type_cache_hasher::equal.
Consequently, TYPE_CANONICAL for the new type remain set to self.
But how did it then work before the patch causing this?
Before the patch that introduced the attribute, rather than assigning
the
result of ix86_build_builtin_va_list_64 directly
sysv_va_list_type_node, an
intermediate build_variant_type_copy was used.
This had as consequence that the copy was named and not added to the
cache,
and that the original in the type cache remained unnamed.
Adding the build_variant_type_copy back fixes the ICE. But I'm not sure
if
that's a correct fix. The copy would have it's canonical type set to
the
original type. But if we'd search for the canonical type of the copy in
the
type cache, we wouldn't find it.
Huh. Can't see how making a copy would affect the type cache -- AFAIK
nothing
adds the record to the type hash.
Correct.
The array type is there
First the array type is constructed by ix86_build_builtin_va_list_64, and
entered into the type cache. Then the type is assigned to
ms_va_list_type_node.
Lateron, the ms_va_list_type_node is returned by ix86_enum_va_list, and
c_common_nodes_and_builtin calls lang_hooks.decls.pushdecl for the node:
...
lang_hooks.decls.pushdecl
(build_decl (UNKNOWN_LOCATION,
TYPE_DECL, get_identifier (pname),
ptype));
...
In that process it adds a name to the type node (to be precise, in
set_underlying_type, case DECL_IS_BUILTIN).
If it adds the name to the original type, then we have a named type in the
type cache. If it adds the name to a copy of the type, then we have an
unnamed type in the type cache.
Ok, as the backend controls what name it gets assigned in enum_va_list_p
it seems to me the backend should assing the same name to the type in
the first place to avoid the inconsistency?
OTOH what set_underlying_type does for the DECL_IS_BUILTIN case is
bogus if its type is already in the type cache as that changes its hash value.
We do have a build_nonshared_array_type which ix86_build_builtin_va_list_64
could use to avoid the type cache (not sure if that would do any good to the
issue we face).
Yes. It seems to me that this is the primary problem here;
set_underlying_type is treating this array type as though it's a
built-in type like 'int', but that doesn't make sense for an array
type like this that can be constructed independently, leading to the
problems we see here. This patch seems to fix the issue:
set_und.diff
Bootstrapped and reg-tested the patch on x86_64.
OK for trunk, 6 branch?
Thanks,
- Tom
Don't treat array as builtin type in set_underlying_type
2016-09-13 Jason Merrill <ja...@redhat.com>
Tom de Vries <t...@codesourcery.com>
PR c++/77427
* c-common.c (set_underlying_type): Don't treat array as builtin type.
* g++.dg/pr77427.C: New test.
---
gcc/c-family/c-common.c | 2 +-
gcc/testsuite/g++.dg/pr77427.C | 17 +++++++++++++++++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 9b5e016..825e63a 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -12303,7 +12303,7 @@ set_underlying_type (tree x)
{
if (x == error_mark_node)
return;
- if (DECL_IS_BUILTIN (x))
+ if (DECL_IS_BUILTIN (x) && TREE_CODE (TREE_TYPE (x)) != ARRAY_TYPE)
{
if (TYPE_NAME (TREE_TYPE (x)) == 0)
TYPE_NAME (TREE_TYPE (x)) = x;
diff --git a/gcc/testsuite/g++.dg/pr77427.C b/gcc/testsuite/g++.dg/pr77427.C
new file mode 100644
index 0000000..544946d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr77427.C
@@ -0,0 +1,17 @@
+// { dg-do compile }
+
+void bar (__builtin_va_list &);
+
+struct c
+{
+ operator const __builtin_va_list &();
+ operator __builtin_va_list &();
+};
+
+void
+foo (void)
+{
+ struct c c1;
+
+ bar (c1);
+}
