| Message ID | 20191213060323.1799590-7-simon.marchi@polymtl.ca |
|---|---|
| State | New |
| Headers | show |
* 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
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;