[6/7] jit: c++-ify gdb_block

Message ID 20191213060323.1799590-7-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.
Add a constructor to gdb_block, change the name field to be a
gdb::unique_xmalloc_ptr.

gdb/ChangeLog:

	* jit.c (struct gdb_block): Add constructor, initialize
	real_block field.
	<name): Change type to gdb::unique_xmalloc_ptr.
	(struct gdb_symtab) <~gdb_symtab>: Free blocks with delete.
	(jit_block_open_impl): Use gdb_block constructor.
	(finalize_symtab): Adjust to gdb::unique_xmalloc_ptr.
---
 gdb/jit.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

-- 
2.24.1

Comments

Tankut Baris Aktemur Dec. 13, 2019, 7:54 a.m. | #1
* On Friday, December 13, 2019 7:03 AM, Simon Marchi wrote:
> 

> Add a constructor to gdb_block, change the name field to be a

> gdb::unique_xmalloc_ptr.

> 

> gdb/ChangeLog:

> 

> 	* jit.c (struct gdb_block): Add constructor, initialize

> 	real_block field.

> 	<name): Change type to gdb::unique_xmalloc_ptr.


Minor typo: "<name" -> "<name>"

Regards,
-Baris

> 	(struct gdb_symtab) <~gdb_symtab>: Free blocks with delete.

> 	(jit_block_open_impl): Use gdb_block constructor.

> 	(finalize_symtab): Adjust to gdb::unique_xmalloc_ptr.


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Simon Marchi Dec. 13, 2019, 3:05 p.m. | #2
On 2019-12-13 2:54 a.m., Aktemur, Tankut Baris wrote:
> * On Friday, December 13, 2019 7:03 AM, Simon Marchi wrote:

>>

>> Add a constructor to gdb_block, change the name field to be a

>> gdb::unique_xmalloc_ptr.

>>

>> gdb/ChangeLog:

>>

>> 	* jit.c (struct gdb_block): Add constructor, initialize

>> 	real_block field.

>> 	<name): Change type to gdb::unique_xmalloc_ptr.

> 

> Minor typo: "<name" -> "<name>"


Oh thanks!  I'll fix it locally, while I wait for more feedback.

Simon
Simon Marchi via Gdb-patches Dec. 13, 2019, 3:10 p.m. | #3
On Fri, Dec 13, 2019, 01:04 Simon Marchi <simon.marchi@polymtl.ca> wrote:

> Add a constructor to gdb_block, change the name field to be a

> gdb::unique_xmalloc_ptr.

>

> gdb/ChangeLog:

>

>         * jit.c (struct gdb_block): Add constructor, initialize

>         real_block field.

>         <name): Change type to gdb::unique_xmalloc_ptr.

>         (struct gdb_symtab) <~gdb_symtab>: Free blocks with delete.

>         (jit_block_open_impl): Use gdb_block constructor.

>         (finalize_symtab): Adjust to gdb::unique_xmalloc_ptr.

> ---

>  gdb/jit.c | 28 +++++++++++++---------------

>  1 file changed, 13 insertions(+), 15 deletions(-)

>

> diff --git a/gdb/jit.c b/gdb/jit.c

> index 1f6de6796e10..56a51f04eb2f 100644

> --- a/gdb/jit.c

> +++ b/gdb/jit.c

> @@ -428,20 +428,28 @@ jit_read_code_entry (struct gdbarch *gdbarch,

>

>  struct gdb_block

>  {

> +  gdb_block (gdb_block *parent, CORE_ADDR begin, CORE_ADDR end,

> +            const char *name)

> +    : parent (parent),

> +      begin (begin),

> +      end (end),

> +      name (name != nullptr ? xstrdup (name) : nullptr)

> +  {}

> +

>    /* The parent of this block.  */

>    struct gdb_block *parent;

>

>    /* Points to the "real" block that is being built out of this

>       instance.  This block will be added to a blockvector, which will

>       then be added to a symtab.  */

> -  struct block *real_block;

> +  struct block *real_block = nullptr;

>

>    /* The first and last code address corresponding to this block.  */

>    CORE_ADDR begin, end;

>

>    /* The name of this block (if any).  If this is non-NULL, the

>       FUNCTION symbol symbol is set to this value.  */

> -  const char *name;

> +  gdb::unique_xmalloc_ptr<char> name;

>  };

>

>  /* Proxy object for building a symtab.  */

> @@ -455,10 +463,7 @@ struct gdb_symtab

>    ~gdb_symtab ()

>    {

>      for (gdb_block *gdb_block_iter : this->blocks)

> -      {

> -        xfree ((void *) gdb_block_iter->name);

> -        xfree (gdb_block_iter);

> -      }

> +      delete gdb_block_iter;

>    }

>


Have you considered making this a vector of unique_ptrs, so you don't have
to manually delete them?


>    /* The list of blocks in this symtab.  These will eventually be

> @@ -534,16 +539,9 @@ jit_block_open_impl (struct gdb_symbol_callbacks *cb,

>                       struct gdb_symtab *symtab, struct gdb_block *parent,

>                       GDB_CORE_ADDR begin, GDB_CORE_ADDR end, const char

> *name)

>  {

> -  struct gdb_block *block = XCNEW (struct gdb_block);

> -

> -  block->begin = (CORE_ADDR) begin;

> -  block->end = (CORE_ADDR) end;

> -  block->name = name ? xstrdup (name) : NULL;

> -  block->parent = parent;

> -

>    /* Place the block at the end of the vector, it will be sorted when the

>       symtab is finalized.  */

> -  symtab->blocks.push_back (block);

> +  symtab->blocks.push_back (new gdb_block (parent, begin, end, name));

>    return symtab->blocks.back ();

>  }

