From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from poodle.tulip.relay.mailchannels.net (poodle.tulip.relay.mailchannels.net [23.83.218.249]) by sourceware.org (Postfix) with ESMTPS id B515D3858C52 for ; Tue, 31 Oct 2023 17:35:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B515D3858C52 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=gotplt.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gotplt.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org B515D3858C52 Authentication-Results: server2.sourceware.org; arc=pass smtp.remote-ip=23.83.218.249 ARC-Seal: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1698773755; cv=pass; b=pYGQupX+XqVccG2FYU00S4fihUpslmQxJu95JxnSubhkvK8o92XxRhz8PvpQsgq4o1eTB8NKRM6tD49dc1cYS3NTOAzBU94Phlkc7+rbAvMkiU2kvKw6GBhABHgAwus23B3l/vmcWhZvZJU+QWNug83IPVeVPyBoR+Xma/nBy9g= ARC-Message-Signature: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1698773755; c=relaxed/simple; bh=XBXHKk95NHbCc0CI4OZfR5T9/XRBHPGHA08m04S1XCM=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=VyMkkTJrvO0qSQBH8otYTpwlrEbqbXG3Dk/cCkTkgHhQZwEXcpG2BhXL66QvZi9ZZqfWLXFJiH+QmdVXYEInQ062IDgxZ8tERA8Fc06j1GH4DtL0EViaQAu5SdXUQ0qmE4q9yhw6xd6ApEsF+P09FbIJYwe/bVY//u108us8pLU= ARC-Authentication-Results: i=2; server2.sourceware.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 C4C0A8186A; Tue, 31 Oct 2023 17:35:51 +0000 (UTC) Received: from pdx1-sub0-mail-a233.dreamhost.com (unknown [127.0.0.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 653C78198F; Tue, 31 Oct 2023 17:35:51 +0000 (UTC) ARC-Seal: i=1; s=arc-2022; d=mailchannels.net; t=1698773751; a=rsa-sha256; cv=none; b=SOibqcRM+H4XklZ0he3LWq6Cdsupk4h5+jKzDNYQUi9fVnuwd8XSiGgFMUEsIS87WaW6Mv a3c4V6zD2pG/Q3slDbsIa3dFFm5j2nNHeboirTnTAyDMhmkB5cJHEhPJge93BqtEpsuzEE Q8uQDxByaBLiX0N84duHV6hHF4pfKyU7xJ9eSDNMTJIs6wzxkvocYaxXguFNQJ92McuHiY KgOdB0viba/JratSRZ8zYyPJO48kdb+cVulsKaFs8bkhv5mur1ZSuknV3g0bS2GuRnFuHe 7Fc9Uh0znwBz3cJvfmuZf7UH0CETaF+v8H/0js0z88DJ4k5Fx3U/OhTAbySl+A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=mailchannels.net; s=arc-2022; t=1698773751; 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=cKY3Sw8Rix+VJzV5rRGlpYBXsDrCgvkWz9dUaiws6Zg=; b=NG3qq0SHD9sDaRq18bZDQ21gdAZ8Kr/egibhsdlrHwvTE4jGG6e79Cftmn1dsdLRD7qjo1 bI6Rp10L7+sxQrD3d45rSXuDI3OkehHwiLSBecvJ95hwV6rLFQFLI3fXfw6drlkQpfYbqk cZdf8vGdGvSiJ97V9f0khVsxugClA2ySTU1kUyzgG0l+E9CN81UrtpB6dlPj9VGExilHiv FNTKQroyKo8rp/1rchTnq/JRb8zERWOd6iHvxy9KZ/kvl8UBuPrrNkn4eN3/kh+Y0oHvtp 4gxieoQZqrHAf4NfpM14TxNjujCDzDQ+BQ2wOFbNM9i6Lm0kYyDMnfevAKS7UA== ARC-Authentication-Results: i=1; rspamd-79d8cddc67-dvlbm; 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-Irritate-Stop: 53b1aec62a4a5f72_1698773751646_3883045376 X-MC-Loop-Signature: 1698773751646:92842191 X-MC-Ingress-Time: 1698773751646 Received: from pdx1-sub0-mail-a233.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384) by 100.115.112.73 (trex/6.9.2); Tue, 31 Oct 2023 17:35:51 +0000 Received: from [192.168.2.12] (bras-vprn-toroon4834w-lp130-02-142-113-138-136.dsl.bell.ca [142.113.138.136]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a233.dreamhost.com (Postfix) with ESMTPSA id 4SKcjy4ZV5z1X; Tue, 31 Oct 2023 10:35:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gotplt.org; s=dreamhost; t=1698773751; bh=cKY3Sw8Rix+VJzV5rRGlpYBXsDrCgvkWz9dUaiws6Zg=; h=Date:Subject:To:Cc:From:Content-Type:Content-Transfer-Encoding; b=kc7PvrWUb9SUgBMzAog1/7pL2pAUzEoNvqyvM7MjFJp5M/efM/FYuiEJUqI4y0Ms2 inzGa2eJqWapBR1btF/b6nyU9IRUIcS7dSuf3touoDVcJsqY6fZTmxitO7f6BSJ1N6 pZKPCPdMUWfMw0fyhuxAXlkFz/VZRKE34X2TPibV9ibD/KQ37NWXPz76HulkZuyglU mpLO3XpFv3YsOUO7uOweXASX/LsjC0vIcLnQX1hIu7C6vFmY+Ie7ynIADO1KuHC7m8 uDJixGL/LFP2k2Qdg2ZWF7e8Wkf/Nz+e3lAkDWZuYuQMYBxW7ZALPWgWHItKlZXQaI son+/dtPN95yw== Message-ID: <3439786d-d62b-4e04-b512-c21bcfc49eb2@gotplt.org> Date: Tue, 31 Oct 2023 13:35:48 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: RFC: the proposal to resolve the missing dependency issue for counted_by attribute Content-Language: en-US To: Qing Zhao , Richard Biener , Joseph Myers , Martin Uecker Cc: Kees Cook , Jakub Jelinek , "isanbard@gmail.com" , GCC Patches References: From: Siddhesh Poyarekar In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3029.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,KAM_ASCII_DIVIDERS,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no 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-10-31 12:26, Qing Zhao wrote: > Hi, > > I wrote a summary based on our extensive discussion, hopefully this can be served as an informal proposal. > > Please take a look at it and let me know any comment or suggestion. > > There are some (???) in the section 3.2 and 3.6, those are my questions seeking for help. -:) > > Thanks again for all the help. > > Qing. > > ======================================================== > Represent the missing dependence for the "counted_by" attribute and its consumers > > Qing Zhao > > 10/30/2023 > ============================================== > > The whole discussion is at: > https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633783.html > > 1. The problem > > There is a data dependency between the size assignment and the implicit use of the size information in the __builtin_dynamic_object_size that is missing in the IL (line 11 and line 13 in the below example). Such information missing will result incorrect code reordering and other code transformations. > > 1 struct A > 2 { > 3 size_t size; > 4 char buf[] __attribute__((counted_by(size))); > 5 }; > 6 > 7 size_t > 8 foo (size_t sz) > 9 { > 10 struct A *obj = __builtin_malloc (sizeof(struct A) + sz * sizeof(char)); > 11 obj->size = sz; > 12 obj->buf[0] = 2; > 13 return __builtin_dynamic_object_size (obj->buf, 1); > 14 } > > Please see a more complicate example in the Appendex 1. > > We need to represent such data dependency correctly in the IL. > > 2. The solution: > > 2.1 Summary > > * Add a new internal function "ACCESS_WITH_SIZE" to carry the size information for every FAM field access; > * In C FE, Replace every FAM field access whose TYPE has the "counted_by" attribute with the new internal function "ACCESS_WITH_SIZE"; > * In every consumer of the size information, for example, BDOS or array bound sanitizer, query the size information or ACCESS_MODE information from the new internal function; > * When the size information and the "ACCESS_MODE" information are not used anymore, possibly at the 2nd object size phase, replace the internal function with the actual FAM field access; > * Some adjustment to inlining heuristic and some SSA passes to mitigate the impact to the optimizer and code generation. > > 2.2 The new internal function > > .ACCESS_WITH_SIZE (PTR, SIZE, ACCESS_MODE) > > INTERNAL_FN (ACCESS_WITH_SIZE, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) > > which returns the "PTR" same as the 1st argument; > > 1st argument "PTR": Pointer to the object; > 2nd argument "SIZE": The size of the pointed object, > if the pointee of the "PTR" has a > * real type, it's the number of the elements of the type; > * void type, it's the number of bytes; > 3rd argument "ACCESS_MODE": > -1: Unknown access semantics > 0: none > 1: read_only > 2: write_only > 3: read_write > > NOTEs, > A. This new internal function is intended for a more general use from all the 3 attributes, "access", "alloc_size", and the new "counted_by", to encode the "size" and "access_mode" information to the corresponding pointer. (in order to resolve PR96503, etc. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503) > B. For "counted_by" and "alloc_size" attributes, the 3rd argument will be -1. > C. In this wrieup, we focus on the implementation details for the "counted_by" attribute. However, this function should be ready to be used by "access" and "alloc_size" without issue. > > 2.3 A new semantic requirement in the user documentation of "counted_by" > > For the following structure including a FAM with a counted_by attribute: > > struct A > { > size_t size; > char buf[] __attribute__((counted_by(size))); > }; > > for any object with such type: > > struct A *obj = __builtin_malloc (sizeof(struct A) + sz * sizeof(char)); > > The setting to the size field should be done before the first reference to the FAM field. A more flexible specification could be stating that validation for a reference to the FAM field will use the latest value assigned to the size field before that reference. That will allow for situations like: o->size = val1; deref (o->buf); o->size = val2; making it clear that deref will see val1 and not val2. > > Such requirement to the user will guarantee that the first reference to the FAM knows the size of the FAM. > > We need to add this additional requirement to the user document. > > 2.4 Replace FAM field accesses with the new function ACCESS_WITH_SIZE > > In C FE: > > for every reference to a FAM, for example, "obj->buf" in the small example, > check whether the corresponding FIELD_DECL has a "counted_by" attribute? > if YES, replace the reference to "obj->buf" with a call to > .ACCESS_WITH_SIZE (obj->buf, obj->size, -1); > > 2.5 Query the size info > > There are multiple consumers of the size info (and ACCESS_MODE info): > > * __builtin_dynamic_object_size; > * array bound sanitizer; > > in these consumers, get the size info from the 2nd argument of the call to > ACCESS_WITH_SIZE (PTR, SIZE, -1) > > 2.6 Eliminate the internal function when not useful anymore > > After the last consumer of the size information in the ACCESS_WITH_SIZE, We should replace the internal call with its first argument. > > Do it in the 2nd object size phase. > > 2.7 Adjustment to inlining heuristic and other IPA analysis > > the FE changes: > > obj->buf > > to > > _1 = obj->buf; > _2 = obj->size; > .ACCESS_WITH_SIZE (_1, _2, -1) > > there are major two changes: > > A. the # of LOADs, the # of Insns of the routine will be increased, > B. additional calls to internal routines. > > As a result: > Inlining decision and some IPA analysis might be changed due to these changes. > > We need to adjust inlining heurisic and IPA analysis properly. > > 3. The patch sets: > > We will provide the following patches: > > 3.1 Provide counted_by attribute to flexible array member field; > which includes: > * "counted_by" attribute documentation; > * C FE handling of the new attribute; > syntax checking, error reporting; > * testing cases; > > 3.2 Convert "counted_by" attribute to/from .ACCESS_WITH_SIZE. > which includes: > * The definition of the new internal function .ACCESS_WITH_SIZE in internal-fn.def. > * C FE converts every access to a FAM with "counted_by" attribute to a call to the internal function .ACCESS_WITH_SIZE. (where in the FE, which routine should look at???) > * Convert every call to .ACCESS_WITH_SIZE to its first argument. where should we do this? in the end of the 2nd object size phase or in the expand phase? (I prefer to do this in the end of the 2nd object size phase, comments???) I'd say be like __bos here, i.e. replace instances in the objsz2 pass and also as a fallback, implement expand_builtin_access_with_size (like expand_builtin_object_size) to replace all remaining instances of the builtin with the first arg. > * adjust inlining heuristic for the additional call and loads (???). > * Other phases need to be adjusted (???). Do we need this? I thought expressing the dependency with the builtin ought to be sufficient. > * verify a call to .ACCESS_WITH_SIZE in tree-cfg.cc. > * provide utility routines to get the size or ACCESS_MODE from the call to .ACCESS_WITH_SIZE. > > 3.3 Use the .ACCESS_WITH_SIZE in builtin object size (sub-object only) > which includes: > * use the size info of the .ACCESS_WITH_SIZE for sub-object. > * testing cases. > > 3.4 Use the .ACCESS_WITH_SIZE in bound sanitizer > which includes: > * use the size info of the .ACCESS_WITH_SIZE for bound sanitizer. > * testing cases. > > 3.5 Improve __bdos to use the counted_by info in whole-object size for the structure with FAM. > 3.6 Support for dynamic array of FMA (???) > https://gcc.gnu.org/pipermail/gcc-patches/2023-October/634568.html I'm not entirely convinced that it's an issue. __bdos will just look at __counted_by__ and use its most recent value (using the above user specification) and with that it's just a matter of application preference. If they want dynamic array behaviour, they could just annotate the array count member with __counted_by__ instead of the array capacity member. Thanks, Sid > > 3.7 Emit warnings when the user breaks the requirments for the new counted_by attribute > compilation time: -Wcounted-by > run time: -fsanitizer=counted-by > > * The setting to the size field should be done before the first reference to the FAM field. > * the array has at least # of elements specified by the size field all the time during the program. > > > 4. Future work > > 4.1 Use the new .ACCESS_WITH_SIZE for other relative attributes, for example, "access" and "alloc_size", to resolve the known issues for them: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503 > > 4.2 Add "counted_by" attribute to general pointers as proposed by the -fbounds-safety proposal to LLVM from Apple (https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854). > > the .ACCESS_WITH_SIZE approach should be adopted naturally. > > > Appendex 1. A more complicated example: > > 1 #include > 2 struct A > 3 { > 4 size_t size; > 5 char buf[] __attribute__((counted_by(size))); > 6 }; > 7 > 8 static size_t > 9 get_size_from (void *ptr) > 10 { > 11 return __builtin_dynamic_object_size (ptr, 1); > 12 } > 13 > 14 void > 15 foo (size_t sz) > 16 { > 17 struct A *obj = __builtin_malloc (sizeof(struct A) + sz * sizeof(char)); > 18 obj->size = sz; > 19 obj->buf[0] = 2; > 20 __builtin_printf (“%d\n", get_size_from (obj->buf)); > 21 return; > 22 } > 23 > 24 int main () > 25 { > 26 foo (20); > 27 return 0; > 28 } > > In this example, the instantiation of the object at line 17 is not in the same routine as the _BDOS call at line 11. > > Appendex 2. Other approaches that have been discussed: > > A. Add an additional argument, the size parameter, to the call to __bdos, > A.1, during FE; > A.2, during gimplification phase; > B. Encode the implicit USE in the type of size, to make the size as “volatile”; > C. Encode the implicit USE in the type of buf, then update the optimization passes to use this implicit USE encoded in the type of buf. > > ** Approach A (both A.1 and A.2) will not work if the object instantiation is not in the same routine as the __bdos call as shown in the above example; > ** Approach B will have a bigger performance impact due to the "volatile" will inhibit a lot of optimizations; > ** Approach C will involve a lot of changes in GCC, and also not very necessary since the ONLY implicit use of the size parameter is in the __bdos call when __bdos can see the real object.