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

Message ID 20200208152758.29385-5-tom@tromey.com
State New
Headers show
Series
  • Share DWARF frame information across inferiors
Related show

Commit Message

Tom Tromey Feb. 8, 2020, 3:27 p.m.
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(-)

-- 
2.17.2

Comments

Hannes Domani via Gdb-patches Feb. 9, 2020, 11:18 p.m. | #1
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

>

>
Luis Machado Feb. 11, 2020, 10:31 a.m. | #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.
Tom Tromey Feb. 12, 2020, 12:19 a.m. | #3
>>>>> "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
Tom Tromey Feb. 12, 2020, 12:20 a.m. | #4
>>>>> "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
Simon Marchi Feb. 12, 2020, 3:36 a.m. | #5
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
Tom Tromey Feb. 12, 2020, 10:20 p.m. | #6
>>>>> "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

Patch

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;
   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;
-
   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'.  */