[PATCHv4,1/3] gdb: Remove a VEC from gdbsupport/btrace-common.h

Message ID 4773109a57a82515f1f734abf794e8574f07488b.1569497661.git.andrew.burgess@embecosm.com
State Superseded
Headers show
Series
  • Remove some uses of VEC
Related show

Commit Message

Andrew Burgess Sept. 26, 2019, 11:41 a.m.
Converts a VEC into a std::vector in gdbsupport/btrace-common.h.  This
commit just performs a mechanical conversion and doesn't do any
refactoring.  One consequence of this is that the std::vector must
actually be a pointer to std::vector as it is placed within a union.
It might be possible in future to refactor to a class hierarchy and
remove the need for a union, but I'd rather have that be a separate
change to make it easier to see the evolution of the code.

gdb/ChangeLog:

	* btrace.c (btrace_compute_ftrace_bts): Update for std::vector,
	make accesses into the vector constant references.
	(btrace_add_pc): Update for std::vector.
	(btrace_stitch_bts): Likewise.
	(parse_xml_btrace_block): Likewise.
	(btrace_maint_update_packets): Likewise.
	(btrace_maint_print_packets): Likewise.
	(maint_info_btrace_cmd): Likewise.
	* btrace.h: Add 'gdbsupport/vec.h' include.
	* gdbsupport/btrace-common.c (btrace_data::fini): Update for
	std::vector.
	(btrace_data::empty): Likewise.
	(btrace_data_append): Likewise.
	* gdbsupport/btrace-common.h: Remove 'vec.h' include and use of
	DEF_VEC_O.
	(typedef btrace_block_s): Delete.
	(struct btrace_block): Add constructor.
	(struct btrace_data_bts) <blocks>: Change to std::vector.
	(struct btrace_data_bts) <blocks_size>: New function.
	(struct btrace_data_bts) <blocks_empty>: New function.
	* nat/linux-btrace.c (perf_event_read_bts): Update for
	std::vector.
	(linux_read_bts): Likewise.

gdb/gdbserver/ChangeLog:

	* linux-low.c (linux_low_read_btrace): Update for change to
	std::vector.
---
 gdb/ChangeLog                  | 26 +++++++++++++++++++
 gdb/btrace.c                   | 58 +++++++++++++++++-------------------------
 gdb/btrace.h                   |  1 +
 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 ++++++------
 8 files changed, 103 insertions(+), 67 deletions(-)

-- 
2.14.5

Comments

Metzger, Markus T Sept. 26, 2019, 1:40 p.m. | #1
Hello Andrew,

