diff mbox

[PATCHv3,2/3] gdb: Change a VEC to std::vector in btrace.{c,h}

Message ID 6ceaca2e6d37e8bfd913ab787229cbd2d3a7110d.1569455610.git.andrew.burgess@embecosm.com
State New, archived
Headers

Commit Message

Andrew Burgess Sept. 25, 2019, 11:59 p.m. UTC 
  Replace a VEC with a std::vector in btrace.h, and update btrace.c to
match.  This code is currently untested, so I've been quite
conservative with the changes - where previously we used VEC_safe_push
which would allocate the vector if it isn't already allocated - I now
check and allocate the std::vector using 'new' before pushing to the
vector.

As the new vector is inside a union I've currently used a pointer to
vector, which makes the code slightly uglier than it might otherwise
be, but again, due to lack of testing I'm reluctant to start
refactoring the code in a big way.

gdb/ChangeLog:

	* btrace.c (btrace_maint_clear): Update to handle change from VEC
	to std::vector.
	(btrace_maint_decode_pt): Likewise.
	(btrace_maint_update_packets): Likewise.
	(btrace_maint_print_packets): Likewise.
	(maint_info_btrace_cmd): Likewise.
	* btrace.h: Remove 'vec.h' header include and use of DEF_VEC_O.
	(typedef btrace_pt_packet_s): Delete.
	(struct btrace_maint_info) <packets>: Change fromm VEC to
	std::vector.
---
 gdb/ChangeLog | 13 +++++++++++++
 gdb/btrace.c  | 39 +++++++++++++++++++++------------------
 gdb/btrace.h  |  6 +-----
 3 files changed, 35 insertions(+), 23 deletions(-)

Comments

Metzger, Markus T Sept. 26, 2019, 8:47 a.m. UTC | #1 
Hello Andrew,

