[PATCHv2,3/3] gdb: Remove a use of VEC from dwarf2read.{c,h}
Commit Message
Removes a use of VEC from dwarf2read.{c,h} and replaces it with
std::vector. As far as possible this is a like for like replacement
with minimal refactoring.
There should be no user visible changes after this commit.
gdb/ChangeLog:
* dwarf2read.c (struct type_unit_group) <tus>: Convert to
std::vector.
(build_type_psymtabs_reader): Update for std::vector.
(build_type_psymtab_dependencies): Likewise.
* dwarf2read.h: Remove use of DEF_VEC_P.
(typedef sig_type_ptr): Delete.
---
gdb/ChangeLog | 9 +++++++++
gdb/dwarf2read.c | 16 ++++++++--------
gdb/dwarf2read.h | 3 ---
3 files changed, 17 insertions(+), 11 deletions(-)
Comments
>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
Andrew> Removes a use of VEC from dwarf2read.{c,h} and replaces it with
Andrew> std::vector. As far as possible this is a like for like replacement
Andrew> with minimal refactoring.
Andrew> + if (tu_group->tus == nullptr)
Andrew> + tu_group->tus = new std::vector <signatured_type *>;
Andrew> + tu_group->tus->push_back (sig_type);
Andrew> + int len = tu_group->tus->size ();
Is it possible to reach here with tus==nullptr?
Andrew> + for (i = 0; i < tu_group->tus->size (); ++i)
Likewise.
Andrew> - VEC_free (sig_type_ptr, tu_group->tus);
Andrew> + delete tu_group->tus;
VEC_free resets tus to nullptr, but I don't know whether or not that's
important.
Tom
Tom raised some good points about patch #3, specifically that some of
the query functions like VEC_length and VEC_empty will return a sane
answer even when the vector pointer is NULL. He also pointed out that
VEC_free resets theh vector pointer back to NULL.
This revision of the series is a little more defensive about checking
for the std::vector pointer being null before dereferencing it. In
places where I'm reasonably sure the vector pointer won't be NULL I've
added an assert - then at least if I'm wrong we'll get a nice error
rather than a random crash.
Thanks,
Andrew
---
Andrew Burgess (3):
gdb: Remove a VEC from gdbsupport/btrace-common.h
gdb: Change a VEC to std::vector in btrace.{c,h}
gdb: Remove a use of VEC from dwarf2read.{c,h}
gdb/ChangeLog | 46 ++++++++++++++++++++
gdb/btrace.c | 98 ++++++++++++++++++++----------------------
gdb/btrace.h | 5 +--
gdb/dwarf2read.c | 17 ++++----
gdb/dwarf2read.h | 3 --
gdb/gdbserver/ChangeLog | 5 +++
gdb/gdbserver/linux-low.c | 8 +---
gdb/gdbsupport/btrace-common.c | 20 ++++-----
gdb/gdbsupport/btrace-common.h | 19 ++++----
gdb/nat/linux-btrace.c | 16 +++----
10 files changed, 139 insertions(+), 98 deletions(-)
On 2019-09-25 7:59 p.m., Andrew Burgess wrote:
> Tom raised some good points about patch #3, specifically that some of
> the query functions like VEC_length and VEC_empty will return a sane
> answer even when the vector pointer is NULL. He also pointed out that
> VEC_free resets theh vector pointer back to NULL.
>
> This revision of the series is a little more defensive about checking
> for the std::vector pointer being null before dereferencing it. In
> places where I'm reasonably sure the vector pointer won't be NULL I've
> added an assert - then at least if I'm wrong we'll get a nice error
> rather than a random crash.
Oh, that's a good point. I gave a quick look at patches 1 and 2, and
they seem good with the extra checks.
About testing, the testsuite contains a bunch of btrace tests in gdb.btrace,
and some more in gdb.python. It would be good to run those tests on a CPU
that supports Intel BTS and PT (which would mean any Broadwell or more recent
CPU) and a GDB linked against libipt. I can give it a try if you are unable
to do it for some reason.
Simon
Only patches #1 and #2 have changed.
#1 - Update with Markus's feedback, and added new nullptr safe
methods to get the size and empty status of the new vector.
Use these throughout and remove lots of assertions.
#2 - Minor updates from Markus's feedback. Moves a vector
allocation out of a loop, and allocates the vector in
btrace_maint_update_packets.
The code is available for testing from here:
https://github.com/T-J-Teru/binutils-gdb/tree/gdb-remove-vec
Thanks,
Andrew
---
Andrew Burgess (3):
gdb: Remove a VEC from gdbsupport/btrace-common.h
gdb: Change a VEC to std::vector in btrace.{c,h}
gdb: Remove a use of VEC from dwarf2read.{c,h}
gdb/ChangeLog | 50 +++++++++++++++++++++
gdb/btrace.c | 98 +++++++++++++++++++-----------------------
gdb/btrace.h | 5 +--
gdb/dwarf2read.c | 17 ++++----
gdb/dwarf2read.h | 3 --
gdb/gdbserver/ChangeLog | 5 +++
gdb/gdbserver/linux-low.c | 8 +---
gdb/gdbsupport/btrace-common.c | 18 ++++----
gdb/gdbsupport/btrace-common.h | 38 ++++++++++++----
gdb/nat/linux-btrace.c | 16 +++----
10 files changed, 157 insertions(+), 101 deletions(-)
Thank you to Markus for the continued feedback. This revision
addresses the points raised against v4.
- Removed helper functions, I'm convinved that the std::vector
pointer should not be null at the places we dereference it.
- Streamlined the removal of the vec.h header, originally I moved
the include in the first patch, and removed it in the second
patch. Now I leave it alone in the first patch, and delete it in
the second.
- I've NOT addressed the long line feedback, one line was 80+ long,
while the other was 79. I'd prefer to keep both of these split
over two lines.
- Patch #3 is unchanged.
Thanks,
Andrew
--
Andrew Burgess (3):
gdb: Remove a VEC from gdbsupport/btrace-common.h
gdb: Change a VEC to std::vector in btrace.{c,h}
gdb: Remove a use of VEC from dwarf2read.{c,h}
gdb/ChangeLog | 47 ++++++++++++++++++++
gdb/btrace.c | 98 +++++++++++++++++++-----------------------
gdb/btrace.h | 5 +--
gdb/dwarf2read.c | 17 ++++----
gdb/dwarf2read.h | 3 --
gdb/gdbserver/ChangeLog | 5 +++
gdb/gdbserver/linux-low.c | 8 +---
gdb/gdbsupport/btrace-common.c | 18 ++++----
gdb/gdbsupport/btrace-common.h | 19 ++++----
gdb/nat/linux-btrace.c | 16 +++----
10 files changed, 135 insertions(+), 101 deletions(-)
Hello Andrew,
> Andrew Burgess (3):
> gdb: Remove a VEC from gdbsupport/btrace-common.h
> gdb: Change a VEC to std::vector in btrace.{c,h}
> gdb: Remove a use of VEC from dwarf2read.{c,h}
>
> gdb/ChangeLog | 47 ++++++++++++++++++++
> gdb/btrace.c | 98 +++++++++++++++++++-----------------------
> gdb/btrace.h | 5 +--
> gdb/dwarf2read.c | 17 ++++----
> gdb/dwarf2read.h | 3 --
> gdb/gdbserver/ChangeLog | 5 +++
> gdb/gdbserver/linux-low.c | 8 +---
> gdb/gdbsupport/btrace-common.c | 18 ++++---- gdb/gdbsupport/btrace-
> common.h | 19 ++++----
> gdb/nat/linux-btrace.c | 16 +++----
> 10 files changed, 135 insertions(+), 101 deletions(-)
The btrace bits look good to me.
Thanks for your patches,
Markus.
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
@@ -620,7 +620,7 @@ struct type_unit_group
/* The TUs that share this DW_AT_stmt_list entry.
This is added to while parsing type units to build partial symtabs,
and is deleted afterwards and not used again. */
- VEC (sig_type_ptr) *tus;
+ std::vector <signatured_type *> *tus;
/* The compunit symtab.
Type units in a group needn't all be defined in the same source file,
@@ -8184,7 +8184,9 @@ build_type_psymtabs_reader (const struct die_reader_specs *reader,
attr = dwarf2_attr_no_follow (type_unit_die, DW_AT_stmt_list);
tu_group = get_type_unit_group (cu, attr);
- VEC_safe_push (sig_type_ptr, tu_group->tus, sig_type);
+ if (tu_group->tus == nullptr)
+ tu_group->tus = new std::vector <signatured_type *>;
+ tu_group->tus->push_back (sig_type);
prepare_one_comp_unit (cu, type_unit_die, language_minimal);
pst = create_partial_symtab (per_cu, "");
@@ -8341,8 +8343,7 @@ build_type_psymtab_dependencies (void **slot, void *info)
struct type_unit_group *tu_group = (struct type_unit_group *) *slot;
struct dwarf2_per_cu_data *per_cu = &tu_group->per_cu;
struct partial_symtab *pst = per_cu->v.psymtab;
- int len = VEC_length (sig_type_ptr, tu_group->tus);
- struct signatured_type *iter;
+ int len = tu_group->tus->size ();
int i;
gdb_assert (len > 0);
@@ -8350,16 +8351,15 @@ build_type_psymtab_dependencies (void **slot, void *info)
pst->number_of_dependencies = len;
pst->dependencies = objfile->partial_symtabs->allocate_dependencies (len);
- for (i = 0;
- VEC_iterate (sig_type_ptr, tu_group->tus, i, iter);
- ++i)
+ for (i = 0; i < tu_group->tus->size (); ++i)
{
+ struct signatured_type *iter = tu_group->tus->at (i);
gdb_assert (iter->per_cu.is_debug_types);
pst->dependencies[i] = iter->per_cu.v.psymtab;
iter->type_unit_group = tu_group;
}
- VEC_free (sig_type_ptr, tu_group->tus);
+ delete tu_group->tus;
return 1;
}
@@ -401,9 +401,6 @@ struct signatured_type
struct dwo_unit *dwo_unit;
};
-typedef struct signatured_type *sig_type_ptr;
-DEF_VEC_P (sig_type_ptr);
-
ULONGEST read_unsigned_leb128 (bfd *, const gdb_byte *, unsigned int *);
/* This represents a '.dwz' file. */