[3/8] Change fde table to a vector

  This removes struct dwarf2_fde_table, replacing it with a typedef of
std::vector.  This simplifies the code somewhat.

gdb/ChangeLog
2020-02-08  Tom Tromey  <tom@tromey.com>

	* dwarf2/frame.c (struct dwarf2_fde_table): Remove.
	(dwarf2_fde_table): Typedef for std::vector.
	(dwarf2_frame_objfile_data): Remove the deleter.
	(dwarf2_frame_find_fde, add_fde, decode_frame_entry_1)
	(decode_frame_entry): Update.
	(dwarf2_build_frame_info): Use "new".

Change-Id: Ie31c7413dce2865b76f5f70374ba7a559112ce5e
---
 gdb/ChangeLog      |   9 ++++
 gdb/dwarf2/frame.c | 102 ++++++++++++---------------------------------
 2 files changed, 35 insertions(+), 76 deletions(-)
  

Great cleanup. Just a few comments...

On 2/8/20 12:27 PM, Tom Tromey wrote:
> This removes struct dwarf2_fde_table, replacing it with a typedef of
> std::vector.  This simplifies the code somewhat.
> 
> gdb/ChangeLog
> 2020-02-08  Tom Tromey  <tom@tromey.com>
> 
> 	* dwarf2/frame.c (struct dwarf2_fde_table): Remove.
> 	(dwarf2_fde_table): Typedef for std::vector.
> 	(dwarf2_frame_objfile_data): Remove the deleter.
> 	(dwarf2_frame_find_fde, add_fde, decode_frame_entry_1)
> 	(decode_frame_entry): Update.
> 	(dwarf2_build_frame_info): Use "new".
> 
> Change-Id: Ie31c7413dce2865b76f5f70374ba7a559112ce5e
> ---
>   gdb/ChangeLog      |   9 ++++
>   gdb/dwarf2/frame.c | 102 ++++++++++++---------------------------------
>   2 files changed, 35 insertions(+), 76 deletions(-)
> 
> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
> index db8e5cd25f3..6d87e598345 100644
> --- a/gdb/dwarf2/frame.c
> +++ b/gdb/dwarf2/frame.c
> @@ -129,11 +129,7 @@ struct dwarf2_fde
>     unsigned char eh_frame_p;
>   };
>   
> -struct dwarf2_fde_table
> -{
> -  int num_entries;
> -  struct dwarf2_fde **entries;
> -};
> +typedef std::vector<dwarf2_fde *> dwarf2_fde_table;
>   
>   /* A minimal decoding of DWARF2 compilation units.  We only decode
>      what's needed to get to the call frame information.  */
> @@ -1471,9 +1467,7 @@ dwarf2_frame_cfa (struct frame_info *this_frame)
>     return get_frame_base (this_frame);
>   }
>   
> -const struct objfile_key<dwarf2_fde_table,
> -			 gdb::noop_deleter<dwarf2_fde_table>>
> -  dwarf2_frame_objfile_data;
> +const struct objfile_key<dwarf2_fde_table> dwarf2_frame_objfile_data;
>   
>   
>   
> @@ -1636,7 +1630,7 @@ dwarf2_frame_find_fde (CORE_ADDR *pc, CORE_ADDR *out_offset)
>   {
>     for (objfile *objfile : current_program_space->objfiles ())
>       {
> -      struct dwarf2_fde_table *fde_table;
> +      dwarf2_fde_table *fde_table;
>         CORE_ADDR offset;
>         CORE_ADDR seek_pc;
>   
> @@ -1648,20 +1642,20 @@ dwarf2_frame_find_fde (CORE_ADDR *pc, CORE_ADDR *out_offset)
>   	}
>         gdb_assert (fde_table != NULL);
>   
> -      if (fde_table->num_entries == 0)
> +      if (fde_table->empty ())
>   	continue;
>   
>         gdb_assert (!objfile->section_offsets.empty ());
>         offset = objfile->text_section_offset ();
>   
> -      gdb_assert (fde_table->num_entries > 0);
> -      if (*pc < offset + fde_table->entries[0]->initial_location)
> +      gdb_assert (!fde_table->empty ());
> +      if (*pc < offset + (*fde_table)[0]->initial_location)
>           continue;
>   
>         seek_pc = *pc - offset;
> -      auto end = fde_table->entries + fde_table->num_entries;
> -      auto it = gdb::binary_search (fde_table->entries, end, seek_pc, bsearch_fde_cmp);
> -      if (it != end)
> +      auto it = gdb::binary_search (fde_table->begin (), fde_table->end (),
> +				    seek_pc, bsearch_fde_cmp);
> +      if (it != fde_table->end ())
>           {
>             *pc = (*it)->initial_location + offset;
>   	  if (out_offset)
> @@ -1674,16 +1668,13 @@ dwarf2_frame_find_fde (CORE_ADDR *pc, CORE_ADDR *out_offset)
>   
>   /* Add a pointer to new FDE to the FDE_TABLE, allocating space for it.  */
>   static void

We don't quite allocate space for it anymore, but we do behind the 
scenes. The comment seems slightly stale. Plus the sentence reads a bit 
funny. I'd update this, but i'm fine keeping it too.

> -add_fde (struct dwarf2_fde_table *fde_table, struct dwarf2_fde *fde)
> +add_fde (dwarf2_fde_table *fde_table, struct dwarf2_fde *fde)
>   {
>     if (fde->address_range == 0)
>       /* Discard useless FDEs.  */
>       return;
>   
> -  fde_table->num_entries += 1;
> -  fde_table->entries = XRESIZEVEC (struct dwarf2_fde *, fde_table->entries,
> -				   fde_table->num_entries);
> -  fde_table->entries[fde_table->num_entries - 1] = fde;
> +  fde_table->push_back (fde);
>   }
>   
>   #define DW64_CIE_ID 0xffffffffffffffffULL
> @@ -1702,7 +1693,7 @@ static const gdb_byte *decode_frame_entry (struct comp_unit *unit,
>   					   const gdb_byte *start,
>   					   int eh_frame_p,
>   					   dwarf2_cie_table &cie_table,
> -					   struct dwarf2_fde_table *fde_table,
> +					   dwarf2_fde_table *fde_table,
>   					   enum eh_frame_type entry_type);
>   
>   /* Decode the next CIE or FDE, entry_type specifies the expected type.
> @@ -1712,7 +1703,7 @@ static const gdb_byte *
>   decode_frame_entry_1 (struct comp_unit *unit, const gdb_byte *start,
>   		      int eh_frame_p,
>                         dwarf2_cie_table &cie_table,
> -                      struct dwarf2_fde_table *fde_table,
> +                      dwarf2_fde_table *fde_table,
>                         enum eh_frame_type entry_type)
>   {
>     struct gdbarch *gdbarch = get_objfile_arch (unit->objfile);
> @@ -2014,7 +2005,7 @@ static const gdb_byte *
>   decode_frame_entry (struct comp_unit *unit, const gdb_byte *start,
>   		    int eh_frame_p,
>   		    dwarf2_cie_table &cie_table,
> -                    struct dwarf2_fde_table *fde_table,
> +                    dwarf2_fde_table *fde_table,
>                       enum eh_frame_type entry_type)
>   {
>     enum { NONE, ALIGN4, ALIGN8, FAIL } workaround = NONE;
> @@ -2128,11 +2119,8 @@ dwarf2_build_frame_info (struct objfile *objfile)
>     struct comp_unit *unit;
>     const gdb_byte *frame_ptr;
>     dwarf2_cie_table cie_table;
> -  struct dwarf2_fde_table fde_table;
> -  struct dwarf2_fde_table *fde_table2;
> -
> -  fde_table.num_entries = 0;
> -  fde_table.entries = NULL;
> +  dwarf2_fde_table fde_table;
> +  dwarf2_fde_table *fde_table2;
>   
>     /* Build a minimal decoding of the DWARF2 compilation unit.  */
>     unit = XOBNEW (&objfile->objfile_obstack, comp_unit);
> @@ -2181,12 +2169,7 @@ dwarf2_build_frame_info (struct objfile *objfile)
>   	      warning (_("skipping .eh_frame info of %s: %s"),
>   		       objfile_name (objfile), e.what ());
>   
> -	      if (fde_table.num_entries != 0)
> -		{
> -                  xfree (fde_table.entries);
> -		  fde_table.entries = NULL;
> -		  fde_table.num_entries = 0;
> -		}
> +	      fde_table.clear ();
>   	      /* The cie_table is discarded below.  */
>   	    }
>   
> @@ -2200,7 +2183,7 @@ dwarf2_build_frame_info (struct objfile *objfile)
>                              &unit->dwarf_frame_size);
>     if (unit->dwarf_frame_size)
>       {
> -      int num_old_fde_entries = fde_table.num_entries;
> +      size_t num_old_fde_entries = fde_table.size ();
>   
>         try
>   	{
> @@ -2215,42 +2198,20 @@ dwarf2_build_frame_info (struct objfile *objfile)
>   	  warning (_("skipping .debug_frame info of %s: %s"),
>   		   objfile_name (objfile), e.what ());
>   
> -	  if (fde_table.num_entries != 0)
> -	    {
> -	      fde_table.num_entries = num_old_fde_entries;
> -	      if (num_old_fde_entries == 0)
> -		{
> -		  xfree (fde_table.entries);
> -		  fde_table.entries = NULL;
> -		}
> -	      else
> -		{
> -		  fde_table.entries
> -		    = XRESIZEVEC (struct dwarf2_fde *, fde_table.entries,
> -				  fde_table.num_entries);
> -		}
> -	    }
> -	  fde_table.num_entries = num_old_fde_entries;
> +	  fde_table.resize (num_old_fde_entries);

Took me a bit to figure out the code above. Quite convoluted in its use 
of the size variables. But the new code looks sane as far as i can tell.

>   	}
>       }
>   
>     /* Copy fde_table to obstack: it is needed at runtime.  */
> -  fde_table2 = XOBNEW (&objfile->objfile_obstack, struct dwarf2_fde_table);
> +  fde_table2 = new dwarf2_fde_table; >