>

> @@ -665,7 +663,7 @@ finalize_symtab (struct gdb_symtab *stab, struct

> objfile *objfile)

>        SYMBOL_BLOCK_VALUE (block_name) = new_block;

>

>        block_name->name = obstack_strdup (&objfile->objfile_obstack,

> -                                        gdb_block_iter->name);

> +                                        gdb_block_iter->name.get ());

>

>        BLOCK_FUNCTION (new_block) = block_name;

>

> --

> 2.24.1

>

>
Simon Marchi Dec. 13, 2019, 3:18 p.m. | #4
On 2019-12-13 10:10 a.m., Christian Biesinger via gdb-patches wrote:
>> @@ -455,10 +463,7 @@ struct gdb_symtab

>>    ~gdb_symtab ()

>>    {

>>      for (gdb_block *gdb_block_iter : this->blocks)

>> -      {

>> -        xfree ((void *) gdb_block_iter->name);

>> -        xfree (gdb_block_iter);

>> -      }

>> +      delete gdb_block_iter;

>>    }

>>

> 

> Have you considered making this a vector of unique_ptrs, so you don't have

> to manually delete them?


Yes, it's done in the following patch.  I went perhaps a bit overboard with the
patch splitting, but I prefer to do these changes in many smaller patches, so that
if something goes wrong, it's easier to identify the culprit.

Simon
Pedro Alves Dec. 13, 2019, 8:57 p.m. | #5
On 12/13/19 3:18 PM, Simon Marchi wrote:
> On 2019-12-13 10:10 a.m., Christian Biesinger via gdb-patches wrote:

>>> @@ -455,10 +463,7 @@ struct gdb_symtab

>>>    ~gdb_symtab ()

>>>    {

>>>      for (gdb_block *gdb_block_iter : this->blocks)

>>> -      {

>>> -        xfree ((void *) gdb_block_iter->name);

>>> -        xfree (gdb_block_iter);

>>> -      }

>>> +      delete gdb_block_iter;

>>>    }

>>>

>>

>> Have you considered making this a vector of unique_ptrs, so you don't have

>> to manually delete them?

> 

> Yes, it's done in the following patch.  I went perhaps a bit overboard with the

> patch splitting, but I prefer to do these changes in many smaller patches, so that

> if something goes wrong, it's easier to identify the culprit.


I wonder whether it wouldn't be simpler to use std::list<object> for these cases.

Thanks,
Pedro Alves
Simon Marchi Dec. 13, 2019, 9:01 p.m. | #6
On 2019-12-13 3:57 p.m., Pedro Alves wrote:
> On 12/13/19 3:18 PM, Simon Marchi wrote:

>> On 2019-12-13 10:10 a.m., Christian Biesinger via gdb-patches wrote:

>>>> @@ -455,10 +463,7 @@ struct gdb_symtab

>>>>    ~gdb_symtab ()

>>>>    {

>>>>      for (gdb_block *gdb_block_iter : this->blocks)

>>>> -      {

>>>> -        xfree ((void *) gdb_block_iter->name);

>>>> -        xfree (gdb_block_iter);

>>>> -      }

>>>> +      delete gdb_block_iter;

>>>>    }

>>>>

>>>

>>> Have you considered making this a vector of unique_ptrs, so you don't have

>>> to manually delete them?

>>

>> Yes, it's done in the following patch.  I went perhaps a bit overboard with the

>> patch splitting, but I prefer to do these changes in many smaller patches, so that

>> if something goes wrong, it's easier to identify the culprit.

> 

> I wonder whether it wouldn't be simpler to use std::list<object> for these cases.


I don't think the code would be much different if use used a list, so I don't expect it
to be much simpler.  If there's a compelling argument for using a list, I can do the
change, but if it's equivalent, I'd rather stick with what is already done.