> @@ -1059,7 +1059,7 @@ btrace_compute_ftrace_bts (struct thread_info *tp,

> 

>    gdbarch = target_gdbarch ();

>    btinfo = &tp->btrace;

> -  blk = VEC_length (btrace_block_s, btrace->blocks);

> +  blk = btrace->blocks_size ();

> 

>    if (btinfo->functions.empty ())

>      level = INT_MAX;


We should be able to guarantee that BTRACE->BLOCKS != nullptr.  We only get
here if the btrace format is set to BTRACE_FORMAT_BTS.  The blocks vector must
be allocated whenever we set the format to BTRACE_FORMAT_BTS.


> @@ -3128,19 +3119,17 @@ btrace_maint_print_packets (struct

> btrace_thread_info *btinfo,

> 

>      case BTRACE_FORMAT_BTS:

>        {

> -	VEC (btrace_block_s) *blocks;

> +	const std::vector <btrace_block> &blocks

> +	  = *btinfo->data.variant.bts.blocks;


Would that fit onto a single line?


> diff --git a/gdb/btrace.h b/gdb/btrace.h

> index ba8d27c879c..d09c424804b 100644

> --- a/gdb/btrace.h

> +++ b/gdb/btrace.h

> @@ -29,6 +29,7 @@

>  #include "gdbsupport/btrace-common.h"

>  #include "target/waitstatus.h" /* For enum target_stop_reason.  */

>  #include "gdbsupport/enum-flags.h"

> +#include "gdbsupport/vec.h"


Didn't you want to remove those includes?


> @@ -141,15 +142,12 @@ btrace_data_append (struct btrace_data *dst,

> 

>  	    /* We copy blocks in reverse order to have the oldest block at

>  	       index zero.  */

> -	    blk = VEC_length (btrace_block_s, src->variant.bts.blocks);

> +	    blk = src->variant.bts.blocks_size ();

>  	    while (blk != 0)

>  	      {

> -		btrace_block_s *block;

> -

> -		block = VEC_index (btrace_block_s, src->variant.bts.blocks,

> -				   --blk);

> -

> -		VEC_safe_push (btrace_block_s, dst->variant.bts.blocks, block);

> +		const btrace_block &block

> +		  = src->variant.bts.blocks->at (--blk);


Would this fit onto one line?

 
> @@ -137,8 +139,28 @@ struct btrace_config

>  struct btrace_data_bts

>  {

>    /* Branch trace is represented as a vector of branch trace blocks starting

> -     with the most recent block.  */

> -  VEC (btrace_block_s) *blocks;

> +     with the most recent block.  This needs to be a pointer as we place

> +     btrace_data_bts into a union.  */

> +  std::vector <btrace_block> *blocks;

> +

> +  /* The BLOCKS vector used to be implemented using a C vector library.  A

> +     property of this library was that the size and empty functions could

> +     be called with a nullptr (to the vector) and still have a sensible

> +     answer return.

> +

> +     When converting to C++ std::vector this method was added here to avoid

> +     introducing errors if we ever did try to call size or empty on a

> +     nullptr.  */

> +  size_t blocks_size () const

> +  {

> +    return (blocks == nullptr) ? 0 : blocks->size ();

> +  }

> +

> +  /* See the comment for BLOCKS_SIZE above.  */

> +  bool blocks_empty () const

> +  {

> +    return (blocks == nullptr) ? true : blocks->empty ();

> +  }


These helpers shouldn't be necessary.  If the format is set to BTRACE_FORMAT_BTS,
the blocks vector must have been allocated.  And it must be freed when the format
is set to any other value - most likely BTRACE_FORMAT_NONE.

Same for BTRACE_FORMAT_PT.  Without trace, the format should be left at
BTRACE_FORMAT_NONE.


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

Patch

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 1b809fb5c08..006cd740554 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1059,7 +1059,7 @@  btrace_compute_ftrace_bts (struct thread_info *tp,
 
   gdbarch = target_gdbarch ();
   btinfo = &tp->btrace;
-  blk = VEC_length (btrace_block_s, btrace->blocks);
+  blk = btrace->blocks_size ();
 
   if (btinfo->functions.empty ())
     level = INT_MAX;
@@ -1068,13 +1068,12 @@  btrace_compute_ftrace_bts (struct thread_info *tp,
 
   while (blk != 0)
     {
-      btrace_block_s *block;
       CORE_ADDR pc;
 
       blk -= 1;
 
-      block = VEC_index (btrace_block_s, btrace->blocks, blk);
-      pc = block->begin;
+      const btrace_block &block = btrace->blocks->at (blk);
+      pc = block.begin;
 
       for (;;)
 	{
@@ -1083,7 +1082,7 @@  btrace_compute_ftrace_bts (struct thread_info *tp,
 	  int size;
 
 	  /* We should hit the end of the block.  Warn if we went too far.  */
-	  if (block->end < pc)
+	  if (block.end < pc)
 	    {
 	      /* Indicate the gap in the trace.  */
 	      bfun = ftrace_new_gap (btinfo, BDE_BTS_OVERFLOW, gaps);
@@ -1119,7 +1118,7 @@  btrace_compute_ftrace_bts (struct thread_info *tp,
 	  ftrace_update_insns (bfun, insn);
 
 	  /* We're done once we pushed the instruction at the end.  */
-	  if (block->end == pc)
+	  if (block.end == pc)
 	    break;
 
 	  /* We can't continue if we fail to compute the size.  */
@@ -1573,7 +1572,6 @@  static void
 btrace_add_pc (struct thread_info *tp)
 {
   struct btrace_data btrace;
-  struct btrace_block *block;
   struct regcache *regcache;
   CORE_ADDR pc;
 
@@ -1581,11 +1579,9 @@  btrace_add_pc (struct thread_info *tp)
   pc = regcache_read_pc (regcache);
 
   btrace.format = BTRACE_FORMAT_BTS;
-  btrace.variant.bts.blocks = NULL;
+  btrace.variant.bts.blocks = new std::vector <btrace_block>;
 
-  block = VEC_safe_push (btrace_block_s, btrace.variant.bts.blocks, NULL);
-  block->begin = pc;
-  block->end = pc;
+  btrace.variant.bts.blocks->emplace_back (pc, pc);
 
   btrace_compute_ftrace (tp, &btrace, NULL);
 }
@@ -1692,11 +1688,11 @@  btrace_stitch_bts (struct btrace_data_bts *btrace, struct thread_info *tp)
 {
   struct btrace_thread_info *btinfo;
   struct btrace_function *last_bfun;
-  btrace_block_s *first_new_block;
+  btrace_block *first_new_block;
 
   btinfo = &tp->btrace;
   gdb_assert (!btinfo->functions.empty ());
-  gdb_assert (!VEC_empty (btrace_block_s, btrace->blocks));
+  gdb_assert (!btrace->blocks_empty ());
 
   last_bfun = &btinfo->functions.back ();
 
@@ -1705,14 +1701,14 @@  btrace_stitch_bts (struct btrace_data_bts *btrace, struct thread_info *tp)
      of the new trace,  though, since we can't fill in the start address.*/
   if (last_bfun->insn.empty ())
     {
-      VEC_pop (btrace_block_s, btrace->blocks);
+      btrace->blocks->pop_back ();
       return 0;
     }
 
   /* Beware that block trace starts with the most recent block, so the
      chronologically first block in the new trace is the last block in
      the new trace's block vector.  */
-  first_new_block = VEC_last (btrace_block_s, btrace->blocks);
+  first_new_block = &btrace->blocks->back ();
   const btrace_insn &last_insn = last_bfun->insn.back ();
 
   /* If the current PC at the end of the block is the same as in our current
@@ -1723,10 +1719,9 @@  btrace_stitch_bts (struct btrace_data_bts *btrace, struct thread_info *tp)
      entries.
      In the second case, the delta trace vector should contain exactly one
      entry for the partial block containing the current PC.  Remove it.  */
-  if (first_new_block->end == last_insn.pc
-      && VEC_length (btrace_block_s, btrace->blocks) == 1)
+  if (first_new_block->end == last_insn.pc && btrace->blocks_size () == 1)
     {
-      VEC_pop (btrace_block_s, btrace->blocks);
+      btrace->blocks->pop_back ();
       return 0;
     }
 
@@ -2030,7 +2025,6 @@  parse_xml_btrace_block (struct gdb_xml_parser *parser,
 			std::vector<gdb_xml_value> &attributes)
 {
   struct btrace_data *btrace;
-  struct btrace_block *block;
   ULONGEST *begin, *end;
 
   btrace = (struct btrace_data *) user_data;
@@ -2042,7 +2036,7 @@  parse_xml_btrace_block (struct gdb_xml_parser *parser,
 
     case BTRACE_FORMAT_NONE:
       btrace->format = BTRACE_FORMAT_BTS;
-      btrace->variant.bts.blocks = NULL;
+      btrace->variant.bts.blocks = new std::vector <btrace_block>;
       break;
 
     default:
@@ -2051,10 +2045,7 @@  parse_xml_btrace_block (struct gdb_xml_parser *parser,
 
   begin = (ULONGEST *) xml_find_attribute (attributes, "begin")->value.get ();
   end = (ULONGEST *) xml_find_attribute (attributes, "end")->value.get ();
-
-  block = VEC_safe_push (btrace_block_s, btrace->variant.bts.blocks, NULL);
-  block->begin = *begin;
-  block->end = *end;
+  btrace->variant.bts.blocks->emplace_back (*begin, *end);
 }
 
 /* Parse a "raw" xml record.  */
@@ -3095,7 +3086,7 @@  btrace_maint_update_packets (struct btrace_thread_info *btinfo,
     case BTRACE_FORMAT_BTS:
       /* Nothing to do - we operate directly on BTINFO->DATA.  */
       *begin = 0;
-      *end = VEC_length (btrace_block_s, btinfo->data.variant.bts.blocks);
+      *end = btinfo->data.variant.bts.blocks_size ();
       *from = btinfo->maint.variant.bts.packet_history.begin;
       *to = btinfo->maint.variant.bts.packet_history.end;
       break;
@@ -3128,19 +3119,17 @@  btrace_maint_print_packets (struct btrace_thread_info *btinfo,
 
     case BTRACE_FORMAT_BTS:
       {
-	VEC (btrace_block_s) *blocks;
+	const std::vector <btrace_block> &blocks
+	  = *btinfo->data.variant.bts.blocks;
 	unsigned int blk;
 
-	blocks = btinfo->data.variant.bts.blocks;
 	for (blk = begin; blk < end; ++blk)
 	  {
-	    const btrace_block_s *block;
-
-	    block = VEC_index (btrace_block_s, blocks, blk);
+	    const btrace_block &block = blocks.at (blk);
 
 	    printf_unfiltered ("%u\tbegin: %s, end: %s\n", blk,
-			       core_addr_to_string_nz (block->begin),
-			       core_addr_to_string_nz (block->end));
+			       core_addr_to_string_nz (block.begin),
+			       core_addr_to_string_nz (block.end));
 	  }
 
 	btinfo->maint.variant.bts.packet_history.begin = begin;
@@ -3443,9 +3432,8 @@  maint_info_btrace_cmd (const char *args, int from_tty)
       break;
 
     case BTRACE_FORMAT_BTS:
-      printf_unfiltered (_("Number of packets: %u.\n"),
-			 VEC_length (btrace_block_s,
-				     btinfo->data.variant.bts.blocks));
+      printf_unfiltered (_("Number of packets: %zu.\n"),
+			 btinfo->data.variant.bts.blocks_size ());
       break;
 
 #if defined (HAVE_LIBIPT)
diff --git a/gdb/btrace.h b/gdb/btrace.h
index ba8d27c879c..d09c424804b 100644
--- a/gdb/btrace.h
+++ b/gdb/btrace.h
@@ -29,6 +29,7 @@ 
 #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>
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index d64c3641ffb..0e4b14e3655 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -7115,9 +7115,7 @@  linux_low_read_btrace (struct btrace_target_info *tinfo, struct buffer *buffer,
 		       enum btrace_read_type type)
 {
   struct btrace_data btrace;
-  struct btrace_block *block;
   enum btrace_error err;
-  int i;
 
   err = linux_read_btrace (&btrace, tinfo, type);
   if (err != BTRACE_ERR_NONE)
@@ -7140,11 +7138,9 @@  linux_low_read_btrace (struct btrace_target_info *tinfo, struct buffer *buffer,
       buffer_grow_str (buffer, "<!DOCTYPE btrace SYSTEM \"btrace.dtd\">\n");
       buffer_grow_str (buffer, "<btrace version=\"1.0\">\n");
 
-      for (i = 0;
-	   VEC_iterate (btrace_block_s, btrace.variant.bts.blocks, i, block);
-	   i++)
+      for (const btrace_block &block : *btrace.variant.bts.blocks)
 	buffer_xml_printf (buffer, "<block begin=\"0x%s\" end=\"0x%s\"/>\n",
-			   paddress (block->begin), paddress (block->end));
+			   paddress (block.begin), paddress (block.end));
 
       buffer_grow_str0 (buffer, "</btrace>\n");
       break;
diff --git a/gdb/gdbsupport/btrace-common.c b/gdb/gdbsupport/btrace-common.c
index 13f1f1a0fdd..d6e41173063 100644
--- a/gdb/gdbsupport/btrace-common.c
+++ b/gdb/gdbsupport/btrace-common.c
@@ -73,7 +73,8 @@  btrace_data::fini ()
       return;
 
     case BTRACE_FORMAT_BTS:
-      VEC_free (btrace_block_s, variant.bts.blocks);
+      delete variant.bts.blocks;
+      variant.bts.blocks = nullptr;
       return;
 
     case BTRACE_FORMAT_PT:
@@ -95,7 +96,7 @@  btrace_data::empty () const
       return true;
 
     case BTRACE_FORMAT_BTS:
-      return VEC_empty (btrace_block_s, variant.bts.blocks);
+      return variant.bts.blocks_empty ();
 
     case BTRACE_FORMAT_PT:
       return (variant.pt.size == 0);
@@ -132,7 +133,7 @@  btrace_data_append (struct btrace_data *dst,
 
 	case BTRACE_FORMAT_NONE:
 	  dst->format = BTRACE_FORMAT_BTS;
-	  dst->variant.bts.blocks = NULL;
+	  dst->variant.bts.blocks = new std::vector <btrace_block>;
 
 	  /* Fall-through.  */
 	case BTRACE_FORMAT_BTS:
@@ -141,15 +142,12 @@  btrace_data_append (struct btrace_data *dst,
 
 	    /* We copy blocks in reverse order to have the oldest block at
 	       index zero.  */
-	    blk = VEC_length (btrace_block_s, src->variant.bts.blocks);
+	    blk = src->variant.bts.blocks_size ();
 	    while (blk != 0)
 	      {
-		btrace_block_s *block;
-
-		block = VEC_index (btrace_block_s, src->variant.bts.blocks,
-				   --blk);
-
-		VEC_safe_push (btrace_block_s, dst->variant.bts.blocks, block);
+		const btrace_block &block
+		  = src->variant.bts.blocks->at (--blk);
+		dst->variant.bts.blocks->push_back (block);
 	      }
 	  }
 	}
diff --git a/gdb/gdbsupport/btrace-common.h b/gdb/gdbsupport/btrace-common.h
index 0b18924882c..3eb065ed03d 100644
--- a/gdb/gdbsupport/btrace-common.h
+++ b/gdb/gdbsupport/btrace-common.h
@@ -26,8 +26,6 @@ 
    inferior.  For presentation purposes, the branch trace is represented as a
    list of sequential control-flow blocks, one such list per thread.  */
 
-#include "vec.h"
-
 /* A branch trace block.
 
    This represents a block of sequential control-flow.  Adjacent blocks will be
@@ -43,11 +41,15 @@  struct btrace_block
 
   /* The address of the first byte of the last instruction in the block.  */
   CORE_ADDR end;
-};
 
-/* Define functions operating on a vector of branch trace blocks.  */
-typedef struct btrace_block btrace_block_s;
-DEF_VEC_O (btrace_block_s);
+  /* Simple constructor.  */
+  btrace_block (CORE_ADDR begin, CORE_ADDR end)
+    : begin (begin),
+      end (end)
+  {
+    /* Nothing.  */
+  }
+};
 
 /* Enumeration of btrace formats.  */
 
@@ -137,8 +139,28 @@  struct btrace_config
 struct btrace_data_bts
 {
   /* Branch trace is represented as a vector of branch trace blocks starting
-     with the most recent block.  */
-  VEC (btrace_block_s) *blocks;
+     with the most recent block.  This needs to be a pointer as we place
+     btrace_data_bts into a union.  */
+  std::vector <btrace_block> *blocks;
+
+  /* The BLOCKS vector used to be implemented using a C vector library.  A
+     property of this library was that the size and empty functions could
+     be called with a nullptr (to the vector) and still have a sensible
+     answer return.
+
+     When converting to C++ std::vector this method was added here to avoid
+     introducing errors if we ever did try to call size or empty on a
+     nullptr.  */
+  size_t blocks_size () const
+  {
+    return (blocks == nullptr) ? 0 : blocks->size ();
+  }
+
+  /* See the comment for BLOCKS_SIZE above.  */
+  bool blocks_empty () const
+  {
+    return (blocks == nullptr) ? true : blocks->empty ();
+  }
 };
 
 /* Configuration information to go with the trace data.  */
diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index 8625291cce9..4fc68dca801 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -271,11 +271,11 @@  perf_event_sample_ok (const struct perf_event_sample *sample)
    In case the buffer overflows during sampling, one sample may have its lower
    part at the end and its upper part at the beginning of the buffer.  */
 
-static VEC (btrace_block_s) *
+static std::vector <btrace_block> *
 perf_event_read_bts (struct btrace_target_info* tinfo, const uint8_t *begin,
 		     const uint8_t *end, const uint8_t *start, size_t size)
 {
-  VEC (btrace_block_s) *btrace = NULL;
+  std::vector <btrace_block> *btrace = new std::vector <btrace_block>;
   struct perf_event_sample sample;
   size_t read = 0;
   struct btrace_block block = { 0, 0 };
@@ -343,7 +343,7 @@  perf_event_read_bts (struct btrace_target_info* tinfo, const uint8_t *begin,
       /* We found a valid sample, so we can complete the current block.  */
       block.begin = psample->bts.to;
 
-      VEC_safe_push (btrace_block_s, btrace, &block);
+      btrace->push_back (block);
 
       /* Start the next block.  */
       block.end = psample->bts.from;
@@ -354,7 +354,7 @@  perf_event_read_bts (struct btrace_target_info* tinfo, const uint8_t *begin,
      reading delta trace, we can fill in the start address later on.
      Otherwise we will prune it.  */
   block.begin = 0;
-  VEC_safe_push (btrace_block_s, btrace, &block);
+  btrace->push_back (block);
 
   return btrace;
 }
@@ -785,7 +785,8 @@  linux_read_bts (struct btrace_data_bts *btrace,
       data_head = *pevent->data_head;
 
       /* Delete any leftover trace from the previous iteration.  */
-      VEC_free (btrace_block_s, btrace->blocks);
+      delete btrace->blocks;
+      btrace->blocks = nullptr;
 
       if (type == BTRACE_READ_DELTA)
 	{
@@ -843,9 +844,8 @@  linux_read_bts (struct btrace_data_bts *btrace,
   /* Prune the incomplete last block (i.e. the first one of inferior execution)
      if we're not doing a delta read.  There is no way of filling in its zeroed
      BEGIN element.  */
-  if (!VEC_empty (btrace_block_s, btrace->blocks)
-      && type != BTRACE_READ_DELTA)
-    VEC_pop (btrace_block_s, btrace->blocks);
+  if (!btrace->blocks_empty () && type != BTRACE_READ_DELTA)
+    btrace->blocks->pop_back ();
 
   return BTRACE_ERR_NONE;
 }