From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id A3A563858D20 for ; Fri, 27 Jan 2023 22:00:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A3A563858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [10.0.0.11] (unknown [217.28.27.60]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 368F11E0D3; Fri, 27 Jan 2023 17:00:13 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1674856813; bh=Aow6IaqjBYIbu4sKPmwbAiRdE1RNXG2axqSxI5QQ9gQ=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=jhvkJm/BuAUnggDIFzpyqNGXf1vLO8kBx+BGOdl8seMONAblTnQouBYCCTyWv3F5q SZy3BukBUYOeF6HNzrQ/D7n5k7VKdkzOOnmXZQEhWt7PhrOa6/Z4QPBVfhZJe/3i3C DkSrWCjsieKN8KI5CnGL2hjegPlRDjpMl/7F6Wag= Message-ID: <5246baed-6ebb-6299-f665-b36f02a5fe1b@simark.ca> Date: Fri, 27 Jan 2023 17:00:12 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: Re: [PATCH] Make addrmap const-correct in cooked index Content-Language: en-US To: Tom Tromey Cc: gdb-patches@sourceware.org References: <20230127162603.4127820-1-tromey@adacore.com> <8446b53b-bd73-637b-7613-45c896d7240f@simark.ca> <87cz6zptg4.fsf@tromey.com> From: Simon Marchi In-Reply-To: <87cz6zptg4.fsf@tromey.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP 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/27/23 16:18, Tom Tromey via Gdb-patches wrote: >>>>>> "Simon" == Simon Marchi writes: > > Simon> -void * > Simon> +const void * > Simon> addrmap_fixed::find (CORE_ADDR addr) const > Simon> { > Simon> const struct addrmap_transition *bottom = &transitions[0]; > > Actually, this is going to rely on casting away const in some spots. > > For example cooked_index::lookup: > > dwarf2_per_cu_data *lookup (CORE_ADDR addr) const > { > return (dwarf2_per_cu_data *) m_addrmap->find (addr); > } > > Probably we can't constify dwarf2_per_cu_data all over. I tried constifying dwarf2_per_cu_data to see where it would break. My thinking is that after the quick symbol implementation has produced all those dwarf2_per_cu_data objects, they are not supposed to change. But there is one ligitimate-ish case where we change one after the fact, when we call set_length in cutu_reader::cutu_reader. So, I redesigned my patch to instead provide both const and non-const versions of find and foreach. I think it's typical for containers in C++ anyway. See v2 below: >From 030f7e72af736f2f46c2c6c033ed1b74bce6bde8 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Fri, 27 Jan 2023 14:46:50 -0500 Subject: [PATCH] gdb: provide const-correct versions of addrmap::find and addrmap::foreach Users of addrmap::find and addrmap::foreach that have a const addrmap should ideally receive const pointers to objects, to indicate they should not be modified. However, users that have a non-const addrmap should still receive a non-const pointer. To achieve this, without adding more virtual methods, make the existing find and foreach virtual methods private and prefix them with "do_". Add small non-const and const wrappers for find and foreach. Obviously, the const can be cast away, but if using static_cast instead of C-style casts, then the compiler won't let you cast the const away. I changed all the callers of addrmap::find and addrmap::foreach I could find to make them use static_cast. Change-Id: Ia8e69d022564f80d961413658fe6068edc71a094 --- gdb/addrmap.c | 10 +++++----- gdb/addrmap.h | 34 ++++++++++++++++++++++++++-------- gdb/dwarf2/cooked-index.h | 2 +- gdb/dwarf2/index-write.c | 2 +- gdb/dwarf2/read.c | 13 +++++++------ 5 files changed, 40 insertions(+), 21 deletions(-) diff --git a/gdb/addrmap.c b/gdb/addrmap.c index 0a6b2e5a25a9..33dc7762d91c 100644 --- a/gdb/addrmap.c +++ b/gdb/addrmap.c @@ -40,7 +40,7 @@ addrmap_fixed::set_empty (CORE_ADDR start, CORE_ADDR end_inclusive, void * -addrmap_fixed::find (CORE_ADDR addr) const +addrmap_fixed::do_find (CORE_ADDR addr) const { const struct addrmap_transition *bottom = &transitions[0]; const struct addrmap_transition *top = &transitions[num_transitions - 1]; @@ -82,7 +82,7 @@ addrmap_fixed::relocate (CORE_ADDR offset) int -addrmap_fixed::foreach (addrmap_foreach_fn fn) +addrmap_fixed::do_foreach (addrmap_foreach_fn fn) const { size_t i; @@ -240,7 +240,7 @@ addrmap_mutable::set_empty (CORE_ADDR start, CORE_ADDR end_inclusive, void * -addrmap_mutable::find (CORE_ADDR addr) const +addrmap_mutable::do_find (CORE_ADDR addr) const { splay_tree_node n = splay_tree_lookup (addr); if (n != nullptr) @@ -317,7 +317,7 @@ addrmap_mutable_foreach_worker (splay_tree_node node, void *data) int -addrmap_mutable::foreach (addrmap_foreach_fn fn) +addrmap_mutable::do_foreach (addrmap_foreach_fn fn) const { return splay_tree_foreach (tree, addrmap_mutable_foreach_worker, &fn); } @@ -368,7 +368,7 @@ addrmap_dump (struct addrmap *map, struct ui_file *outfile, void *payload) addrmap entry defines the end of the range). */ bool previous_matched = false; - auto callback = [&] (CORE_ADDR start_addr, void *obj) + auto callback = [&] (CORE_ADDR start_addr, const void *obj) { QUIT; diff --git a/gdb/addrmap.h b/gdb/addrmap.h index a5b784efaf25..e00dda6e7115 100644 --- a/gdb/addrmap.h +++ b/gdb/addrmap.h @@ -36,8 +36,10 @@ /* The type of a function used to iterate over the map. OBJ is NULL for unmapped regions. */ -typedef gdb::function_view - addrmap_foreach_fn; +using addrmap_foreach_fn + = gdb::function_view; +using addrmap_foreach_const_fn + = gdb::function_view; /* The base class for addrmaps. */ struct addrmap @@ -85,7 +87,11 @@ struct addrmap void *obj) = 0; /* Return the object associated with ADDR in MAP. */ - virtual void *find (CORE_ADDR addr) const = 0; + const void *find (CORE_ADDR addr) const + { return this->do_find (addr); } + + void *find (CORE_ADDR addr) + { return this->do_find (addr); } /* Relocate all the addresses in MAP by OFFSET. (This can be applied to either mutable or immutable maps.) */ @@ -95,7 +101,19 @@ struct addrmap If FN ever returns a non-zero value, the iteration ceases immediately, and the value is returned. Otherwise, this function returns 0. */ - virtual int foreach (addrmap_foreach_fn fn) = 0; + int foreach (addrmap_foreach_const_fn fn) const + { return this->do_foreach (fn); } + + int foreach (addrmap_foreach_fn fn) + { return this->do_foreach (fn); } + + +private: + /* Worker for find, implemented by sub-classes. */ + virtual void *do_find (CORE_ADDR addr) const = 0; + + /* Worker for foreach, implemented by sub-classes. */ + virtual int do_foreach (addrmap_foreach_fn fn) const = 0; }; struct addrmap_mutable; @@ -111,11 +129,11 @@ struct addrmap_fixed : public addrmap, void set_empty (CORE_ADDR start, CORE_ADDR end_inclusive, void *obj) override; - void *find (CORE_ADDR addr) const override; void relocate (CORE_ADDR offset) override; - int foreach (addrmap_foreach_fn fn) override; private: + void *do_find (CORE_ADDR addr) const override; + int do_foreach (addrmap_foreach_fn fn) const override; /* A transition: a point in an address map where the value changes. The map maps ADDR to VALUE, but if ADDR > 0, it maps ADDR-1 to @@ -149,11 +167,11 @@ struct addrmap_mutable : public addrmap void set_empty (CORE_ADDR start, CORE_ADDR end_inclusive, void *obj) override; - void *find (CORE_ADDR addr) const override; void relocate (CORE_ADDR offset) override; - int foreach (addrmap_foreach_fn fn) override; private: + void *do_find (CORE_ADDR addr) const override; + int do_foreach (addrmap_foreach_fn fn) const override; /* A splay tree, with a node for each transition; there is a transition at address T if T-1 and T map to different objects. diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h index e12376cdf982..7a8c18975e5a 100644 --- a/gdb/dwarf2/cooked-index.h +++ b/gdb/dwarf2/cooked-index.h @@ -254,7 +254,7 @@ class cooked_index found. */ dwarf2_per_cu_data *lookup (CORE_ADDR addr) { - return (dwarf2_per_cu_data *) m_addrmap->find (addr); + return static_cast (m_addrmap->find (addr)); } /* Create a new cooked_index_entry and register it with this object. diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c index ced58eab6612..7b1b2d6520cb 100644 --- a/gdb/dwarf2/index-write.c +++ b/gdb/dwarf2/index-write.c @@ -474,7 +474,7 @@ add_address_entry (data_buf &addr_vec, int addrmap_index_data::operator() (CORE_ADDR start_addr, void *obj) { - dwarf2_per_cu_data *per_cu = (dwarf2_per_cu_data *) obj; + dwarf2_per_cu_data *per_cu = static_cast (obj); if (previous_valid) add_address_entry (addr_vec, diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index cd937f24ee74..9d8952a4eb8d 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -4276,8 +4276,9 @@ dwarf2_base_index_functions::find_per_cu (dwarf2_per_bfd *per_bfd, { if (per_bfd->index_addrmap == nullptr) return nullptr; - return ((struct dwarf2_per_cu_data *) - per_bfd->index_addrmap->find (adjusted_pc)); + + void *obj = per_bfd->index_addrmap->find (adjusted_pc); + return static_cast (obj); } struct compunit_symtab * @@ -18276,8 +18277,8 @@ cooked_indexer::scan_attributes (dwarf2_per_cu_data *scanning_per_cu, else if (*parent_entry == nullptr) { CORE_ADDR lookup = form_addr (origin_offset, origin_is_dwz); - *parent_entry - = (cooked_index_entry *) m_die_range_map.find (lookup); + void *obj = m_die_range_map.find (lookup); + *parent_entry = static_cast (obj); } unsigned int bytes_read; @@ -18568,8 +18569,8 @@ cooked_indexer::make_index (cutu_reader *reader) for (const auto &entry : m_deferred_entries) { CORE_ADDR key = form_addr (entry.die_offset, m_per_cu->is_dwz); - cooked_index_entry *parent - = (cooked_index_entry *) m_die_range_map.find (key); + void *obj = m_die_range_map.find (key); + cooked_index_entry *parent = static_cast (obj); m_index_storage->add (entry.die_offset, entry.tag, entry.flags, entry.name, parent, m_per_cu); } base-commit: 35e1763185224b2bf634e19cdfd06dab0ba914e3 -- 2.39.0