Patchwork [PATCHv2,3/3] gdb: Remove a use of VEC from dwarf2read.{c,h}

login
register
mail settings
Submitter Andrew Burgess
Date Sept. 25, 2019, 3:54 p.m.
Message ID <0dcd5cbc1c863f32bbc513db657e3c2aec2dadb7.1569425799.git.andrew.burgess@embecosm.com>
Download mbox | patch
Permalink /patch/34657/
State New
Headers show

Comments

Andrew Burgess - Sept. 25, 2019, 3:54 p.m.
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(-)
Tom Tromey - Sept. 25, 2019, 10:08 p.m.
>>>>> "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
Andrew Burgess - Sept. 25, 2019, 11:59 p.m.
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(-)
Simon Marchi - Sept. 26, 2019, 2:52 a.m.
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
Andrew Burgess - Sept. 26, 2019, 11:41 a.m.
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(-)
Andrew Burgess - Oct. 1, 2019, 11:42 a.m.
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(-)
Metzger, Markus T - Oct. 1, 2019, 12:04 p.m.
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

Patch

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 1052501c351..7cc0dc3c92d 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -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;
 }
diff --git a/gdb/dwarf2read.h b/gdb/dwarf2read.h
index d5a02990d41..aee8742a79c 100644
--- a/gdb/dwarf2read.h
+++ b/gdb/dwarf2read.h
@@ -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.  */