[07/14] Add dwarf2_per_cu_data::index

  Currently a dwarf2_per_cu_data can hold a link to the corresponding
compunit_symtab.  However, once these objects are shared across
objfiles, a simple pointer won't work.

Instead, this link will be stored in the dwarf2_unshareable object.
To enable this, we add an index to each dwarf2_per_cu_data and
signatured_type.

2020-02-15  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.h (struct dwarf2_per_objfile) <allocate_per_cu,
	allocate_signatured_type>: Declare new methods.
	(struct dwarf2_per_cu_data) <index>: New member.
	(struct dwarf2_per_objfile) <num_psymtabs>: New member.
	* dwarf2/read.c (dwarf2_per_objfile::allocate_per_cu)
	(dwarf2_per_objfile::allocate_signatured_type): New methods.
	(create_cu_from_index_list): Use allocate_per_cu.
	(create_signatured_type_table_from_index)
	(create_signatured_type_table_from_debug_names)
	(create_debug_type_hash_table, add_type_unit)
	(read_comp_units_from_section): Use allocate_signatured_type.
---
 gdb/ChangeLog     | 14 +++++++++++++
 gdb/dwarf2/read.c | 52 +++++++++++++++++++++++++++--------------------
 gdb/dwarf2/read.h | 18 ++++++++++++++++
 3 files changed, 62 insertions(+), 22 deletions(-)
  

On 2/15/20 1:54 PM, Tom Tromey wrote:
> Currently a dwarf2_per_cu_data can hold a link to the corresponding
> compunit_symtab.  However, once these objects are shared across
> objfiles, a simple pointer won't work.
> 
> Instead, this link will be stored in the dwarf2_unshareable object.
> To enable this, we add an index to each dwarf2_per_cu_data and
> signatured_type.
> 
> 2020-02-15  Tom Tromey  <tom@tromey.com>
> 
> 	* dwarf2/read.h (struct dwarf2_per_objfile) <allocate_per_cu,
> 	allocate_signatured_type>: Declare new methods.
> 	(struct dwarf2_per_cu_data) <index>: New member.
> 	(struct dwarf2_per_objfile) <num_psymtabs>: New member.
> 	* dwarf2/read.c (dwarf2_per_objfile::allocate_per_cu)
> 	(dwarf2_per_objfile::allocate_signatured_type): New methods.
> 	(create_cu_from_index_list): Use allocate_per_cu.
> 	(create_signatured_type_table_from_index)
> 	(create_signatured_type_table_from_debug_names)
> 	(create_debug_type_hash_table, add_type_unit)
> 	(read_comp_units_from_section): Use allocate_signatured_type.
> ---
>   gdb/ChangeLog     | 14 +++++++++++++
>   gdb/dwarf2/read.c | 52 +++++++++++++++++++++++++++--------------------
>   gdb/dwarf2/read.h | 18 ++++++++++++++++
>   3 files changed, 62 insertions(+), 22 deletions(-)
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index cb04eb19beb..281d39ad271 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -2415,18 +2415,36 @@ dwarf2_per_objfile::get_tu (int index)
>     return this->all_type_units[index];
>   }
>   
> +/* See read.h.  */
> +
> +dwarf2_per_cu_data *
> +dwarf2_per_objfile::allocate_per_cu ()
> +{
> +  dwarf2_per_cu_data *result = OBSTACK_ZALLOC (&obstack, dwarf2_per_cu_data);
> +  result->index = num_psymtabs++;
> +  return result;
> +}
> +
> +/* See read.h.  */
> +
> +signatured_type *
> +dwarf2_per_objfile::allocate_signatured_type ()
> +{
> +  signatured_type *result = OBSTACK_ZALLOC (&obstack, signatured_type);
> +  result->per_cu.index = num_psymtabs++;
> +  return result;
> +}
> +
>   /* Return a new dwarf2_per_cu_data allocated on the dwarf2_per_objfile
>      obstack, and constructed with the specified field values.  */
>   
>   static dwarf2_per_cu_data *
>   create_cu_from_index_list (struct dwarf2_per_objfile *dwarf2_per_objfile,
> -                          struct dwarf2_section_info *section,
> -                          int is_dwz,
> -                          sect_offset sect_off, ULONGEST length)
> +			   struct dwarf2_section_info *section,
> +			   int is_dwz,
> +			   sect_offset sect_off, ULONGEST length)

