This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 07/31/2016 10:28 AM, Jason Merrill wrote:
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?

I don't think that would work in cases like this:

  struct S1 {
    struct S2 { int i, a[]; } s2;
    union U { int x; } u;
  };

that need to be treated differently from this one:

  union U1 {
    struct S { int i, a[]; } s;
    union U2 { int x; } u2;
  };


+     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?

6.7.2.1, p3:

  A structure or union shall not contain a member with incomplete
  or function type [...], except that the last member of a structure
  with more than one named member may have incomplete array type; such
  a structure (and any union containing, possibly recursively, a member
  that is such a structure) shall not be a member of a structure or
  an element of an 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 { };

I don't see any anonymous aggregates in this example, so how does
anonctx come into it?

If there is no anonctx then diagnose_flexrrays will have to figure
out whether the array is a member of an anonymous struct and if so,
find the struct of which it's effectively a member, and otherwise
use the array's immediate context.  The above was an example of
the result of a simple implementation of your suggestion.  As I
said, the code could probably be tweaked to produce the same result
as it does now in both cases (anonymous and not), but at the cost
of additional complexity and, IMO, to the detriment of clarity.
What aspect of clarity do you find lacking in the current approach?

  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?

Because TYPE_ANONYMOUS_P is set for unnamed structs that are not
anonymous, as in

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

It looks like what I actually need here is anon_aggrname_p alone.
Let me change that before committing the patch or posting a new
version if one is still needed.

FWIW, I spent lots of time last week learning how these different
macros map on to language features and under what circumstances.
I find some of them quite hard to work with because they add
a layer of complexity between concepts in the language and their
implementation in GCC.  In cases like this one when their names
are deceptively close to the language concepts but they don't
quite map 1-to-1, they can be downright misleading.

Martin


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]