> On Mar 9, 2023, at 7:20 AM, Richard Biener <rguent...@suse.de> wrote:
>
> On Fri, 24 Feb 2023, Qing Zhao wrote:
>
>> GCC extension accepts the case when a struct with a C99 flexible array member
>> is embedded into another struct or union (possibly recursively).
>> __builtin_object_size should treat such struct as flexible size.
>>
>> gcc/c/ChangeLog:
>>
>> PR tree-optimization/101832
>> * c-decl.cc (finish_struct): Set TYPE_INCLUDE_FLEXARRAY for
>> struct/union type.
>
> I can't really comment on the correctness of this part but since
> only the C frontend will ever set this and you are using it from
> addr_object_size which is also used for other C family languages
> (at least), I wonder if you can really test
>
> + if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
>
> there.
You mean for C++ and also other C family languages (other than C), the above
bit cannot be set?
Yes, that’s true. The bit is only set for C. So is the bit DECL_NOT_FLEXARRAY,
which is only set for C too.
So, I am wondering whether the bit DECL_NOT_FLEXARRAY should be also set in
middle end? Or we can set DECL_NOT_FLEXARRAY in C++ FE too? And then we can set
TYPE_INCLUDE_FLEXARRAY also in C++ FE?
What’s your suggestion?
(I personally feel that DECL_NOT_FLEXARRAY and TYPE_INCLUDE_FLEXARRAY should be
set in the same places).
>
> Originally I was suggesting to set this flag in stor-layout.cc
> which eventually all languages funnel their types through and
> if there's language specific handling use a langhook (with the
> default implementation preserving the status quo).
If we decide to set the bits in stor-layout.cc, where is the best place to do
it? I checked the star-layout.cc code, looks like “layout_type” might be the
place where we can set these bits for RECORD_TYPE, UNION_TYPE?
>
> Some more comments below ...
>
>> gcc/cp/ChangeLog:
>>
>> PR tree-optimization/101832
>> * module.cc (trees_out::core_bools): Stream out new bit
>> type_include_flexarray.
>> (trees_in::core_bools): Stream in new bit type_include_flexarray.
>>
>> gcc/ChangeLog:
>>
>> PR tree-optimization/101832
>> * print-tree.cc (print_node): Print new bit type_include_flexarray.
>> * tree-core.h (struct tree_type_common): New bit
>> type_include_flexarray.
>> * tree-object-size.cc (addr_object_size): Handle structure/union type
>> when it has flexible size.
>> * tree-streamer-in.cc (unpack_ts_type_common_value_fields): Stream
>> in new bit type_include_flexarray.
>> * tree-streamer-out.cc (pack_ts_type_common_value_fields): Stream
>> out new bit type_include_flexarray.
>> * tree.h (TYPE_INCLUDE_FLEXARRAY): New macro
>> TYPE_INCLUDE_FLEXARRAY.
>>
>> gcc/testsuite/ChangeLog:
>>
>> PR tree-optimization/101832
>> * gcc.dg/builtin-object-size-pr101832.c: New test.
>> ---
>> gcc/c/c-decl.cc | 12 ++
>> gcc/cp/module.cc | 2 +
>> gcc/print-tree.cc | 5 +
>> .../gcc.dg/builtin-object-size-pr101832.c | 134 ++++++++++++++++++
>> gcc/tree-core.h | 4 +-
>> gcc/tree-object-size.cc | 79 +++++++----
>> gcc/tree-streamer-in.cc | 1 +
>> gcc/tree-streamer-out.cc | 1 +
>> gcc/tree.h | 6 +
>> 9 files changed, 215 insertions(+), 29 deletions(-)
>> create mode 100644 gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
>>
>> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
>> index 08078eadeb8..f589a2f5192 100644
>> --- a/gcc/c/c-decl.cc
>> +++ b/gcc/c/c-decl.cc
>> @@ -9284,6 +9284,18 @@ finish_struct (location_t loc, tree t, tree
>> fieldlist, tree attributes,
>> /* Set DECL_NOT_FLEXARRAY flag for FIELD_DECL x. */
>> DECL_NOT_FLEXARRAY (x) = !is_flexible_array_member_p (is_last_field,
>> x);
>>
>> + /* Set TYPE_INCLUDE_FLEXARRAY for the context of x, t
>> + * when x is an array. */
>> + if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE)
>> + TYPE_INCLUDE_FLEXARRAY (t) = flexible_array_member_type_p (TREE_TYPE
>> (x)) ;
>> + /* Recursively set TYPE_INCLUDE_FLEXARRAY for the context of x, t
>> + when x is the last field. */
>> + else if ((TREE_CODE (TREE_TYPE (x)) == RECORD_TYPE
>> + || TREE_CODE (TREE_TYPE (x)) == UNION_TYPE)
>> + && TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (x))
>> + && is_last_field)
>> + TYPE_INCLUDE_FLEXARRAY (t) = true;
>> +
>> if (DECL_NAME (x)
>> || RECORD_OR_UNION_TYPE_P (TREE_TYPE (x)))
>> saw_named_field = true;
>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
>> index ac2fe66b080..c750361b704 100644
>> --- a/gcc/cp/module.cc
>> +++ b/gcc/cp/module.cc
>> @@ -5371,6 +5371,7 @@ trees_out::core_bools (tree t)
>> WB (t->type_common.lang_flag_5);
>> WB (t->type_common.lang_flag_6);
>> WB (t->type_common.typeless_storage);
>> + WB (t->type_common.type_include_flexarray);
>> }
>>
>> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
>> @@ -5551,6 +5552,7 @@ trees_in::core_bools (tree t)
>> RB (t->type_common.lang_flag_5);
>> RB (t->type_common.lang_flag_6);
>> RB (t->type_common.typeless_storage);
>> + RB (t->type_common.type_include_flexarray);
>> }
>>
>> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
>> diff --git a/gcc/print-tree.cc b/gcc/print-tree.cc
>> index 1f3afcbbc86..efacdb7686f 100644
>> --- a/gcc/print-tree.cc
>> +++ b/gcc/print-tree.cc
>> @@ -631,6 +631,11 @@ print_node (FILE *file, const char *prefix, tree node,
>> int indent,
>> && TYPE_CXX_ODR_P (node))
>> fputs (" cxx-odr-p", file);
>>
>> + if ((code == RECORD_TYPE
>> + || code == UNION_TYPE)
>> + && TYPE_INCLUDE_FLEXARRAY (node))
>> + fputs (" include-flexarray", file);
>> +
>> /* The transparent-union flag is used for different things in
>> different nodes. */
>> if ((code == UNION_TYPE || code == RECORD_TYPE)
>> diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
>> b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
>> new file mode 100644
>> index 00000000000..60078e11634
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
>> @@ -0,0 +1,134 @@
>> +/* PR 101832:
>> + GCC extension accepts the case when a struct with a C99 flexible array
>> + member is embedded into another struct (possibly recursively).
>> + __builtin_object_size will treat such struct as flexible size.
>> + However, when a structure with non-C99 flexible array member, i.e,
>> trailing
>> + [0], [1], or [4], is embedded into anther struct, the stucture will not
>> + be treated as flexible size. */
>> +/* { dg-do run } */
>> +/* { dg-options "-O2" } */
>> +
>> +#include "builtin-object-size-common.h"
>> +
>> +#define expect(p, _v) do { \
>> + size_t v = _v; \
>> + if (p == v) \
>> + __builtin_printf ("ok: %s == %zd\n", #p, p); \
>> + else {\
>> + __builtin_printf ("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
>> + FAIL (); \
>> + } \
>> +} while (0);
>> +
>> +
>> +struct A {
>> + int n;
>> + char data[];
>> +};
>> +
>> +struct B {
>> + int m;
>> + struct A a;
>> +};
>> +
>> +struct C {
>> + int q;
>> + struct B b;
>> +};
>> +
>> +struct A0 {
>> + int n;
>> + char data[0];
>> +};
>> +
>> +struct B0 {
>> + int m;
>> + struct A0 a;
>> +};
>> +
>> +struct C0 {
>> + int q;
>> + struct B0 b;
>> +};
>> +
>> +struct A1 {
>> + int n;
>> + char data[1];
>> +};
>> +
>> +struct B1 {
>> + int m;
>> + struct A1 a;
>> +};
>> +
>> +struct C1 {
>> + int q;
>> + struct B1 b;
>> +};
>> +
>> +struct An {
>> + int n;
>> + char data[8];
>> +};
>> +
>> +struct Bn {
>> + int m;
>> + struct An a;
>> +};
>> +
>> +struct Cn {
>> + int q;
>> + struct Bn b;
>> +};
>> +
>> +volatile void *magic1, *magic2;
>> +
>> +int main (int argc, char *argv[])
>> +{
>> + struct B *outer;
>> + struct C *outest;
>> +
>> + /* Make sure optimization can't find some other object size. */
>> + outer = (void *)magic1;
>> + outest = (void *)magic2;
>> +
>> + expect (__builtin_object_size (&outer->a, 1), -1);
>> + expect (__builtin_object_size (&outest->b, 1), -1);
>> + expect (__builtin_object_size (&outest->b.a, 1), -1);
>> +
>> + struct B0 *outer0;
>> + struct C0 *outest0;
>> +
>> + /* Make sure optimization can't find some other object size. */
>> + outer0 = (void *)magic1;
>> + outest0 = (void *)magic2;
>> +
>> + expect (__builtin_object_size (&outer0->a, 1), sizeof (outer0->a));
>> + expect (__builtin_object_size (&outest0->b, 1), sizeof (outest0->b));
>> + expect (__builtin_object_size (&outest0->b.a, 1), sizeof (outest0->b.a));
>> +
>> + struct B1 *outer1;
>> + struct C1 *outest1;
>> +
>> + /* Make sure optimization can't find some other object size. */
>> + outer1 = (void *)magic1;
>> + outest1 = (void *)magic2;
>> +
>> + expect (__builtin_object_size (&outer1->a, 1), sizeof (outer1->a));
>> + expect (__builtin_object_size (&outest1->b, 1), sizeof (outest1->b));
>> + expect (__builtin_object_size (&outest1->b.a, 1), sizeof (outest1->b.a));
>> +
>> + struct Bn *outern;
>> + struct Cn *outestn;
>> +
>> + /* Make sure optimization can't find some other object size. */
>> + outern = (void *)magic1;
>> + outestn = (void *)magic2;
>> +
>> + expect (__builtin_object_size (&outern->a, 1), sizeof (outern->a));
>> + expect (__builtin_object_size (&outestn->b, 1), sizeof (outestn->b));
>> + expect (__builtin_object_size (&outestn->b.a, 1), sizeof (outestn->b.a));
>> +
>> + DONE ();
>> + return 0;
>> +}
>> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
>> index acd8deea34e..705d5702b9c 100644
>> --- a/gcc/tree-core.h
>> +++ b/gcc/tree-core.h
>> @@ -1718,7 +1718,9 @@ struct GTY(()) tree_type_common {
>> unsigned empty_flag : 1;
>> unsigned indivisible_p : 1;
>> RECORD_OR_UNION_CHECK
>
> it looks like the bit could be eventually shared with
> no_named_args_stdarg_p (but its accessor doesn't use
> FUNC_OR_METHOD_CHECK)
You mean to let “type_include_flexarray” share the same bit with
“no_named_args_stdarg_p” in order to save 1-bit space in IR?
Then change the
/* True if this is a stdarg function with no named arguments (C2x
(...) prototype, where arguments can be accessed with va_start and
va_arg), as opposed to an unprototyped function. */
#define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
(TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
/* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
at the last field recursively. */
#define TYPE_INCLUDE_FLEXARRAY(NODE) \
(TYPE_CHECK (NODE)->type_common.type_include_flexarray)
To:
union {
unsigned no_named_args_stdarg_p : 1;
/* TYPE_INCLUDE_FLEXARRAY flag for RECORD_TYPE and UNION_TYPE. */
unsigned int type_include_flexarray : 1;
} shared_bit;
unsigned spare : 15;
/* True if this is a stdarg function with no named arguments (C2x
(...) prototype, where arguments can be accessed with va_start and
va_arg), as opposed to an unprototyped function. */
#define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
(FUNC_OR_METHOD_CHECK (NODE)->type_common.shared_bit.no_named_args_stdarg_p)
/* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
at the last field recursively. */
#define TYPE_INCLUDE_FLEXARRAY(NODE) \
(RECORD_OR_UNION_CHECK(NODE)->type_common.shared_bit.type_include_flexarray)
>
>> alias_set_type alias_set;
>> tree pointer_to;
>> diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
>> index 9a936a91983..22b3c72ea6e 100644
>> --- a/gcc/tree-object-size.cc
>> +++ b/gcc/tree-object-size.cc
>> @@ -633,45 +633,68 @@ addr_object_size (struct object_size_info *osi,
>> const_tree ptr,
>> v = NULL_TREE;
>> break;
>> case COMPONENT_REF:
>> - if (TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
>> + /* When the ref is not to an array, a record or a union, it
>> + will not have flexible size, compute the object size
>> + directly. */
>> + if ((TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
>> + && (TREE_CODE (TREE_TYPE (v)) != RECORD_TYPE)
>> + && (TREE_CODE (TREE_TYPE (v)) != UNION_TYPE))
>> {
>> v = NULL_TREE;
>> break;
>> }
>> - is_flexible_array_mem_ref = array_ref_flexible_size_p (v);
>> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> - != UNION_TYPE
>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> - != QUAL_UNION_TYPE)
>> - break;
>> - else
>> - v = TREE_OPERAND (v, 0);
>> - if (TREE_CODE (v) == COMPONENT_REF
>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> - == RECORD_TYPE)
>> + if (TREE_CODE (TREE_TYPE (v)) == RECORD_TYPE
>> + || TREE_CODE (TREE_TYPE (v)) == UNION_TYPE)
>> + /* if the record or union does not include a flexible array
>> + recursively, compute the object size directly. */
>> {
>> - /* compute object size only if v is not a
>> - flexible array member. */
>> - if (!is_flexible_array_mem_ref)
>> + if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
>> {
>> v = NULL_TREE;
>> break;
>> }
>> - v = TREE_OPERAND (v, 0);
>> + else
>> + v = TREE_OPERAND (v, 0);
>> }
>> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> - != UNION_TYPE
>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> - != QUAL_UNION_TYPE)
>> - break;
>> - else
>> - v = TREE_OPERAND (v, 0);
>> - if (v != pt_var)
>> - v = NULL_TREE;
>> else
>> - v = pt_var;
>> + {
>> + /* Now the ref is to an array type. */
>> + is_flexible_array_mem_ref
>> + = array_ref_flexible_size_p (v);
>> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> + != UNION_TYPE
>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> + != QUAL_UNION_TYPE)
>> + break;
>> + else
>> + v = TREE_OPERAND (v, 0);
>> + if (TREE_CODE (v) == COMPONENT_REF
>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> + == RECORD_TYPE)
>> + {
>> + /* compute object size only if v is not a
>> + flexible array member. */
>> + if (!is_flexible_array_mem_ref)
>> + {
>> + v = NULL_TREE;
>> + break;
>> + }
>> + v = TREE_OPERAND (v, 0);
>> + }
>> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> + != UNION_TYPE
>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> + != QUAL_UNION_TYPE)
>> + break;
>> + else
>> + v = TREE_OPERAND (v, 0);
>> + if (v != pt_var)
>> + v = NULL_TREE;
>> + else
>> + v = pt_var;
>> + }
>> break;
>> default:
>> v = pt_var;
>> diff --git a/gcc/tree-streamer-in.cc b/gcc/tree-streamer-in.cc
>> index d4dc30f048f..c19ede0631d 100644
>> --- a/gcc/tree-streamer-in.cc
>> +++ b/gcc/tree-streamer-in.cc
>> @@ -390,6 +390,7 @@ unpack_ts_type_common_value_fields (struct bitpack_d
>> *bp, tree expr)
>> TYPE_TRANSPARENT_AGGR (expr) = (unsigned) bp_unpack_value (bp, 1);
>> TYPE_FINAL_P (expr) = (unsigned) bp_unpack_value (bp, 1);
>> TYPE_CXX_ODR_P (expr) = (unsigned) bp_unpack_value (bp, 1);
>> + TYPE_INCLUDE_FLEXARRAY (expr) = (unsigned) bp_unpack_value (bp, 1);
>> }
>> else if (TREE_CODE (expr) == ARRAY_TYPE)
>> TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1);
>> diff --git a/gcc/tree-streamer-out.cc b/gcc/tree-streamer-out.cc
>> index d107229da5c..73e4b4e547c 100644
>> --- a/gcc/tree-streamer-out.cc
>> +++ b/gcc/tree-streamer-out.cc
>> @@ -357,6 +357,7 @@ pack_ts_type_common_value_fields (struct bitpack_d *bp,
>> tree expr)
>> bp_pack_value (bp, flag_wpa && TYPE_CANONICAL (expr)
>> ? TYPE_CXX_ODR_P (TYPE_CANONICAL (expr))
>> : TYPE_CXX_ODR_P (expr), 1);
>> + bp_pack_value (bp, TYPE_INCLUDE_FLEXARRAY (expr), 1);
>> }
>> else if (TREE_CODE (expr) == ARRAY_TYPE)
>> bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1);
>> diff --git a/gcc/tree.h b/gcc/tree.h
>> index 92ac0e6a214..ab1cdc3dc85 100644
>> --- a/gcc/tree.h
>> +++ b/gcc/tree.h
>> @@ -778,6 +778,12 @@ extern void omp_clause_range_check_failed (const_tree,
>> const char *, int,
>> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
>> (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
>>
>> +/* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
>> + at the last field recursively. */
>> +#define TYPE_INCLUDE_FLEXARRAY(NODE) \
>> + (TYPE_CHECK (NODE)->type_common.type_include_flexarray)
>
> Please use RECORD_OR_UNION_CHECK.
Okay.
> The comment "at the last field"
> doesn't seem to match implementation? (at least not obviously)
The current implementation (in gcc/c/c-decl.cc) guarantees that it’s at the
last field. (And recursively).
> Given this is a generic header expanding on what a "flexible array member"
> is to the middle-end here would be good. Especially the guarantees
> made when the bit is _not_ set (we've evaded that with DECL_NOT_FLEXARRAY
> vs. DECL_FLEXARRAY).
The bit TYPE_INCLUDE_FLEXARRAY default is FALSE, i.e, not including a flex
array by default.
It’s only set to TRUE when the struct/union includes a flex array at the last
field (and recursively). So, I think it should be correct.
Let me know if I missed anything here.
Thanks a lot for your comments.
Qing
>
> Thanks,
> Richard.
