From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 92A2A385802F for ; Fri, 3 Feb 2023 07:49:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 92A2A385802F 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-out1.suse.de (Postfix) with ESMTP id AAC3034090; Fri, 3 Feb 2023 07:49:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1675410596; 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=yiPX4mX4RxtHosjjKXNxyBtm32Kc4qz8QMWmuy6sbLE=; b=YHeQZO7IVTjal66F40s/iXOWiNFCy3SyrQ9DmV6qKS160+AOURKWUSaN4QBrhmPPXlpTx1 Z0O7mYujc3SESfQjMz+vbFbKO3U09wRBO30l9gYQLecTgEWrOATxofZGXkAc0+FLFWYfeZ PySHBxQKIIxwHEWZ3vgUc0dvTgTxA9o= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1675410596; 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=yiPX4mX4RxtHosjjKXNxyBtm32Kc4qz8QMWmuy6sbLE=; b=rySwazrMWtT6Filhf9cBdIOu8gh8emdAYFv26xe8B8+WVmm8w6B+AoXuemyHZPtFU/Y+fB nsPI7fGRC9EAYNBQ== 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 614132C141; Fri, 3 Feb 2023 07:49:56 +0000 (UTC) Date: Fri, 3 Feb 2023 07:49:56 +0000 (UTC) From: Richard Biener To: Qing Zhao cc: "gcc-patches@gcc.gnu.org" , "siddhesh@gotplt.org" , "keescook@chromium.org" Subject: Re: [PATCH 1/2] Handle component_ref to a structre/union field including flexible array member [PR101832] In-Reply-To: <3B30CFBF-5004-41A4-940D-1F23C010403B@oracle.com> Message-ID: References: <20230131141140.3610133-1-qing.zhao@oracle.com> <20230131141140.3610133-2-qing.zhao@oracle.com> <812910BC-870E-4432-870D-538024F1A510@oracle.com> <3B30CFBF-5004-41A4-940D-1F23C010403B@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=-5.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,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 Thu, 2 Feb 2023, Qing Zhao wrote: > > > > On Feb 2, 2023, at 8:54 AM, Richard Biener wrote: > > > > On Thu, 2 Feb 2023, Qing Zhao wrote: > > > >> > >> [...] > >>>>>> + return flexible_size_type_p (TREE_TYPE (last)); > >>>>> > >>>>> For types with many members this can become quite slow (IIRC we had > >>>>> bugs about similar walks of all fields in types), and this function > >>>>> looks like it's invoked multiple times on the same type per TU. > >>>>> > >>>>> In principle the property is fixed at the time we lay out a record > >>>>> type, so we might want to compute it at that time and record the > >>>>> result. > >>>> > >>>> You mean in FE? > >>> > >>> Yes, either in the frontend or in the middle-ends layout_type. > >>> > >>>> Yes, that?s better and cleaner. > >>>> > >>>> I will add one more field in the TYPE structure to record this information and check this field during middle end. > >>>> > >>>> I had the same thought in the beginning, but not sure whether adding a > >>>> new field in IR is necessary or not, other places in middle end might > >>>> not use this new field. > >>> > >>> It might be interesting to search for other code walking all fields of > >>> a type to determine this or similar info. > >> > >> There is one which is defined in tree.cc but only is referenced in c/c-decl.cc: > >> > >> /* Determine whether TYPE is a structure with a flexible array member, > >> or a union containing such a structure (possibly recursively). */ > >> flexible_array_type_p > >> > >> However, this routine is a little different than the one I tried to add: > >> > >> In the current routine ?flexible_array_type_p?, only one level nesting in the structure is accepted, multiple nesting in structure is not permitted. > >> > >> So, my question is: shall we accept multiple nesting in structure? i.e. > > > > If we don't reject the testcase with an error, then yes. > > Gcc currently accepts the multiple nesting in structure without error. > So, we will continue to accept such extension as long as the flex array > is at the end of the structure. At the same time, for the case the flex > array is in the middle of the structure, issue additional warnings now > to discourage such usage, and deprecate this case in a future release. > > Does this sound reasonable? Please don't mix several issues - I think the flex array in the middle of a structure is separate and we shouldn't report that as flexible_array_type_p or flexible_size_type_p since the size of the containing structure is not variable. For diagnostic purposes the intended use case is to treat a pointer to a structure that appears to have a fixed size but has (recursive) a member with a flexible array at the end as having variable size. Just the same as array_at_struct_end_p treats this for the case of accesses involving such a type. For the middle position case that's not the case. Richard. > Qing > > > >> struct A { > >> int n; > >> char data[];/* Content following header */ > >> }; > >> > >> struct B { > >> int m; > >> struct A a; > >> }; > >> > >> struct C { > >> int q; > >> struct B b; > >> }; > >> > >> Qing > >>> > >>>> thanks. > >>>> > >>>> Qing > >>>> > >>>>> > >>>>>> + return false; > >>>>>> + case UNION_TYPE: > >>>>>> + for (x = TYPE_FIELDS (type); x != NULL_TREE; x = DECL_CHAIN (x)) > >>>>>> + { > >>>>>> + if (TREE_CODE (x) == FIELD_DECL > >>>>>> + && flexible_array_type_p (TREE_TYPE (x))) > >>>>>> + return true; > >>>>>> + } > >>>>>> + return false; > >>>>>> + default: > >>>>>> + return false; > >>>>>> + } > >>>>>> +} > >>>>>> + > >>>>>> /* Compute __builtin_object_size for PTR, which is a ADDR_EXPR. > >>>>>> OBJECT_SIZE_TYPE is the second argument from __builtin_object_size. > >>>>>> If unknown, return size_unknown (object_size_type). */ > >>>>>> @@ -633,45 +669,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 the record or union does not have flexible size > >>>>>> + compute the object size directly. */ > >>>>>> + if (TREE_CODE (TREE_TYPE (v)) == RECORD_TYPE > >>>>>> + || TREE_CODE (TREE_TYPE (v)) == UNION_TYPE) > >>>>>> { > >>>>>> - /* compute object size only if v is not a > >>>>>> - flexible array member. */ > >>>>>> - if (!is_flexible_array_mem_ref) > >>>>>> + if (!flexible_size_type_p (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; > >>>>>> > >>>>> > >>>>> -- > >>>>> Richard Biener > >>>>> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, > >>>>> Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; > >>>>> HRB 36809 (AG Nuernberg) > >>>> > >>>> > >>> > >>> -- > >>> Richard Biener > >>> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, > >>> Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; > >>> HRB 36809 (AG Nuernberg) > >> > >> > > > > -- > > Richard Biener > > SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, > > Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; > > HRB 36809 (AG Nuernberg) > > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)