This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] accept flexible arrays in struct in unions (c++/71912 - [6/7 regression])
On Fri, Jul 29, 2016 at 7:22 PM, Martin Sebor <msebor@gmail.com> wrote:
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.
Not from the context of the array, but when we see a field following the
flexible array (or aggregate ending in a flexible array), can't we look
at the DECL_CONTEXT of that field and see whether it's a union or not?
+ For example, in the following, the flexible array member
+ S::U::X::a overlaps S::U::Y::i and so AFTER[1] is set to refer to
+ the latter. This potential problem is indepenent of union U's
"independent"
+ membership in struct S. In addition, in the definition of struct
+ S, S::U::x::a is followed by S::z, and so AFTER[0] is set to refer
+ to the latter. The two problems result in two diagnostics, the
+ first one being a pedantic warning and the second a hard error.
+
+ struct S {
+ union U {
+ struct X { int i, a[]; } x;
+ struct Y { long i, a[]; } y;
+ } u;
+ int z;
+ };
Hmm, I'm not convinced that the first problem is really a problem. Only
one of the union members is active at any time, so overlapping with
another union member seems is irrelevant.
I also can't find the language in the C standard that prohibits nesting
a struct ending with a flexible array in another struct or union, do you
have a citation?
+ /* 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 { };
I don't see any anonymous aggregates in this example, so how does
anonctx come into it?
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[]; }; };
Ah, it hasn't been set yet here because we don't know yet whether we're
declaring a named member or an anonymous aggregate. So yes,
TYPE_ANONYMOUS_P is the right test here. But why do you also check
anon_aggrname_p directly, when it's used to implement TYPE_ANONYMOUS_P?
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.
Makes sense.
Jason