Simon
Pedro Alves Dec. 13, 2019, 10:20 p.m. | #7
On 12/13/19 9:01 PM, Simon Marchi wrote:
> On 2019-12-13 3:57 p.m., Pedro Alves wrote:

>> I wonder whether it wouldn't be simpler to use std::list<object> for these cases.

> 

> I don't think the code would be much different if use used a list, so I don't expect it

> to be much simpler.  If there's a compelling argument for using a list, I can do the

> change, but if it's equivalent, I'd rather stick with what is already done.

> 


It wouldn't be that much simpler, but simpler regardless, I believe.
Instead of a vector of heap-allocated pointers, with means an extra levels of
indirection and use of a unique_ptr to manage memory, the list would hold
the objects directly, and you'd get address stability encoded in the
data structure.  I don't think changing it would be anything remotely
complicated, since vector/list's apis are quite similar, if not the same
for the uses here.  Seems like a win to me.  But I'm not unhappy with
what you have.

Thanks,
Pedro Alves
Simon Marchi Dec. 14, 2019, 5:38 p.m. | #8
On 2019-12-13 5:20 p.m., Pedro Alves wrote:
> On 12/13/19 9:01 PM, Simon Marchi wrote:

>> On 2019-12-13 3:57 p.m., Pedro Alves wrote:

>>> I wonder whether it wouldn't be simpler to use std::list<object> for these cases.

>>

>> I don't think the code would be much different if use used a list, so I don't expect it

>> to be much simpler.  If there's a compelling argument for using a list, I can do the

>> change, but if it's equivalent, I'd rather stick with what is already done.

>>

> 

> It wouldn't be that much simpler, but simpler regardless, I believe.

> Instead of a vector of heap-allocated pointers, with means an extra levels of

> indirection and use of a unique_ptr to manage memory, the list would hold

> the objects directly, and you'd get address stability encoded in the

> data structure.  I don't think changing it would be anything remotely

> complicated, since vector/list's apis are quite similar, if not the same

> for the uses here.  Seems like a win to me.  But I'm not unhappy with

> what you have.


Ok, I can do a v2 with that.

Simon

Patch

diff --git a/gdb/jit.c b/gdb/jit.c
index 1f6de6796e10..56a51f04eb2f 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -428,20 +428,28 @@  jit_read_code_entry (struct gdbarch *gdbarch,
 
 struct gdb_block
 {
+  gdb_block (gdb_block *parent, CORE_ADDR begin, CORE_ADDR end,
+	     const char *name)
+    : parent (parent),
+      begin (begin),
+      end (end),
+      name (name != nullptr ? xstrdup (name) : nullptr)
+  {}
+
   /* The parent of this block.  */
   struct gdb_block *parent;
 
   /* Points to the "real" block that is being built out of this
      instance.  This block will be added to a blockvector, which will
      then be added to a symtab.  */
-  struct block *real_block;
+  struct block *real_block = nullptr;
 
   /* The first and last code address corresponding to this block.  */
   CORE_ADDR begin, end;
 
   /* The name of this block (if any).  If this is non-NULL, the
      FUNCTION symbol symbol is set to this value.  */
-  const char *name;
+  gdb::unique_xmalloc_ptr<char> name;
 };
 
 /* Proxy object for building a symtab.  */
@@ -455,10 +463,7 @@  struct gdb_symtab
   ~gdb_symtab ()
   {
     for (gdb_block *gdb_block_iter : this->blocks)
-      {
-        xfree ((void *) gdb_block_iter->name);
-        xfree (gdb_block_iter);
-      }
+      delete gdb_block_iter;
   }
 
   /* The list of blocks in this symtab.  These will eventually be
@@ -534,16 +539,9 @@  jit_block_open_impl (struct gdb_symbol_callbacks *cb,
                      struct gdb_symtab *symtab, struct gdb_block *parent,
                      GDB_CORE_ADDR begin, GDB_CORE_ADDR end, const char *name)
 {
-  struct gdb_block *block = XCNEW (struct gdb_block);
-
-  block->begin = (CORE_ADDR) begin;
-  block->end = (CORE_ADDR) end;
-  block->name = name ? xstrdup (name) : NULL;
-  block->parent = parent;
-
   /* Place the block at the end of the vector, it will be sorted when the
      symtab is finalized.  */
-  symtab->blocks.push_back (block);
+  symtab->blocks.push_back (new gdb_block (parent, begin, end, name));
   return symtab->blocks.back ();
 }
 
@@ -665,7 +663,7 @@  finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
       SYMBOL_BLOCK_VALUE (block_name) = new_block;
 
       block_name->name = obstack_strdup (&objfile->objfile_obstack,
-					 gdb_block_iter->name);
+					 gdb_block_iter->name.get ());
 
       BLOCK_FUNCTION (new_block) = block_name;