[4/8] Store the comp_unit instead of the FDE table

  This changes the DWARF frame code to store the comp_unit on the
objfile, rather than storing the FDE table.  It also changes the
comp_unit to be heap-allocated using "new".

This change makes it simpler for a later patch to add a field to the
comp_unit, and to have deallaction work properly.  This in turn is
important for making the frame data be independent of the objfile.

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

	* dwarf2/frame.c (struct comp_unit): Add initializers.
	(dwarf2_frame_objfile_data): Store a comp_unit.
	(dwarf2_frame_find_fde): Update.
	(dwarf2_build_frame_info): Use "new".

Change-Id: Id339892824caf47e7909ed5ee7e2f0556d645dba
---
 gdb/ChangeLog      |  7 +++++++
 gdb/dwarf2/frame.c | 43 ++++++++++++++++++++-----------------------
 2 files changed, 27 insertions(+), 23 deletions(-)
  

On Sat, Feb 8, 2020, 10:28 Tom Tromey <tom@tromey.com> wrote:

> This changes the DWARF frame code to store the comp_unit on the
> objfile, rather than storing the FDE table.  It also changes the
> comp_unit to be heap-allocated using "new".
>
> This change makes it simpler for a later patch to add a field to the
> comp_unit, and to have deallaction work properly.  This in turn is
> important for making the frame data be independent of the objfile.
>
> gdb/ChangeLog
> 2020-02-08  Tom Tromey  <tom@tromey.com>
>
>         * dwarf2/frame.c (struct comp_unit): Add initializers.
>         (dwarf2_frame_objfile_data): Store a comp_unit.
>         (dwarf2_frame_find_fde): Update.
>         (dwarf2_build_frame_info): Use "new".
>
> Change-Id: Id339892824caf47e7909ed5ee7e2f0556d645dba
> ---
>  gdb/ChangeLog      |  7 +++++++
>  gdb/dwarf2/frame.c | 43 ++++++++++++++++++++-----------------------
>  2 files changed, 27 insertions(+), 23 deletions(-)
>
> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
> index 6d87e598345..0e74b8e7e68 100644
> --- a/gdb/dwarf2/frame.c
> +++ b/gdb/dwarf2/frame.c
> @@ -137,24 +137,27 @@ typedef std::vector<dwarf2_fde *> dwarf2_fde_table;
>  struct comp_unit
>  {
>    /* Keep the bfd convenient.  */
> -  bfd *abfd;
> +  bfd *abfd = nullptr;
>
> -  struct objfile *objfile;
> +  struct objfile *objfile = nullptr;
>
>    /* Pointer to the .debug_frame section loaded into memory.  */
> -  const gdb_byte *dwarf_frame_buffer;
> +  const gdb_byte *dwarf_frame_buffer = nullptr;
>
>    /* Length of the loaded .debug_frame section.  */
> -  bfd_size_type dwarf_frame_size;
> +  bfd_size_type dwarf_frame_size = 0;
>
>    /* Pointer to the .debug_frame section.  */
> -  asection *dwarf_frame_section;
> +  asection *dwarf_frame_section = nullptr;
>
>    /* Base for DW_EH_PE_datarel encodings.  */
> -  bfd_vma dbase;
> +  bfd_vma dbase = 0;
>
>    /* Base for DW_EH_PE_textrel encodings.  */
> -  bfd_vma tbase;
> +  bfd_vma tbase = 0;
> +
> +  /* The FDE table.  */
> +  dwarf2_fde_table fde_table;
>  };
>
>  static struct dwarf2_fde *dwarf2_frame_find_fde (CORE_ADDR *pc,
> @@ -1467,7 +1470,7 @@ dwarf2_frame_cfa (struct frame_info *this_frame)
>    return get_frame_base (this_frame);
>  }
>
> -const struct objfile_key<dwarf2_fde_table> dwarf2_frame_objfile_data;
> +const struct objfile_key<comp_unit> dwarf2_frame_objfile_data;
>
>
>
> @@ -1630,18 +1633,18 @@ dwarf2_frame_find_fde (CORE_ADDR *pc, CORE_ADDR
> *out_offset)
>  {
>    for (objfile *objfile : current_program_space->objfiles ())
>      {
> -      dwarf2_fde_table *fde_table;
>        CORE_ADDR offset;
>        CORE_ADDR seek_pc;
>
> -      fde_table = dwarf2_frame_objfile_data.get (objfile);
> -      if (fde_table == NULL)
> +      comp_unit *unit = dwarf2_frame_objfile_data.get (objfile);
> +      if (unit == NULL)
>         {
>           dwarf2_build_frame_info (objfile);
> -         fde_table = dwarf2_frame_objfile_data.get (objfile);
> +         unit = dwarf2_frame_objfile_data.get (objfile);
>         }
> -      gdb_assert (fde_table != NULL);
> +      gdb_assert (unit != NULL);
>
> +      dwarf2_fde_table *fde_table = &unit->fde_table;
>        if (fde_table->empty ())
>         continue;
>
> @@ -2120,14 +2123,11 @@ dwarf2_build_frame_info (struct objfile *objfile)
>    const gdb_byte *frame_ptr;
>    dwarf2_cie_table cie_table;
>    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);
> +  unit = new comp_unit;
>

Move the declaration here?

   unit->abfd = objfile->obfd;
>    unit->objfile = objfile;
>

These two could be constructor arguments?

-  unit->dbase = 0;
> -  unit->tbase = 0;
>
>    if (objfile->separate_debug_objfile_backlink == NULL)
>      {
> @@ -2202,9 +2202,6 @@ dwarf2_build_frame_info (struct objfile *objfile)
>         }
>      }
>
> -  /* Copy fde_table to obstack: it is needed at runtime.  */
> -  fde_table2 = new dwarf2_fde_table;
> -
>    if (!fde_table.empty ())
>      {
>        struct dwarf2_fde *fde_prev = NULL;
> @@ -2249,13 +2246,13 @@ dwarf2_build_frame_info (struct objfile *objfile)
>               && fde_prev->initial_location == fde->initial_location)
>             continue;
>
> -         fde_table2->push_back (fde);
> +         unit->fde_table.push_back (fde);
>           fde_prev = fde;
>         }
> -      fde_table2->shrink_to_fit ();
> +      unit->fde_table.shrink_to_fit ();
>      }
>
> -  dwarf2_frame_objfile_data.set (objfile, fde_table2);
> +  dwarf2_frame_objfile_data.set (objfile, unit);
>  }
>
>  /* Handle 'maintenance show dwarf unwinders'.  */
> --
> 2.17.2
>
>
  