Did only formatting change above?

>   {
> -  dwarf2_per_cu_data *the_cu
> -    = OBSTACK_ZALLOC (&dwarf2_per_objfile->obstack,
> -                     struct dwarf2_per_cu_data);
> +  dwarf2_per_cu_data *the_cu = dwarf2_per_objfile->allocate_per_cu ();
>     the_cu->sect_off = sect_off;
>     the_cu->length = length;
>     the_cu->dwarf2_per_objfile = dwarf2_per_objfile;
> @@ -2517,8 +2535,7 @@ create_signatured_type_table_from_index
>         signature = extract_unsigned_integer (bytes + 16, 8, BFD_ENDIAN_LITTLE);
>         bytes += 3 * 8;
>   
> -      sig_type = OBSTACK_ZALLOC (&dwarf2_per_objfile->obstack,
> -				 struct signatured_type);
> +      sig_type = dwarf2_per_objfile->allocate_signatured_type ();
>         sig_type->signature = signature;
>         sig_type->type_offset_in_tu = type_offset_in_tu;
>         sig_type->per_cu.is_debug_types = 1;
> @@ -2574,8 +2591,7 @@ create_signatured_type_table_from_debug_names
>   				     section->buffer + to_underlying (sect_off),
>   				     rcuh_kind::TYPE);
>   
> -      sig_type = OBSTACK_ZALLOC (&dwarf2_per_objfile->obstack,
> -				 struct signatured_type);
> +      sig_type = dwarf2_per_objfile->allocate_signatured_type ();
>         sig_type->signature = cu_header.signature;
>         sig_type->type_offset_in_tu = cu_header.type_cu_offset_in_tu;
>         sig_type->per_cu.is_debug_types = 1;
> @@ -6091,8 +6107,7 @@ create_debug_type_hash_table (struct dwarf2_per_objfile *dwarf2_per_objfile,
>   	  /* N.B.: type_offset is not usable if this type uses a DWO file.
>   	     The real type_offset is in the DWO file.  */
>   	  dwo_tu = NULL;
> -	  sig_type = OBSTACK_ZALLOC (&dwarf2_per_objfile->obstack,
> -				     struct signatured_type);
> +	  sig_type = dwarf2_per_objfile->allocate_signatured_type ();
>   	  sig_type->signature = header.signature;
>   	  sig_type->type_offset_in_tu = header.type_cu_offset_in_tu;
>   	  sig_type->per_cu.dwarf2_per_objfile = dwarf2_per_objfile;
> @@ -6207,8 +6222,7 @@ add_type_unit (struct dwarf2_per_objfile *dwarf2_per_objfile, ULONGEST sig,
>         == dwarf2_per_objfile->all_type_units.capacity ())
>       ++dwarf2_per_objfile->tu_stats.nr_all_type_units_reallocs;
>   
> -  signatured_type *sig_type = OBSTACK_ZALLOC (&dwarf2_per_objfile->obstack,
> -					      struct signatured_type);
> +  signatured_type *sig_type = dwarf2_per_objfile->allocate_signatured_type ();
>   
>     dwarf2_per_objfile->all_type_units.push_back (sig_type);
>     sig_type->signature = sig;
> @@ -7835,16 +7849,10 @@ read_comp_units_from_section (struct dwarf2_per_objfile *dwarf2_per_objfile,
>   
>         /* Save the compilation unit for later lookup.  */
>         if (cu_header.unit_type != DW_UT_type)
> -	{
> -	  this_cu = XOBNEW (&dwarf2_per_objfile->obstack,
> -			    struct dwarf2_per_cu_data);
> -	  memset (this_cu, 0, sizeof (*this_cu));
> -	}
> +	this_cu = dwarf2_per_objfile->allocate_per_cu ();
>         else
>   	{
> -	  auto sig_type = XOBNEW (&dwarf2_per_objfile->obstack,
> -				  struct signatured_type);
> -	  memset (sig_type, 0, sizeof (*sig_type));
> +	  auto sig_type = dwarf2_per_objfile->allocate_signatured_type ();
>   	  sig_type->signature = cu_header.signature;
>   	  sig_type->type_offset_in_tu = cu_header.type_cu_offset_in_tu;
>   	  this_cu = &sig_type->per_cu;
> diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
> index 06a0fa3a19a..5fc7f7f72e5 100644
> --- a/gdb/dwarf2/read.h
> +++ b/gdb/dwarf2/read.h
> @@ -128,6 +128,17 @@ struct dwarf2_per_objfile
>   
>     /* Free all cached compilation units.  */
>     void free_cached_comp_units ();
> +
> +  /* A convenience function to allocate a dwarf2_per_cu_data (or
> +     object holding one, using C-style derivation).  The returned
> +     object has its "index" field set properly.  The object is
> +     allocated on the dwarf2_per_objfile obstack.  FIXME the index
> +     here is weird since it may not (!?!?) be the same as the other
> +     index  */

I'm a bit lost on the above comment. Instead of having a FIXME here, is 
there something we can do better at this point? Or maybe it will be 
addressed later in the series?

I couldn't quite understand the problems with the indexes, but you're 
more familiar with the DWARF reading code than i am.

> +  dwarf2_per_cu_data *allocate_per_cu ();
> +
> +  signatured_type *allocate_signatured_type ();
> +
>   private:
>     /* This function is mapped across the sections and remembers the
>        offset and size of each of the debugging sections we are
> @@ -255,6 +266,10 @@ public:
>     /* CUs that are queued to be read.  */
>     std::queue<dwarf2_queue_item> queue;
>   
> +  /* The total number of per_cu and signatured_type objects that have
> +     been created for this reader.  */
> +  size_t num_psymtabs = 0;
> +
>     /* 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.  */
> @@ -336,6 +351,9 @@ struct dwarf2_per_cu_data
>        This flag is only valid if is_debug_types is true.  */
>     unsigned int tu_read : 1;
>   
> +  /* Our index in the unshared "all_cutus" vector.  */
> +  unsigned index;
> +

What is the all_cutus vector? I don't see it being added later in the 
series. Should we make it more clear? Maybe you meant "all CU's/TU's"?

>     /* The section this CU/TU lives in.
>        If the DIE refers to a DWO file, this is always the original die,
>        not the DWO file.  */
>
  

On 2020-02-15 11:54 a.m., Tom Tromey wrote:
> @@ -255,6 +266,10 @@ public:
>    /* CUs that are queued to be read.  */
>    std::queue<dwarf2_queue_item> queue;
>  
> +  /* The total number of per_cu and signatured_type objects that have
> +     been created for this reader.  */
> +  size_t num_psymtabs = 0;

Make this private, and name it m_num_psymtabs?

> +
>    /* 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.  */
> @@ -336,6 +351,9 @@ struct dwarf2_per_cu_data
>       This flag is only valid if is_debug_types is true.  */
>    unsigned int tu_read : 1;
>  
> +  /* Our index in the unshared "all_cutus" vector.  */
> +  unsigned index;

The "all_cutus" vector does not exist (at least at the moment, maybe it's added later
in the series?).  I presume you are talking about the logical/combined vector made of
all_comp_units and all_type_units, accessible using dwarf2_per_objfile::get_cutu?

This logical/combined vector has all the comp units first, then the type units.  Using
the new interface allocate_per_cu/allocate_signatured_type, the indices are handed out
in any order.  I don't know if it's actually possible to have it in this order, but let's
say we read:

- a CU
- a TU
- a CU

The CUs will have been handed out indices 0 and 2, and the TU will have been handed out
index 1.  But in reality, when accessing them using get_cutu, the two CUs will be at 0 and
1, while the TU will be at 2.  Again, not sure if it matters, but I thought I would point
it out.

Or maybe this is what you meant by "the index here is weird since it may not (!?!?) be the
same as the other index", this is not the same index?  I guess I'll read the rest of the
series and find out.

Simon
  

On 2020-02-18 11:36 p.m., Simon Marchi wrote:
> The "all_cutus" vector does not exist (at least at the moment, maybe it's added later
> in the series?).  I presume you are talking about the logical/combined vector made of
> all_comp_units and all_type_units, accessible using dwarf2_per_objfile::get_cutu?
> 
> This logical/combined vector has all the comp units first, then the type units.  Using
> the new interface allocate_per_cu/allocate_signatured_type, the indices are handed out
> in any order.  I don't know if it's actually possible to have it in this order, but let's
> say we read:
> 
> - a CU
> - a TU
> - a CU
> 
> The CUs will have been handed out indices 0 and 2, and the TU will have been handed out
> index 1.  But in reality, when accessing them using get_cutu, the two CUs will be at 0 and
> 1, while the TU will be at 2.  Again, not sure if it matters, but I thought I would point
> it out.
> 
> Or maybe this is what you meant by "the index here is weird since it may not (!?!?) be the
> same as the other index", this is not the same index?  I guess I'll read the rest of the
> series and find out.
> 
> Simon

Ok, after reading the next patch I saw that this is indeed referring to an index into another
vector.  So I suppose it's just that the "all_cutus" reference is stale, it should say "symtabs"?
  

>>>>> "Luis" == Luis Machado <luis.machado@linaro.org> writes:

>> static dwarf2_per_cu_data *
>> create_cu_from_index_list (struct dwarf2_per_objfile *dwarf2_per_objfile,
>> -                          struct dwarf2_section_info *section,
>> -                          int is_dwz,
>> -                          sect_offset sect_off, ULONGEST length)
>> +			   struct dwarf2_section_info *section,
>> +			   int is_dwz,
>> +			   sect_offset sect_off, ULONGEST length)

Luis> Did only formatting change above?

Yeah.  I backed this bit out.

>> +  /* A convenience function to allocate a dwarf2_per_cu_data (or
>> +     object holding one, using C-style derivation).  The returned
>> +     object has its "index" field set properly.  The object is
>> +     allocated on the dwarf2_per_objfile obstack.  FIXME the index
>> +     here is weird since it may not (!?!?) be the same as the other
>> +     index  */

Luis> I'm a bit lost on the above comment. Instead of having a FIXME here,
Luis> is there something we can do better at this point? Or maybe it will be 
Luis> addressed later in the series?

Luis> I couldn't quite understand the problems with the indexes, but you're
Luis> more familiar with the DWARF reading code than i am.

I updated the comment, and added a comment by allocate_signatured_type.
Part of that comment referred to some earlier code that also tried to
unify signatured_type and type_unit_group into this allocation function.

The FIXME was more of a note to myself about whether this index could
somehow be reused, because the reader sometimes uses an index to look in
all_comp_units and all_type_units.

I think it turns out they are equivalent, but it seems like it doesn't
matter, because there's no need to use this value to index into the
all_type_units / all_comp_units vectors.

>> +  /* Our index in the unshared "all_cutus" vector.  */
>> +  unsigned index;

Luis> What is the all_cutus vector? I don't see it being added later in the
Luis> series. Should we make it more clear? Maybe you meant "all CU's/TU's"?

I changed it to "symtabs" as Simon pointed out down-thread.

Tom
  

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> +  size_t num_psymtabs = 0;

Simon> Make this private, and name it m_num_psymtabs?

Good idea, I did this.

Tom
  

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> Ok, after reading the next patch I saw that this is indeed
Simon> referring to an index into another vector.  So I suppose it's
Simon> just that the "all_cutus" reference is stale, it should say
Simon> "symtabs"?

Yep.  I've updated the comment.

Tom