From: Qing Zhao <qing.zhao@oracle.com>
To: Richard Biener <rguenther@suse.de>,
Joseph Myers <joseph@codesourcery.com>,
Jakub Jelinek <jakub@redhat.com>
Cc: "siddhesh@gotplt.org" <siddhesh@gotplt.org>,
"keescook@chromium.org" <keescook@chromium.org>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>
Subject: Re: [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832]
Date: Tue, 14 Mar 2023 13:09:54 +0000 [thread overview]
Message-ID: <7B33B0F1-98CE-45D8-940A-6D685BBB2554@oracle.com> (raw)
In-Reply-To: <nycvar.YFH.7.77.849.2303140903160.18795@jbgna.fhfr.qr>
> On Mar 14, 2023, at 5:04 AM, Richard Biener <rguenther@suse.de> wrote:
>
> On Mon, 13 Mar 2023, Qing Zhao wrote:
>
>> Hi, Richard,
>>
>> Do you have more comments on my responds to your previous questions?
>
> No, generally we're not good at naming the shared bits, so keeping the
> old name is fine here.
Okay. I will keep the old name for it.
>
> Btw, I do not feel competent enough to approve the patch, instead that's
> on the burden of the C frontend maintainers and the people understanding
> the object-size code more.
So, Joseph and Jakub should be the ones to approve these patches?
I will update the patches with your suggestions for the bit.
And send the 5th version .
Thanks.
Qing
>
> Richard.
>
>> thanks.
>>
>> Qing
>>
>>> On Mar 10, 2023, at 8:48 AM, Qing Zhao via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>>
>>>
>>>
>>>> On Mar 10, 2023, at 2:54 AM, Richard Biener <rguenther@suse.de> wrote:
>>>>
>>>> On Thu, 9 Mar 2023, Qing Zhao wrote:
>>>>
>>>>>
>>>>>
>>>>>> On Mar 9, 2023, at 7:20 AM, Richard Biener <rguenther@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).
>>>>
>>>> I was wondering if the above test errors on the conservative side
>>>> correctly - it will now, for all but C, cut off some thing where it
>>>> didn't before?
>>>
>>> As long as the default value of TYPE_INCLUDE_FLEXARRAY reflects the correct conservative behavior, then the testing should be correct, right?
>>>
>>> The default value of TYPE_INCLUDE_FLEXARRAY is 0, i.e, FALSE, means that the TYPE does not include a flex array by default, this is the correct behavior. Only when the TYPE does include a flexiarray, it will be set to TRUE. So, I think it’s correct.
>>>
>>> This is a different situation for DECL_NOT_FLEXARRAY, by default, the compiler will treat ALL trailing arrays as FMA, so in order to keep the correct conservative behavior, we should keep the default value for DECL_NOT_FLEXARRAY as it’s a FMA, i.e, DECL_NOT_FLEXARRAY being FALSE, by default. Only when the array is NOT a FMA, we set it to true.
>>>
>>> So, the default value for TYPE_INCLUDE_FLEXARRAY is correct.
>>>>
>>>>>>
>>>>>> 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?
>>>>
>>>> Yes, it would be layout_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)
>>>>
>>>> No, we're just overloading bits by using the same name for different
>>>> purposes. I don't think such a union would pack nicely. We overload
>>>> by documenting the uses, in the above cases the uses are separated
>>>> by the kind of the tree node, RECORD/UNION_TYPE vs. FUNCTION/METHOD_TYPE
>>>> and the accessor macros should be updated to use the appropriate
>>>> tree check macros covering that so we don't try to inspect the
>>>> "wrong bit”.
>>> Okay, I see. Then should I change the name of that bit to one that reflect two usages?
>>>>
>>>>>>
>>>>>>> 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.
>>>>
>>>> See above - I was looking at the use (but I'm not familiar with that
>>>> code), and wondered how the change affects uses from other frontends.
>>>
>>> Please see my explanation above, I think the default behavior from other FE should be kept correctly with this patch.
>>>
>>> thanks.
>>>
>>> Qing
>>>>
>>>> Richard.
>>
>>
>
> --
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
> Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
> HRB 36809 (AG Nuernberg)
next prev parent reply other threads:[~2023-03-14 13:10 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-24 18:35 [V4][PATCH 0/2] Handle component_ref to a structure/union field including FAM for builtin_object_size Qing Zhao
2023-02-24 18:35 ` [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832] Qing Zhao
2023-03-03 0:03 ` Qing Zhao
2023-03-09 12:20 ` Richard Biener
2023-03-09 16:11 ` Qing Zhao
2023-03-10 7:54 ` Richard Biener
2023-03-10 13:48 ` Qing Zhao
2023-03-13 22:42 ` Qing Zhao
2023-03-14 9:04 ` Richard Biener
2023-03-14 13:09 ` Qing Zhao [this message]
2023-02-24 18:35 ` [V4][PATCH 2/2] Update documentation to clarify a GCC extension Qing Zhao
2023-03-03 0:03 ` Qing Zhao
2023-03-12 23:14 ` Sandra Loosemore
2023-03-13 12:46 ` Qing Zhao
2023-03-15 3:26 ` Sandra Loosemore
2023-03-15 14:42 ` Qing Zhao
2023-03-03 0:02 ` [V4][PATCH 0/2] Handle component_ref to a structure/union field including FAM for builtin_object_size Qing Zhao
2023-03-08 18:45 ` Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7B33B0F1-98CE-45D8-940A-6D685BBB2554@oracle.com \
--to=qing.zhao@oracle.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=joseph@codesourcery.com \
--cc=keescook@chromium.org \
--cc=rguenther@suse.de \
--cc=siddhesh@gotplt.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).