On 2/8/20 12:27 PM, Tom Tromey wrote:
> This changes the DWARF frame code to store the comp_unit on the
> objfile, rather than storing the FDE table.  It also changes the
> comp_unit to be heap-allocated using "new".
> 
> This change makes it simpler for a later patch to add a field to the
> comp_unit, and to have deallaction work properly.  This in turn is
> important for making the frame data be independent of the objfile.
> 
> gdb/ChangeLog
> 2020-02-08  Tom Tromey  <tom@tromey.com>
> 
> 	* dwarf2/frame.c (struct comp_unit): Add initializers.
> 	(dwarf2_frame_objfile_data): Store a comp_unit.
> 	(dwarf2_frame_find_fde): Update.
> 	(dwarf2_build_frame_info): Use "new".
> 
> Change-Id: Id339892824caf47e7909ed5ee7e2f0556d645dba
> ---
>   gdb/ChangeLog      |  7 +++++++
>   gdb/dwarf2/frame.c | 43 ++++++++++++++++++++-----------------------
>   2 files changed, 27 insertions(+), 23 deletions(-)
> 
> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
> index 6d87e598345..0e74b8e7e68 100644
> --- a/gdb/dwarf2/frame.c
> +++ b/gdb/dwarf2/frame.c
> @@ -137,24 +137,27 @@ typedef std::vector<dwarf2_fde *> dwarf2_fde_table;
>   struct comp_unit
>   {
>     /* Keep the bfd convenient.  */
> -  bfd *abfd;
> +  bfd *abfd = nullptr;
>   
> -  struct objfile *objfile;
> +  struct objfile *objfile = nullptr;
>   
>     /* Pointer to the .debug_frame section loaded into memory.  */
> -  const gdb_byte *dwarf_frame_buffer;
> +  const gdb_byte *dwarf_frame_buffer = nullptr;
>   
>     /* Length of the loaded .debug_frame section.  */
> -  bfd_size_type dwarf_frame_size;
> +  bfd_size_type dwarf_frame_size = 0;
>   
>     /* Pointer to the .debug_frame section.  */
> -  asection *dwarf_frame_section;
> +  asection *dwarf_frame_section = nullptr;
>   
>     /* Base for DW_EH_PE_datarel encodings.  */
> -  bfd_vma dbase;
> +  bfd_vma dbase = 0;
>   
>     /* Base for DW_EH_PE_textrel encodings.  */
> -  bfd_vma tbase;
> +  bfd_vma tbase = 0;
> +
> +  /* The FDE table.  */
> +  dwarf2_fde_table fde_table;
>   };
>   
>   static struct dwarf2_fde *dwarf2_frame_find_fde (CORE_ADDR *pc,
> @@ -1467,7 +1470,7 @@ dwarf2_frame_cfa (struct frame_info *this_frame)
>     return get_frame_base (this_frame);
>   }
>   
> -const struct objfile_key<dwarf2_fde_table> dwarf2_frame_objfile_data;
> +const struct objfile_key<comp_unit> dwarf2_frame_objfile_data;
>   
>   
>   
> @@ -1630,18 +1633,18 @@ dwarf2_frame_find_fde (CORE_ADDR *pc, CORE_ADDR *out_offset)
>   {
>     for (objfile *objfile : current_program_space->objfiles ())
>       {
> -      dwarf2_fde_table *fde_table;
>         CORE_ADDR offset;
>         CORE_ADDR seek_pc;
>   
> -      fde_table = dwarf2_frame_objfile_data.get (objfile);
> -      if (fde_table == NULL)
> +      comp_unit *unit = dwarf2_frame_objfile_data.get (objfile);
> +      if (unit == NULL)
>   	{

NULL -> nullptr.

>   	  dwarf2_build_frame_info (objfile);
> -	  fde_table = dwarf2_frame_objfile_data.get (objfile);
> +	  unit = dwarf2_frame_objfile_data.get (objfile);
>   	}
> -      gdb_assert (fde_table != NULL);
> +      gdb_assert (unit != NULL);

NULL -> nullptr

>   
> +      dwarf2_fde_table *fde_table = &unit->fde_table;
>         if (fde_table->empty ())
>   	continue;
>   
> @@ -2120,14 +2123,11 @@ dwarf2_build_frame_info (struct objfile *objfile)
>     const gdb_byte *frame_ptr;
>     dwarf2_cie_table cie_table;
>     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);
> +  unit = new comp_unit;
>     unit->abfd = objfile->obfd;
>     unit->objfile = objfile;
> -  unit->dbase = 0;
> -  unit->tbase = 0;
>   
>     if (objfile->separate_debug_objfile_backlink == NULL)
>       {
> @@ -2202,9 +2202,6 @@ dwarf2_build_frame_info (struct objfile *objfile)
>   	}
>       }
>   
> -  /* Copy fde_table to obstack: it is needed at runtime.  */
> -  fde_table2 = new dwarf2_fde_table;
> -

