Re: [PATCH] accept flexible arrays in struct in unions (c++/71912 - [6/7 regression])

[Jason Merrill <jason@redhat.com>]

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

On 10/05/2016 05:43 PM, Martin Sebor wrote:
> On 09/23/2016 12:00 PM, Jason Merrill wrote:
>> On Thu, Sep 22, 2016 at 7:39 PM, Martin Sebor <msebor@gmail.com> wrote:
>>> On 09/21/2016 02:43 PM, Jason Merrill wrote:
>>>> On Tue, Sep 20, 2016 at 1:01 PM, Martin Sebor <msebor@gmail.com> wrote:
>>>>> On 09/16/2016 12:19 PM, Jason Merrill wrote:
>>>>>> On 09/14/2016 01:03 PM, Martin Sebor wrote:
>>>>>>>
>>>>>>> +      /* Type of the member.  */
>>>>>>> +      tree fldtype = TREE_CODE (fld) == FIELD_DECL ? TREE_TYPE
>>>>>>> (fld) : fld;
>>>>>>
>>>>>> Why set "fldtype" to be a TYPE_DECL rather than its type?
>>>>>
>>>>> I'm not sure I understand the question but (IIRC) the purpose of
>>>>> this code is to detect invalid uses of flexible array members in
>>>>> typedefs such as this:
>>>>>
>>>>>    struct S { typedef struct { int i, a[]; } X2 [2]; };
>>>>>
>>>>> Sadly, it doesn't do anything for
>>>>>
>>>>>    struct X { int i, a[]; };
>>>>>    struct S { typedef struct X X2 [2]; };
>>>>
>>>> The issue is I don't see anything that uses fldtype as a decl, and
>>>> it's strange to have a variable called "*type" that can either be a
>>>> type or a decl, which later code still assumes will be a type.
>>>
>>> I suspect I'm simply looking at it from a different point of view.
>>> The definition
>>>
>>>   tree fldtype = TREE_CODE (fld) == FIELD_DECL ? TREE_TYPE (fld) : fld
>>>
>>> denotes the type of the field if fld is a data member.  Otherwise,
>>> it denotes a type (like a typedef).
>>
>> FLD is always a _DECL.  The result of this assignment is that fldtype
>> refers to either a _TYPE or a _DECL.  This creates a strange
>> asymmetry, and none of the following code seems to depend on it.
>> I would prefer
>>
>> tree fldtype = TREE_TYPE (fld);
>>
>> so that it's always a _TYPE.
>
> I mentioned that this code is important to avoid diagnosing typedefs,
> but the example I showed wasn't the right one.  The examples that do
> depend on it are these two:
>
>   struct S1 {
>     typedef int A [0];
>     A a;
>   };
>
>   struct S2 {
>     typedef struct { int i, a[]; } A1;
>     typedef struct { int i, a[]; } A2;
>   };
>
> The expected (pedantic) warning is:
>
>   warning: zero-size array member ‘S1::a’ in an otherwise empty ‘struct S1’
>
> With the change you're asking we end up with:
>
>   warning: zero-size array member ‘S1::a’ not at end of ‘struct S1’
>
> followed by a bogus
>
>   error: flexible array member ‘S2::A1::a’ not at end of ‘struct S2’
>
> So I still don't understand what you're looking for.  It sounds to
> me like you're asking me to rewrite the subsequent code that uses
> fldtype to use the expression above because you don't like that
> fldtype can be either a _DECL or _TYPE.  But repeating that check
> four times doesn't make sense, so what am I missing?

I'm asking you to clarify the logic.  It seems that your change to 
fldtype affects these two tests:

>           if (eltype == fldtype || TYPE_UNNAMED_P (eltype))

>       if (TREE_CODE (fldtype) != ARRAY_TYPE)

...but this is extremely subtle.  It would be a lot clearer to check fld 
for FIELD_DECL or TYPE_DECL explicitly rather than relying on these 
places that treat fldtype as a type to do the right thing because you've 
obfuscated it.  But I'm tired of going back and forth on this, so here's 
a patch.

And now that I notice it, there seems to be no reason to handle typedefs 
deep in the code for handling fields, it's simpler to handle them up top.

[-- Attachment #2: typedef.diff --]
[-- Type: text/x-patch, Size: 2580 bytes --]

diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index 03cf2eb..e6f7167 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -6752,49 +6752,39 @@ find_flexarrays (tree t, flexmems_t *fmem, bool base_p,
 	   typedef struct { int i, a[], j; } S;   // bug c++/72753
            S s [2];                               // bug c++/68489
       */
-      bool anon_type_p
-	= (TREE_CODE (fld) == TYPE_DECL
-	   && DECL_IMPLICIT_TYPEDEF_P (fld)
-	   && anon_aggrname_p (DECL_NAME (fld)));
+      if (TREE_CODE (fld) == TYPE_DECL
+	  && DECL_IMPLICIT_TYPEDEF_P (fld)
+	  && CLASS_TYPE_P (TREE_TYPE (fld))
+	  && anon_aggrname_p (DECL_NAME (fld)))
+	{
+	  /* Check the nested unnamed type referenced via a typedef
+	     independently of FMEM (since it's not a data member of
+	     the enclosing class).  */
+	  check_flexarrays (TREE_TYPE (fld));
+	  continue;
+	}
 
       /* Skip anything that's GCC-generated or not a (non-static) data
-	 member or typedef.  */
-      if ((DECL_ARTIFICIAL (fld) && !anon_type_p)
-	  || (TREE_CODE (fld) != FIELD_DECL && TREE_CODE (fld) != TYPE_DECL))
+	 member.  */
+      if (DECL_ARTIFICIAL (fld) || TREE_CODE (fld) != FIELD_DECL)
 	continue;
 
       /* Type of the member.  */
-      tree fldtype = TREE_CODE (fld) == FIELD_DECL ? TREE_TYPE (fld) : fld;
+      tree fldtype = TREE_TYPE (fld);
       if (fldtype == error_mark_node)
 	return;
 
       /* Determine the type of the array element or object referenced
 	 by the member so that it can be checked for flexible array
 	 members if it hasn't been yet.  */
-      tree eltype = TREE_TYPE (fld);
-      if (eltype == error_mark_node)
-	return;
-
+      tree eltype = fldtype;
       while (TREE_CODE (eltype) == ARRAY_TYPE
 	     || TREE_CODE (eltype) == POINTER_TYPE
 	     || TREE_CODE (eltype) == REFERENCE_TYPE)
 	eltype = TREE_TYPE (eltype);
 
-      if (TREE_CODE (fld) == TYPE_DECL
-	  && TYPE_CANONICAL (eltype) == TYPE_CANONICAL (t))
-	continue;
-
       if (RECORD_OR_UNION_TYPE_P (eltype))
 	{
-	  if (anon_type_p)
-	    {
-	      /* Check the nested unnamed type referenced via a typedef
-		 independently of FMEM (since it's not a data member of
-		 the enclosing class).  */
-	      check_flexarrays (eltype);
-	      continue;
-	    }
-
 	  if (fmem->array && !fmem->after[bool (pun)])
 	    {
 	      /* Once the member after the flexible array has been found
@@ -6843,8 +6833,6 @@ find_flexarrays (tree t, flexmems_t *fmem, bool base_p,
 	      if (base_p)
 		continue;
 	    }
-	  else if (TREE_CODE (fld) == TYPE_DECL)
-	    continue;
 	}
 
       if (field_nonempty_p (fld))