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

login
register
mail settings
Submitter Simon Marchi
Date Dec. 13, 2019, 6:03 a.m.
Message ID <20191213060323.1799590-7-simon.marchi@polymtl.ca>
Download mbox | patch
Permalink /patch/36831/
State New
Headers show

Comments

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(-)
Tankut Baris Aktemur - Dec. 13, 2019, 7:54 a.m.
* 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.
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
Doug Evans via gdb-patches - Dec. 13, 2019, 3:10 p.m.
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.
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.
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.
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.
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.
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;