The comment mentioning obstack is now obsolete and can be removed.

> -  if (fde_table.num_entries == 0)
> -    {
> -      fde_table2->entries = NULL;
> -      fde_table2->num_entries = 0;
> -    }
> -  else
> +  if (!fde_table.empty ())
>       {
>         struct dwarf2_fde *fde_prev = NULL;
>         struct dwarf2_fde *first_non_zero_fde = NULL;
> -      int i;
>   
>         /* Prepare FDE table for lookups.  */
> -      std::sort (fde_table.entries, fde_table.entries + fde_table.num_entries,
> -		 fde_is_less_than);
> +      std::sort (fde_table.begin (), fde_table.end (), fde_is_less_than);
>   
>         /* Check for leftovers from --gc-sections.  The GNU linker sets
>   	 the relevant symbols to zero, but doesn't zero the FDE *end*
> @@ -2264,10 +2225,8 @@ dwarf2_build_frame_info (struct objfile *objfile)
>   	 Start by finding the first FDE with non-zero start.  Below
>   	 we'll discard all FDEs that start at zero and overlap this
>   	 one.  */
> -      for (i = 0; i < fde_table.num_entries; i++)
> +      for (struct dwarf2_fde *fde : fde_table)
>   	{
> -	  struct dwarf2_fde *fde = fde_table.entries[i];
> -
>   	  if (fde->initial_location != 0)
>   	    {
>   	      first_non_zero_fde = fde;
> @@ -2278,11 +2237,8 @@ dwarf2_build_frame_info (struct objfile *objfile)
>         /* Since we'll be doing bsearch, squeeze out identical (except
>   	 for eh_frame_p) fde entries so bsearch result is predictable.
>   	 Also discard leftovers from --gc-sections.  */
> -      fde_table2->num_entries = 0;
> -      for (i = 0; i < fde_table.num_entries; i++)
> +      for (struct dwarf2_fde *fde : fde_table)
>   	{
> -	  struct dwarf2_fde *fde = fde_table.entries[i];
> -
>   	  if (fde->initial_location == 0
>   	      && first_non_zero_fde != NULL
>   	      && (first_non_zero_fde->initial_location
> @@ -2293,16 +2249,10 @@ dwarf2_build_frame_info (struct objfile *objfile)
>   	      && fde_prev->initial_location == fde->initial_location)
>   	    continue;
>   
> -	  obstack_grow (&objfile->objfile_obstack, &fde_table.entries[i],
> -			sizeof (fde_table.entries[0]));
> -	  ++fde_table2->num_entries;
> +	  fde_table2->push_back (fde);
>   	  fde_prev = fde;
>   	}
> -      fde_table2->entries
> -	= (struct dwarf2_fde **) obstack_finish (&objfile->objfile_obstack);
> -
> -      /* Discard the original fde_table.  */
> -      xfree (fde_table.entries);
> +      fde_table2->shrink_to_fit ();
>       }
>   
>     dwarf2_frame_objfile_data.set (objfile, fde_table2);
> 

Otherwise LGTM.
  

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

Luis> We don't quite allocate space for it anymore, but we do behind the
Luis> scenes. The comment seems slightly stale. Plus the sentence reads a
Luis> bit funny. I'd update this, but i'm fine keeping it too.

I simplified it to just:

    /* Add FDE to FDE_TABLE.  */

>> /* Copy fde_table to obstack: it is needed at runtime.  */
>> -  fde_table2 = XOBNEW (&objfile->objfile_obstack, struct dwarf2_fde_table);
>> +  fde_table2 = new dwarf2_fde_table; >

Luis> The comment mentioning obstack is now obsolete and can be removed.

I've removed it.  I think this is the one you mentioned in another patch
review -- but it is simple enough to just zap it here and fix up the
conflict.

Tom
  

On 2020-02-08 10:27 a.m., Tom Tromey wrote:
> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
> index db8e5cd25f3..6d87e598345 100644
> --- a/gdb/dwarf2/frame.c
> +++ b/gdb/dwarf2/frame.c
> @@ -129,11 +129,7 @@ struct dwarf2_fde
>    unsigned char eh_frame_p;
>  };
>  
> -struct dwarf2_fde_table
> -{
> -  int num_entries;
> -  struct dwarf2_fde **entries;
> -};
> +typedef std::vector<dwarf2_fde *> dwarf2_fde_table;

As a further change, I think we could consider make this a vector of objects, rather
than a vector of pointers.

>  /* A minimal decoding of DWARF2 compilation units.  We only decode
>     what's needed to get to the call frame information.  */
> @@ -1471,9 +1467,7 @@ dwarf2_frame_cfa (struct frame_info *this_frame)
>    return get_frame_base (this_frame);
>  }
>  
> -const struct objfile_key<dwarf2_fde_table,
> -			 gdb::noop_deleter<dwarf2_fde_table>>
> -  dwarf2_frame_objfile_data;
> +const struct objfile_key<dwarf2_fde_table> dwarf2_frame_objfile_data;

Can you make this static, while at it?

> @@ -2215,42 +2198,20 @@ dwarf2_build_frame_info (struct objfile *objfile)
>  	  warning (_("skipping .debug_frame info of %s: %s"),
>  		   objfile_name (objfile), e.what ());
>  
> -	  if (fde_table.num_entries != 0)
> -	    {
> -	      fde_table.num_entries = num_old_fde_entries;
> -	      if (num_old_fde_entries == 0)
> -		{
> -		  xfree (fde_table.entries);
> -		  fde_table.entries = NULL;
> -		}
> -	      else
> -		{
> -		  fde_table.entries
> -		    = XRESIZEVEC (struct dwarf2_fde *, fde_table.entries,
> -				  fde_table.num_entries);
> -		}
> -	    }
> -	  fde_table.num_entries = num_old_fde_entries;
> +	  fde_table.resize (num_old_fde_entries);
>  	}
>      }
>  
>    /* Copy fde_table to obstack: it is needed at runtime.  */

