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 544FE3858D20 for ; Fri, 27 Jan 2023 19:58:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 544FE3858D20 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 CD6681E110; Fri, 27 Jan 2023 14:58:19 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1674849500; bh=alJBjDHuDaphDlj+vQ8mHxbqckIIIxNMTfZPGqpO0gE=; h=Date:Subject:To:References:From:In-Reply-To:From; b=My2QbnI+VuK6UnvKaNXQskGwz7QzmaXW+ek9JbEWD+S0udjbisXDKCD5PRzSdF8+A 2jFH64ft/br18RvsqzORihDOzyA0n7D/hTbLtQXyuSCN8HQXNMPHOvMoTl0kEvg+y3 vxoKsjz35jP9VrmaNeobwFjd3JtDCkg7rEWcC210= Message-ID: <8446b53b-bd73-637b-7613-45c896d7240f@simark.ca> Date: Fri, 27 Jan 2023 14:58:18 -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 , gdb-patches@sourceware.org References: <20230127162603.4127820-1-tromey@adacore.com> From: Simon Marchi In-Reply-To: <20230127162603.4127820-1-tromey@adacore.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 11:26, Tom Tromey via Gdb-patches wrote: > After the cooked index is created, the addrmaps should be const. This > is slightly complicated by the fact that addrmap::foreach is not const > -- however, it should be. The underlying splay tree might be > rearranged during a foreach, but if so, this is a classic case where > 'mutable' semantics make sense. I tried making the parameter to addrmap_foreach_fn const too (which makes sense I think, if we're going to make foreach const), and came up with a slightly larger patch. It also made the find method return const, but it also makes sense given that it is marked const too. See the patch below, I think we can rebase yours on top of that. > diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c > index 09b3fd70b26..6f228cdc749 100644 > --- a/gdb/dwarf2/cooked-index.c > +++ b/gdb/dwarf2/cooked-index.c > @@ -401,10 +401,10 @@ cooked_index_vector::lookup (CORE_ADDR addr) > > /* See cooked-index.h. */ > > -std::vector > +std::vector > cooked_index_vector::get_addrmaps () You can make the get_addrmaps method const itself. Otherwise, LGTM. Simon ---- >From 5ca5ca3ebc138a3474e7708a5696f43a7abff0da Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Fri, 27 Jan 2023 14:46:50 -0500 Subject: [PATCH] gdb: constify addrmap::find and addrmap::foreach Constify the return type of addrmap::find. Since find is a const method, I think it makes sense for it to return a pointer to const, indicating to the callers that they should not modify the returned object. Then, make the foreach method const, and change the parameter to addrmap_foreach_fn to `const void *`. Obviously, the const can be cast away, but if using static_cast instead of C-style casts (like the change I did in addrmap_index_data::operator()), then the compiler won't let you cast the const away. Change-Id: Ia8e69d022564f80d961413658fe6068edc71a094 --- gdb/addrmap.c | 16 ++++++++-------- gdb/addrmap.h | 18 +++++++++--------- gdb/dwarf2/index-write.c | 10 ++++++---- 3 files changed, 23 insertions(+), 21 deletions(-) diff --git a/gdb/addrmap.c b/gdb/addrmap.c index 0a6b2e5a25a9..43a521f38b23 100644 --- a/gdb/addrmap.c +++ b/gdb/addrmap.c @@ -39,7 +39,7 @@ addrmap_fixed::set_empty (CORE_ADDR start, CORE_ADDR end_inclusive, } -void * +const void * addrmap_fixed::find (CORE_ADDR addr) const { const struct addrmap_transition *bottom = &transitions[0]; @@ -82,7 +82,7 @@ addrmap_fixed::relocate (CORE_ADDR offset) int -addrmap_fixed::foreach (addrmap_foreach_fn fn) +addrmap_fixed::foreach (addrmap_foreach_fn fn) const { size_t i; @@ -239,7 +239,7 @@ addrmap_mutable::set_empty (CORE_ADDR start, CORE_ADDR end_inclusive, } -void * +const void * addrmap_mutable::find (CORE_ADDR addr) const { splay_tree_node n = splay_tree_lookup (addr); @@ -265,7 +265,7 @@ addrmap_fixed::addrmap_fixed (struct obstack *obstack, addrmap_mutable *mut) size_t transition_count = 0; /* Count the number of transitions in the tree. */ - mut->foreach ([&] (CORE_ADDR start, void *obj) + mut->foreach ([&] (CORE_ADDR start, const void *obj) { ++transition_count; return 0; @@ -283,7 +283,7 @@ addrmap_fixed::addrmap_fixed (struct obstack *obstack, addrmap_mutable *mut) /* Copy all entries from the splay tree to the array, in order of increasing address. */ - mut->foreach ([&] (CORE_ADDR start, void *obj) + mut->foreach ([&] (CORE_ADDR start, const void *obj) { transitions[num_transitions].addr = start; transitions[num_transitions].value = obj; @@ -317,7 +317,7 @@ addrmap_mutable_foreach_worker (splay_tree_node node, void *data) int -addrmap_mutable::foreach (addrmap_foreach_fn fn) +addrmap_mutable::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; @@ -449,7 +449,7 @@ test_addrmap () CHECK_ADDRMAP_FIND (map2, array, 13, 19, nullptr); /* Iterate over both addrmaps. */ - auto callback = [&] (CORE_ADDR start_addr, void *obj) + auto callback = [&] (CORE_ADDR start_addr, const void *obj) { if (start_addr == core_addr (nullptr)) SELF_CHECK (obj == nullptr); diff --git a/gdb/addrmap.h b/gdb/addrmap.h index a5b784efaf25..008a2dfdd22d 100644 --- a/gdb/addrmap.h +++ b/gdb/addrmap.h @@ -36,8 +36,8 @@ /* 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; /* The base class for addrmaps. */ struct addrmap @@ -85,7 +85,7 @@ struct addrmap void *obj) = 0; /* Return the object associated with ADDR in MAP. */ - virtual void *find (CORE_ADDR addr) const = 0; + virtual const void *find (CORE_ADDR addr) const = 0; /* Relocate all the addresses in MAP by OFFSET. (This can be applied to either mutable or immutable maps.) */ @@ -95,7 +95,7 @@ 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; + virtual int foreach (addrmap_foreach_fn fn) const = 0; }; struct addrmap_mutable; @@ -111,9 +111,9 @@ 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; + const void *find (CORE_ADDR addr) const override; void relocate (CORE_ADDR offset) override; - int foreach (addrmap_foreach_fn fn) override; + int foreach (addrmap_foreach_fn fn) const override; private: @@ -123,7 +123,7 @@ struct addrmap_fixed : public addrmap, struct addrmap_transition { CORE_ADDR addr; - void *value; + const void *value; }; /* The number of transitions in TRANSITIONS. */ @@ -149,9 +149,9 @@ 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; + const void *find (CORE_ADDR addr) const override; void relocate (CORE_ADDR offset) override; - int foreach (addrmap_foreach_fn fn) override; + int foreach (addrmap_foreach_fn fn) const override; private: diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c index ced58eab6612..648e51376eb5 100644 --- a/gdb/dwarf2/index-write.c +++ b/gdb/dwarf2/index-write.c @@ -433,7 +433,8 @@ write_hash_table (mapped_symtab *symtab, data_buf &output, data_buf &cpool) } } -typedef std::unordered_map cu_index_map; +using cu_index_map + = std::unordered_map; /* Helper struct for building the address table. */ struct addrmap_index_data @@ -446,7 +447,7 @@ struct addrmap_index_data data_buf &addr_vec; cu_index_map &cu_index_htab; - int operator() (CORE_ADDR start_addr, void *obj); + int operator() (CORE_ADDR start_addr, const void *obj); /* True if the previous_* fields are valid. We can't write an entry until we see the next entry (since it is only then @@ -472,9 +473,10 @@ add_address_entry (data_buf &addr_vec, /* Worker function for traversing an addrmap to build the address table. */ int -addrmap_index_data::operator() (CORE_ADDR start_addr, void *obj) +addrmap_index_data::operator() (CORE_ADDR start_addr, const void *obj) { - dwarf2_per_cu_data *per_cu = (dwarf2_per_cu_data *) obj; + const dwarf2_per_cu_data *per_cu + = static_cast (obj); if (previous_valid) add_address_entry (addr_vec, base-commit: 90a7c55706695c0afa8f8393c589cd90bdacfef4 -- 2.39.0