Message ID | 20230127162603.4127820-1-tromey@adacore.com |
---|---|
State | New |
Headers |
Return-Path: <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 0FC3A385B504 for <patchwork@sourceware.org>; Fri, 27 Jan 2023 16:26:38 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0FC3A385B504 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1674836798; bh=nHyS37I+MDlKDNl3JqeUvgBOyXCDE+8+sLG97HFAIpU=; h=To:Cc:Subject:Date:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=rS3Q5byFNGdiK1aLIGC50cSWrISUmJSUzd5JErjGpIzPjNnGCnRxM1RbU/xUFwrB7 LX+ECNeR6SIpj/vOgYqkv8Asr4OC4JLqngx2lUW3rBkwJNLoEi2ffhea939htadPnE msIxvPnmR5YvJmsDkwvKX8944Azk/d3hZINaCJIc= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mail-io1-xd35.google.com (mail-io1-xd35.google.com [IPv6:2607:f8b0:4864:20::d35]) by sourceware.org (Postfix) with ESMTPS id D38903858C2F for <gdb-patches@sourceware.org>; Fri, 27 Jan 2023 16:26:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D38903858C2F Received: by mail-io1-xd35.google.com with SMTP id h184so2099978iof.9 for <gdb-patches@sourceware.org>; Fri, 27 Jan 2023 08:26:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=nHyS37I+MDlKDNl3JqeUvgBOyXCDE+8+sLG97HFAIpU=; b=bzgvhRZEQ4SmBdY6G90WKKX5xZrpxqTjgvMXTHXv0/2XuERrKWrgVrVNPGhBDtIYqy 2FyKQGOsfm4IFZIGQ1I3X+dBV4icTtXSVhCsdkVIPzb8kK0rduJcownHo5f9KMfPyvA8 DDeG2rPinVkvTRkva+P2ZPSu6SIefHhH0qUe4rbDkAdEbMZQXujAioRDEcEZiEcKgETa QhFAc/tig0vh7O7+AScxNjLpIL/ykz+l+MpAaF52kcOkYpsL8sG/Sse5zUjtsSaKazIz 8fquc+EXoBR6VAv9fMsk317hob+eaYx/Ogs6OVxWxizAm5Jldr10PLdNlJvs/mOF0iCh iPeg== X-Gm-Message-State: AFqh2kom1va+ZuE80ZPkzOEewZ+MiDoeGq0/B9WDjMzcq1D24A5gMjB0 HuWjH+6sO5tbszH06IDC14n1cmfAOQhFYn7R X-Google-Smtp-Source: AMrXdXsBeq5UIxS3kVRB1p3KDgIxQCAw/fe1Zfl9PdjXhzZoX4dMAsugIX3bgZjzrQ8HLhebAyMacg== X-Received: by 2002:a5e:981a:0:b0:707:63b0:f323 with SMTP id s26-20020a5e981a000000b0070763b0f323mr23289404ioj.17.1674836773089; Fri, 27 Jan 2023 08:26:13 -0800 (PST) Received: from localhost.localdomain (75-166-146-144.hlrn.qwest.net. [75.166.146.144]) by smtp.gmail.com with ESMTPSA id e26-20020a0566380cda00b003a7cbe1d235sm1631860jak.12.2023.01.27.08.26.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 27 Jan 2023 08:26:12 -0800 (PST) To: gdb-patches@sourceware.org Cc: Tom Tromey <tromey@adacore.com> Subject: [PATCH] Make addrmap const-correct in cooked index Date: Fri, 27 Jan 2023 09:26:03 -0700 Message-Id: <20230127162603.4127820-1-tromey@adacore.com> X-Mailer: git-send-email 2.38.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list <gdb-patches.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=subscribe> From: Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> Reply-To: Tom Tromey <tromey@adacore.com> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> |
Series |
Make addrmap const-correct in cooked index
|
|
Commit Message
Tom Tromey
Jan. 27, 2023, 4:26 p.m. UTC
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. --- gdb/addrmap.c | 4 ++-- gdb/addrmap.h | 6 +++--- gdb/dwarf2/cooked-index.c | 4 ++-- gdb/dwarf2/cooked-index.h | 2 +- gdb/dwarf2/index-write.c | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-)
Comments
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<addrmap *> > +std::vector<const addrmap *> > 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 <simon.marchi@polymtl.ca> 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<int (CORE_ADDR start_addr, void *obj)> - addrmap_foreach_fn; +using addrmap_foreach_fn + = gdb::function_view<int (CORE_ADDR start_addr, const void *obj)>; /* 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<dwarf2_per_cu_data *, unsigned int> cu_index_map; +using cu_index_map + = std::unordered_map<const dwarf2_per_cu_data *, unsigned int>; /* 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<const dwarf2_per_cu_data *> (obj); if (previous_valid) add_address_entry (addr_vec, base-commit: 90a7c55706695c0afa8f8393c589cd90bdacfef4
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes: Simon> I tried making the parameter to addrmap_foreach_fn const too (which Simon> makes sense I think, if we're going to make foreach const), and came up Simon> with a slightly larger patch. It also made the find method return Simon> const, but it also makes sense given that it is marked const too. Simon> See the patch below, I think we can rebase yours on top of that. Looks good to me. Thank you. >> -std::vector<addrmap *> >> +std::vector<const addrmap *> >> cooked_index_vector::get_addrmaps () Simon> You can make the get_addrmaps method const itself. Will do. Tom
>>>>> "Simon" == Simon Marchi <simark@simark.ca> 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.
Tom
On 1/27/23 16:18, Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi <simark@simark.ca> 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 suppose in this case we can consider that it's the DWARF code that is broken, and that it knows the risks of casting away the const. But at least the addrmap code will be correct. Simon
On 1/27/23 16:20, Simon Marchi via Gdb-patches wrote: > > > On 1/27/23 16:18, Tom Tromey wrote: >>>>>>> "Simon" == Simon Marchi <simark@simark.ca> 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 suppose in this case we can consider that it's the DWARF code that is > broken, and that it knows the risks of casting away the const. But at > least the addrmap code will be correct. Or, if the client code legitimately wants to modify the contained objects, then we provide a non-const version of addrmap::find that returns a non-const pointer. Simon
On 1/27/23 16:18, Tom Tromey via Gdb-patches wrote: >>>>>> "Simon" == Simon Marchi <simark@simark.ca> 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 <simon.marchi@polymtl.ca> 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<int (CORE_ADDR start_addr, void *obj)> - addrmap_foreach_fn; +using addrmap_foreach_fn + = gdb::function_view<int (CORE_ADDR start_addr, void *obj)>; +using addrmap_foreach_const_fn + = gdb::function_view<int (CORE_ADDR start_addr, const void *obj)>; /* 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<dwarf2_per_cu_data *> (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<dwarf2_per_cu_data *> (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<dwarf2_per_cu_data *> (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 <cooked_index_entry *> (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 <cooked_index_entry *> (obj); m_index_storage->add (entry.die_offset, entry.tag, entry.flags, entry.name, parent, m_per_cu); } base-commit: 35e1763185224b2bf634e19cdfd06dab0ba914e3
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes: >> I suppose in this case we can consider that it's the DWARF code that is >> broken, and that it knows the risks of casting away the const. But at >> least the addrmap code will be correct. Simon> Or, if the client code legitimately wants to modify the contained Simon> objects, then we provide a non-const version of addrmap::find that Simon> returns a non-const pointer. I was thinking about this weekend, and it seems to me that addrmap is just a container and should be able to hold any type -- const or not -- much the way a vector does. The issue on the gdb side is that addrmap isn't templatized and so has to use void* as the type. Anyway from this view, using 'const void *' seems wrong because it moves the type buglet from addrmap to the users of it. Tom
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
Simon> So, I redesigned my patch to instead provide both const and non-const
Simon> versions of find and foreach. I think it's typical for containers in
Simon> C++ anyway. See v2 below:
This looks good to me. It addresses my objections.
Simon> diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
Simon> index ced58eab6612..7b1b2d6520cb 100644
Simon> --- a/gdb/dwarf2/index-write.c
Simon> +++ b/gdb/dwarf2/index-write.c
Simon> @@ -474,7 +474,7 @@ add_address_entry (data_buf &addr_vec,
Simon> int
Simon> addrmap_index_data::operator() (CORE_ADDR start_addr, void *obj)
Simon> {
Simon> - dwarf2_per_cu_data *per_cu = (dwarf2_per_cu_data *) obj;
Simon> + dwarf2_per_cu_data *per_cu = static_cast<dwarf2_per_cu_data *> (obj);
This area could probably be constified at some point.
Tom
On 1/30/23 09:26, Tom Tromey via Gdb-patches wrote: >>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes: > > Simon> So, I redesigned my patch to instead provide both const and non-const > Simon> versions of find and foreach. I think it's typical for containers in > Simon> C++ anyway. See v2 below: > > This looks good to me. It addresses my objections. Thanks, will push. > Simon> diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c > Simon> index ced58eab6612..7b1b2d6520cb 100644 > Simon> --- a/gdb/dwarf2/index-write.c > Simon> +++ b/gdb/dwarf2/index-write.c > Simon> @@ -474,7 +474,7 @@ add_address_entry (data_buf &addr_vec, > Simon> int > Simon> addrmap_index_data::operator() (CORE_ADDR start_addr, void *obj) > Simon> { > Simon> - dwarf2_per_cu_data *per_cu = (dwarf2_per_cu_data *) obj; > Simon> + dwarf2_per_cu_data *per_cu = static_cast<dwarf2_per_cu_data *> (obj); > > This area could probably be constified at some point. I think it will be, when applying you patch that constifies cooked_index_vector::get_addrmaps. Simon
diff --git a/gdb/addrmap.c b/gdb/addrmap.c index 0a6b2e5a25a..47ceaca48ad 100644 --- a/gdb/addrmap.c +++ b/gdb/addrmap.c @@ -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; @@ -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); } diff --git a/gdb/addrmap.h b/gdb/addrmap.h index a5b784efaf2..659d3c977a8 100644 --- a/gdb/addrmap.h +++ b/gdb/addrmap.h @@ -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; @@ -113,7 +113,7 @@ struct addrmap_fixed : public addrmap, void *obj) override; 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: @@ -151,7 +151,7 @@ struct addrmap_mutable : public addrmap void *obj) override; 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/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<addrmap *> +std::vector<const addrmap *> cooked_index_vector::get_addrmaps () { - std::vector<addrmap *> result; + std::vector<const addrmap *> result; for (const auto &index : m_vector) result.push_back (index->m_addrmap); return result; diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h index 55eaf9955ab..dc3fe6e262f 100644 --- a/gdb/dwarf2/cooked-index.h +++ b/gdb/dwarf2/cooked-index.h @@ -359,7 +359,7 @@ class cooked_index_vector : public dwarf_scanner_base /* Return a new vector of all the addrmaps used by all the indexes held by this object. */ - std::vector<addrmap *> get_addrmaps (); + std::vector<const addrmap *> get_addrmaps (); /* Return the entry that is believed to represent the program's "main". This will return NULL if no such entry is available. */ diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c index ced58eab661..9b4a02bc694 100644 --- a/gdb/dwarf2/index-write.c +++ b/gdb/dwarf2/index-write.c @@ -500,7 +500,7 @@ addrmap_index_data::operator() (CORE_ADDR start_addr, void *obj) in the index file. */ static void -write_address_map (struct addrmap *addrmap, data_buf &addr_vec, +write_address_map (const addrmap *addrmap, data_buf &addr_vec, cu_index_map &cu_index_htab) { struct addrmap_index_data addrmap_index_data (addr_vec, cu_index_htab);