I think that comment is stale.

> -  fde_table2 = XOBNEW (&objfile->objfile_obstack, struct dwarf2_fde_table);
> +  fde_table2 = new dwarf2_fde_table;
>  
> -  if (fde_table.num_entries == 0)
> -    {
> -      fde_table2->entries = NULL;
> -      fde_table2->num_entries = 0;
> -    }
> -  else
> +  if (!fde_table.empty ())

If you want to simplify the code even further, you could get rid of this check, since the
code below should cope well with an empty vector.

> @@ -2293,16 +2249,10 @@ dwarf2_build_frame_info (struct objfile *objfile)
>  	      && fde_prev->initial_location == fde->initial_location)
>  	    continue;
>  
> -	  obstack_grow (&objfile->objfile_obstack, &fde_table.entries[i],
> -			sizeof (fde_table.entries[0]));
> -	  ++fde_table2->num_entries;
> +	  fde_table2->push_back (fde);
>  	  fde_prev = fde;
>  	}
> -      fde_table2->entries
> -	= (struct dwarf2_fde **) obstack_finish (&objfile->objfile_obstack);
> -
> -      /* Discard the original fde_table.  */
> -      xfree (fde_table.entries);
> +      fde_table2->shrink_to_fit ();

That shrink_to_fit appears to be pointless, as fde_table2 is created empty and we only
push_back.  Maybe you meant to create it with an initial size?

