Ping [PATCH] c++/42121 - diagnose invalid flexible array members

Thanks for the review and the helpful hints!

I've reworked and simplified the diagnostic part of the patch and
corrected the remaining issues I uncovered while testing the new
version (failing to reject some invalid flexible array members in
base classes).  Please find the new version in the attachment.

FWIW, in the patch, I tried to address only the reported problems
with flexible array members without changing how zero-length arrays
are treated.  That means that the latter are accepted in more cases
(with a pedantic warning) than the latter.  For example, the
following is accepted:

     struct A {
         int a[0];   // pedantic warning: zero-size array member
         int n;      // not at end of struct A
     };

while this is rejected

     struct B {
         int a[];    // hard error: flexible array member not at
         int n;      // end of struct B
     };

It would be easy to change the patch to treat zero-length arrays
more like flexible array members if that's viewed as desirable.

I also tried to avoid rejecting flexible array members that are
currently accepted and that are safe.  For example, GCC currently
accepts both flexible array members and zero-length arrays in base
classes (even polymorphic ones).  The patch continues to accept
those for compatibility with code that relies on it as long as
the flexible array members didn't overlap other members in derived
classes.  For example, this is still accepted:

     struct A { int x; };
     struct B { int n, a[]; };
     struct C: A, B { };

but this is rejected:

     struct D: B, A { };

My replies to your comments are below.

On 12/04/2015 10:51 AM, Jason Merrill wrote:
> On 12/03/2015 11:42 PM, Martin Sebor wrote:
>> +      if (next && TREE_CODE (next) == FIELD_DECL)
>
> This will break if there's a non-field between the array and the next
> field.

You're right, I missed that case in my testing.  Fixed.

>> @@ -4114,7 +4115,10 @@ walk_subobject_offsets (tree type,
>>
>>        /* Avoid recursing into objects that are not interesting.  */
>>        if (!CLASS_TYPE_P (element_type)
>> -      || !CLASSTYPE_CONTAINS_EMPTY_CLASS_P (element_type))
>> +      || !CLASSTYPE_CONTAINS_EMPTY_CLASS_P (element_type)
>> +      || !domain
>> +      /* Flexible array members have no upper bound.  */
>> +      || !TYPE_MAX_VALUE (domain))
>
> Why is this desirable?  We do want to avoid empty bases at the same
> address as a flexible array of the same type.

As we discussed on IRC, this bit is fine.  I added a few tests for
the layout to make sure the offset of flexible array members matches
the size of the containing class.  While adding these tests I found
a couple of regressions unrelated to my changes (68727 and 68711) so
it was time well spent.

>> +      && !tree_int_cst_equal (size_max_node, TYPE_MAX_VALUE (dom)))
>
> This can be integer_minus_onep or integer_all_onesp.

Thanks.

>
>> +         its fields.  The recursive call to the function will
>> +         either return 0 or the flexible array member whose
>
> Let's say NULL_TREE here rather than 0.

Sure.

>
>> +  {
>> +    bool dummy = false;
>> +    check_flexarrays (t, TYPE_FIELDS (t), &dummy);
>> +  }
>
> This should be called from check_bases_and_members, or even integrated
> into check_field_decls.

I tried moving it to check_bases_and_members there but with more
testing found out that calling it there was too early. In order
to detect invalid flexible array members in virtual base classes
without rejecting valid ones, the primary base class needs to
have been determined.  That happens in in layout_class_type()
called later on in finish_struct_1(). So I moved it just past
that call.

>
>> -      else if (name)
>> -        pedwarn (input_location, OPT_Wpedantic, "ISO C++ forbids
>> zero-size array %qD", name);
>
> Why?

At one point, the diagnostic would emit a badly messed up name
in some corner cases.  I think it might have been when I set
TYPE_DOMAIN to NULL_TREE rather than with the current approach
(I can't reproduce it anymore). I've restored the else block.

> Can we leave TYPE_DOMAIN null for flexible arrays so you don't need to
> add special new handling all over the place?

This was my initial approach until I noticed that it diverged
from C where TYPE_DOMAIN is set to the range [0, NULL_TREE], so
I redid it for consistency.

>
>> -    tree decl;
>> +    tree decl = NULL_TREE;
>
> Why?

To avoid an ICE later on.  I didn't spend too much time trying
to understand how the control flow changed to trigger it but my
guess is that it has to do with the change to the upper bound.

/home/msebor/scm/fsf/gcc-42121/gcc/testsuite/g++.dg/ext/flexary2.C:16:9: 
internal compiler error: tree check: expected tree that contains 'typed' 
structure, have '<invalid tree code>' in cp_apply_type_quals_to_decl, at 
cp/typeck.c:9134
0x13103fe tree_contains_struct_check_failed(tree_node const*, 
tree_node_structure_enum, char const*, int, char const*)
          /home/msebor/scm/fsf/gcc-42121/gcc/tree.c:9760
0x726b12 contains_struct_check(tree_node*, tree_node_structure_enum, 
char const*, int, char const*)
          /home/msebor/scm/fsf/gcc-42121/gcc/tree.h:3070
0x901d20 cp_apply_type_quals_to_decl(int, tree_node*)
          /home/msebor/scm/fsf/gcc-42121/gcc/cp/typeck.c:9134
0x77bf8b grokdeclarator(cp_declarator const*, cp_decl_specifier_seq*, 
decl_context, int, tree_node**)
          /home/msebor/scm/fsf/gcc-42121/gcc/cp/decl.c:11423

>
>> +++ b/gcc/testsuite/g++.dg/cpp0x/bad_array_new2.C
...
> Why?

This wasn't meant to be a part of the patch. I removed it.

Martin

Ping [PATCH] c++/42121 - diagnose invalid flexible array members

Commit Message

Patch