[C++ Patch] PR 66644

[C++ Patch] PR 66644

[Paolo Carlini]

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

Hi,

only when Jakub bumped some bugs in preparation for the release I noted 
that this one remained assigned to me for way too much time...

Roughly speaking, the problem is caused by the fact that when we have a 
GNU anonymous struct inside a union the fields are flattened out and 
appear to be just individual fields of the union. Thus we end up 
rejecting test1 below because multiple fields are initialized and we 
don't simply handle them later on as NSDMIs. A GNU anonymous struct is 
required for the issue to show up, thus the details of the way we want 
to handle such code are debatable, but we know that, eg, both EDG and 
clang accept test1 too (in relaxed GNU mode, at least), besides test2 
and test3.

Given the underlying cause of the rejection I could easily imagine other 
less straightforward ways to match the cases at issues in 
check_field_decl (eg, along the lines DECL_CONTEXT (field) == t ??), but 
the below certainly passes testing on x86_64-linux.

Thanks,
Paolo.

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

[-- Attachment #2: CL_66644 --]
[-- Type: text/plain, Size: 292 bytes --]

/cp
2016-04-28  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/66644
	* class.c (check_field_decl): Do not reject multiple initialized
	fields in anonymous struct.

/testsuite
2016-04-28  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/66644
	* g++.dg/cpp0x/nsdmi-anon-struct1.C: New.

[-- Attachment #3: patch_66644 --]
[-- Type: text/plain, Size: 1412 bytes --]

Index: cp/class.c
===================================================================
--- cp/class.c	(revision 235557)
+++ cp/class.c	(working copy)
@@ -3623,7 +3623,11 @@ check_field_decl (tree field,
     {
       /* `build_class_init_list' does not recognize
 	 non-FIELD_DECLs.  */
-      if (TREE_CODE (t) == UNION_TYPE && *any_default_members != 0)
+      if (TREE_CODE (t) == UNION_TYPE && *any_default_members != 0
+	  /* As a GNU extension initializing in C++11 multiple fields
+	     of an anonymous struct living inside a union is fine.  */
+	  && !(TREE_CODE (DECL_CONTEXT (field)) == RECORD_TYPE
+	       && ANON_AGGR_TYPE_P (DECL_CONTEXT (field))))
 	error ("multiple fields in union %qT initialized", t);
       *any_default_members = 1;
     }
Index: testsuite/g++.dg/cpp0x/nsdmi-anon-struct1.C
===================================================================
--- testsuite/g++.dg/cpp0x/nsdmi-anon-struct1.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/nsdmi-anon-struct1.C	(working copy)
@@ -0,0 +1,30 @@
+// PR c++/66644
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wno-pedantic" }
+
+struct test1  
+{
+  union
+  {
+    struct { char a=0, b=0; };
+    char buffer[16];
+  };
+};
+
+struct test2 
+{
+  union  
+  {
+    struct { char a=0, b; };
+    char buffer[16];
+  };
+};
+
+struct test3
+{
+  union
+  {
+    struct { char a, b; } test2{0,0};
+    char buffer[16];
+  };
+};

Re: [C++ Patch] PR 66644

[Jason Merrill]

I would expect this to cause a false negative on a union of two 
anonymous structs, both of which have initialized members.

I think better would be to have a local any_default_members rather than 
passing the same pointer through all levels.

Also, you can look at 'type' rather than DECL_CONTEXT (field).

Jason

Re: [C++ Patch] PR 66644

[Paolo Carlini]

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

Hi Jason,

On 28/04/2016 23:45, Jason Merrill wrote:
> I would expect this to cause a false negative on a union of two 
> anonymous structs, both of which have initialized members.
>
> I think better would be to have a local any_default_members rather 
> than passing the same pointer through all levels.
>
> Also, you can look at 'type' rather than DECL_CONTEXT (field).
.
... and thanks a lot for your very helpful reply. Now I realize that 
something was wrong in the general logic here, not a tiny detail in a 
conditional. Thus the below tries to closely follow your advice: the 
idea is that any_default_members as required by check_field_decls should 
still compute to the same value but the logic to emit the "multiple 
fields in union initialized" diagnostic in check_field_decl is 
different: it relies on the incoming any_default_members, not on what is 
computed and returned at that level. Thus we can reject, eg, your case 
of two anonymous struct. Also, for test5, which we already correctly 
rejected, we do not emit an additional redundant error, and that's more 
evidence that something bigger was wrong in check_field_decl. Anyway, 
the below passes testing. How does it look?

Thanks,
Paolo.

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

[-- Attachment #2: patch_66644_2 --]
[-- Type: text/plain, Size: 4701 bytes --]

Index: cp/class.c
===================================================================
--- cp/class.c	(revision 235615)
+++ cp/class.c	(working copy)
@@ -139,7 +139,7 @@ static int count_fields (tree);
 static int add_fields_to_record_type (tree, struct sorted_fields_type*, int);
 static void insert_into_classtype_sorted_fields (tree, tree, int);
 static bool check_bitfield_decl (tree);
-static void check_field_decl (tree, tree, int *, int *, int *);
+static bool check_field_decl (tree, tree, int *, int *, bool);
 static void check_field_decls (tree, tree *, int *, int *);
 static tree *build_base_field (record_layout_info, tree, splay_tree, tree *);
 static void build_base_fields (record_layout_info, splay_tree, tree *);
@@ -3541,14 +3541,15 @@ check_bitfield_decl (tree field)
    enclosing type T.  Issue any appropriate messages and set appropriate
    flags.  */
 
-static void
+static bool
 check_field_decl (tree field,
 		  tree t,
 		  int* cant_have_const_ctor,
 		  int* no_const_asn_ref,
-		  int* any_default_members)
+		  bool any_default_members)
 {
   tree type = strip_array_types (TREE_TYPE (field));
+  bool any_default_members_field = false;
 
   /* In C++98 an anonymous union cannot contain any fields which would change
      the settings of CANT_HAVE_CONST_CTOR and friends.  */
@@ -3558,12 +3559,13 @@ check_field_decl (tree field,
      structs.  So, we recurse through their fields here.  */
   else if (ANON_AGGR_TYPE_P (type))
     {
-      tree fields;
-
-      for (fields = TYPE_FIELDS (type); fields; fields = DECL_CHAIN (fields))
+      for (tree fields = TYPE_FIELDS (type); fields;
+	   fields = DECL_CHAIN (fields))
 	if (TREE_CODE (fields) == FIELD_DECL && !DECL_C_BIT_FIELD (field))
-	  check_field_decl (fields, t, cant_have_const_ctor,
-			    no_const_asn_ref, any_default_members);
+	  any_default_members_field |= check_field_decl (fields, t,
+							 cant_have_const_ctor,
+							 no_const_asn_ref,
+							 any_default_members);
     }
   /* Check members with class type for constructors, destructors,
      etc.  */
@@ -3623,10 +3625,12 @@ check_field_decl (tree field,
     {
       /* `build_class_init_list' does not recognize
 	 non-FIELD_DECLs.  */
-      if (TREE_CODE (t) == UNION_TYPE && *any_default_members != 0)
+      if (TREE_CODE (t) == UNION_TYPE && any_default_members)
 	error ("multiple fields in union %qT initialized", t);
-      *any_default_members = 1;
+      any_default_members_field = true;
     }
+
+  return any_default_members_field;
 }
 
 /* Check the data members (both static and non-static), class-scoped
@@ -3662,7 +3666,7 @@ check_field_decls (tree t, tree *access_decls,
   tree *field;
   tree *next;
   bool has_pointers;
-  int any_default_members;
+  bool any_default_members;
   int cant_pack = 0;
   int field_access = -1;
 
@@ -3672,7 +3676,7 @@ check_field_decls (tree t, tree *access_decls,
   has_pointers = false;
   /* Assume none of the members of this class have default
      initializations.  */
-  any_default_members = 0;
+  any_default_members = false;
 
   for (field = &TYPE_FIELDS (t); *field; field = next)
     {
@@ -3868,10 +3872,10 @@ check_field_decls (tree t, tree *access_decls,
       /* We set DECL_C_BIT_FIELD in grokbitfield.
 	 If the type and width are valid, we'll also set DECL_BIT_FIELD.  */
       if (! DECL_C_BIT_FIELD (x) || ! check_bitfield_decl (x))
-	check_field_decl (x, t,
-			  cant_have_const_ctor_p,
-			  no_const_asn_ref_p,
-			  &any_default_members);
+	any_default_members |= check_field_decl (x, t,
+						 cant_have_const_ctor_p,
+						 no_const_asn_ref_p,
+						 any_default_members);
 
       /* Now that we've removed bit-field widths from DECL_INITIAL,
 	 anything left in DECL_INITIAL is an NSDMI that makes the class
Index: testsuite/g++.dg/cpp0x/nsdmi-anon-struct1.C
===================================================================
--- testsuite/g++.dg/cpp0x/nsdmi-anon-struct1.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/nsdmi-anon-struct1.C	(working copy)
@@ -0,0 +1,48 @@
+// PR c++/66644
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wno-pedantic" }
+
+struct test1  
+{
+  union
+  {
+    struct { char a=0, b=0; };
+    char buffer[16];
+  };
+};
+
+struct test2 
+{
+  union  
+  {
+    struct { char a=0, b; };
+    char buffer[16];
+  };
+};
+
+struct test3
+{
+  union
+  {
+    struct { char a, b; } test2{0,0};
+    char buffer[16];
+  };
+};
+
+struct test4
+{
+  union  
+  {   // { dg-error "multiple fields" }
+    struct { char a=0, b=0; };
+    struct { char c=0, d; };
+  };
+};
+
+struct test5
+{
+  union
+  {
+    union { char a=0, b=0; };  // { dg-error "multiple fields" }
+    char buffer[16];
+  };
+};

Re: [C++ Patch] PR 66644

[Jason Merrill]

On 04/28/2016 08:18 PM, Paolo Carlini wrote:
>     else if (ANON_AGGR_TYPE_P (type))
>       {
> -      tree fields;
> -
> -      for (fields = TYPE_FIELDS (type); fields; fields = DECL_CHAIN (fields))
> +      for (tree fields = TYPE_FIELDS (type); fields;
> +	   fields = DECL_CHAIN (fields))
>   	if (TREE_CODE (fields) == FIELD_DECL && !DECL_C_BIT_FIELD (field))
> -	  check_field_decl (fields, t, cant_have_const_ctor,
> -			    no_const_asn_ref, any_default_members);
> +	  any_default_members_field |= check_field_decl (fields, t,
> +							 cant_have_const_ctor,
> +							 no_const_asn_ref,
> +							 any_default_members);

The logic here seems convoluted.  I guess we don't need to handle 
anonymous structs and unions differently here because we'll call 
check_field_decls for the anonymous union itself, and complain then?  In 
that case, instead of passing down any_default_members at all, can we 
just pass it up and complain in check_field_decls?

Jason

Re: [C++ Patch] PR 66644

[Paolo Carlini]

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

Hi,

On 29/04/2016 15:58, Jason Merrill wrote:
> On 04/28/2016 08:18 PM, Paolo Carlini wrote:
>>     else if (ANON_AGGR_TYPE_P (type))
>>       {
>> -      tree fields;
>> -
>> -      for (fields = TYPE_FIELDS (type); fields; fields = DECL_CHAIN 
>> (fields))
>> +      for (tree fields = TYPE_FIELDS (type); fields;
>> +       fields = DECL_CHAIN (fields))
>>       if (TREE_CODE (fields) == FIELD_DECL && !DECL_C_BIT_FIELD (field))
>> -      check_field_decl (fields, t, cant_have_const_ctor,
>> -                no_const_asn_ref, any_default_members);
>> +      any_default_members_field |= check_field_decl (fields, t,
>> +                             cant_have_const_ctor,
>> +                             no_const_asn_ref,
>> +                             any_default_members);
>
> The logic here seems convoluted.  I guess we don't need to handle 
> anonymous structs and unions differently here because we'll call 
> check_field_decls for the anonymous union itself, and complain then?  
> In that case, instead of passing down any_default_members at all, can 
> we just pass it up and complain in check_field_decls?
Indeed. Code reworked along these lines becomes even comprehensible ;) 
Something as simple as the below passes testing....

Thanks,
Paolo.

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

[-- Attachment #2: patch_66644_3 --]
[-- Type: text/plain, Size: 4924 bytes --]

Index: cp/class.c
===================================================================
--- cp/class.c	(revision 235643)
+++ cp/class.c	(working copy)
@@ -139,7 +139,7 @@ static int count_fields (tree);
 static int add_fields_to_record_type (tree, struct sorted_fields_type*, int);
 static void insert_into_classtype_sorted_fields (tree, tree, int);
 static bool check_bitfield_decl (tree);
-static void check_field_decl (tree, tree, int *, int *, int *);
+static bool check_field_decl (tree, tree, int *, int *);
 static void check_field_decls (tree, tree *, int *, int *);
 static tree *build_base_field (record_layout_info, tree, splay_tree, tree *);
 static void build_base_fields (record_layout_info, splay_tree, tree *);
@@ -3541,14 +3541,14 @@ check_bitfield_decl (tree field)
    enclosing type T.  Issue any appropriate messages and set appropriate
    flags.  */
 
-static void
+static bool
 check_field_decl (tree field,
 		  tree t,
 		  int* cant_have_const_ctor,
-		  int* no_const_asn_ref,
-		  int* any_default_members)
+		  int* no_const_asn_ref)
 {
   tree type = strip_array_types (TREE_TYPE (field));
+  bool any_default_members = false;
 
   /* In C++98 an anonymous union cannot contain any fields which would change
      the settings of CANT_HAVE_CONST_CTOR and friends.  */
@@ -3558,12 +3558,12 @@ check_field_decl (tree field,
      structs.  So, we recurse through their fields here.  */
   else if (ANON_AGGR_TYPE_P (type))
     {
-      tree fields;
-
-      for (fields = TYPE_FIELDS (type); fields; fields = DECL_CHAIN (fields))
+      for (tree fields = TYPE_FIELDS (type); fields;
+	   fields = DECL_CHAIN (fields))
 	if (TREE_CODE (fields) == FIELD_DECL && !DECL_C_BIT_FIELD (field))
-	  check_field_decl (fields, t, cant_have_const_ctor,
-			    no_const_asn_ref, any_default_members);
+	  any_default_members |= check_field_decl (fields, t,
+						   cant_have_const_ctor,
+						   no_const_asn_ref);
     }
   /* Check members with class type for constructors, destructors,
      etc.  */
@@ -3620,13 +3620,11 @@ check_field_decl (tree field,
   check_abi_tags (t, field);
 
   if (DECL_INITIAL (field) != NULL_TREE)
-    {
-      /* `build_class_init_list' does not recognize
-	 non-FIELD_DECLs.  */
-      if (TREE_CODE (t) == UNION_TYPE && *any_default_members != 0)
-	error ("multiple fields in union %qT initialized", t);
-      *any_default_members = 1;
-    }
+    /* `build_class_init_list' does not recognize
+       non-FIELD_DECLs.  */
+    any_default_members = true;
+
+  return any_default_members;
 }
 
 /* Check the data members (both static and non-static), class-scoped
@@ -3662,7 +3660,7 @@ check_field_decls (tree t, tree *access_decls,
   tree *field;
   tree *next;
   bool has_pointers;
-  int any_default_members;
+  bool any_default_members;
   int cant_pack = 0;
   int field_access = -1;
 
@@ -3672,7 +3670,7 @@ check_field_decls (tree t, tree *access_decls,
   has_pointers = false;
   /* Assume none of the members of this class have default
      initializations.  */
-  any_default_members = 0;
+  any_default_members = false;
 
   for (field = &TYPE_FIELDS (t); *field; field = next)
     {
@@ -3867,11 +3865,16 @@ check_field_decls (tree t, tree *access_decls,
 
       /* We set DECL_C_BIT_FIELD in grokbitfield.
 	 If the type and width are valid, we'll also set DECL_BIT_FIELD.  */
-      if (! DECL_C_BIT_FIELD (x) || ! check_bitfield_decl (x))
-	check_field_decl (x, t,
-			  cant_have_const_ctor_p,
-			  no_const_asn_ref_p,
-			  &any_default_members);
+      if ((! DECL_C_BIT_FIELD (x) || ! check_bitfield_decl (x))
+	  && check_field_decl (x, t,
+			       cant_have_const_ctor_p,
+			       no_const_asn_ref_p))
+	{
+	  if (any_default_members
+	      && TREE_CODE (t) == UNION_TYPE)
+	    error ("multiple fields in union %qT initialized", t);
+	  any_default_members = true;
+	}
 
       /* Now that we've removed bit-field widths from DECL_INITIAL,
 	 anything left in DECL_INITIAL is an NSDMI that makes the class
Index: testsuite/g++.dg/cpp0x/nsdmi-anon-struct1.C
===================================================================
--- testsuite/g++.dg/cpp0x/nsdmi-anon-struct1.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/nsdmi-anon-struct1.C	(working copy)
@@ -0,0 +1,48 @@
+// PR c++/66644
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wno-pedantic" }
+
+struct test1  
+{
+  union
+  {
+    struct { char a=0, b=0; };
+    char buffer[16];
+  };
+};
+
+struct test2 
+{
+  union  
+  {
+    struct { char a=0, b; };
+    char buffer[16];
+  };
+};
+
+struct test3
+{
+  union
+  {
+    struct { char a, b; } test2{0,0};
+    char buffer[16];
+  };
+};
+
+struct test4
+{
+  union  
+  {   // { dg-error "multiple fields" }
+    struct { char a=0, b=0; };
+    struct { char c=0, d; };
+  };
+};
+
+struct test5
+{
+  union
+  {
+    union { char a=0, b=0; };  // { dg-error "multiple fields" }
+    char buffer[16];
+  };
+};

Re: [C++ Patch] PR 66644

[Jason Merrill]

OK, thanks.

Jason

[C++ Patch] PR 66644

[Paolo Carlini]

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

Hi,

this is about the anonymous struct extension, but I think submitter is 
right that we should accept the testcase (indeed both clang and edg 
already do): in the below I propose to simply add a test of 
ANON_AGGR_TYPE_P (DECL_CONTEXT (field)). Note that, given the definition 
of ANON_AGGR_TYPE_P, should not be necessary to take care explicitly of 
anonymous unions: to be safe I'm adding a negative test for those.

Tested x86_64-linux.

Thanks,
Paolo.

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

[-- Attachment #2: CL_66644 --]
[-- Type: text/plain, Size: 286 bytes --]

/cp
2015-07-08  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/66644
	* class.c (check_field_decl): Do not reject multiple initialized
	fields in anonymous struct.

/testsuite
2015-07-08  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/66644
	* g++.dg/cpp0x/anon-struct1.C: New.

[-- Attachment #3: patch_66644 --]
[-- Type: text/plain, Size: 1120 bytes --]

Index: cp/class.c
===================================================================
--- cp/class.c	(revision 225556)
+++ cp/class.c	(working copy)
@@ -3568,7 +3568,8 @@ check_field_decl (tree field,
     {
       /* `build_class_init_list' does not recognize
 	 non-FIELD_DECLs.  */
-      if (TREE_CODE (t) == UNION_TYPE && *any_default_members != 0)
+      if (TREE_CODE (t) == UNION_TYPE && *any_default_members != 0
+	  && !ANON_AGGR_TYPE_P (DECL_CONTEXT (field)))
 	error ("multiple fields in union %qT initialized", t);
       *any_default_members = 1;
     }
Index: testsuite/g++.dg/cpp0x/anon-struct1.C
===================================================================
--- testsuite/g++.dg/cpp0x/anon-struct1.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/anon-struct1.C	(working copy)
@@ -0,0 +1,21 @@
+// PR c++/66644
+// { dg-do compile { target c++11 } }
+// { dg-options "" }
+
+struct testP
+{
+  union U
+  {
+    struct { char a=0, b=0; };
+    char buffer[16];
+  };
+};
+
+struct testN
+{
+  union U
+  {
+    union { char a=0, b=0; };  // { dg-error "multiple fields" }
+    char buffer[16];
+  };
+};