[2/3] Avoid creating an initializer for a flexible array member

If we do create such an initializer, we end up with an error_mark_node
during gimplification, because in cp-gimplify.c we pass this
VEC_INIT_EXPR of the flexible array member to build_vec_init, for which
it spits on an error_mark_node.  This happens in e.g. the test case
g++.dg/ext/array1.C.

This patch makes it so that we avoid generating an initializer for a
flexible array member, thus avoiding to end up with an error_mark_node
during gimplification.  The same kind of thing is done ~90 lines down
from the code I changed.

gcc/cp/ChangeLog:

	* init.c (perform_member_init): Avoid creating an initializer
	for a flexible array member.
---
 gcc/cp/init.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

On 12/31/2015 08:40 AM, Patrick Palka wrote:
> If we do create such an initializer, we end up with an error_mark_node
> during gimplification, because in cp-gimplify.c we pass this
> VEC_INIT_EXPR of the flexible array member to build_vec_init, for which
> it spits on an error_mark_node.  This happens in e.g. the test case
> g++.dg/ext/array1.C.
>
> This patch makes it so that we avoid generating an initializer for a
> flexible array member, thus avoiding to end up with an error_mark_node
> during gimplification.  The same kind of thing is done ~90 lines down
> from the code I changed.

I don't think this change is correct (or complete).  We could
decide that the kind of code it's meant to allow should be
accepted in general but if we did, this change alone would not
be sufficient.

In the way of background, the existing special treatment for
flexible array members was added to prevent rejecting cases
like this one:

   struct A { A (int); };
   struct B {
       B ();
       int n;
       A a[];
   };

Since B's ctor above doesn't initialize the flexible array
member it's well-formed.  Ditto if B's ctor is defined but
avoids initializing B::a.

The proposed change has the effect of also accepting the
following modified version of the example above that is
currently rejected:

   struct A { A (int); };
   struct B {
       B (): n (), a () { }
       int n;
       A a[];
   };

In this modified example, B's ctor attempts to default-initialize
the flexible array member and its elements using their respective
default ctors even though no such ctor for the latter exists.

Since C++ flexible array members are a GCC extension whose C++
specific aspects are essentially undocumented it's open to
discussion whether or not code like this should be accepted.
Clang rejects it, but one could make the argument that since
the flexible array member has no elements, default initializing
it should be allowed even if the same initialization would be
ill-formed if the array wasn't empty.

If the code should be accepted as written above then it should
also be accepted if B's ctor were omitted or defaulted. I.e.,
the following should be treated the same as the above:

   struct A { A (int); };
   struct B {
       // ditto with B() = default;
       int n;
       A a[];
   } b;

The patch doesn't handle either of these cases and the code
is rejected both with and without it.

Martin

PS While looking at this I experimented with zero-length
arrays to see if they might serve as a precedent.
Unfortunately, I discovered that those aren't currently
handled entirely consistently either.  I opened bug 69127
with the details.

On Sun, Jan 3, 2016 at 3:14 PM, Martin Sebor <msebor@gmail.com> wrote:
> On 12/31/2015 08:40 AM, Patrick Palka wrote:
>>
>> If we do create such an initializer, we end up with an error_mark_node
>> during gimplification, because in cp-gimplify.c we pass this
>> VEC_INIT_EXPR of the flexible array member to build_vec_init, for which
>> it spits on an error_mark_node.  This happens in e.g. the test case
>> g++.dg/ext/array1.C.
>>
>> This patch makes it so that we avoid generating an initializer for a
>> flexible array member, thus avoiding to end up with an error_mark_node
>> during gimplification.  The same kind of thing is done ~90 lines down
>> from the code I changed.
>
>
> I don't think this change is correct (or complete).  We could
> decide that the kind of code it's meant to allow should be
> accepted in general but if we did, this change alone would not
> be sufficient.
>
> In the way of background, the existing special treatment for
> flexible array members was added to prevent rejecting cases
> like this one:
>
>   struct A { A (int); };
>   struct B {
>       B ();
>       int n;
>       A a[];
>   };
>
> Since B's ctor above doesn't initialize the flexible array
> member it's well-formed.  Ditto if B's ctor is defined but
> avoids initializing B::a.
>
> The proposed change has the effect of also accepting the
> following modified version of the example above that is
> currently rejected:
>
>   struct A { A (int); };
>   struct B {
>       B (): n (), a () { }
>       int n;
>       A a[];
>   };
>
> In this modified example, B's ctor attempts to default-initialize
> the flexible array member and its elements using their respective
> default ctors even though no such ctor for the latter exists.
>
> Since C++ flexible array members are a GCC extension whose C++
> specific aspects are essentially undocumented it's open to
> discussion whether or not code like this should be accepted.
> Clang rejects it, but one could make the argument that since
> the flexible array member has no elements, default initializing
> it should be allowed even if the same initialization would be
> ill-formed if the array wasn't empty.
>
> If the code should be accepted as written above then it should
> also be accepted if B's ctor were omitted or defaulted. I.e.,
> the following should be treated the same as the above:
>
>   struct A { A (int); };
>   struct B {
>       // ditto with B() = default;
>       int n;
>       A a[];
>   } b;
>
> The patch doesn't handle either of these cases and the code
> is rejected both with and without it.

Good catch.  I had not intended to change the validity of certain uses
of flexible array members with this patch.

So at least for now it seems that we should not avoid building a
VEC_INIT_EXPR of a flexible array member, since calling
build_vec_init_expr is necessary to diagnose some invalid uses of
flexible array members. So, I see three possible solutions to avoid
propagating an error_mark_node to the gimplifier while preserving the
exact behavior of flexible array members:

Continue to unconditionally call_build_vec_init_expr() in
perform_member_init(), and

1. Don't add the resulting VEC_INIT_EXPR to the statement tree if the
array in question has no upper bound (i.e. is a flexible array
member); or
2. Modify build_vec_init() to return an empty statement instead of
returning error_mark_node, if the array to be initialized has no upper
bound (and its initializer is NULL_TREE); or
3. Modify cp_gimplify_expr() to avoid calling build_vec_init() if the
VEC_INIT_EXPR in question is for an array with no upper bound (and its
initializer is NULL_TREE).

Which approach is best? The third one is the most conservative and the
one least likely to cause inadvertent changes in behavior elsewhere, I
think.

(In the end however, this "bug" is mostly harmless, and a patch fixing
it is really only useful in combination with patch #3/3 to avoid a
spurious ICE-on-valid.)