[5/7] jit: make gdb_object::symtabs a vector of unique_ptr

Message ID 20191213060323.1799590-6-simon.marchi@polymtl.ca
State New
Headers show
Series
  • Fix and cleanups in jit.c
Related show

Commit Message

Simon Marchi Dec. 13, 2019, 6:03 a.m.
This makes the gdb_object hold everything.  When it gets destroyed, all
contained symtabs and contained blocks get destroyed.

gdb/ChangeLog:

	* jit.c (struct gdb_object) <symtabs>: Change type to
	std::vector<std::unique_ptr<gdb_symtab>>.
	(jit_symtab_open_impl): Adjust.
	(jit_object_close_impl): Adjust.
---
 gdb/jit.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

-- 
2.24.1

Comments

Pedro Alves Dec. 13, 2019, 5:54 p.m. | #1
On 12/13/19 6:03 AM, Simon Marchi wrote:
>  struct gdb_object

>  {

> -  std::vector<gdb_symtab *> symtabs;

> +  std::vector<std::unique_ptr<gdb_symtab>> symtabs;

>  };


Could this be a vector or objects instead of a vector or pointers?

Like:

 std::vector<gdb_symtab> symtabs;

> +  object->symtabs.emplace_back (new gdb_symtab (file_name));

> +  return object->symtabs.back ().get ();

>  }


and:

 object->symtabs.emplace_back (file_name);
 return &object->symtabs.back ();

Thanks,
Pedro Alves
Simon Marchi Dec. 13, 2019, 6:45 p.m. | #2
On 2019-12-13 12:54 p.m., Pedro Alves wrote:
> On 12/13/19 6:03 AM, Simon Marchi wrote:

>>  struct gdb_object

>>  {

>> -  std::vector<gdb_symtab *> symtabs;

>> +  std::vector<std::unique_ptr<gdb_symtab>> symtabs;

>>  };

> 

> Could this be a vector or objects instead of a vector or pointers?

> 

> Like:

> 

>  std::vector<gdb_symtab> symtabs;


I don't think so.  We return these pointers to the JIT debug readers, which then pass
it back to the block_open and symtab_close callbacks.  It's important that the objects
don't move during their lifetime.  If we had a vector of objects, the pointers we return
to the user would get invalidated the moment the vector is resized.

> 

>> +  object->symtabs.emplace_back (new gdb_symtab (file_name));

>> +  return object->symtabs.back ().get ();

>>  }

> 

> and:

> 

>  object->symtabs.emplace_back (file_name);

>  return &object->symtabs.back ();

> 

> Thanks,

> Pedro Alves

>
Simon Marchi Dec. 13, 2019, 6:51 p.m. | #3
On 2019-12-13 1:45 p.m., Simon Marchi wrote:
> I don't think so.  We return these pointers to the JIT debug readers, which then pass

> it back to the block_open and symtab_close callbacks.  It's important that the objects

> don't move during their lifetime.  If we had a vector of objects, the pointers we return

> to the user would get invalidated the moment the vector is resized.


Actually, it would be good to document this, so we don't change it to a vector of
objects, out of good intention.  I would add this to document the gdb_object::symtabs
field:

  /* Symtabs of this object.

     This is a vector of pointers, rather than a vector of objects, because the
     pointers are returned to the user's debug info reader, so it's important
     that the objects don't change location during their lifetime (which would
     happen with a vector of objects getting resized.  */

I would add a similar comment later in the series to document gdb_symtab::blocks.

Simon
Pedro Alves Dec. 13, 2019, 7:42 p.m. | #4
On 12/13/19 6:51 PM, Simon Marchi wrote:
> On 2019-12-13 1:45 p.m., Simon Marchi wrote:

>> I don't think so.  We return these pointers to the JIT debug readers, which then pass

>> it back to the block_open and symtab_close callbacks.  It's important that the objects

>> don't move during their lifetime.  If we had a vector of objects, the pointers we return

>> to the user would get invalidated the moment the vector is resized.

> 

> Actually, it would be good to document this, so we don't change it to a vector of

> objects, out of good intention.  I would add this to document the gdb_object::symtabs

> field:

> 

>   /* Symtabs of this object.

> 

>      This is a vector of pointers, rather than a vector of objects, because the

>      pointers are returned to the user's debug info reader, so it's important

>      that the objects don't change location during their lifetime (which would

>      happen with a vector of objects getting resized.  */

> 

> I would add a similar comment later in the series to document gdb_symtab::blocks.


That looks good to me.

Thanks,
Pedro Alves

Patch

diff --git a/gdb/jit.c b/gdb/jit.c
index bb855e09f59b..1f6de6796e10 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -476,7 +476,7 @@  struct gdb_symtab
 
 struct gdb_object
 {
-  std::vector<gdb_symtab *> symtabs;
+  std::vector<std::unique_ptr<gdb_symtab>> symtabs;
 };
 
 /* The type of the `private' data passed around by the callback
@@ -521,9 +521,8 @@  jit_symtab_open_impl (struct gdb_symbol_callbacks *cb,
 {
   /* CB stays unused.  See comment in jit_object_open_impl.  */
 
-  gdb_symtab *symtab = new gdb_symtab (file_name);
-  object->symtabs.push_back (symtab);
-  return symtab;
+  object->symtabs.emplace_back (new gdb_symtab (file_name));
+  return object->symtabs.back ().get ();
 }
 
 /* Called by readers to open a new gdb_block.  This function also
@@ -722,8 +721,6 @@  finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
 	    BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
 	}
     }
-
-  delete stab;
 }
 
 /* Called when closing a gdb_objfile.  Converts OBJ to a proper
@@ -742,8 +739,8 @@  jit_object_close_impl (struct gdb_symbol_callbacks *cb,
 			   OBJF_NOT_FILENAME);
   objfile->per_bfd->gdbarch = target_gdbarch ();
 
-  for (gdb_symtab *symtab : obj->symtabs)
-    finalize_symtab (symtab, objfile);
+  for (const std::unique_ptr<gdb_symtab> &symtab : obj->symtabs)
+    finalize_symtab (symtab.get (), objfile);
 
   add_objfile_entry (objfile, *priv_data);