> @@ -2986,8 +2986,10 @@ btrace_maint_decode_pt (struct btrace_maint_info
> *maint,
>  	  if (maint_btrace_pt_skip_pad == 0 || packet.packet.type != ppt_pad)
>  	    {
>  	      packet.errcode = pt_errcode (errcode);
> -	      VEC_safe_push (btrace_pt_packet_s, maint->variant.pt.packets,
> -			     &packet);
> +	      if (maint->variant.pt.packets == NULL)
> +		maint->variant.pt.packets
> +		  = new std::vector <btrace_pt_packet>;
> +	      maint->variant.pt.packets->push_back (packet);

We shouldn't do that check inside the loop.  I used VEC_safe_push for the realloc aspect.


> @@ -3098,11 +3101,13 @@ btrace_maint_update_packets (struct
> btrace_thread_info *btinfo,
> 
>  #if defined (HAVE_LIBIPT)
>      case BTRACE_FORMAT_PT:
> -      if (VEC_empty (btrace_pt_packet_s, btinfo->maint.variant.pt.packets))
> +      if (btinfo->maint.variant.pt.packets == nullptr
> +	  || btinfo->maint.variant.pt.packets->empty ())
>  	btrace_maint_update_pt_packets (btinfo);

Here, I'd check for nullptr in order to allocate the vector and call
btrace_maint_update_pt_packets.  Afterwards, I'd check for emptiness and delete
the vector again if there are no packets.

We can also leave an empty vector if you prefer that.

Regards,
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
Andrew Burgess Sept. 26, 2019, 11:33 a.m. UTC | #2 
* Metzger, Markus T <markus.t.metzger@intel.com> [2019-09-26 08:47:28 +0000]:

> Hello Andrew,
> 
> > @@ -2986,8 +2986,10 @@ btrace_maint_decode_pt (struct btrace_maint_info
> > *maint,
> >  	  if (maint_btrace_pt_skip_pad == 0 || packet.packet.type != ppt_pad)
> >  	    {
> >  	      packet.errcode = pt_errcode (errcode);
> > -	      VEC_safe_push (btrace_pt_packet_s, maint->variant.pt.packets,
> > -			     &packet);
> > +	      if (maint->variant.pt.packets == NULL)
> > +		maint->variant.pt.packets
> > +		  = new std::vector <btrace_pt_packet>;
> > +	      maint->variant.pt.packets->push_back (packet);
> 
> We shouldn't do that check inside the loop.  I used VEC_safe_push
> for the realloc aspect.

Thanks, I pushed the vector allocation out of the loop.

> 
> 
> > @@ -3098,11 +3101,13 @@ btrace_maint_update_packets (struct
> > btrace_thread_info *btinfo,
> > 
> >  #if defined (HAVE_LIBIPT)
> >      case BTRACE_FORMAT_PT:
> > -      if (VEC_empty (btrace_pt_packet_s, btinfo->maint.variant.pt.packets))
> > +      if (btinfo->maint.variant.pt.packets == nullptr
> > +	  || btinfo->maint.variant.pt.packets->empty ())
> >  	btrace_maint_update_pt_packets (btinfo);
> 
> Here, I'd check for nullptr in order to allocate the vector and call
> btrace_maint_update_pt_packets.  Afterwards, I'd check for emptiness and delete
> the vector again if there are no packets.
> 
> We can also leave an empty vector if you prefer that.

I hope I've interpreted your suggestion correctly.  Please do check my
next version and correct me if I've not understood you.

Thanks,
Andrew



> 
> Regards,
> 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
>
diff mbox

Patch

diff --git a/gdb/btrace.c b/gdb/btrace.c
index e3cf5d88f53..349e609addb 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1827,7 +1827,7 @@  btrace_maint_clear (struct btrace_thread_info *btinfo)
 
 #if defined (HAVE_LIBIPT)
     case BTRACE_FORMAT_PT:
-      xfree (btinfo->maint.variant.pt.packets);
+      delete btinfo->maint.variant.pt.packets;
 
       btinfo->maint.variant.pt.packets = NULL;
       btinfo->maint.variant.pt.packet_history.begin = 0;
@@ -2986,8 +2986,10 @@  btrace_maint_decode_pt (struct btrace_maint_info *maint,
 	  if (maint_btrace_pt_skip_pad == 0 || packet.packet.type != ppt_pad)
 	    {
 	      packet.errcode = pt_errcode (errcode);
-	      VEC_safe_push (btrace_pt_packet_s, maint->variant.pt.packets,
-			     &packet);
+	      if (maint->variant.pt.packets == NULL)
+		maint->variant.pt.packets
+		  = new std::vector <btrace_pt_packet>;
+	      maint->variant.pt.packets->push_back (packet);
 	    }
 	}
 
@@ -2995,8 +2997,9 @@  btrace_maint_decode_pt (struct btrace_maint_info *maint,
 	break;
 
       packet.errcode = pt_errcode (errcode);
-      VEC_safe_push (btrace_pt_packet_s, maint->variant.pt.packets,
-		     &packet);
+      if (maint->variant.pt.packets == NULL)
+	maint->variant.pt.packets = new std::vector <btrace_pt_packet>;
+      maint->variant.pt.packets->push_back (packet);
 
       warning (_("Error at trace offset 0x%" PRIx64 ": %s."),
 	       packet.offset, pt_errstr (packet.errcode));
@@ -3098,11 +3101,13 @@  btrace_maint_update_packets (struct btrace_thread_info *btinfo,
 
 #if defined (HAVE_LIBIPT)
     case BTRACE_FORMAT_PT:
-      if (VEC_empty (btrace_pt_packet_s, btinfo->maint.variant.pt.packets))
+      if (btinfo->maint.variant.pt.packets == nullptr
+	  || btinfo->maint.variant.pt.packets->empty ())
 	btrace_maint_update_pt_packets (btinfo);
 
       *begin = 0;
-      *end = VEC_length (btrace_pt_packet_s, btinfo->maint.variant.pt.packets);
+      *end = ((btinfo->maint.variant.pt.packets == nullptr)
+	      ? 0 : btinfo->maint.variant.pt.packets->size ());
       *from = btinfo->maint.variant.pt.packet_history.begin;
       *to = btinfo->maint.variant.pt.packet_history.end;
       break;
@@ -3145,23 +3150,21 @@  btrace_maint_print_packets (struct btrace_thread_info *btinfo,
 #if defined (HAVE_LIBIPT)
     case BTRACE_FORMAT_PT:
       {
-	VEC (btrace_pt_packet_s) *packets;
+	std::vector <btrace_pt_packet> *packets;
 	unsigned int pkt;
 
 	packets = btinfo->maint.variant.pt.packets;
 	for (pkt = begin; pkt < end; ++pkt)
 	  {
-	    const struct btrace_pt_packet *packet;
-
-	    packet = VEC_index (btrace_pt_packet_s, packets, pkt);
+	    const struct btrace_pt_packet &packet = packets->at (pkt);
 
 	    printf_unfiltered ("%u\t", pkt);
-	    printf_unfiltered ("0x%" PRIx64 "\t", packet->offset);
+	    printf_unfiltered ("0x%" PRIx64 "\t", packet.offset);
 
-	    if (packet->errcode == pte_ok)
-	      pt_print_packet (&packet->packet);
+	    if (packet.errcode == pte_ok)
+	      pt_print_packet (&packet.packet);
 	    else
-	      printf_unfiltered ("[error: %s]", pt_errstr (packet->errcode));
+	      printf_unfiltered ("[error: %s]", pt_errstr (packet.errcode));
 
 	    printf_unfiltered ("\n");
 	  }
@@ -3452,9 +3455,9 @@  maint_info_btrace_cmd (const char *args, int from_tty)
 			   version.ext != NULL ? version.ext : "");
 
 	btrace_maint_update_pt_packets (btinfo);
-	printf_unfiltered (_("Number of packets: %u.\n"),
-			   VEC_length (btrace_pt_packet_s,
-				       btinfo->maint.variant.pt.packets));
+	printf_unfiltered (_("Number of packets: %zu.\n"),
+			   ((btinfo->maint.variant.pt.packets == nullptr)
+			    ? 0 : btinfo->maint.variant.pt.packets->size ()));
       }
       break;
 #endif /* defined (HAVE_LIBIPT)  */
diff --git a/gdb/btrace.h b/gdb/btrace.h
index d09c424804b..208c089fa7c 100644
--- a/gdb/btrace.h
+++ b/gdb/btrace.h
@@ -29,7 +29,6 @@ 
 #include "gdbsupport/btrace-common.h"
 #include "target/waitstatus.h" /* For enum target_stop_reason.  */
 #include "gdbsupport/enum-flags.h"
-#include "gdbsupport/vec.h"
 
 #if defined (HAVE_LIBIPT)
 #  include <intel-pt.h>
@@ -265,9 +264,6 @@  struct btrace_pt_packet
   struct pt_packet packet;
 };
 
-/* Define functions operating on a vector of packets.  */
-typedef struct btrace_pt_packet btrace_pt_packet_s;
-DEF_VEC_O (btrace_pt_packet_s);
 #endif /* defined (HAVE_LIBIPT)  */
 
 /* Branch trace iteration state for "maintenance btrace packet-history".  */
@@ -301,7 +297,7 @@  struct btrace_maint_info
     struct
     {
       /* A vector of decoded packets.  */
-      VEC (btrace_pt_packet_s) *packets;
+      std::vector <btrace_pt_packet> *packets;
 
       /* The packet history iterator.
 	 We are iterating over the above PACKETS vector.  */