Ok. I mentioned the stale comment in another patch, so please ignore it.

>     if (!fde_table.empty ())
>       {
>         struct dwarf2_fde *fde_prev = NULL;
> @@ -2249,13 +2246,13 @@ dwarf2_build_frame_info (struct objfile *objfile)
>   	      && fde_prev->initial_location == fde->initial_location)
>   	    continue;
>   
> -	  fde_table2->push_back (fde);
> +	  unit->fde_table.push_back (fde);
>   	  fde_prev = fde;
>   	}
> -      fde_table2->shrink_to_fit ();
> +      unit->fde_table.shrink_to_fit ();
>       }
>   
> -  dwarf2_frame_objfile_data.set (objfile, fde_table2);
> +  dwarf2_frame_objfile_data.set (objfile, unit);
>   }
>   
>   /* Handle 'maintenance show dwarf unwinders'.  */
> 

Otherwise LGTM.
  

>>>>> "Christian" == Christian Biesinger <cbiesinger@google.com> writes:

>  -  unit = XOBNEW (&objfile->objfile_obstack, comp_unit);
>  +  unit = new comp_unit;

Christian> Move the declaration here?

> unit->abfd = objfile->obfd;
> unit->objfile = objfile;

Christian> These two could be constructor arguments?

I made both of these changes.

Tom
  

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

>> +      comp_unit *unit = dwarf2_frame_objfile_data.get (objfile);
>> +      if (unit == NULL)
>> {

Luis> NULL -> nullptr.

>> dwarf2_build_frame_info (objfile);
>> -	  fde_table = dwarf2_frame_objfile_data.get (objfile);
>> +	  unit = dwarf2_frame_objfile_data.get (objfile);
>> }
>> -      gdb_assert (fde_table != NULL);
>> +      gdb_assert (unit != NULL);

Luis> NULL -> nullptr

I made these changes.

Tom
  

On 2020-02-08 10:27 a.m., Tom Tromey wrote:
> @@ -2120,14 +2123,11 @@ dwarf2_build_frame_info (struct objfile *objfile)
>    const gdb_byte *frame_ptr;
>    dwarf2_cie_table cie_table;
>    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);
> +  unit = new comp_unit;

Not mandatory, but I'd suggest using an std::unique_ptr here, to hold this
variable, and then "release" it into the per-objfile map (since it can only
hold bare pointers).

Simon
  

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

>> -  unit = XOBNEW (&objfile->objfile_obstack, comp_unit);
>> +  unit = new comp_unit;

Simon> Not mandatory, but I'd suggest using an std::unique_ptr here, to hold this
Simon> variable, and then "release" it into the per-objfile map (since it can only
Simon> hold bare pointers).

Good idea, I did this.

Tom