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 D43B93858D28 for ; Mon, 4 Oct 2021 16:14:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D43B93858D28 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 1AC8B222DC; Mon, 4 Oct 2021 16:14:53 +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 05B0B13AF3; Mon, 4 Oct 2021 16:14:53 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id nekmAH0oW2HCMgAAMHmgww (envelope-from ); Mon, 04 Oct 2021 16:14:53 +0000 Subject: Re: [PATCH 1/4] [gdb/symtab] Fix htab_find_slot call in read_call_site_scope To: Simon Marchi , gdb-patches@sourceware.org References: <20211001123328.22314-1-tdevries@suse.de> <113a7cab-f06b-32ad-caa1-b0c87e67335b@polymtl.ca> <24809991-4982-9951-a7f7-514a2d01cd10@polymtl.ca> <17aa014b-2500-14f0-81af-a5de2a98e657@suse.de> From: Tom de Vries Message-ID: <6fbc3a03-fc42-8b14-4590-d60657e70446@suse.de> Date: Mon, 4 Oct 2021 18:14:52 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-6.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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 Oct 2021 16:14:55 -0000 On 10/4/21 5:41 PM, Simon Marchi wrote: > On 2021-10-04 08:46, Tom de Vries wrote: >> On 10/4/21 2:05 PM, Simon Marchi wrote: >>>>> Ah, I had not seen this comment. So it was on purpose. Still, I think >>>>> that it makes it more confusing than anything. The patch LGTM. >>>> >>>> And, this follow-up commit reverts everything except the comment. >>>> >>>> Any comments? >>>> >>>> Thanks, >>>> - Tom >>>> >>> >>> Is there a problem with having the lookups done with just the pc? >> >> Well, there's the problem that I describe in the commit message. I >> don't known of any other problem. >> >>> If we >>> were to replace this with some C++ hash table, say std::unordered_map, it >>> would be std::unordered_map. >> >> Right, because there's a separation between key and element. >> >>> So doing >>> the lookups using just the pc in the htab makes sense to me. >> >> I'm sorry, I don't really understand what you're trying to say here. >> >> Do you agree with the patch? Do you disagree with the patch. Are you >> suggesting an alternative solution? > > My thinking was: why not keep core_addr_eq and core_addr_hash, and pass > a pointer to a CORE_ADDR when calling htab_find_slot. And drop the > requirement for call_site::pc to be the first member of call_site. But > I now realize that this may not be a correct use of htab: htab_find > passes entries to the eq func. So if we have core_addr_eq as the eq > func and htab passes a call_site* entry to core_addr_eq, it won't work. > Now, we don't actually use htab_find, so it would still work for us. > But that would be like setting up a trap for whoever tries to use > htab_find in the future. > All find variants (with the explicit exception of find_empty_slot_for_expand) call eq_f on both an element argument and a hash table element. Consequently, if we break first-field-type compatibility between the two, the element argument must be of the same type as the hash table element, and the eq function must use hash table element type. This is not a future problem, this is an actual problem I ran into when I tried it out moving the pc to the second field position, and the problem is described in the commit log. > So, the patche LGTM. Ack, thanks for the review, I'll commit. Thanks, - Tom