From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from hedgehog.birch.relay.mailchannels.net (hedgehog.birch.relay.mailchannels.net [23.83.209.81]) by sourceware.org (Postfix) with ESMTPS id E11F53858D28 for ; Thu, 26 Jan 2023 17:16:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E11F53858D28 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=gotplt.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gotplt.org X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id 1E78A3E12D7; Thu, 26 Jan 2023 17:16:09 +0000 (UTC) Received: from pdx1-sub0-mail-a304.dreamhost.com (unknown [127.0.0.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 8580B3E1C05; Thu, 26 Jan 2023 17:16:08 +0000 (UTC) ARC-Seal: i=1; s=arc-2022; d=mailchannels.net; t=1674753368; a=rsa-sha256; cv=none; b=z//tU0RBPXzBhApoeZq+MPAwXOjhhKh+cjaJpXLneX3130/RMpgFHTRmVAJ3W0awIfKu3M j0Dn4YalnL1ZunSY6EDMOF3jYxsWZMP1bB/d7ae+LT+lvFwpvgr+aD+Ku70KPlhQb8wQ2q d/GIxlHCPWaOneziE/zAhGN4HgnkwiJcLLub8PV525gJzoHURXLn9JtfbD7BZlL9nn1335 SSnQEHtavF81wScWkUuJOg2K+PZGyZqatNGIXLB8vIbntR03lDa4vnF8R3Mm03q26NEGOc CZKyDzOP9CpddwVgN+zZg3iwlGVQgW6utL9qD0BBCA7g5A8HYrCFrCDOj/PbBA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=mailchannels.net; s=arc-2022; t=1674753368; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=Mj4dk9AbXlL86kJgdRrqd5COWzXJtOB4cESwCt/+Kx8=; b=3HQSILJYC+RCPkh/ScthhzV7g6cpH/UJ5PMTjZk+xUct82MPVf858cwYvFKM81mDazpwEc /ybUVe1OUlXr0Ye4lGROLY5r6ljanwq9zWSpGv0L80BXp6HEtu8C/j6BGf6JhlmF4RDw1f LlEs6XH71/r9coPKu6sQk5z5bH4aS6xITu6untQNAkgMzU4zliXwl6IoJ0FK1tDwu2MvxU RAt/3cw6E0m01LhEN7qyWH0iAXVhoibwSMz72aN6vMUvotOsZnJcgKBX1W4Np8Xi2ZCtXX 5DiiwGUNSQ6E1VwSk3k/reJM/X461mPbV4JYaZnF9zS5b0nNraKZhm4oqGipOg== ARC-Authentication-Results: i=1; rspamd-9476994bd-hdglp; auth=pass smtp.auth=dreamhost smtp.mailfrom=siddhesh@gotplt.org X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Chemical-Belong: 25a746264360b188_1674753368904_193787793 X-MC-Loop-Signature: 1674753368904:1414434758 X-MC-Ingress-Time: 1674753368903 Received: from pdx1-sub0-mail-a304.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384) by 100.97.48.83 (trex/6.7.1); Thu, 26 Jan 2023 17:16:08 +0000 Received: from [192.168.0.182] (bras-base-toroon4834w-grc-23-76-68-24-147.dsl.bell.ca [76.68.24.147]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a304.dreamhost.com (Postfix) with ESMTPSA id 4P2nRW3MsFz2P; Thu, 26 Jan 2023 09:16:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gotplt.org; s=dreamhost; t=1674753368; bh=Mj4dk9AbXlL86kJgdRrqd5COWzXJtOB4cESwCt/+Kx8=; h=Date:Subject:To:Cc:From:Content-Type:Content-Transfer-Encoding; b=qopACUDyNsZBPNJR9klda5hXN98Bif2A6O7VdXQBEEgXB+1I7/4E3otzTGk5BrUiy pQmsHUtmPA01yY11yy6GiE2GKX/6eihOMHY+rmVHFxxZlO2TqtwA63SPqZgFjaxTZA 7a2JIV4eALHkLDvZP2DBDts8mJEWCcAg7tV0w6q3/zYLZcQpolVQjAxLxP8qRGiTfO 2w8iWPy8hjICMdFqk/5dlfl/Qyyt9VLIQAff6px7Bd7kc8DidLyIGwtj10qyqgvmG1 5Zud7QUH3kGgAwEl+carbl6vesSkiha/W7RMXpEV+dOF0MC6WQ52CTsEIXjh/9Fev7 P5htkdwyOt9Ww== Message-ID: <00feef0c-0b2f-a600-1f55-4976f9ef21d0@gotplt.org> Date: Thu, 26 Jan 2023 12:16:05 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.0 Subject: Re: [PATCH 2/2] tree-object-size: More consistent behaviour with flex arrays Content-Language: en-US To: Qing Zhao Cc: gcc Patches , Jakub Jelinek , kees Cook References: <20221221222554.4141678-1-siddhesh@gotplt.org> <20221221222554.4141678-3-siddhesh@gotplt.org> <50FEF91F-65B2-4BDC-92C2-BEB8A4A5E3C3@oracle.com> From: Siddhesh Poyarekar In-Reply-To: <50FEF91F-65B2-4BDC-92C2-BEB8A4A5E3C3@oracle.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3031.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,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 2023-01-26 11:20, Qing Zhao wrote: > Hi, Siddhesh, > > Thanks a lot for this patch, after -fstrict-flex-array functionality has been added into GCC, > I think that making the tree-object-size to have consistent behavior with flex arrays is a > valuable and natural work that need to be added. > > I also like the comments you added into tree-object-size.cc, making the code much easier to be understood. > > Minor comments below: > >> On Dec 21, 2022, at 5:25 PM, Siddhesh Poyarekar wrote: >> >> The tree object size pass tries to fail when it detects a flex array in >> the struct, but it ends up doing the right thing only when the flex >> array is in the outermost struct. For nested cases (such as arrays >> nested in a union or an inner struct), it ends up taking whatever value >> the flex array is declared with, using zero for the standard flex array, >> i.e. []. >> >> Rework subobject size computation to make it more consistent across the >> board, honoring -fstrict-flex-arrays. With this change, any array at >> the end of the struct will end up causing __bos to use the allocated >> value of the outer object, bailing out in the maximum case when it can't >> find it. In the minimum case, it will return the subscript value or the >> allocated value of the outer object, whichever is larger. > > I see from the changes in the testing case, there are the following major changes for the existing behavior (can be show with the testing case) > > > ****For non-nested structures: > > struct A > { > char a[10]; > int b; > char c[10]; > }; > > 1. The Minimum size of the reference to the subobject that is a trailing array of a structure is changed from “0” to “sizeof the subobject" >> - if (__builtin_object_size (&p->c, 3) != 0) > + if (__builtin_object_size (&p->c, 3) != 10) > > ****For nested structures: > > struct D > { > int i; > struct D1 > { > char b; > char a[10]; > } j; > }; > > 2. The Maximum size of the reference to the subobject that is a trailing array of the inner structure is changed from “sizeof the subobject” to “-1" >> - if (__builtin_object_size (&d->j.a[3], 1) != sizeof (d->j.a) - 3) >> + if (__builtin_object_size (&d->j.a[3], 1) != (size_t) -1) > > . > 3. The Minimum size of the reference to the subobject that is a trailing array of the inner structure is changed from “0” to “sizeof the subobject" > - if (__builtin_object_size ((char *) &e->j[0], 3) != 0) >> + if (__builtin_object_size ((char *) &e->j[0], 3) != sizeof (e->j)) All of the above is correct, thanks for the high level review! > > I think that all the above changes are good. My only concern is, for the change of the Minimum size of the reference to the subobject that is a trailing array (the above case 1 and 3), will there be any negtive impact on the existing application that use it? I doubt it, because the 0 return value for minimum object size is essentially a failure to determine minimum object size, i.e. a special value. This change can be interpreted as succeeding to get the minimum object size in more cases. Likewise for the maximum object size change, the change can be interpreted as failing to get the maximum object size in more cases. Does that sound reasonable? >> + /* If the subobject size cannot be easily inferred or is smaller than >> + the whole size, just use the whole size. */ > > Should the above comment be: > > + /* If the subobject size cannot be easily inferred or is larger than > + the whole size, just use the whole size. */ > >> if (! TYPE_SIZE_UNIT (TREE_TYPE (var)) >> || ! tree_fits_uhwi_p (TYPE_SIZE_UNIT (TREE_TYPE (var))) >> || (pt_var_size && TREE_CODE (pt_var_size) == INTEGER_CST >> && tree_int_cst_lt (pt_var_size, >> TYPE_SIZE_UNIT (TREE_TYPE (var))))) >> var = pt_var; > Oops, yes indeed, fixed in my copy. Thanks, Sid