Re: [PATCH 4/7] Class-fy partial_die_info

Simon Marchi
2018-01-29T01:15:28Z

On 2018-01-25 04:38 AM, Yao Qi wrote:
> This patch is to class-fy partial_die_info. Two things special here,
>
> - disable assignment operator, but keep copy ctor, which is used in
> load_partial_dies,
> - have a private ctor which is only used by dwarf2_cu::find_partial_die,
> I don't want other code use it, so make it private,

Hi Yao,

From what I understand, the only reason to have that private constructor is
to construct a temporary partial_die_info object used to search in the htab,
is that right? If so, converting that htab_t to an std::unordered_map would
remove the need for all this, since you don't need to construct an object
to search it. See the diff below that applies on top of this patch.

It's not thoroughly tested and I am not sure of the validity of the
per_cu->cu->partial_dies.empty () call in find_partial_die, but I think it
should work. Plus, it adds some type-safety, which I am a big fan of.

But otherwise, the patch is fine with me.

Simon

From dacaf6028e921efeb8c7420db7f663e5b4d7e8f1 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Sun, 28 Jan 2018 19:47:01 -0500
Subject: [PATCH] use std::unordered_map

---
gdb/dwarf2read.c | 115 +++++++++++++------------------------------------------
1 file changed, 26 insertions(+), 89 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 8996f49bae..330ca87a1d 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -652,6 +652,8 @@ struct delayed_method_info
struct die_info *die;
};

+struct partial_die_info;
+
/* Internal state when decoding a particular compilation unit. */
struct dwarf2_cu
{
@@ -686,9 +688,9 @@ struct dwarf2_cu
distinguish these in buildsym.c. */
struct pending **list_in_scope = nullptr;

- /* Hash table holding all the loaded partial DIEs
- with partial_die->offset.SECT_OFF as hash. */
- htab_t partial_dies = nullptr;
+ /* Hash map holding all the loaded partial DIEs
+ with their section offset as the key. */
+ std::unordered_map<sect_offset, partial_die_info *> partial_dies;

/* Storage for things with the same lifetime as this read-in compilation
unit, including partial DIEs. */
@@ -1493,36 +1495,6 @@ struct partial_die_info
struct partial_die_info *die_parent = nullptr;
struct partial_die_info *die_child = nullptr;
struct partial_die_info *die_sibling = nullptr;
-
- friend struct partial_die_info *
- dwarf2_cu::find_partial_die (sect_offset sect_off);
-
- private:
- /* Only need to do look up in dwarf2_cu::find_partial_die. */
- partial_die_info (sect_offset sect_off)
- : partial_die_info (sect_off, DW_TAG_padding, 0)
- {
- }
-
- partial_die_info (sect_offset sect_off_, enum dwarf_tag tag_,
- int has_children_)
- : sect_off (sect_off_), tag (tag_), has_children (has_children_)
- {
- is_external = 0;
- is_declaration = 0;
- has_type = 0;
- has_specification = 0;
- has_pc_info = 0;
- may_be_inlined = 0;
- main_subprogram = 0;
- scope_set = 0;
- has_byte_size = 0;
- has_const_value = 0;
- has_template_arguments = 0;
- fixup_called = 0;
- is_dwz = 0;
- spec_is_dwz = 0;
- }
};

/* This data structure holds the information of an abbrev. */
@@ -2180,10 +2152,6 @@ static const gdb_byte *skip_one_die (const struct die_reader_specs *reader,
const gdb_byte *info_ptr,
struct abbrev_info *abbrev);

