On 07/26/2016 12:53 PM, Jason Merrill wrote:
> On 07/23/2016 01:18 PM, Martin Sebor wrote:
>> +  /* A pair of the first non-static non-empty data members following
>> +     either the flexible array member, if found, or the zero-length
>> +     array member otherwise.  AFTER[1] refers to the first such data
>> +     member of a union that the struct containing the flexible array
>> +     member or zero-length array is a member, or NULL when no such
>> +     union exists.  AFTER[0] refers to the first such data member
>> +     that is not a member of such a union.  */
>
> This is pretty hard to follow, could you add an example?  Why do we want
> to track these separately rather than look at DECL_CONTEXT at diagnostic
> time?

Sure, I've added an example.

Whether or not a given flexible array member is valid depends not
only on the context in which it's defined (its enclosing class) but
also on the members of other classes whose objects may be defined
in the same or other contexts (e.g., enclosing structs).  I don't
know of a way to reach those other members from the context of
the ARRAY.

>
>> +  /* The type in which an anonymous struct or union containing ARRAY
>> +     is defined or null if no such anonymous struct or union exists.  */
>> +  tree anonctx;
>
> It seems clearer to find this at diagnostic time by following TYPE_CONTEXT.

I tried this approach and while it's doable (with recursion) I'm
not happy with the results.  The diagnostics point to places that
I think are unclear.  For example, given:

   struct A { int i, a[]; };
   struct B { long j, b[]; };
   struct D: A, B { };

the current patch prints:

   error: flexible array member ‘A::a’ not at end of ‘struct D’
    struct A { int i, a[]; };
                        ^
   note: next member ‘long int B::j’ declared here
    struct B { long j, b[]; };
                    ^
   note: in the definition of ‘struct D’
    struct D: A, B { };
           ^

while using the suggested approach and determining the context from
ARRAY the error is:

  error: flexible array member ‘A::a’ not at end of ‘struct B’

That looks confusing to me.  I might be able to tweak the function
I wrote to do this (below) to get the same results but that would
likely come at the expense of clarity and more effort for (IMO)
little gain.

   static tree
   anon_context (tree t)
   {
     if (TREE_CODE (t) == FIELD_DECL)
       return anon_context (DECL_CONTEXT (t));

     return ANON_AGGR_TYPE_P (t) ? TYPE_CONTEXT (t) : t;
   }

>
>>       /* Find the next non-static data member if it exists.  */
>>       for (next = fld;
>>            (next = DECL_CHAIN (next))
>>              && TREE_CODE (next) != FIELD_DECL; );
>
> I think next_initializable_field would be useful here.

The loop above advances to the next data member.  When FLD is (for
example) a flexible array member, next_initializable_field returns
the same argument again.  So I'm not sure the function would help
simplify the code (assuming that's what you were thinking).  But
the latest patch also looks for types and so it's even less likely
to be useful.

With more testing I realized that there were other invalid cases
that the function was missing so I enhanced it to cover those
(arrays, pointers, and references to member structs containing
flexible array members, and typedefs aliasing unnamed members
structs).

>
>>        if (TREE_CODE (fld) != TYPE_DECL
>>           && RECORD_OR_UNION_TYPE_P (fldtype)
>> -         && TYPE_ANONYMOUS_P (fldtype))
>> +         && VAR_DECL != TREE_CODE (fld)
>> +         && (FIELD_DECL != TREE_CODE (fld) || !DECL_FIELD_IS_BASE
>> (fld)))
>
> Please put the constant on the RHS of comparisons.

Done.

>
> It seems that you're only interested in FIELD_DECL here, so maybe move
> up the
>
>>       /* Skip anything that's not a (non-static) data member.  */
>>       if (TREE_CODE (fld) != FIELD_DECL)
>>         continue;
>
> and remove the checks for TYPE_DECL and VAR_DECL?  For that matter, you
> could also move up the DECL_ARTIFICIAL check so you don't need to check
> DECL_FIELD_IS_BASE.

Yes, that's clearer, thanks.

>
>> +         /* Is the member an anonymous struct or union?  */
>> +         bool anon_p = (!TYPE_ANONYMOUS_P (fldtype)
>> +                        || !DECL_NAME (fld)
>> +                        || anon_aggrname_p (DECL_NAME (fld)));
>
> This looks like anon_p will be true for any non-static data member of
> non-anonymous type?  Maybe you want ANON_AGGR_TYPE_P?

Yes, that works, thanks.

>
>> +      /* If this member isn't anonymous and a prior non-flexible array
>> +         member has been seen in one of the enclosing structs, clear
>> +         the FIRST member since it doesn't contribute to the flexible
>> +         array struct's members.  */
>> +      if (first && !array && !anon_p)
>> +        fmem->first = NULL_TREE;
>
> Why is this needed?  For a non-anonymous member, presumably we'll give
> an error when analyzing the type of the member on its own, so we
> shouldn't need to complain again here.

Consider the definitions below.  In each case the unnamed struct
is processed first, before the enclosing struct and its members.
As a result, whether or not a member of its type has a name is
not known until the enclosing struct is processed.  This bit of
code is there to distinguish and correctly handle these cases.

   struct A {
     int i;
     struct { int a[]; };   // valid (in G++)
   };

and

   struct B {
     int i;
     struct { int a[]; } s;   // invalid in C or G++
   };

>
> So we silently allow a zero-length array followed by a flexible array?
> Why?

You mean as in the following?

   struct S {
     int i, a [0], b[];
   };

That's been accepted in both C and C++ and this patch doesn't change
it.  If you'd like to see it changed that's fine with me but I would
prefer to do it independently of this bug fix.

>> +        TREE_NO_WARNING (fmem->array) = 1;
>> +      }
>
> There doesn't seem to be anything that checks TREE_NO_WARNING for the
> zero-size array case.

Well spotted, thanks.

>
>> +        /* Avoid issuing further duiagnostics after the error above.  */
>
> Typo.
>
>>   if (fmem == &flexmems
>>       && !TYPE_ANONYMOUS_P (t) && !anon_aggrname_p (TYPE_IDENTIFIER (t)))
>
> I think you want ANON_AGGR_TYPE_P here, too.

Here, ANON_AGGR_TYPE_P(t) returns false for anonymous structs such
as the one below and using it would cause false positives:

   struct S { int i; struct { int a[]; }; };

>
> I also wonder about integrating this with layout_class_type, since that
> is also looking at layout issues.

I haven't investigated this but unless you are suggesting to simply
move the call to check_flexarrays to the end of layout_class_type,
it sounds like a bigger change than I would feel comfortable making
in 6.2.

I would prefer to keep the scope of this patch minimal so it can
be safely backported to 6.2.  For 7.0, given enough time in the
schedule, I'd like to fix 68489 - arrays of flexible array members
are silently accepted, and also the related 72753 and 72754 that
I just raised.  I suspect that will likely involve changing how
or from where these functions are invoked from (possibly also
from finish_decl) so it seems like a good time to look into
integrating it into layout_class_type.

Martin