Simon
  

>> @@ -2215,42 +2198,20 @@ dwarf2_build_frame_info (struct objfile *objfile)
>>  	  warning (_("skipping .debug_frame info of %s: %s"),
>>  		   objfile_name (objfile), e.what ());
>>  
>> -	  if (fde_table.num_entries != 0)
>> -	    {
>> -	      fde_table.num_entries = num_old_fde_entries;
>> -	      if (num_old_fde_entries == 0)
>> -		{
>> -		  xfree (fde_table.entries);
>> -		  fde_table.entries = NULL;
>> -		}
>> -	      else
>> -		{
>> -		  fde_table.entries
>> -		    = XRESIZEVEC (struct dwarf2_fde *, fde_table.entries,
>> -				  fde_table.num_entries);
>> -		}
>> -	    }
>> -	  fde_table.num_entries = num_old_fde_entries;
>> +	  fde_table.resize (num_old_fde_entries);
>>  	}
>>      }
>>  
>>    /* Copy fde_table to obstack: it is needed at runtime.  */
> 
> I think that comment is stale.

Sorry, I just noticed that Luis had already pointed this out.

Simon
  

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

Simon> As a further change, I think we could consider make this a vector
Simon> of objects, rather than a vector of pointers.

I think I'd rather leave this for a future change.
It makes the patch a lot less obvious, to me anyway.  find_fde returns a
pointer -- but is that safe?  I think so, but I am not 100% certain; but
with the current code it definitely is.

>> +const struct objfile_key<dwarf2_fde_table> dwarf2_frame_objfile_data;

Simon> Can you make this static, while at it?

I did this.

>> +  if (!fde_table.empty ())

Simon> If you want to simplify the code even further, you could get rid
Simon> of this check, since the code below should cope well with an
Simon> empty vector.

Thanks, I did this too.

>> +      fde_table2->shrink_to_fit ();

Simon> That shrink_to_fit appears to be pointless, as fde_table2 is
Simon> created empty and we only push_back.  Maybe you meant to create
Simon> it with an initial size?

IIUC push_back normally uses some resize factor; the shrink_to_fit is a
request to discard any excess memory allocated this way.

FWIW another thing that could be done in this code is to use the
remove_if to remove elements in-place from fde_table.  This is another
thing I'd rather do separately.

Tom
  

On 2020-02-12 5:36 p.m., Tom Tromey wrote:
> Simon> That shrink_to_fit appears to be pointless, as fde_table2 is
> Simon> created empty and we only push_back.  Maybe you meant to create
> Simon> it with an initial size?
> 
> IIUC push_back normally uses some resize factor; the shrink_to_fit is a
> request to discard any excess memory allocated this way.

Of course, my bad.

Simon