[6/7] jit: c++-ify gdb_block
Commit Message
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(-)
Comments
* 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
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
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
>
>
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
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
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
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
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
@@ -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;