Re: [PATCH 05/14] Introduce dwarf2_unshareable and move die_type_hash

Luis Machado
2020-02-18T11:23:05Z

Just one nit...

On 2/15/20 1:54 PM, Tom Tromey wrote:
> The goal of this series is to share dwarf2_per_objfile across
> objfiles. However, this is difficult to do, because it has some
> members that are inherently per-objfile.
>
> This patch introduces a new "unshareable" object. The idea here is
> that we'll have one such object per objfile, and we'll temporarily
> update the pointer when needed. This is a bit ugly, but it's
> convenient, because it doesn't require a huge rewriting of the DWARF
> code. (I would be in favor of such a rewriting, but ... it's huge.)
>
> This patch further moves one such member, the DIE type hash, to the
> new structure.
>
> In this patch, the new object is managed by the dwarf2_per_objfile.
> However, by the end of the series, this will change.
>
> gdb/ChangeLog
> 2020-02-15 Tom Tromey <tom@tromey.com>
>
> * dwarf2/read.h (struct dwarf2_unshareable): New type.
> (struct dwarf2_per_objfile) <die_type_hash>: Move to
> dwarf2_unshareable.
> <unshareable>: New member.
> * dwarf2/read.c (dwarf2_per_objfile::dwarf2_per_objfile):
> Initialize "unshareable".
> (dwarf2_per_objfile::~dwarf2_per_objfile): Delete unshareable.
> (set_die_type, get_die_type_at_offset): Update.
> ---
> gdb/ChangeLog | 11 +++++++++++
> gdb/dwarf2/read.c | 19 +++++++++++++------
> gdb/dwarf2/read.h | 28 +++++++++++++++++++++++-----
> 3 files changed, 47 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index a897b11693a..cb04eb19beb 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -1774,7 +1774,10 @@ dwarf2_per_objfile::dwarf2_per_objfile (struct objfile *objfile_,
> const dwarf2_debug_sections *names,
> bool can_copy_)
> : objfile (objfile_),
> - can_copy (can_copy_)
> + can_copy (can_copy_),
> + /* Temporarily just allocate one per objfile -- we don't have
> + sharing yet. */
> + unshareable (new dwarf2_unshareable)
> {
> if (names == NULL)
> names = &dwarf2_elf_names;
> @@ -1796,6 +1799,8 @@ dwarf2_per_objfile::~dwarf2_per_objfile ()
> for (signatured_type *sig_type : all_type_units)
> sig_type->per_cu.imported_symtabs_free ();
>
> + delete unshareable;
> +
> /* Everything else should be on the objfile obstack. */
> }
>
> @@ -24434,8 +24439,8 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
> cu->per_cu->addr_type ()))
> add_dyn_prop (DYN_PROP_DATA_LOCATION, prop, type);
>
> - if (dwarf2_per_objfile->die_type_hash == NULL)
> - dwarf2_per_objfile->die_type_hash
> + if (dwarf2_per_objfile->unshareable->die_type_hash == NULL)
> + dwarf2_per_objfile->unshareable->die_type_hash
> = htab_up (htab_create_alloc (127,
> per_cu_offset_and_type_hash,
> per_cu_offset_and_type_eq,
> @@ -24445,7 +24450,8 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
> ofs.sect_off = die->sect_off;
> ofs.type = type;
> slot = (struct dwarf2_per_cu_offset_and_type **)
> - htab_find_slot (dwarf2_per_objfile->die_type_hash.get (), &ofs, INSERT);
> + htab_find_slot (dwarf2_per_objfile->unshareable->die_type_hash.get (),
> + &ofs, INSERT);
> if (*slot)
> complaint (_("A problem internal to GDB: DIE %s has type already set"),
> sect_offset_str (die->sect_off));
> @@ -24465,13 +24471,14 @@ get_die_type_at_offset (sect_offset sect_off,
> struct dwarf2_per_cu_offset_and_type *slot, ofs;
> struct dwarf2_per_objfile *dwarf2_per_objfile = per_cu->dwarf2_per_objfile;
>
> - if (dwarf2_per_objfile->die_type_hash == NULL)
> + if (dwarf2_per_objfile->unshareable->die_type_hash == NULL)
> return NULL;
>
> ofs.per_cu = per_cu;
> ofs.sect_off = sect_off;
> slot = ((struct dwarf2_per_cu_offset_and_type *)
> - htab_find (dwarf2_per_objfile->die_type_hash.get (), &ofs));
> + htab_find (dwarf2_per_objfile->unshareable->die_type_hash.get (),
> + &ofs));
> if (slot)
> return slot->type;
> else
> diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
> index 5fa8dec9bfb..06a0fa3a19a 100644
> --- a/gdb/dwarf2/read.h
> +++ b/gdb/dwarf2/read.h
> @@ -66,6 +66,24 @@ struct dwarf2_queue_item
> enum language pretend_language;
> };
>
> +/* Some DWARF data cannot (currently) be shared across objfiles. Such
> + data is stored in this object.
> +
> + dwarf2_per_objfile holds a pointer to an instance of this object.
> + This pointer is temporarily set when expanding CUs. This hackish
> + approach is due to the history of the DWARF reader. In the past,
> + all objects were stored per-objfile, and this object was introduced
> + in the process of separating out the shareable and per-objfile
> + state. */
> +
> +struct dwarf2_unshareable
> +{
> + /* Table mapping type DIEs to their struct type *.
> + This is NULL if not allocated yet.

"This is nullptr"

> + The mapping is done via (CU/TU + DIE offset) -> type. */
> + htab_up die_type_hash;
> +};
> +
> /* Collection of data recorded per objfile.
> This hangs off of dwarf2_objfile_data_key. */
>
> @@ -214,11 +232,6 @@ public:
> symbols. */
> bool reading_partial_symbols = false;
>
> - /* Table mapping type DIEs to their struct type *.
> - This is NULL if not allocated yet.
> - The mapping is done via (CU/TU + DIE offset) -> type. */
> - htab_up die_type_hash;
> -
> /* The CUs we recently read. */
> std::vector<dwarf2_per_cu_data *> just_read_cus;
>
> @@ -241,6 +254,11 @@ public:
>
> /* CUs that are queued to be read. */
> std::queue<dwarf2_queue_item> queue;
> +
> + /* State that cannot be shared across objfiles. This is normally
> + nullptr and is temporarily set to the correct value at the entry
> + points of the reader. */
> + dwarf2_unshareable *unshareable = nullptr;
> };
>
> /* Get the dwarf2_per_objfile associated to OBJFILE. */
>