From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 8Fc/LcMFWmFRUwAAWB0awg (envelope-from ) for ; Sun, 03 Oct 2021 15:34:27 -0400 Received: by simark.ca (Postfix, from userid 112) id B67481EE1A; Sun, 3 Oct 2021 15:34:27 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-2.0 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,NICE_REPLY_A,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 196671EDDB for ; Sun, 3 Oct 2021 15:34:27 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 9D62B385840A for ; Sun, 3 Oct 2021 19:34:26 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 9D62B385840A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1633289666; bh=MQlBHDKMeFsQRJJVk6NCiFHwLSgw3N6JZRp+PSrlMu8=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=PqcugORYstjluIRT/mLPmifhbZMxvpM6kan4JDxFs4lp4DIF9JSF5dpAlaKl9sIFd KNkB8eOWWYb9nLQ1T3gpjnnJkLsHTznuRMajm0HWXLRyaeQoJLZf8+bsggnBP19GiI 6tnwBCNZbbrmWfHyF1z2GIDYL0W3Vw5Iu2vnEUI4= Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 351733858D3C for ; Sun, 3 Oct 2021 19:34:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 351733858D3C 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 3A3EB22173; Sun, 3 Oct 2021 19:34:06 +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 18E6713A09; Sun, 3 Oct 2021 19:34:06 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id nq9kBK4FWmEYKgAAMHmgww (envelope-from ); Sun, 03 Oct 2021 19:34:06 +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> Message-ID: Date: Sun, 3 Oct 2021 21:34:05 +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: <113a7cab-f06b-32ad-caa1-b0c87e67335b@polymtl.ca> Content-Type: multipart/mixed; boundary="------------27CB6EB71249A13633920CB2" Content-Language: en-US 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: , From: Tom de Vries via Gdb-patches Reply-To: Tom de Vries Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" This is a multi-part message in MIME format. --------------27CB6EB71249A13633920CB2 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 10/1/21 3:09 PM, Simon Marchi wrote: > > > On 2021-10-01 08:33, Tom de Vries via Gdb-patches wrote: >> From: Simon Marchi >> >> In read_call_site_scope we have: >> ... >> call_site_local.pc = pc; >> slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT); >> ... >> >> The call passes a call_site pointer as element. OTOH, the hashtab is created >> using hash_f == core_addr_hash and eq_f == core_addr_eq, so the element >> will be accessed through a CORE_ADDR pointer. >> >> This is not wrong (at least in C), given that pc is the first field in >> call_site. >> >> Nevertheless, as in call_site_for_pc, make the htab_find_slot call match the >> used hash_f and eq_f by using &pc instead: >> ... >> slot = htab_find_slot (cu->call_site_htab, &pc, INSERT); >> ... >> >> Tested on x86_64-linux. >> >> Co-Authored-By: Tom de Vries >> --- >> gdb/dwarf2/read.c | 5 ++--- >> gdb/gdbtypes.h | 4 +--- >> 2 files changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c >> index 00aa64dd0ab..23870c04e74 100644 >> --- a/gdb/dwarf2/read.c >> +++ b/gdb/dwarf2/read.c >> @@ -13341,7 +13341,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu) >> struct gdbarch *gdbarch = objfile->arch (); >> CORE_ADDR pc, baseaddr; >> struct attribute *attr; >> - struct call_site *call_site, call_site_local; >> + struct call_site *call_site; >> void **slot; >> int nparams; >> struct die_info *child_die; >> @@ -13369,8 +13369,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu) >> cu->call_site_htab = htab_create_alloc_ex (16, core_addr_hash, core_addr_eq, >> NULL, &objfile->objfile_obstack, >> hashtab_obstack_allocate, NULL); >> - call_site_local.pc = pc; >> - slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT); >> + slot = htab_find_slot (cu->call_site_htab, &pc, INSERT); >> if (*slot != NULL) >> { >> complaint (_("Duplicate PC %s for DW_TAG_call_site " >> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h >> index 2a641122aec..84b751e82e3 100644 >> --- a/gdb/gdbtypes.h >> +++ b/gdb/gdbtypes.h >> @@ -1783,9 +1783,7 @@ struct call_site_parameter >> >> struct call_site >> { >> - /* * Address of the first instruction after this call. It must be >> - the first field as we overload core_addr_hash and core_addr_eq >> - for it. */ > > 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 --------------27CB6EB71249A13633920CB2 Content-Type: text/x-patch; charset=UTF-8; name="0001-gdb-symtab-Add-call_site_eq-and-call_site_hash.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename*0="0001-gdb-symtab-Add-call_site_eq-and-call_site_hash.patch" [gdb/symtab] Add call_site_eq and call_site_hash In commit b4c919f7525 "[gdb/symtab] Fix htab_find_slot call in read_call_site_scope" , I removed the comment: ... It must be the first field as we overload core_addr_hash and core_addr_eq for it. ... for field pc of struct call_site. However, this was not tested, and when indeed moving field pc to the second location, we run into a testsuite failure in gdb.trace/entry-values.exp. This is caused by core_addr_eq (the eq_f function for the htab) being called with a pointer to the pc field (as passed into htab_find_slot) and a pointer to a hash table element. Now that pc is no longer the first field, the pointer to hash table element no longer points to the pc field. This could be fixed by simply reinstating the comment, but we're trying to get rid of this kind of tricks that make refactoring more difficult. Instead, fix this by: - reverting commit b4c919f7525, apart from the comment removal, such that we're passing a pointer to element to htab_find_slot - updating the htab_find_slot call in compunit_symtab::find_call_site in a similar manner - adding a call_site_eq and call_site_hash, and using these in the hash table instead of core_addr_eq and core_addr_hash. Tested on x86_64-linux, both with and without a trigger patch that moves pc to the second location in struct call_site. --- gdb/dwarf2/read.c | 7 ++++--- gdb/gdbtypes.h | 15 +++++++++++++++ gdb/symtab.c | 5 ++++- 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index 2d4ca08b667..75d6853fd5b 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -13341,7 +13341,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu) struct gdbarch *gdbarch = objfile->arch (); CORE_ADDR pc, baseaddr; struct attribute *attr; - struct call_site *call_site; + struct call_site *call_site, call_site_local; void **slot; int nparams; struct die_info *child_die; @@ -13366,10 +13366,11 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu) pc = gdbarch_adjust_dwarf2_addr (gdbarch, pc); if (cu->call_site_htab == NULL) - cu->call_site_htab = htab_create_alloc_ex (16, core_addr_hash, core_addr_eq, + cu->call_site_htab = htab_create_alloc_ex (16, call_site_hash, call_site_eq, NULL, &objfile->objfile_obstack, hashtab_obstack_allocate, NULL); - slot = htab_find_slot (cu->call_site_htab, &pc, INSERT); + call_site_local.pc = pc; + slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT); if (*slot != NULL) { complaint (_("Duplicate PC %s for DW_TAG_call_site " diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h index 6d09576208d..8021cb21ecc 100644 --- a/gdb/gdbtypes.h +++ b/gdb/gdbtypes.h @@ -1824,6 +1824,21 @@ struct call_site struct call_site_parameter parameter[1]; }; +static inline int +call_site_eq (const void *a_, const void *b_) +{ + const struct call_site *a = (const call_site *)a_; + const struct call_site *b = (const call_site *)b_; + return core_addr_eq (&a->pc, &b->pc); +} + +static inline hashval_t +call_site_hash (const void *a_) +{ + const struct call_site *a = (const call_site *)a_; + return core_addr_hash (&a->pc); +} + /* The type-specific info for TYPE_CODE_FIXED_POINT types. */ struct fixed_point_type_info diff --git a/gdb/symtab.c b/gdb/symtab.c index 6ec5d95401e..84193ddaae3 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -334,10 +334,13 @@ search_domain_name (enum search_domain e) call_site * compunit_symtab::find_call_site (CORE_ADDR pc) const { + struct call_site call_site_local; if (m_call_site_htab == nullptr) return nullptr; - void **slot = htab_find_slot (m_call_site_htab, &pc, NO_INSERT); + call_site_local.pc = pc; + void **slot + = htab_find_slot (m_call_site_htab, &call_site_local, NO_INSERT); if (slot == nullptr) return nullptr; --------------27CB6EB71249A13633920CB2--