-static hashval_t partial_die_hash (const void *item);
-
-static int partial_die_eq (const void *item_lhs, const void *item_rhs);
-
static struct dwarf2_per_cu_data *dwarf2_find_containing_comp_unit
(sect_offset sect_off, unsigned int offset_in_dwz,
struct dwarf2_per_objfile *dwarf2_per_objfile);
@@ -18259,14 +18227,7 @@ load_partial_dies (const struct die_reader_specs *reader,
if (cu->per_cu->load_all_dies)
load_all = 1;

- cu->partial_dies
- = htab_create_alloc_ex (cu->header.length / 12,
- partial_die_hash,
- partial_die_eq,
- NULL,
- &cu->comp_unit_obstack,
- hashtab_obstack_allocate,
- dummy_obstack_deallocate);
+ cu->partial_dies.clear ();

while (1)
{
@@ -18459,14 +18420,7 @@ load_partial_dies (const struct die_reader_specs *reader,
|| abbrev->tag == DW_TAG_variable
|| abbrev->tag == DW_TAG_namespace
|| part_die->is_declaration)
- {
- void **slot;
-
- slot = htab_find_slot_with_hash (cu->partial_dies, part_die,
- to_underlying (part_die->sect_off),
- INSERT);
- *slot = part_die;
- }
+ cu->partial_dies[part_die->sect_off] = part_die;

/* For some DIEs we want to follow their children (if any). For C
we have no reason to follow the children of structures; for other
@@ -18512,8 +18466,22 @@ load_partial_dies (const struct die_reader_specs *reader,

partial_die_info::partial_die_info (sect_offset sect_off_,
struct abbrev_info *abbrev)
- : partial_die_info (sect_off_, abbrev->tag, abbrev->has_children)
-{
+ : sect_off (sect_off_), tag (abbrev->tag), has_children (abbrev->has_children)
+{
+ is_external = 0;
+ is_declaration = 0;
+ has_type = 0;
+ has_specification = 0;
+ has_pc_info = 0;
+ may_be_inlined = 0;
+ main_subprogram = 0;
+ scope_set = 0;
+ has_byte_size = 0;
+ has_const_value = 0;
+ has_template_arguments = 0;
+ fixup_called = 0;
+ is_dwz = 0;
+ spec_is_dwz = 0;
}

/* Read a minimal amount of information into the minimal die structure. */
@@ -18735,14 +18703,9 @@ read_partial_die (const struct die_reader_specs *reader,
struct partial_die_info *
dwarf2_cu::find_partial_die (sect_offset sect_off)
{
- struct partial_die_info *lookup_die = NULL;
- struct partial_die_info part_die (sect_off);
-
- lookup_die = ((struct partial_die_info *)
- htab_find_with_hash (partial_dies, &part_die,
- to_underlying (sect_off)));
+ auto it = partial_dies.find (sect_off);

- return lookup_die;
+ return it != partial_dies.end () ? it->second : nullptr;
}

/* Find a partial DIE at OFFSET, which may or may not be in CU,
@@ -18782,7 +18745,7 @@ find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
per_cu = dwarf2_find_containing_comp_unit (sect_off, offset_in_dwz,
dwarf2_per_objfile);

- if (per_cu->cu == NULL || per_cu->cu->partial_dies == NULL)
+ if (per_cu->cu == NULL || per_cu->cu->partial_dies.empty ())
load_partial_comp_unit (per_cu);

per_cu->cu->last_used = 0;
@@ -25483,32 +25446,6 @@ dwarf2_clear_marks (struct dwarf2_per_cu_data *per_cu)
}
}

-/* Trivial hash function for partial_die_info: the hash value of a DIE
- is its offset in .debug_info for this objfile. */
-
-static hashval_t
-partial_die_hash (const void *item)
-{
- const struct partial_die_info *part_die
- = (const struct partial_die_info *) item;
-
- return to_underlying (part_die->sect_off);
-}
-
-/* Trivial comparison function for partial_die_info structures: two DIEs
- are equal if they have the same offset. */
-
-static int
-partial_die_eq (const void *item_lhs, const void *item_rhs)
-{
- const struct partial_die_info *part_die_lhs
- = (const struct partial_die_info *) item_lhs;
- const struct partial_die_info *part_die_rhs
- = (const struct partial_die_info *) item_rhs;
-
- return part_die_lhs->sect_off == part_die_rhs->sect_off;
-}
-
static struct cmd_list_element *set_dwarf_cmdlist;
static struct cmd_list_element *show_dwarf_cmdlist;

--
2.16.1