[C++ Patch] PR 44625

[C++ Patch] PR 44625

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 133 bytes --]

Hi,

a patchlet for a pretty old ice-on-invalid regression. Tested x86_64-linux.

Ok for mainline?

Thanks,
Paolo.

////////////////

[-- Attachment #2: CL_44625 --]
[-- Type: text/plain, Size: 261 bytes --]

/cp
2011-06-23  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/44625
	* pt.c (tsubst_copy_and_build): Do not use BASELINK_P on a NULL_TREE.

/testsuite
2011-06-23  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/44625
	* g++.dg/template/crash107.C: New.

[-- Attachment #3: patch_44625 --]
[-- Type: text/plain, Size: 1360 bytes --]

Index: testsuite/g++.dg/template/crash107.C
===================================================================
--- testsuite/g++.dg/template/crash107.C	(revision 0)
+++ testsuite/g++.dg/template/crash107.C	(revision 0)
@@ -0,0 +1,20 @@
+// PR c++/44625
+// { dg-do compile }
+// { dg-options "" }
+
+template<typename FP_> struct Vec { // { dg-message "note" }
+    Vec& operator^=(Vec& rhs)     {
+        union {
+            struct {FP_ x,y,z;};
+        }; // { dg-error "anonymous struct" }
+        X = y*rhs.z() - z*rhs.y(); // { dg-error "not declared|no member" }
+    }
+    Vec& operator^(Vec& rhs) {
+        return Vec(*this)^=rhs; // { dg-message "required" }
+    }
+};
+Vec<double> v(3,4,12); // { dg-error "no matching" }
+// { dg-message "note" { target *-*-* } 16 }
+Vec<double> V(12,4,3);  // { dg-error "no matching" }
+// { dg-message "note" { target *-*-* } 18 }
+Vec<double> c = v^V;   // { dg-message "required" }
Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 175328)
+++ cp/pt.c	(working copy)
@@ -13252,6 +13252,9 @@ tsubst_copy_and_build (tree t,
 	object_type = TREE_TYPE (object);
 
 	member = TREE_OPERAND (t, 1);
+	if (!member)
+	  return error_mark_node;
+
 	if (BASELINK_P (member))
 	  member = tsubst_baselink (member,
 				    non_reference (TREE_TYPE (object)),

Re: [C++ Patch] PR 44625

[Jason Merrill]

Why are we creating a COMPONENT_REF with a null op1 in the first place?

Jason

Re: [C++ Patch] PR 44625

[Paolo Carlini]

Hi,
> Why are we creating a COMPONENT_REF with a null op1 in the first place?
For now, what I figured out is the following: build_anon_union_vars 
calls build_min_nt (COMPONENT_REF, object, DECL_NAME (field), NULL_TREE) 
with a null third argument, which becomes the null op1.

As a matter of fact, the possibility that DECL_NAME (field) could be 
null is considered in build_anon_union_vars itself, because right after 
the above mentioned build_min_nt call, there is a conditional 'if 
(DECL_NAME (field)) ...'

I don't know if the above is enough for you to suggest the way we should 
go...

Paolo.

Re: [C++ Patch] PR 44625

[Jason Merrill]

On 06/23/2011 11:30 AM, Paolo Carlini wrote:
>> Why are we creating a COMPONENT_REF with a null op1 in the first place?

> As a matter of fact, the possibility that DECL_NAME (field) could be
> null is considered in build_anon_union_vars itself, because right after
> the above mentioned build_min_nt call, there is a conditional 'if
> (DECL_NAME (field)) ...'

Indeed.  The code is using DECL_NAME in templates so that tsubst can 
look them up again by name, but as we see in this PR that can't work if 
the field has no name.  We need a different strategy for handling 
anonymous aggregates nested in other anonymous aggregates in templates.

Jason

Re: [C++ Patch] PR 44625

[Paolo Carlini]

On 06/23/2011 05:47 PM, Jason Merrill wrote:
> Indeed.  The code is using DECL_NAME in templates so that tsubst can 
> look them up again by name, but as we see in this PR that can't work 
> if the field has no name.  We need a different strategy for handling 
> anonymous aggregates nested in other anonymous aggregates in templates.
Ok, then, from what you saying I understand that it should be possible 
to actually construct a reject-valid or an ice-on-valid in this area, 
isn't just about improving the diagnostic, that is only the tip of the 
iceberg, so to speak. I guess it's not for me, at this time...

Paolo.

Re: [C++ Patch] PR 44625

[Jason Merrill]

On 06/23/2011 11:52 AM, Paolo Carlini wrote:
> Ok, then, from what you saying I understand that it should be possible
> to actually construct a reject-valid or an ice-on-valid in this area,
> isn't just about improving the diagnostic, that is only the tip of the
> iceberg, so to speak. I guess it's not for me, at this time...

For the time being you could improve the diagnostic by adding a sorry 
for the case of null DECL_NAME in a template in build_anon_union_vars.

Jason

Re: [C++ Patch] PR 44625

[Jason Merrill]

Actually, 9.5 says

A union of the form
   union { member-specification } ;
is called an anonymous union; it defines an unnamed object of unnamed 
type. The member-specification of an anonymous union shall only define 
non-static data members. [ Note: Nested types and functions cannot
be declared within an anonymous union. — end note ]

So we should be able to just reject nested anonymous aggregates and not 
worry about how to make them work.

Jason

Re: [C++ Patch] PR 44625

[Paolo Carlini]

On 06/23/2011 06:05 PM, Jason Merrill wrote:
> Actually, 9.5 says
>
> A union of the form
>   union { member-specification } ;
> is called an anonymous union; it defines an unnamed object of unnamed 
> type. The member-specification of an anonymous union shall only define 
> non-static data members. [ Note: Nested types and functions cannot
> be declared within an anonymous union. — end note ]
>
> So we should be able to just reject nested anonymous aggregates and 
> not worry about how to make them work.
Yes, but we are accepting already some of that as an extension. If I 
compile the testcase with -pedantic-errors I get:

44625.C:8:31: error: ISO C++ prohibits anonymous structs [-pedantic]
44625.C:9:9: error: anonymous struct not inside named type

thus, it's not clear to me where we should stop, exactly.

Paolo.

Re: [C++ Patch] PR 44625

[Paolo Carlini]

On 06/23/2011 06:11 PM, Paolo Carlini wrote:
> On 06/23/2011 06:05 PM, Jason Merrill wrote:
>> Actually, 9.5 says
>>
>> A union of the form
>>   union { member-specification } ;
>> is called an anonymous union; it defines an unnamed object of unnamed 
>> type. The member-specification of an anonymous union shall only 
>> define non-static data members. [ Note: Nested types and functions 
>> cannot
>> be declared within an anonymous union. — end note ]
>>
>> So we should be able to just reject nested anonymous aggregates and 
>> not worry about how to make them work.
> Yes, but we are accepting already some of that as an extension. If I 
> compile the testcase with -pedantic-errors I get:
>
> 44625.C:8:31: error: ISO C++ prohibits anonymous structs [-pedantic]
> 44625.C:9:9: error: anonymous struct not inside named type
Of course only the first message is new with -pedantic-errors.

Thus, the idea would be rejecting *nested* anonymous, now I see. Uhmm.

Paolo.

Re: [C++ Patch] PR 44625

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

On 06/23/2011 06:05 PM, Jason Merrill wrote:
> So we should be able to just reject nested anonymous aggregates and 
> not worry about how to make them work.
The below appears to work pretty well, regtests fine. I had to tweak the 
existing error17.C, we don't emit anymore the warning about no members 
in the nested anonymous struct, because we bail out early. Doesn't seem 
a serious issue to me...

Ok?

Paolo.

//////////////////////

[-- Attachment #2: CL_44625_2 --]
[-- Type: text/plain, Size: 325 bytes --]

/cp
2011-06-23  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/44625
	* decl2.c (build_anon_union_vars): Early return error_mark_node
	for a nested anonymous struct.

/testsuite
2011-06-23  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/44625
	* g++.dg/template/crash107.C: New.
	* g++.dg/template/error17.C: Adjust.

[-- Attachment #3: patch_44625_2 --]
[-- Type: text/plain, Size: 1974 bytes --]

Index: testsuite/g++.dg/template/error17.C
===================================================================
--- testsuite/g++.dg/template/error17.C	(revision 175330)
+++ testsuite/g++.dg/template/error17.C	(working copy)
@@ -6,5 +6,4 @@ foo()
 {
   union { struct { }; }; // { dg-error "prohibits anonymous struct" "anon" }
   // { dg-error "not inside" "not inside" { target *-*-* } 7 }
-  // { dg-warning "no members" "no members" { target *-*-* } 7 }
 }
Index: testsuite/g++.dg/template/crash107.C
===================================================================
--- testsuite/g++.dg/template/crash107.C	(revision 0)
+++ testsuite/g++.dg/template/crash107.C	(revision 0)
@@ -0,0 +1,20 @@
+// PR c++/44625
+// { dg-do compile }
+// { dg-options "" }
+
+template<typename FP_> struct Vec { // { dg-message "note" }
+    Vec& operator^=(Vec& rhs)     {
+        union {
+            struct {FP_ x,y,z;};
+        }; // { dg-error "anonymous struct" }
+        X = y*rhs.z() - z*rhs.y(); // { dg-error "not declared|no member" }
+    }
+    Vec& operator^(Vec& rhs) {
+        return Vec(*this)^=rhs; // { dg-message "required" }
+    }
+};
+Vec<double> v(3,4,12); // { dg-error "no matching" }
+// { dg-message "note" { target *-*-* } 16 }
+Vec<double> V(12,4,3);  // { dg-error "no matching" }
+// { dg-message "note" { target *-*-* } 18 }
+Vec<double> c = v^V;   // { dg-message "required" }
Index: cp/decl2.c
===================================================================
--- cp/decl2.c	(revision 175335)
+++ cp/decl2.c	(working copy)
@@ -1327,7 +1327,10 @@ build_anon_union_vars (tree type, tree object)
   /* Rather than write the code to handle the non-union case,
      just give an error.  */
   if (TREE_CODE (type) != UNION_TYPE)
-    error ("anonymous struct not inside named type");
+    {
+      error ("anonymous struct not inside named type");
+      return error_mark_node;
+    }
 
   for (field = TYPE_FIELDS (type);
        field != NULL_TREE;

Re: [C++ Patch] PR 44625

[Jason Merrill]

OK.

Jason

Re: [C++ Patch] PR 44625

[Paolo Carlini]

On 06/23/2011 06:01 PM, Jason Merrill wrote:
> On 06/23/2011 11:52 AM, Paolo Carlini wrote:
>> Ok, then, from what you saying I understand that it should be possible
>> to actually construct a reject-valid or an ice-on-valid in this area,
>> isn't just about improving the diagnostic, that is only the tip of the
>> iceberg, so to speak. I guess it's not for me, at this time...
> For the time being you could improve the diagnostic by adding a sorry 
> for the case of null DECL_NAME in a template in build_anon_union_vars.
Indeed. Let me try then...

Paolo.

[C++ Patch] PR 44625

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 264 bytes --]

Hi,

I have this patchlet for another case where we use BASELINK_P on
NULL_TREE. Tested x86_64-linux. I propose to apply it to mainline and
4_5-branch only and then close the PR: it's a regression, but just a P5
ice-on-invalid.

Thanks,
Paolo.

//////////////////

[-- Attachment #2: CL_44625 --]
[-- Type: text/plain, Size: 262 bytes --]

/cp
2010-07-05  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/44625
	* pt.c (tsubst_copy_and_build): Do not apply BASELINK_P on
	NULL_TREE.

/testsuite
2010-07-05  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/44625
	* g++.dg/template/crash102.C: New.

[-- Attachment #3: patch_44625 --]
[-- Type: text/plain, Size: 1558 bytes --]

Index: testsuite/g++.dg/template/crash102.C
===================================================================
--- testsuite/g++.dg/template/crash102.C	(revision 0)
+++ testsuite/g++.dg/template/crash102.C	(revision 0)
@@ -0,0 +1,19 @@
+// PR c++/44625
+// { dg-do compile }
+// { dg-options "" }
+
+template<typename FP_> struct Vec { // { dg-message "note" }
+    Vec& operator^=(Vec& rhs)     {
+        union {
+            struct {FP_ x,y,z;};
+        }; // { dg-error "anonymous struct" }
+        y*rhs.z() - z*rhs.y(); // { dg-error "no member" }
+	return *this;
+    }
+    Vec& operator^(Vec& rhs) {
+        return Vec(*this)^=rhs; // { dg-message "instantiated" }
+    }
+};
+Vec<double> v(3,4,12);  // { dg-error "no matching" }
+Vec<double> V(12,4,3);  // { dg-error "no matching" }
+Vec<double> c = v^V;    // { dg-message "instantiated" }
Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 161825)
+++ cp/pt.c	(working copy)
@@ -12625,7 +12625,9 @@ tsubst_copy_and_build (tree t,
       {
 	tree object;
 	tree object_type;
-	tree member;
+	tree member = TREE_OPERAND (t, 1);
+	if (!member)
+	  return error_mark_node;
 
 	object = tsubst_non_call_postfix_expression (TREE_OPERAND (t, 0),
 						     args, complain, in_decl);
@@ -12634,7 +12636,6 @@ tsubst_copy_and_build (tree t,
 	  mark_used (object);
 	object_type = TREE_TYPE (object);
 
-	member = TREE_OPERAND (t, 1);
 	if (BASELINK_P (member))
 	  member = tsubst_baselink (member,
 				    non_reference (TREE_TYPE (object)),

Re: [C++ Patch] PR 44625

[Jason Merrill]

On 07/05/2010 07:52 AM, Paolo Carlini wrote:
> I have this patchlet for another case where we use BASELINK_P on
> NULL_TREE. Tested x86_64-linux. I propose to apply it to mainline and
> 4_5-branch only and then close the PR: it's a regression, but just a P5
> ice-on-invalid.

Seems like we ought to catch this when we're creating the COMPONENT_REF, 
rather than when we're trying to substitute into it; we shouldn't create 
a COMPONENT_REF with a null operand 1.

Jason

Re: [C++ Patch] PR 44625

[Paolo Carlini]

On 07/05/2010 03:10 PM, Jason Merrill wrote:
> On 07/05/2010 07:52 AM, Paolo Carlini wrote:
>> I have this patchlet for another case where we use BASELINK_P on
>> NULL_TREE. Tested x86_64-linux. I propose to apply it to mainline and
>> 4_5-branch only and then close the PR: it's a regression, but just a P5
>> ice-on-invalid.
> Seems like we ought to catch this when we're creating the
> COMPONENT_REF, rather than when we're trying to substitute into it; we
> shouldn't create a COMPONENT_REF with a null operand 1.
Ok, I'll look into it.

Paolo.