Message ID | 1451576417-8710-2-git-send-email-patrick@parcs.ath.cx |
---|---|
State | New |
Headers | show |
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.)
diff --git a/gcc/cp/init.c b/gcc/cp/init.c index 09c1183..0011178 100644 --- a/gcc/cp/init.c +++ b/gcc/cp/init.c @@ -645,9 +645,14 @@ perform_member_init (tree member, tree init) /* mem() means value-initialization. */ if (TREE_CODE (type) == ARRAY_TYPE) { - init = build_vec_init_expr (type, init, tf_warning_or_error); - init = build2 (INIT_EXPR, type, decl, init); - finish_expr_stmt (init); + /* Initialize the array only if it's not a flexible + array member (i.e., if it has an upper bound). */ + if (TYPE_DOMAIN (type) && TYPE_MAX_VALUE (TYPE_DOMAIN (type))) + { + init = build_vec_init_expr (type, init, tf_warning_or_error); + init = build2 (INIT_EXPR, type, decl, init); + finish_expr_stmt (init); + } } else {