From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.223.130]) by sourceware.org (Postfix) with ESMTPS id D27363858D1E for ; Mon, 15 Jan 2024 14:14:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D27363858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de ARC-Filter: OpenARC Filter v1.0.0 sourceware.org D27363858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=195.135.223.130 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1705328089; cv=none; b=A407xLSF4hwiTt7teZLvAU60vrG9L0wD1eKLSjZVF06vDGSRORAhm4n8HBdYx5VJM9PAdOeUbIR/uwBhcgmAMBkL9fOhuoqa3lb8otLGuzB998LR1K9roWg4qNpHPu/6Ekew3dmZoS9iIC9nlcyvyVhJbtveS0FhPx16NuwQ6HI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1705328089; c=relaxed/simple; bh=5igHmV1ET+4ANgrBzA94k4xUZywDoHXs+3AsFPVTov8=; h=DKIM-Signature:DKIM-Signature:DKIM-Signature:DKIM-Signature: Message-ID:Date:MIME-Version:Subject:To:From; b=IYFB1YP+zNEu09oxHwfcueGriPhyiLWOPrh86kIyY1HAT3wygAeh2F86Z7doGOrYddGyg0JWF6x1NsWnM/6ASvG982b3hQX/8LixfcOu3hvVxQ0qo6ieAuRroDslSUj5xdCHaa/Muq+Dia7v780t3AVgmiJwMt4ZAdk+LtAS7nU= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from imap1.dmz-prg2.suse.org (imap1.dmz-prg2.suse.org [IPv6:2a07:de40:b281:104:10:150:64:97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 6F6D1221AB; Mon, 15 Jan 2024 14:14:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1705328085; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=y55H7OvUWzgcUO5QESpctITG8U45tuDSmcIXeZkKDLE=; b=wx9PJuV0uBQ87ALylOpXyg8Vjn2hIsYnQCtV1oMOVPzV1sUjgpOld4GAjNCLosvfH1wJlR eyHfcf/Qkj/ElTuXvipElKhkzIwx7UAUpMy2To2i/uqzrA+fqvEorZayoymqWiIQAUTUex C41T+7RPMEfcApuR4bxmR6hrnhgog5g= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1705328085; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=y55H7OvUWzgcUO5QESpctITG8U45tuDSmcIXeZkKDLE=; b=dBik6phW8PxPPPuBk+EcEfiSA0GYle0Tr6B3MGSQGH1HalqGRYpYYm7Zp3w3Ca+CnTqqId RAQNCXKsP5iPazAg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1705328085; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=y55H7OvUWzgcUO5QESpctITG8U45tuDSmcIXeZkKDLE=; b=wx9PJuV0uBQ87ALylOpXyg8Vjn2hIsYnQCtV1oMOVPzV1sUjgpOld4GAjNCLosvfH1wJlR eyHfcf/Qkj/ElTuXvipElKhkzIwx7UAUpMy2To2i/uqzrA+fqvEorZayoymqWiIQAUTUex C41T+7RPMEfcApuR4bxmR6hrnhgog5g= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1705328085; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=y55H7OvUWzgcUO5QESpctITG8U45tuDSmcIXeZkKDLE=; b=dBik6phW8PxPPPuBk+EcEfiSA0GYle0Tr6B3MGSQGH1HalqGRYpYYm7Zp3w3Ca+CnTqqId RAQNCXKsP5iPazAg== Received: from imap1.dmz-prg2.suse.org (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by imap1.dmz-prg2.suse.org (Postfix) with ESMTPS id 4CA38136F5; Mon, 15 Jan 2024 14:14:45 +0000 (UTC) Received: from dovecot-director2.suse.de ([2a07:de40:b281:106:10:150:64:167]) by imap1.dmz-prg2.suse.org with ESMTPSA id gtOjENU9pWWAbwAAD6G6ig (envelope-from ); Mon, 15 Jan 2024 14:14:45 +0000 Message-ID: Date: Mon, 15 Jan 2024 15:15:35 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] Remove addrmap_fixed::set_entry Content-Language: en-US To: Tom Tromey , gdb-patches@sourceware.org References: <20240114183056.1999705-1-tom@tromey.com> From: Tom de Vries In-Reply-To: <20240114183056.1999705-1-tom@tromey.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Authentication-Results: smtp-out1.suse.de; dkim=pass header.d=suse.de header.s=susede2_rsa header.b=wx9PJuV0; dkim=pass header.d=suse.de header.s=susede2_ed25519 header.b=dBik6phW X-Spamd-Result: default: False [-3.30 / 50.00]; ARC_NA(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; R_DKIM_ALLOW(-0.20)[suse.de:s=susede2_rsa,suse.de:s=susede2_ed25519]; XM_UA_NO_VERSION(0.01)[]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; BAYES_HAM(-3.00)[100.00%]; MIME_GOOD(-0.10)[text/plain]; SPAMHAUS_XBL(0.00)[2a07:de40:b281:104:10:150:64:97:from]; RCVD_COUNT_THREE(0.00)[3]; DKIM_SIGNED(0.00)[suse.de:s=susede2_rsa,suse.de:s=susede2_ed25519]; DKIM_TRACE(0.00)[suse.de:+]; RCPT_COUNT_TWO(0.00)[2]; MX_GOOD(-0.01)[]; DBL_BLOCKED_OPENRESOLVER(0.00)[suse.de:dkim,suse.de:email]; FUZZY_BLOCKED(0.00)[rspamd.com]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+]; RCVD_TLS_ALL(0.00)[]; MID_RHS_MATCH_FROM(0.00)[] X-Rspamd-Server: rspamd1.dmz-prg2.suse.org X-Rspamd-Queue-Id: 6F6D1221AB X-Spam-Level: X-Spam-Score: -3.30 X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00,DKIM_INVALID,DKIM_SIGNED,GIT_PATCH_0,KAM_DMARC_STATUS,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 List-Id: On 1/14/24 19:30, Tom Tromey wrote: > It occurred to me that there is no reason for addrmap_fixed::set_entry > to exist. This patch removes it and removes the abstract virtual > function from the base class. This then required a few minor changes > in the DWARF reader. I consider this a type-safety improvement. > LGTM. Reviewed-By: Tom de Vries Thanks, - Tom > Tested by rebuilding. > --- > gdb/addrmap.c | 9 ----- > gdb/addrmap.h | 81 +++++++++++++++++++++----------------------- > gdb/dwarf2/aranges.c | 2 +- > gdb/dwarf2/aranges.h | 4 +-- > gdb/dwarf2/read.c | 8 ++--- > 5 files changed, 45 insertions(+), 59 deletions(-) > > diff --git a/gdb/addrmap.c b/gdb/addrmap.c > index 2753f85e0a6..162ab84763b 100644 > --- a/gdb/addrmap.c > +++ b/gdb/addrmap.c > @@ -30,15 +30,6 @@ static_assert (sizeof (splay_tree_value) >= sizeof (void *)); > > /* Fixed address maps. */ > > -void > -addrmap_fixed::set_empty (CORE_ADDR start, CORE_ADDR end_inclusive, > - void *obj) > -{ > - internal_error ("addrmap_fixed_set_empty: " > - "fixed addrmaps can't be changed\n"); > -} > - > - > void * > addrmap_fixed::do_find (CORE_ADDR addr) const > { > diff --git a/gdb/addrmap.h b/gdb/addrmap.h > index c6417169443..ba83607ad8c 100644 > --- a/gdb/addrmap.h > +++ b/gdb/addrmap.h > @@ -46,46 +46,6 @@ struct addrmap > { > virtual ~addrmap () = default; > > - /* In the mutable address map MAP, associate the addresses from START > - to END_INCLUSIVE that are currently associated with NULL with OBJ > - instead. Addresses mapped to an object other than NULL are left > - unchanged. > - > - As the name suggests, END_INCLUSIVE is also mapped to OBJ. This > - convention is unusual, but it allows callers to accurately specify > - ranges that abut the top of the address space, and ranges that > - cover the entire address space. > - > - This operation seems a bit complicated for a primitive: if it's > - needed, why not just have a simpler primitive operation that sets a > - range to a value, wiping out whatever was there before, and then > - let the caller construct more complicated operations from that, > - along with some others for traversal? > - > - It turns out this is the mutation operation we want to use all the > - time, at least for now. Our immediate use for address maps is to > - represent lexical blocks whose address ranges are not contiguous. > - We walk the tree of lexical blocks present in the debug info, and > - only create 'struct block' objects after we've traversed all a > - block's children. If a lexical block declares no local variables > - (and isn't the lexical block for a function's body), we omit it > - from GDB's data structures entirely. > - > - However, this means that we don't decide to create a block (and > - thus record it in the address map) until after we've traversed its > - children. If we do decide to create the block, we do so at a time > - when all its children have already been recorded in the map. So > - this operation --- change only those addresses left unset --- is > - actually the operation we want to use every time. > - > - It seems simpler to let the code which operates on the > - representation directly deal with the hair of implementing these > - semantics than to provide an interface which allows it to be > - implemented efficiently, but doesn't reveal too much of the > - representation. */ > - virtual void set_empty (CORE_ADDR start, CORE_ADDR end_inclusive, > - void *obj) = 0; > - > /* Return the object associated with ADDR in MAP. */ > const void *find (CORE_ADDR addr) const > { return this->do_find (addr); } > @@ -127,8 +87,6 @@ struct addrmap_fixed : public addrmap, > addrmap_fixed (struct obstack *obstack, addrmap_mutable *mut); > DISABLE_COPY_AND_ASSIGN (addrmap_fixed); > > - void set_empty (CORE_ADDR start, CORE_ADDR end_inclusive, > - void *obj) override; > void relocate (CORE_ADDR offset) override; > > private: > @@ -165,8 +123,45 @@ struct addrmap_mutable : public addrmap > ~addrmap_mutable (); > DISABLE_COPY_AND_ASSIGN (addrmap_mutable); > > + /* In the mutable address map MAP, associate the addresses from START > + to END_INCLUSIVE that are currently associated with NULL with OBJ > + instead. Addresses mapped to an object other than NULL are left > + unchanged. > + > + As the name suggests, END_INCLUSIVE is also mapped to OBJ. This > + convention is unusual, but it allows callers to accurately specify > + ranges that abut the top of the address space, and ranges that > + cover the entire address space. > + > + This operation seems a bit complicated for a primitive: if it's > + needed, why not just have a simpler primitive operation that sets a > + range to a value, wiping out whatever was there before, and then > + let the caller construct more complicated operations from that, > + along with some others for traversal? > + > + It turns out this is the mutation operation we want to use all the > + time, at least for now. Our immediate use for address maps is to > + represent lexical blocks whose address ranges are not contiguous. > + We walk the tree of lexical blocks present in the debug info, and > + only create 'struct block' objects after we've traversed all a > + block's children. If a lexical block declares no local variables > + (and isn't the lexical block for a function's body), we omit it > + from GDB's data structures entirely. > + > + However, this means that we don't decide to create a block (and > + thus record it in the address map) until after we've traversed its > + children. If we do decide to create the block, we do so at a time > + when all its children have already been recorded in the map. So > + this operation --- change only those addresses left unset --- is > + actually the operation we want to use every time. > + > + It seems simpler to let the code which operates on the > + representation directly deal with the hair of implementing these > + semantics than to provide an interface which allows it to be > + implemented efficiently, but doesn't reveal too much of the > + representation. */ > void set_empty (CORE_ADDR start, CORE_ADDR end_inclusive, > - void *obj) override; > + void *obj); > void relocate (CORE_ADDR offset) override; > > private: > diff --git a/gdb/dwarf2/aranges.c b/gdb/dwarf2/aranges.c > index 0a4ae63801d..4287a5a884b 100644 > --- a/gdb/dwarf2/aranges.c > +++ b/gdb/dwarf2/aranges.c > @@ -26,7 +26,7 @@ > bool > read_addrmap_from_aranges (dwarf2_per_objfile *per_objfile, > dwarf2_section_info *section, > - addrmap *mutable_map, > + addrmap_mutable *mutable_map, > deferred_warnings *warn) > { > /* Caller must ensure that the section has already been read. */ > diff --git a/gdb/dwarf2/aranges.h b/gdb/dwarf2/aranges.h > index d923e2d4905..2ba8a05620d 100644 > --- a/gdb/dwarf2/aranges.h > +++ b/gdb/dwarf2/aranges.h > @@ -22,7 +22,7 @@ > > class dwarf2_per_objfile; > class dwarf2_section_info; > -class addrmap; > +class addrmap_mutable; > > /* Read the address map data from DWARF-5 .debug_aranges, and use it > to populate given addrmap. Returns true on success, false on > @@ -30,7 +30,7 @@ class addrmap; > > extern bool read_addrmap_from_aranges (dwarf2_per_objfile *per_objfile, > dwarf2_section_info *section, > - addrmap *mutable_map, > + addrmap_mutable *mutable_map, > deferred_warnings *warn); > > #endif /* GDB_DWARF2_ARANGES_H */ > diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c > index a50248c4d56..1b5e566bb01 100644 > --- a/gdb/dwarf2/read.c > +++ b/gdb/dwarf2/read.c > @@ -907,7 +907,7 @@ static enum pc_bounds_kind dwarf2_get_pc_bounds (struct die_info *, > unrelocated_addr *, > unrelocated_addr *, > struct dwarf2_cu *, > - addrmap *, > + addrmap_mutable *, > void *); > > static void get_scope_pc_bounds (struct die_info *, > @@ -11013,7 +11013,7 @@ dwarf2_ranges_process (unsigned offset, struct dwarf2_cu *cu, dwarf_tag tag, > static int > dwarf2_ranges_read (unsigned offset, unrelocated_addr *low_return, > unrelocated_addr *high_return, struct dwarf2_cu *cu, > - addrmap *map, void *datum, dwarf_tag tag) > + addrmap_mutable *map, void *datum, dwarf_tag tag) > { > dwarf2_per_objfile *per_objfile = cu->per_objfile; > int low_set = 0; > @@ -11123,7 +11123,7 @@ dwarf2_get_pc_bounds_entry_point (die_info *die, unrelocated_addr *low, > static pc_bounds_kind > dwarf_get_pc_bounds_ranges_or_highlow_pc (die_info *die, unrelocated_addr *low, > unrelocated_addr *high, dwarf2_cu *cu, > - addrmap *map, void *datum) > + addrmap_mutable *map, void *datum) > { > gdb_assert (low != nullptr); > gdb_assert (high != nullptr); > @@ -11192,7 +11192,7 @@ dwarf_get_pc_bounds_ranges_or_highlow_pc (die_info *die, unrelocated_addr *low, > static enum pc_bounds_kind > dwarf2_get_pc_bounds (struct die_info *die, unrelocated_addr *lowpc, > unrelocated_addr *highpc, struct dwarf2_cu *cu, > - addrmap *map, void *datum) > + addrmap_mutable *map, void *datum) > { > dwarf2_per_objfile *per_objfile = cu->per_objfile; >