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 E8A253857B82 for ; Mon, 4 Jul 2022 07:04:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E8A253857B82 Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 010CF22BAF; Mon, 4 Jul 2022 07:04:03 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id CC27113451; Mon, 4 Jul 2022 07:04:02 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id gz6xMOKQwmLGFwAAMHmgww (envelope-from ); Mon, 04 Jul 2022 07:04:02 +0000 Content-Type: multipart/mixed; boundary="------------iYY1wYrMKJwSSiMN9z4bIWmL" Message-ID: <138377b8-eb0f-e212-7235-4ca483c67169@suse.de> Date: Mon, 4 Jul 2022 09:04:02 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [PATCH 3/5] [gdb/symtab] Work around fsanitize=address false positive for per_ cu->lang Content-Language: en-US To: Pedro Alves , gdb-patches@sourceware.org Cc: Tom Tromey References: <20220629152914.13149-1-tdevries@suse.de> <20220629152914.13149-3-tdevries@suse.de> <1cdbd4f9-4da7-0dd3-ea91-496797f2ad72@palves.net> <62bc9947-ac55-a976-d225-5c828207f558@palves.net> From: Tom de Vries In-Reply-To: <62bc9947-ac55-a976-d225-5c828207f558@palves.net> X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Jul 2022 07:04:06 -0000 This is a multi-part message in MIME format. --------------iYY1wYrMKJwSSiMN9z4bIWmL Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 6/29/22 20:28, Pedro Alves wrote: > On 2022-06-29 19:25, Pedro Alves wrote: >> On 2022-06-29 18:38, Pedro Alves wrote: >>> On 2022-06-29 16:29, Tom de Vries via Gdb-patches wrote: >>>> When building gdb with -fsanitize=thread and gcc 12, and running test-case >>>> gdb.dwarf2/dwz.exp, we run into a data race between: >>>> ... >>>> Read of size 1 at 0x7b200000300d by thread T2:^M >>>> #0 cutu_reader::cutu_reader(dwarf2_per_cu_data*, dwarf2_per_objfile*, \ >>>> abbrev_table*, dwarf2_cu*, bool, abbrev_cache*) gdb/dwarf2/read.c:6164 \ >>>> (gdb+0x82ec95)^M >>>> ... >>>> and: >>>> ... >>>> Previous write of size 1 at 0x7b200000300d by main thread:^M >>>> #0 prepare_one_comp_unit gdb/dwarf2/read.c:23588 (gdb+0x86f973)^M >>>> ... >>>> >>>> In other words, between: >>>> ... >>>> if (this_cu->reading_dwo_directly) >>>> ... >>>> and: >>>> ... >>>> cu->per_cu->lang = pretend_language; >>>> ... >>>> >>>> Both fields are part of the same bitfield, and writing to one field while >>>> reading from another is not a problem, so this is a false positive. >>> >>> I don't understand this sentence. Particularly "same bitfield", or >>> really "Both fields are part of the same bitfield,". How can two bitfields >>> be part of the same bitfield? >>> >>> Anyhow, both bitfields are part of a sequence of contiguous bitfields, here >>> stripped of comments: >>> >>> unsigned int queued : 1; >>> unsigned int is_debug_types : 1; >>> unsigned int is_dwz : 1; >>> unsigned int reading_dwo_directly : 1; >>> unsigned int tu_read : 1; >>> mutable bool m_header_read_in : 1; >>> bool addresses_seen : 1; >>> unsigned int mark : 1; >>> bool files_read : 1; >>> ENUM_BITFIELD (dwarf_unit_type) unit_type : 8; >>> ENUM_BITFIELD (language) lang : LANGUAGE_BITS; >>> >>> Per C++11, they're all part of the same _memory location_. From N3253 (C++11), intro.memory: >>> >>> "A memory location is either an object of scalar type or a maximal sequence of adjacent bit-fields all having >>> non-zero width. (...) Two threads of execution (1.10) can update and access separate memory locations >>> without interfering with each other. >>> (...) >>> [ Note: Thus a bit-field and an adjacent non-bit-field are in separate memory locations, and therefore can be >>> concurrently updated by two threads of execution without interference. The same applies to two bit-fields, >>> if one is declared inside a nested struct declaration and the other is not, or if the two are separated by >>> a zero-length bit-field declaration, or if they are separated by a non-bit-field declaration. It is not safe to >>> concurrently update two bit-fields in the same struct if all fields between them are also bit-fields of non-zero >>> width. — end note ]" >>> >>> And while it is true that in practice writing to one bit-field from one thread and reading from another, >>> if they reside on the same location, is OK in practice, it is still undefined behavior. Ack, thanks for pointing that out, I was not aware of this. I've reformulated things in terms of "memory location". >>> >>> Note the escape hatch mentioned above: >>> >>> "if the two are separated by a zero-length bit-field declaration" >>> >>> Thus, a change like this: >>> >>> unsigned int queued : 1; >>> unsigned int is_debug_types : 1; >>> unsigned int is_dwz : 1; >>> unsigned int reading_dwo_directly : 1; >>> unsigned int tu_read : 1; >>> mutable bool m_header_read_in : 1; >>> bool addresses_seen : 1; >>> unsigned int mark : 1; >>> bool files_read : 1; >>> ENUM_BITFIELD (dwarf_unit_type) unit_type : 8; >>> + >>> + /* Ensure lang is a separate memory location, so we can update >>> + it concurrently with other bitfields. */ >>> + char :0; >>> + >>> ENUM_BITFIELD (language) lang : LANGUAGE_BITS; >>> >>> >>> ... should work. >> >> The "if one is declared inside a nested struct declaration and the other >> is not" escape hatch may be interesting too, as in, we'd write: >> >> struct { >> ENUM_BITFIELD (language) lang : LANGUAGE_BITS; >> }; >> >> ... and since the struct is anonymous, nothing else needs to change. >> >> In patch #4, you'd just do this too: >> >> struct { >> ENUM_BITFIELD (dwarf_unit_type) unit_type : 8; >> }; >> >> The "wrapping" syntax seems to read a bit better, particularly since this >> way you don't have to worry about putting a :0 bitfield before and >> another after. > Done. > I keep coming back, sorry... :-P > > Another thought is that in both patches #3 and #4, it's reading_dwo_directly > that is racing with two other bitfields. So I wonder whether it's _that_ one > that should be moved to a separate memory location. I've also tried that, but I got similar errors back, with the same writes but different reads. Also added field addresses_seen, which I found using testing with board cc-with-dwz-m. Thanks, - Tom --------------iYY1wYrMKJwSSiMN9z4bIWmL Content-Type: text/x-patch; charset=UTF-8; name="0001-gdb-symtab-Fix-fsanitize-address-errors-for-per_cu-fields.patch" Content-Disposition: attachment; filename*0="0001-gdb-symtab-Fix-fsanitize-address-errors-for-per_cu-fiel"; filename*1="ds.patch" Content-Transfer-Encoding: base64 W2dkYi9zeW10YWJdIEZpeCBmc2FuaXRpemU9YWRkcmVzcyBlcnJvcnMgZm9yIHBlcl9jdSBm aWVsZHMKCldoZW4gYnVpbGRpbmcgZ2RiIHdpdGggLWZzYW5pdGl6ZT10aHJlYWQgYW5kIGdj YyAxMiwgYW5kIHJ1bm5pbmcgdGVzdC1jYXNlCmdkYi5kd2FyZjIvZHd6LmV4cCwgd2UgcnVu IGludG8gYSBkYXRhIHJhY2UgYmV0d2VlbjoKLi4uCiAgUmVhZCBvZiBzaXplIDEgYXQgMHg3 YjIwMDAwMDMwMGQgYnkgdGhyZWFkIFQyOl5NCiAgICAjMCBjdXR1X3JlYWRlcjo6Y3V0dV9y ZWFkZXIoZHdhcmYyX3Blcl9jdV9kYXRhKiwgZHdhcmYyX3Blcl9vYmpmaWxlKiwgXAogICAg YWJicmV2X3RhYmxlKiwgZHdhcmYyX2N1KiwgYm9vbCwgYWJicmV2X2NhY2hlKikgZ2RiL2R3 YXJmMi9yZWFkLmM6NjE2NCBcCiAgICAoZ2RiKzB4ODJlYzk1KV5NCi4uLgphbmQ6Ci4uLgog IFByZXZpb3VzIHdyaXRlIG9mIHNpemUgMSBhdCAweDdiMjAwMDAwMzAwZCBieSBtYWluIHRo cmVhZDpeTQogICAgIzAgcHJlcGFyZV9vbmVfY29tcF91bml0IGdkYi9kd2FyZjIvcmVhZC5j OjIzNTg4IChnZGIrMHg4NmY5NzMpXk0KLi4uCgpJbiBvdGhlciB3b3JkcywgYmV0d2VlbjoK Li4uCiAgaWYgKHRoaXNfY3UtPnJlYWRpbmdfZHdvX2RpcmVjdGx5KQouLi4KYW5kOgouLi4K ICAgIGN1LT5wZXJfY3UtPmxhbmcgPSBwcmV0ZW5kX2xhbmd1YWdlOwouLi4KCkxpa2V3aXNl LCB3ZSBydW4gaW50byBhIGRhdGEgcmFjZSBiZXR3ZWVuOgouLi4KICBXcml0ZSBvZiBzaXpl IDEgYXQgMHg3YjIwMDAwMDMwMGUgYnkgdGhyZWFkIFQ0OgogICAgIzAgcHJvY2Vzc19wc3lt dGFiX2NvbXBfdW5pdCBnZGIvZHdhcmYyL3JlYWQuYzo2Nzg5IChnZGIrMHg4MzA3MjApCi4u LgphbmQ6Ci4uLgogIFByZXZpb3VzIHJlYWQgb2Ygc2l6ZSAxIGF0IDB4N2IyMDAwMDAzMDBl IGJ5IG1haW4gdGhyZWFkOgogICAgIzAgY3V0dV9yZWFkZXI6OmN1dHVfcmVhZGVyKGR3YXJm Ml9wZXJfY3VfZGF0YSosIGR3YXJmMl9wZXJfb2JqZmlsZSosIFwKICAgIGFiYnJldl90YWJs ZSosIGR3YXJmMl9jdSosIGJvb2wsIGFiYnJldl9jYWNoZSopIGdkYi9kd2FyZjIvcmVhZC5j OjYxNjQgXAogICAgKGdkYisweDgyZWRhYikKLi4uCgpJbiBvdGhlciB3b3JkcywgYmV0d2Vl bjoKLi4uCiAgICAgIHRoaXNfY3UtPnVuaXRfdHlwZSA9IERXX1VUX3BhcnRpYWw7Ci4uLgph bmQ6Ci4uLgogIGlmICh0aGlzX2N1LT5yZWFkaW5nX2R3b19kaXJlY3RseSkKLi4uCgpMaWtl d2lzZSBmb3IgdGhlIHdyaXRlIHRvIGFkZHJlc3Nlc19zZWVuIGluIGNvb2tlZF9pbmRleGVy OjpjaGVja19ib3VuZHMgYW5kIGEKcmVhZCBmcm9tIGlzX2R3eiBpbiBkd2FyZjJfZmluZF9j b250YWluaW5nX2NvbXBfdW5pdCBmb3IgdGVzdC1jYXNlCmdkYi5kd2FyZjIvZHcyLWRpci1m aWxlLW5hbWUuZXhwIGFuZCB0YXJnZXQgYm9hcmQgY2Mtd2l0aC1kd3otbS4KClRoZSBwcm9i bGVtIGlzIHRoYXQgdGhlIHdyaXR0ZW4gZmllbGRzIGFyZSBwYXJ0IG9mIHRoZSBzYW1lIG1l bW9yeSBsb2NhdGlvbiBhcwp0aGUgcmVhZCBmaWVsZHMsIHNvIGV4ZWN1dGluZyBhIHJlYWQg YW5kIHdyaXRlIGluIGRpZmZlcmVudCB0aHJlYWRzIGlzCnVuZGVmaW5lZCBiZWhhdm91ci4K Ck1ha2luZyB0aGUgd3JpdHRlbiBmaWVsZHMgc2VwYXJhdGUgbWVtb3J5IGxvY2F0aW9ucyBm aXhlcyBpdDoKLi4uCiAgc3RydWN0IHsKICAgIEVOVU1fQklURklFTEQgKGR3YXJmX3VuaXRf dHlwZSkgdW5pdF90eXBlIDogODsKICB9OwogIHN0cnVjdCB7CiAgICBFTlVNX0JJVEZJRUxE IChsYW5ndWFnZSkgbGFuZyA6IExBTkdVQUdFX0JJVFM7CiAgfTsKICBzdHJ1Y3QgewogICAg Ym9vbCBhZGRyZXNzZXNfc2VlbiA6IDE7CiAgfTsKLi4uCgpUaGlzIGluY3JlYXNlcyB0aGUg c2l6ZSBvZiBzdHJ1Y3QgZHdhcmYyX3Blcl9jdV9kYXRhIGZyb20gMTIwIHRvIDEyOCAoZm9y IC1tNjQpLgoKVGhlIHNldCBvZiBmaWVsZHMgaGFzIGJlZW4gZXN0YWJsaXNoZWQgZXhwZXJp bWVudGFsbHkgdG8gYmUgdGhlIG1pbmltYWwgc2V0IHRvCmdldCByaWQgb2YgdGhpcyB0eXBl IG9mIC1mc2FuaXRpemU9dGhyZWFkIGVycm9ycywgYnV0IG1vcmUgZmllbGRzIG1pZ2h0CnJl cXVpcmUgdGhlIHNhbWUgdHJlYXRtZW50LgoKTG9va2luZyBhdCB0aGUgcHJvcGVydGllcyBv ZiB0aGUgbGFuZyBmaWVsZCwgdW5saWtlIGR3YXJmX3ZlcnNpb24gaXQncyBub3QKYXZhaWxh YmxlIGluIHRoZSB1bml0IGhlYWRlciwgc28gaXQgd2lsbCBiZSBzZXQgdGhlIGZpcnN0IHRp bWUgZHVyaW5nIHRoZQpwYXJhbGxlbCBjb29rZWQgaW5kZXggcmVhZGluZy4gIFRoZSBzYW1l IGhvbGRzIGZvciB1bml0X3R5cGUsIGFuZCBsaWtld2lzZQpmb3IgYWRkcmVzc2VzX3NlZW4u CgpUZXN0ZWQgb24geDg2XzY0LWxpbnV4LgoKLS0tCiBnZGIvZHdhcmYyL3JlYWQuaCB8IDE3 ICsrKysrKysrKysrKy0tLS0tCiAxIGZpbGUgY2hhbmdlZCwgMTIgaW5zZXJ0aW9ucygrKSwg NSBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9nZGIvZHdhcmYyL3JlYWQuaCBiL2dkYi9k d2FyZjIvcmVhZC5oCmluZGV4IGI3YTAzOTMzYWE1Li5jNGIwMDdkMDY0ZCAxMDA2NDQKLS0t IGEvZ2RiL2R3YXJmMi9yZWFkLmgKKysrIGIvZ2RiL2R3YXJmMi9yZWFkLmgKQEAgLTE2Myw3 ICsxNjMsOSBAQCBzdHJ1Y3QgZHdhcmYyX3Blcl9jdV9kYXRhCiAKICAgLyogSWYgYWRkcmVz c2VzIGhhdmUgYmVlbiByZWFkIGZvciB0aGlzIENVICh1c3VhbGx5IGZyb20KICAgICAgLmRl YnVnX2FyYW5nZXMpLCB0aGVuIHRoaXMgZmxhZyBpcyBzZXQuICAqLwotICBib29sIGFkZHJl c3Nlc19zZWVuIDogMTsKKyAgc3RydWN0IHsKKyAgICBib29sIGFkZHJlc3Nlc19zZWVuIDog MTsKKyAgfTsKIAogICAvKiBBIHRlbXBvcmFyeSBtYXJrIGJpdCB1c2VkIHdoZW4gaXRlcmF0 aW5nIG92ZXIgYWxsIENVcyBpbgogICAgICBleHBhbmRfc3ltdGFic19tYXRjaGluZy4gICov CkBAIC0xNzMsMTEgKzE3NSwxNiBAQCBzdHJ1Y3QgZHdhcmYyX3Blcl9jdV9kYXRhCiAgICAg IHBvaW50IGluIHRyeWluZyB0byByZWFkIGl0IGFnYWluIG5leHQgdGltZS4gICovCiAgIGJv b2wgZmlsZXNfcmVhZCA6IDE7CiAKLSAgLyogVGhlIHVuaXQgdHlwZSBvZiB0aGlzIENVLiAg Ki8KLSAgRU5VTV9CSVRGSUVMRCAoZHdhcmZfdW5pdF90eXBlKSB1bml0X3R5cGUgOiA4Owor ICAvKiBQdXQgdGhpcyBpbiBhIHN0cnVjdCB0byBlbnN1cmUgYSBzZXBhcmF0ZSBtZW1vcnkg bG9jYXRpb24uICAqLworICBzdHJ1Y3QgeworICAgIC8qIFRoZSB1bml0IHR5cGUgb2YgdGhp cyBDVS4gICovCisgICAgRU5VTV9CSVRGSUVMRCAoZHdhcmZfdW5pdF90eXBlKSB1bml0X3R5 cGUgOiA4OworICB9OwogCi0gIC8qIFRoZSBsYW5ndWFnZSBvZiB0aGlzIENVLiAgKi8KLSAg RU5VTV9CSVRGSUVMRCAobGFuZ3VhZ2UpIGxhbmcgOiBMQU5HVUFHRV9CSVRTOworICBzdHJ1 Y3QgeworICAgIC8qIFRoZSBsYW5ndWFnZSBvZiB0aGlzIENVLiAgKi8KKyAgICBFTlVNX0JJ VEZJRUxEIChsYW5ndWFnZSkgbGFuZyA6IExBTkdVQUdFX0JJVFM7CisgIH07CiAKICAgLyog VHJ1ZSBpZiB0aGlzIENVIGhhcyBiZWVuIHNjYW5uZWQgYnkgdGhlIGluZGV4ZXI7IGZhbHNl IGlmCiAgICAgIG5vdC4gICovCg== --------------iYY1wYrMKJwSSiMN9z4bIWmL--