On 12/7/24 5:34 AM, Jakub Jelinek wrote:
Hi!
On the pr117516.C testcase check_flexarrays and its helper functions
have exponential complexity, plus it reports the same bug over and over
again in some cases instead of reporting perhaps other bugs.
The functions want to diagnose flexible array member (and strangely [0]
arrays too) followed by some other non-empty or array members in the same
strcuture, or followed by other non-empty or array members in a containing
structure (any of them), or flexible array members/[0] arrays in structures
with no other non-empty members, or those nested in other structures.
Strangely it doesn't complain if flexible array member is in a structure
used in an array.
As can be seeen on e.g. the flexary41.C test, it keeps reporting the
same bug over and over:
flexary41.C:5:24: error: flexible array member ‘A::b’ not at end of ‘struct A’
flexary41.C:5:24: error: flexible array member ‘A::b’ not at end of ‘struct B’
flexary41.C:5:24: error: flexible array member ‘A::b’ not at end of ‘struct C’
flexary41.C:5:24: error: flexible array member ‘A::b’ not at end of ‘struct D’
flexary41.C:13:39: error: flexible array member ‘E::<unnamed struct>::n’ not at
end of ‘struct E’
flexary41.C:18:23: error: flexible array member ‘H::t’ not at end of ‘struct K’
flexary41.C:25:36: note: next member ‘int K::ab’ declared here
flexary41.C:25:8: note: in the definition of ‘struct K’
The bug that A::b is followed by A::c is one bug reported 4 times, while it
doesn't report the other bugs, that B::e flexarray is followed by B::f
and that C::h flexarray is followed by C::i.
That is because it always walks all the structures/unions of all the members
and just finds the first flexarray in there.
Now, this has horrible complexity plus it doesn't seem really useful to
users. So, for cases where a flexible array member is followed by a
non-empty other member in the same structure, the following patch just
reports it once when finalizing that structure, and otherwise just recurses
in structures solely into the last member, so that it can report cases like
struct X { int a; int b[]; };
struct Y { X c; int d; };
or
struct Z { X c; };
i.e. correct use of flexarray in X but following it by another member in Y
or just nesting it (the former is error, the latter pedwarn as before).
By only looking at the last member for structures we get rid of the complexity.
Note, the patch doesn't do anything about unions, I think we still could
spend a lot of time compiling.
struct S { char s; };
union U0 { S a, b; };
union U1 { union U0 a, b; };
union U2 { union U1 a, b; };
...
union U32 { union U31 a, b; };
struct T { union U32 a; int b; };
Not really sure what we could do about that, all the elements are "last"
(but admittedly I haven't studied in detail how the original code worked
in union, there is fmem->after[pun] where pun is whether it is somewhere
inside of a union). Perhaps in a hash table marking unions which don't have
any flexarrays at the end, nested or not, so that we don't walk them again?
Plus if we find some with flexarray at the end, maybe there is no point
to look other union members? In any case, I think that is less severe,
because people usually don't nest unions deeply.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2024-12-07 Jakub Jelinek <ja...@redhat.com>
PR c++/117516
* class.cc (field_nonempty_p): Formatting fixes. Use
integer_zerop instead of tree_int_cst_equal with size_zero_node.
(struct flexmems_t): Change type of first member from tree to bool.
(find_flexarrays): Add nested_p argument. Change pun argument type
from tree to bool, adjust uses. Formatting fixes. If BASE_P or
NESTED_P and T is RECORD_TYPE, start looking only at the last
non-empty or array FIELD_DECL. Adjust recursive call, set first
if it was a nested call and found an array.
(diagnose_invalid_flexarray, diagnose_flexarrays, check_flexarrays):
Formatting fixes.
* g++.dg/ext/flexary9.C: Expect different wording of one of the
warnings and at a different line.
* g++.dg/ext/flexary19.C: Likewise.
* g++.dg/ext/flexary41.C: New test.
* g++.dg/other/pr117516.C: New test.
--- gcc/cp/class.cc.jj 2024-12-06 11:00:21.418671398 +0100
+++ gcc/cp/class.cc 2024-12-06 13:55:39.074460055 +0100
@@ -141,8 +141,8 @@ static bool accessible_nvdtor_p (tree);
/* Used by find_flexarrays and related functions. */
struct flexmems_t;
static void diagnose_flexarrays (tree, const flexmems_t *);
-static void find_flexarrays (tree, flexmems_t *, bool = false,
- tree = NULL_TREE, tree = NULL_TREE);
+static void find_flexarrays (tree, flexmems_t *, bool, bool = false,
+ bool = false, tree = NULL_TREE);
static void check_flexarrays (tree, flexmems_t * = NULL, bool = false);
static void check_bases (tree, int *, int *);
static void check_bases_and_members (tree);
@@ -7362,11 +7362,9 @@ field_nonempty_p (const_tree fld)
if (TREE_CODE (fld) == FIELD_DECL
&& TREE_CODE (type) != ERROR_MARK
&& (DECL_NAME (fld) || RECORD_OR_UNION_TYPE_P (type)))
- {
- return TYPE_SIZE (type)
- && (TREE_CODE (TYPE_SIZE (type)) != INTEGER_CST
- || !tree_int_cst_equal (size_zero_node, TYPE_SIZE (type)));
- }
+ return (TYPE_SIZE (type)
+ && (TREE_CODE (TYPE_SIZE (type)) != INTEGER_CST
+ || !integer_zerop (TYPE_SIZE (type))));
We can also leave out the TREE_CODE check since integer_zerop is false
for non-constants?
@@ -7378,8 +7376,9 @@ struct flexmems_t
/* The first flexible array member or non-zero array member found
in the order of layout. */
tree array;
- /* First non-static non-empty data member in the class or its bases. */
- tree first;
+ /* True if there is a non-static non-empty data member in the class or
+ its bases. */
+ bool first;
/* The first non-static non-empty data member 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
@@ -7400,24 +7399,50 @@ struct flexmems_t
array, in that order of preference, among members of class T (but not
its base classes), and set members of FMEM accordingly.
BASE_P is true if T is a base class of another class.
- PUN is set to the outermost union in which the flexible array member
- (or zero-length array) is defined if one such union exists, otherwise
- to NULL.
- Similarly, PSTR is set to a data member of the outermost struct of
+ PUN is true when inside of a union (perhaps recursively).
+ PSTR is set to a data member of the outermost struct of
which the flexible array is a member if one such struct exists,
otherwise to NULL. */
This needs to document the new nested_p parameter.
static void
find_flexarrays (tree t, flexmems_t *fmem, bool base_p,
- tree pun /* = NULL_TREE */,
+ bool nested_p /* = false */, bool pun /* = false */,
tree pstr /* = NULL_TREE */)
{
- /* Set the "pointer" to the outermost enclosing union if not set
- yet and maintain it for the remainder of the recursion. */
- if (!pun && TREE_CODE (t) == UNION_TYPE)
- pun = t;
+ if (TREE_CODE (t) == UNION_TYPE)
+ pun = true;
- for (tree fld = TYPE_FIELDS (t); fld; fld = DECL_CHAIN (fld))
+ tree fld = TYPE_FIELDS (t);
+ if ((base_p || nested_p) && TREE_CODE (t) == RECORD_TYPE)
+ {
+ /* In bases or in nested structures, only process the last
+ non-static data member. If we have say
+ struct A { int a; int b[]; int c; };
+ struct B { int d; int e[]; int f; };
+ struct C : A { int g; B h, i; int j; };
+ then the A::b followed by A::c should have been diagnosed
+ already when completing struct A, and B::e followed by B::f
+ when completing struct B, so no need to repeat that when completing
+ struct C. So, only look at the last member so we cover e.g.
+ struct D { int k; int l[]; };
+ struct E : D { int m; };
+ struct F { D n; int o; };
+ where flexible array member at the end of D is fine, but it isn't
+ correct that E::m in E or F::o in F follows it. */
+ tree last_fld = NULL_TREE;
+ for (; fld; fld = DECL_CHAIN (fld))
+ if (fld == error_mark_node)
+ return;
+ else if (DECL_ARTIFICIAL (fld) || TREE_CODE (fld) != FIELD_DECL)
+ continue;
+ else if (TREE_TYPE (fld) == error_mark_node)
+ return;
How about using next_subobject_field to skip non-fields?
+ else if (TREE_CODE (TREE_TYPE (fld)) == ARRAY_TYPE
+ || field_nonempty_p (fld))
+ last_fld = fld;
+ fld = last_fld;
+ }
+ for (; fld; fld = DECL_CHAIN (fld))
{
if (fld == error_mark_node)
return;
@@ -7462,11 +7487,11 @@ find_flexarrays (tree t, flexmems_t *fme
if (RECORD_OR_UNION_TYPE_P (eltype))
{
- if (fmem->array && !fmem->after[bool (pun)])
+ if (fmem->array && !fmem->after[pun])
{
/* Once the member after the flexible array has been found
we're done. */
- fmem->after[bool (pun)] = fld;
+ fmem->after[pun] = fld;
break;
}
@@ -7480,32 +7505,43 @@ find_flexarrays (tree t, flexmems_t *fme
are already in the process of being checked by one of the
recursive calls). */
- tree first = fmem->first;
+ bool first = fmem->first;
tree array = fmem->array;
+ bool maybe_anon_p = TYPE_UNNAMED_P (eltype);
+ if (tree ctx = maybe_anon_p ? TYPE_CONTEXT (eltype) : NULL_TREE)
+ maybe_anon_p = RECORD_OR_UNION_TYPE_P (ctx);
/* If this member isn't anonymous and a prior non-flexible array
member has been seen in one of the enclosing structs, clear
the FIRST member since it doesn't contribute to the flexible
array struct's members. */
if (first && !array && !ANON_AGGR_TYPE_P (eltype))
- fmem->first = NULL_TREE;
+ fmem->first = false;
- find_flexarrays (eltype, fmem, false, pun,
- !pstr && TREE_CODE (t) == RECORD_TYPE ? fld :
pstr);
+ find_flexarrays (eltype, fmem, false, !maybe_anon_p, pun,
+ !pstr && TREE_CODE (t) == RECORD_TYPE
+ ? fld : pstr);
if (fmem->array != array)
- continue;
-
- if (first && !array && !ANON_AGGR_TYPE_P (eltype))
{
- /* Restore the FIRST member reset above if no flexible
- array member has been found in this member's struct. */
- fmem->first = first;
+ /* If the recursive call will have nested_p set,
+ it looks just at the last field and we do not
This should be past tense, since the call was just above: "If the
recursive call passed true to nested_p, it only looked at the last field..."
OK with these tweaks.
Jason
