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 A67E33858D32 for ; Thu, 7 Jul 2022 10:18:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A67E33858D32 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 D1BB321F7C; Thu, 7 Jul 2022 10:18:14 +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 B7A5213461; Thu, 7 Jul 2022 10:18:14 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id Z/q4K+ayxmIFMAAAMHmgww (envelope-from ); Thu, 07 Jul 2022 10:18:14 +0000 Content-Type: multipart/mixed; boundary="------------YQEF1Teh9z8GCrIOCZO4V2oj" Message-ID: <4af2061e-9583-93fe-f33b-dcf6828ccee3@suse.de> Date: Thu, 7 Jul 2022 12:18:14 +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] Introduce struct packed template, fix -fsanitize=thread for per_cu fields Content-Language: en-US To: Pedro Alves , Tom Tromey Cc: gdb-patches@sourceware.org References: <20220629152914.13149-1-tdevries@suse.de> <20220629152914.13149-3-tdevries@suse.de> <8735fgvie4.fsf@tromey.com> <8c6ba70f-305d-c3ed-591d-36f1f38d52a6@suse.de> From: Tom de Vries In-Reply-To: X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00, BODY_8BITS, 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: Thu, 07 Jul 2022 10:18:17 -0000 This is a multi-part message in MIME format. --------------YQEF1Teh9z8GCrIOCZO4V2oj Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 7/6/22 21:20, Pedro Alves wrote: > On 2022-07-04 8:45 p.m., Tom de Vries via Gdb-patches wrote: >> On 7/4/22 20:32, Tom Tromey wrote: >>>>>>>> "Tom" == Tom de Vries writes: >>> >>> Tom>  /* The number of bits needed to represent all languages, with enough >>> Tom>     padding to allow for reasonable growth.  */ >>> Tom> -#define LANGUAGE_BITS 5 >>> Tom> +#define LANGUAGE_BITS 8 >>> >>> This will negatively affect the size of symbols and so I think it should >>> be avoided. >>> >> >> Ack, Pedro suggested a way to avoid this: >> ... >> +  struct { >> +    /* The language of this CU.  */ >> +    ENUM_BITFIELD (language) m_lang : LANGUAGE_BITS; >> +  }; >> ... >> > > It actually doesn't avoid it in this case, We were merely discussing the usage of LANGUAGE_BITS for general_symbol_info::m_language, and indeed using the "struct { ... };" approach avoids changing the LANGUAGE_BITS and introducing a penalty on symbol size (which is a more numerous entity than CUs). Still, of course it's also good to keep the dwarf2_per_cu_data struct as small as possible, so thanks for looking into this. > as the following field will end up > moved to the next byte, so if LANGUAGE_BITS is 5, we'll end up with 3 bits gap. > > Actually, it's worse than that -- it will align m_lang to ENUM_BITFIELD(language)'s > natural alignment, so it can introduce byte padding before and after too. :-/ :-( > > We can see it with "ptype /o" after applying your patch using the struct{} > trick. Note the "3-byte padding" below: > > ... > /* 13: 5 | 1 */ bool m_header_read_in : 1; > /* XXX 2-bit hole */ > /* 14 | 1 */ struct { > /* 14: 0 | 1 */ bool addresses_seen : 1; > /* XXX 7-bit padding */ > > /* total size (bytes): 1 */ > }; > /* 15: 0 | 4 */ unsigned int mark : 1; > /* 15: 1 | 1 */ bool files_read : 1; > /* XXX 6-bit hole */ > /* 16 | 4 */ struct { > /* 16: 0 | 4 */ dwarf_unit_type unit_type : 8; > /* XXX 3-byte padding */ <<<<<< 3 bytes > > /* total size (bytes): 4 */ > }; > /* 20 | 4 */ struct { > /* 20: 0 | 4 */ language lang : 5; > /* XXX 3-bit padding */ > /* XXX 3-byte padding */ <<<<<< 3 bytes > > /* total size (bytes): 4 */ > }; > ... > > > > So, maybe we really want something else... How about this alternative patch below? > > I wrote the new struct packed template using the array for storage before I > wrote the alternative to use __attribute__((packed)), so I left the array > version in there, as a pure standards-conforming implementation. We can just > remove that array implementation completely, and use the ATTRIBUTE_PACKED macro > if you don't think it's worth it. All compilers worth their salt support attribute > packed, or something like it, I believe. The resulting gdbsupport/pack.h would > be quite smaller. > > I tested this on x86-64 Ubuntu 20.04, both attribute packed no attribute > versions, saw no regressions. Also smoke-tested with Clang (which uses the > attribute packed implementation too, as it defines __GNUC__). > > I have not actually tested this with -fsanitize=thread, though. Would you > be up for testing that, Tom, if this approach looks reasonable? > Yes, of course. I've applied the patch and then started with my latest approach which avoid locks and uses atomics: ... diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h index f98d8b27649..bc1af0ec2d3 100644 --- a/gdb/dwarf2/read.h +++ b/gdb/dwarf2/read.h @@ -108,6 +108,7 @@ struct dwarf2_per_cu_data m_header_read_in (false), mark (false), files_read (false), + m_lang (language_unknown), scanned (false) { } @@ -180,7 +181,7 @@ struct dwarf2_per_cu_data packed m_unit_type = (dwarf_unit_type) 0; /* The language of this CU. */ - packed m_lang = language_unknown; + std::atomic m_lang __attribute__((packed)); public: /* True if this CU has been scanned by the indexer; false if @@ -332,11 +333,13 @@ struct dwarf2_per_cu_data void set_lang (enum language lang) { - /* We'd like to be more strict here, similar to what is done in - set_unit_type, but currently a partial unit can go from unknown to - minimal to ada to c. */ - if (m_lang != lang) - m_lang = lang; + enum language nope = language_unknown; + if (m_lang.compare_exchange_strong (nope, lang)) + return; + nope = lang; + if (m_lang.compare_exchange_strong (nope, lang)) + return; + gdb_assert_not_reached (); } /* Free any cached file names. */ ... I've tried both: ... packed, LANGUAGE_BYTES> m_lang = language_unknown; ... and: ... std::atomic> m_lang = language_unknown; ... and both give compilation errors: ... src/gdb/dwarf2/read.h:184:58: error: could not convert ‘language_unknown’ from ‘language’ to ‘std::atomic >’ std::atomic> m_lang = language_unknown; ^~~~~~~~~~~~~~~~ ... and: ... src/gdb/../gdbsupport/packed.h:84:47: error: bit-field ‘std::atomic packed, 1>::m_val’ with non-integral type ... Maybe one of the two should work and the pack template needs further changes, I'm not sure. Note btw that the attribute packed works here: ... + std::atomic m_lang __attribute__((packed)); ... in the sense that it's got alignment 1: ... struct atomic m_lang \ __attribute__((__aligned__(1))); /* 16 4 */ ... but given that there's no LANGUAGE_BITS/BYTES, we're back to size 4 for the m_lang field, and size 128 overall. So for now I've settled for: ... + std::atomic m_lang; ... which does get me back to size 120. WIP patch attached. Thanks, - Tom --------------YQEF1Teh9z8GCrIOCZO4V2oj Content-Type: text/x-patch; charset=UTF-8; name="0002-fix.patch" Content-Disposition: attachment; filename="0002-fix.patch" Content-Transfer-Encoding: base64 Zml4CgotLS0KIGdkYi9kZWZzLmggICAgICAgIHwgIDMgKysrCiBnZGIvZHdhcmYyL3JlYWQu aCB8IDIyICsrKysrKysrKysrKysrLS0tLS0tLS0KIDIgZmlsZXMgY2hhbmdlZCwgMTcgaW5z ZXJ0aW9ucygrKSwgOCBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9nZGIvZGVmcy5oIGIv Z2RiL2RlZnMuaAppbmRleCAxOWYzNzlkNjU4OC4uYzEyOWJmNjMzYTEgMTAwNjQ0Ci0tLSBh L2dkYi9kZWZzLmgKKysrIGIvZ2RiL2RlZnMuaApAQCAtMjM1LDYgKzIzNSw5IEBAIGdkYl9z dGF0aWNfYXNzZXJ0IChucl9sYW5ndWFnZXMgPD0gKDEgPDwgTEFOR1VBR0VfQklUUykpOwog LyogVGhlIG51bWJlciBvZiBieXRlcyBuZWVkZWQgdG8gcmVwcmVzZW50IGFsbCBsYW5ndWFn ZXMuICAqLwogI2RlZmluZSBMQU5HVUFHRV9CWVRFUyAoKExBTkdVQUdFX0JJVFMgKyBIT1NU X0NIQVJfQklUIC0gMSkgLyBIT1NUX0NIQVJfQklUKQogCisjZGVmaW5lIExBTkdVQUdFX0NP TlRBSU5FUiB1bnNpZ25lZCBjaGFyCitnZGJfc3RhdGljX2Fzc2VydCAoc2l6ZW9mIChMQU5H VUFHRV9DT05UQUlORVIpID49IExBTkdVQUdFX0JZVEVTKTsKKwogZW51bSBwcmVjaXNpb25f dHlwZQogICB7CiAgICAgc2luZ2xlX3ByZWNpc2lvbiwKZGlmZiAtLWdpdCBhL2dkYi9kd2Fy ZjIvcmVhZC5oIGIvZ2RiL2R3YXJmMi9yZWFkLmgKaW5kZXggZjk4ZDhiMjc2NDkuLmY0MzYy ZmUzZWRlIDEwMDY0NAotLS0gYS9nZGIvZHdhcmYyL3JlYWQuaAorKysgYi9nZGIvZHdhcmYy L3JlYWQuaApAQCAtMTA4LDYgKzEwOCw3IEBAIHN0cnVjdCBkd2FyZjJfcGVyX2N1X2RhdGEK ICAgICAgIG1faGVhZGVyX3JlYWRfaW4gKGZhbHNlKSwKICAgICAgIG1hcmsgKGZhbHNlKSwK ICAgICAgIGZpbGVzX3JlYWQgKGZhbHNlKSwKKyAgICAgIG1fbGFuZyAobGFuZ3VhZ2VfdW5r bm93biksCiAgICAgICBzY2FubmVkIChmYWxzZSkKICAgewogICB9CkBAIC0xODAsNyArMTgx LDcgQEAgc3RydWN0IGR3YXJmMl9wZXJfY3VfZGF0YQogICBwYWNrZWQ8ZHdhcmZfdW5pdF90 eXBlLCAxPiBtX3VuaXRfdHlwZSA9IChkd2FyZl91bml0X3R5cGUpIDA7CiAKICAgLyogVGhl IGxhbmd1YWdlIG9mIHRoaXMgQ1UuICAqLwotICBwYWNrZWQ8bGFuZ3VhZ2UsIExBTkdVQUdF X0JZVEVTPiBtX2xhbmcgPSBsYW5ndWFnZV91bmtub3duOworICBzdGQ6OmF0b21pYzxMQU5H VUFHRV9DT05UQUlORVI+IG1fbGFuZzsKIAogcHVibGljOgogICAvKiBUcnVlIGlmIHRoaXMg Q1UgaGFzIGJlZW4gc2Nhbm5lZCBieSB0aGUgaW5kZXhlcjsgZmFsc2UgaWYKQEAgLTMyNiwx NyArMzI3LDIyIEBAIHN0cnVjdCBkd2FyZjJfcGVyX2N1X2RhdGEKIAogICBlbnVtIGxhbmd1 YWdlIGxhbmcgKCkgY29uc3QKICAgewotICAgIGdkYl9hc3NlcnQgKG1fbGFuZyAhPSBsYW5n dWFnZV91bmtub3duKTsKLSAgICByZXR1cm4gbV9sYW5nOworICAgIExBTkdVQUdFX0NPTlRB SU5FUiBsYyA9IG1fbGFuZzsKKyAgICBlbnVtIGxhbmd1YWdlIGwgPSAoZW51bSBsYW5ndWFn ZSlsYzsKKyAgICBnZGJfYXNzZXJ0IChsICE9IGxhbmd1YWdlX3Vua25vd24pOworICAgIHJl dHVybiBsOwogICB9CiAKICAgdm9pZCBzZXRfbGFuZyAoZW51bSBsYW5ndWFnZSBsYW5nKQog ICB7Ci0gICAgLyogV2UnZCBsaWtlIHRvIGJlIG1vcmUgc3RyaWN0IGhlcmUsIHNpbWlsYXIg dG8gd2hhdCBpcyBkb25lIGluCi0gICAgICAgc2V0X3VuaXRfdHlwZSwgIGJ1dCBjdXJyZW50 bHkgYSBwYXJ0aWFsIHVuaXQgY2FuIGdvIGZyb20gdW5rbm93biB0bwotICAgICAgIG1pbmlt YWwgdG8gYWRhIHRvIGMuICAqLwotICAgIGlmIChtX2xhbmcgIT0gbGFuZykKLSAgICAgIG1f bGFuZyA9IGxhbmc7CisgICAgTEFOR1VBR0VfQ09OVEFJTkVSIGxjID0gKExBTkdVQUdFX0NP TlRBSU5FUilsYW5nOworICAgIExBTkdVQUdFX0NPTlRBSU5FUiBub3BlID0gKExBTkdVQUdF X0NPTlRBSU5FUilsYW5ndWFnZV91bmtub3duOworICAgIGlmIChtX2xhbmcuY29tcGFyZV9l eGNoYW5nZV9zdHJvbmcgKG5vcGUsIGxjKSkKKyAgICAgIHJldHVybjsKKyAgICBub3BlID0g bGFuZzsKKyAgICBpZiAobV9sYW5nLmNvbXBhcmVfZXhjaGFuZ2Vfc3Ryb25nIChub3BlLCBs YykpCisgICAgICByZXR1cm47CisgICAgZ2RiX2Fzc2VydF9ub3RfcmVhY2hlZCAoKTsKICAg fQogCiAgIC8qIEZyZWUgYW55IGNhY2hlZCBmaWxlIG5hbWVzLiAgKi8K --------------YQEF1Teh9z8GCrIOCZO4V2oj--