From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id D725D3858D33 for ; Thu, 9 Mar 2023 12:20:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D725D3858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 507D220084; Thu, 9 Mar 2023 12:20:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1678364410; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=SPLpirzYtUDYBsimSZBwANrZGdalNMqHSOPFfypkuhk=; b=VHUgugBnYzRjQ06h94Zl3qQfBp0gt7CJmvLXkLMfRfyFArlDAebFF0FzqdavYvCy33FZup InoNeRkK/Lc600MAZkK5/uJYAtcccALWKmVfKMHl8cVx6LR3xIrhG2Gi+26Y8rIMCcdPEy 4NzNvN7ySjDyAl/4rnjYpPgqMngXSwk= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1678364410; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=SPLpirzYtUDYBsimSZBwANrZGdalNMqHSOPFfypkuhk=; b=XRT6it2jeXJOB8wmlRaNKnubPTnDtPRb+Ez95cVc5sTNuoCR5yTnkDkBsxD3mCJJadMAG9 CCl6GvwQ4SYQAMBg== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 27D652C141; Thu, 9 Mar 2023 12:20:05 +0000 (UTC) Date: Thu, 9 Mar 2023 12:20:09 +0000 (UTC) From: Richard Biener To: Qing Zhao cc: joseph@codesourcery.com, siddhesh@gotplt.org, keescook@chromium.org, gcc-patches@gcc.gnu.org Subject: Re: [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832] In-Reply-To: <20230224183505.4112295-2-qing.zhao@oracle.com> Message-ID: References: <20230224183505.4112295-1-qing.zhao@oracle.com> <20230224183505.4112295-2-qing.zhao@oracle.com> User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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. 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). 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; > unsigned no_named_args_stdarg_p : 1; > - unsigned spare : 15; > + /* TYPE_INCLUDE_FLEXARRAY flag for RECORD_TYPE and UNION_TYPE. */ > + unsigned int type_include_flexarray : 1; > + unsigned spare : 14; 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) > 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. The comment "at the last field" doesn't seem to match implementation? (at least not obviously) 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). Thanks, Richard.