[PATCH 1/2] Make parse_static_tracepoint_marker_definition work with multiple static tracepoint definitions

[PATCH 1/2] Make parse_static_tracepoint_marker_definition work with multiple static tracepoint definitions

[Simon Marchi]

Since I modify the parse_static_tracepoint_marker_definition function in
the next patch, I wanted to write a unit test for it.  Doing so showed
that it doesn't handle multiple consecutive static tracepoint
definitions separated by commas.  However, the RSP documentation [1]
states that servers may return multiple definitions, like:

  1234:6d61726b657231:6578747261207374756666,abba:6d61726b657232:

The problem is that the function uses strlen to compute the length of
the last field (the extra field).  If there are additional definitions
in addition to the one we are currently parsing, the returned length
will include those definitions, and we'll try to hex-decode past the
extra field.

This patch changes parse_static_tracepoint_marker_definition to consider
the case where the current definition is followed by a comma and more
definitions.  It also adds the unit test that found the issue in the
first place.

I don't think this causes any backwards compatibility issues, because
the previous code only handled single static tracepoint definitions, and
the new code handles that correctly.

gdb/ChangeLog:

	* tracepoint.c (parse_static_tracepoint_marker_definition):
	Consider case where the definition is followed by more
	definitions.
	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
	tracepoint-selftests.c.
	* unittests/tracepoint-selftests.c: New.

[1] https://sourceware.org/gdb/onlinedocs/gdb/Tracepoint-Packets.html#qTfSTM
---
 gdb/Makefile.in                      |  1 +
 gdb/tracepoint.c                     | 14 ++++++--
 gdb/unittests/tracepoint-selftests.c | 70 ++++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 3 deletions(-)
 create mode 100644 gdb/unittests/tracepoint-selftests.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 957654c9bd..5a2f4c8cac 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -427,6 +427,7 @@ SUBDIR_UNITTESTS_SRCS = \
 	unittests/scoped_fd-selftests.c \
 	unittests/scoped_mmap-selftests.c \
 	unittests/scoped_restore-selftests.c \
+	unittests/tracepoint-selftests.c \
 	unittests/xml-utils-selftests.c
 
 SUBDIR_UNITTESTS_OBS = $(patsubst %.c,%.o,$(SUBDIR_UNITTESTS_SRCS))
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index f34f103875..843041f739 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -3720,12 +3720,20 @@ parse_static_tracepoint_marker_definition (const char *line, const char **pp,
   p += 2 * end;
   p++;  /* skip a colon */
 
-  marker->extra = (char *) xmalloc (strlen (p) + 1);
-  end = hex2bin (p, (gdb_byte *) marker->extra, strlen (p) / 2);
+  /* This definition may be followed by another one, separated by a comma.  */
+  int hex_len;
+  endp = strchr (p, ',');
+  if (endp != nullptr)
+    hex_len = endp - p;
+  else
+    hex_len = strlen (p);
+
+  marker->extra = (char *) xmalloc (hex_len / 2 + 1);
+  end = hex2bin (p, (gdb_byte *) marker->extra, hex_len / 2);
   marker->extra[end] = '\0';
 
   if (pp)
-    *pp = p;
+    *pp = p + hex_len;
 }
 
 /* Release a static tracepoint marker's contents.  Note that the
diff --git a/gdb/unittests/tracepoint-selftests.c b/gdb/unittests/tracepoint-selftests.c
new file mode 100644
index 0000000000..21ca3d0914
--- /dev/null
+++ b/gdb/unittests/tracepoint-selftests.c
@@ -0,0 +1,70 @@
+/* Self tests for tracepoint-related code for GDB, the GNU debugger.
+
+   Copyright (C) 2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "selftest.h"
+#include "tracepoint.h"
+
+namespace selftests {
+namespace tracepoint_tests {
+
+static void
+test_parse_static_tracepoint_marker_definition ()
+{
+  static_tracepoint_marker marker;
+  const char def[] = ("1234:6d61726b657231:6578747261207374756666,"
+		      "abba:6d61726b657232:,"
+		      "cafe:6d61726b657233:6d6f72657374756666");
+  const char *start = def;
+  const char *end;
+
+  parse_static_tracepoint_marker_definition (start, &end, &marker);
+
+  SELF_CHECK (marker.address == 0x1234);
+  SELF_CHECK (strcmp (marker.str_id, "marker1") == 0);
+  SELF_CHECK (strcmp (marker.extra, "extra stuff") == 0);
+  SELF_CHECK (end == strchr (start, ','));
+
+  start = end + 1;
+  parse_static_tracepoint_marker_definition (start, &end, &marker);
+
+  SELF_CHECK (marker.address == 0xabba);
+  SELF_CHECK (strcmp (marker.str_id, "marker2") == 0);
+  SELF_CHECK (strcmp (marker.extra, "") == 0);
+  SELF_CHECK (end == strchr (start, ','));
+
+  start = end + 1;
+  parse_static_tracepoint_marker_definition (start, &end, &marker);
+
+  SELF_CHECK (marker.address == 0xcafe);
+  SELF_CHECK (strcmp (marker.str_id, "marker3") == 0);
+  SELF_CHECK (strcmp (marker.extra, "morestuff") == 0);
+  SELF_CHECK (end == def + strlen (def));
+}
+
+} /* namespace tracepoint_tests */
+} /* namespace selftests */
+
+void
+_initialize_tracepoint_selftests ()
+{
+  selftests::register_test
+    ("parse_static_tracepoint_marker_definition",
+     selftests::tracepoint_tests::test_parse_static_tracepoint_marker_definition);
+}
-- 
2.16.1

[PATCH 2/2] Get rid of VEC(static_tracepoint_marker_p)

[Simon Marchi]

This patch replaces VEC(static_tracepoint_marker_p) with std::vector,
and does some c++ification around that.  I thought a new overload of
hex2str was useful, so I added it as well as corresponding unit tests.
I also added an overload of ui_out::field_string that takes an
std::string directly.

gdb/ChangeLog:

	* tracepoint.h (struct static_tracepoint_marker): Initialize
	fields, define default constructor, move constructor and move
	assignment, disable the rest.
	<str_id, extra>: Make std::string.
	(release_static_tracepoint_marker): Remove.
	(free_current_marker): Remove.
	* tracepoint.c (free_current_marker): Remove.
	(parse_static_tracepoint_marker_definition): Adjust to
	std::string, use new hex2str overload.
	(release_static_tracepoint_marker): Remove.
	(print_one_static_tracepoint_marker): Get marker by reference
	and adjust to std::string.
	(info_static_tracepoint_markers_command): Adjust to std::vector
	changes
	* target.h (static_tracepoint_marker_p): Remove typedef.
	(DEF_VEC_P(static_tracepoint_marker_p)): Remove.
	(struct target_ops) <to_static_tracepoint_marker_at>: Return
	bool.
	<to_static_tracepoint_markers_by_strid>: Return std::vector.
	* target-debug.h
	(target_debug_print_VEC_static_tracepoint_marker_p_p): Remove.
	(target_debug_print_std_vector_static_tracepoint_marker): New.
	(target_debug_print_struct_static_tracepoint_marker_p): Rename
	to...
	(target_debug_print_static_tracepoint_marker_p): ... this.
	* target-delegates.c: Re-generate.
	* breakpoint.h (struct tracepoint) <static_trace_marker_id>:
	Make std::string.
	* breakpoint.c (init_breakpoint_sal): Adjust to std::string.
	(decode_static_tracepoint_spec): Adjust to std::vector.
	(tracepoint_print_one_detail): Adjust to std::string.
	(strace_marker_decode_location): Adjust to std::string.
	(update_static_tracepoint): Adjust to std::string, remove call
	to release_static_tracepoint_marker.
	* linux-nat.c (linux_child_static_tracepoint_markers_by_strid):
	Adjust to std::vector.
	* remote.c (remote_static_tracepoint_marker_at): Return bool.
	(remote_static_tracepoint_markers_by_strid): Adjust to
	std::vector.
	* ui-out.h (class ui_out) <field_string>: New overload with
	std::string.
	* common/rsp-low.h (hex2str): New overload with explicit count
	of bytes.
	* common/rsp-low.c (hex2str): New overload with explicit count
	of bytes.
	* unittests/rsp-low-selftests.c (test_hex2str): New function.
	(_initialize_rsp_low_selftests): Add test_hex2str test.
	* unittests/tracepoint-selftests.c
	(test_parse_static_tracepoint_marker_definition): Adjust to
	std::string.
---
 gdb/breakpoint.c                     | 68 ++++++++++++------------------
 gdb/breakpoint.h                     |  2 +-
 gdb/common/rsp-low.c                 | 13 ++++--
 gdb/common/rsp-low.h                 |  4 ++
 gdb/linux-nat.c                      | 27 +++---------
 gdb/remote.c                         | 34 ++++-----------
 gdb/target-debug.h                   |  6 +--
 gdb/target-delegates.c               | 30 ++++++-------
 gdb/target.h                         | 15 +++----
 gdb/tracepoint.c                     | 82 +++++++++---------------------------
 gdb/tracepoint.h                     | 22 ++++++----
 gdb/ui-out.h                         |  2 +
 gdb/unittests/rsp-low-selftests.c    | 12 ++++++
 gdb/unittests/tracepoint-selftests.c | 12 +++---
 14 files changed, 134 insertions(+), 195 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 91ecca62fc..80f96886e4 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -8923,27 +8923,24 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 		  const char *p
 		    = &event_location_to_string (b->location.get ())[3];
 		  const char *endp;
-		  char *marker_str;
 
 		  p = skip_spaces (p);
 
 		  endp = skip_to_space (p);
 
-		  marker_str = savestring (p, endp - p);
-		  t->static_trace_marker_id = marker_str;
+		  t->static_trace_marker_id.assign (p, endp - p);
 
 		  printf_filtered (_("Probed static tracepoint "
 				     "marker \"%s\"\n"),
-				   t->static_trace_marker_id);
+				   t->static_trace_marker_id.c_str ());
 		}
 	      else if (target_static_tracepoint_marker_at (sal.pc, &marker))
 		{
-		  t->static_trace_marker_id = xstrdup (marker.str_id);
-		  release_static_tracepoint_marker (&marker);
+		  t->static_trace_marker_id = std::move (marker.str_id);
 
 		  printf_filtered (_("Probed static tracepoint "
 				     "marker \"%s\"\n"),
-				   t->static_trace_marker_id);
+				   t->static_trace_marker_id.c_str ());
 		}
 	      else
 		warning (_("Couldn't determine the static "
@@ -9283,10 +9280,8 @@ find_condition_and_thread (const char *tok, CORE_ADDR pc,
 static std::vector<symtab_and_line>
 decode_static_tracepoint_spec (const char **arg_p)
 {
-  VEC(static_tracepoint_marker_p) *markers = NULL;
   const char *p = &(*arg_p)[3];
   const char *endp;
-  int i;
 
   p = skip_spaces (p);
 
@@ -9294,26 +9289,21 @@ decode_static_tracepoint_spec (const char **arg_p)
 
   std::string marker_str (p, endp - p);
 
-  markers = target_static_tracepoint_markers_by_strid (marker_str.c_str ());
-  if (VEC_empty(static_tracepoint_marker_p, markers))
+  std::vector<static_tracepoint_marker> markers
+    = target_static_tracepoint_markers_by_strid (marker_str.c_str ());
+  if (markers.empty ())
     error (_("No known static tracepoint marker named %s"),
 	   marker_str.c_str ());
 
   std::vector<symtab_and_line> sals;
-  sals.reserve (VEC_length(static_tracepoint_marker_p, markers));
+  sals.reserve (markers.size ());
 
-  for (i = 0; i < VEC_length(static_tracepoint_marker_p, markers); i++)
+  for (const static_tracepoint_marker &marker : markers)
     {
-      struct static_tracepoint_marker *marker;
-
-      marker = VEC_index (static_tracepoint_marker_p, markers, i);
-
-      symtab_and_line sal = find_pc_line (marker->address, 0);
-      sal.pc = marker->address;
+      symtab_and_line sal = find_pc_line (marker.address, 0);
+      sal.pc = marker.address;
       sals.push_back (sal);
-
-      release_static_tracepoint_marker (marker);
-    }
+   }
 
   *arg_p = endp;
   return sals;
@@ -12924,7 +12914,7 @@ tracepoint_print_one_detail (const struct breakpoint *self,
 			     struct ui_out *uiout)
 {
   struct tracepoint *tp = (struct tracepoint *) self;
-  if (tp->static_trace_marker_id)
+  if (!tp->static_trace_marker_id.empty ())
     {
       gdb_assert (self->type == bp_static_tracepoint);
 
@@ -13208,7 +13198,7 @@ strace_marker_decode_location (struct breakpoint *b,
       return sals;
     }
   else
-    error (_("marker %s not found"), tp->static_trace_marker_id);
+    error (_("marker %s not found"), tp->static_trace_marker_id.c_str ());
 }
 
 static struct breakpoint_ops strace_marker_breakpoint_ops;
@@ -13489,14 +13479,12 @@ update_static_tracepoint (struct breakpoint *b, struct symtab_and_line sal)
 
   if (target_static_tracepoint_marker_at (pc, &marker))
     {
-      if (strcmp (tp->static_trace_marker_id, marker.str_id) != 0)
+      if (tp->static_trace_marker_id != marker.str_id)
 	warning (_("static tracepoint %d changed probed marker from %s to %s"),
-		 b->number,
-		 tp->static_trace_marker_id, marker.str_id);
+		 b->number, tp->static_trace_marker_id.c_str (),
+		 marker.str_id.c_str ());
 
-      xfree (tp->static_trace_marker_id);
-      tp->static_trace_marker_id = xstrdup (marker.str_id);
-      release_static_tracepoint_marker (&marker);
+      tp->static_trace_marker_id = std::move (marker.str_id);
 
       return sal;
     }
@@ -13506,28 +13494,26 @@ update_static_tracepoint (struct breakpoint *b, struct symtab_and_line sal)
   if (!sal.explicit_pc
       && sal.line != 0
       && sal.symtab != NULL
-      && tp->static_trace_marker_id != NULL)
+      && !tp->static_trace_marker_id.empty ())
     {
-      VEC(static_tracepoint_marker_p) *markers;
-
-      markers
-	= target_static_tracepoint_markers_by_strid (tp->static_trace_marker_id);
+      std::vector<static_tracepoint_marker> markers
+	= target_static_tracepoint_markers_by_strid
+	    (tp->static_trace_marker_id.c_str ());
 
-      if (!VEC_empty(static_tracepoint_marker_p, markers))
+      if (!markers.empty ())
 	{
 	  struct symbol *sym;
 	  struct static_tracepoint_marker *tpmarker;
 	  struct ui_out *uiout = current_uiout;
 	  struct explicit_location explicit_loc;
 
-	  tpmarker = VEC_index (static_tracepoint_marker_p, markers, 0);
+	  tpmarker = &markers[0];
 
-	  xfree (tp->static_trace_marker_id);
-	  tp->static_trace_marker_id = xstrdup (tpmarker->str_id);
+	  tp->static_trace_marker_id = std::move (tpmarker->str_id);
 
 	  warning (_("marker for static tracepoint %d (%s) not "
 		     "found at previous line number"),
-		   b->number, tp->static_trace_marker_id);
+		   b->number, tp->static_trace_marker_id.c_str ());
 
 	  symtab_and_line sal2 = find_pc_line (tpmarker->address, 0);
 	  sym = find_pc_sect_function (tpmarker->address, NULL);
@@ -13564,8 +13550,6 @@ update_static_tracepoint (struct breakpoint *b, struct symtab_and_line sal)
 
 	  /* Might be nice to check if function changed, and warn if
 	     so.  */
-
-	  release_static_tracepoint_marker (tpmarker);
 	}
     }
   return sal;
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 8bb81d8d17..d8e2b73edc 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -889,7 +889,7 @@ struct tracepoint : public breakpoint
   ULONGEST traceframe_usage;
 
   /* The static tracepoint marker id, if known.  */
-  char *static_trace_marker_id;
+  std::string static_trace_marker_id;
 
   /* LTTng/UST allow more than one marker with the same ID string,
      although it unadvised because it confuses tools.  When setting
diff --git a/gdb/common/rsp-low.c b/gdb/common/rsp-low.c
index 10ad9d872e..79860637ca 100644
--- a/gdb/common/rsp-low.c
+++ b/gdb/common/rsp-low.c
@@ -147,12 +147,19 @@ hex2bin (const char *hex)
 
 std::string
 hex2str (const char *hex)
+{
+  return hex2str (hex, strlen (hex));
+}
+
+/* See rsp-low.h.  */
+
+std::string
+hex2str (const char *hex, int count)
 {
   std::string ret;
-  size_t len = strlen (hex);
 
-  ret.reserve (len / 2);
-  for (size_t i = 0; i < len; ++i)
+  ret.reserve (count);
+  for (size_t i = 0; i < count; ++i)
     {
       if (hex[0] == '\0' || hex[1] == '\0')
 	{
diff --git a/gdb/common/rsp-low.h b/gdb/common/rsp-low.h
index 068eabc196..f01db3736e 100644
--- a/gdb/common/rsp-low.h
+++ b/gdb/common/rsp-low.h
@@ -62,6 +62,10 @@ gdb::byte_vector hex2bin (const char *hex);
 
 extern std::string hex2str (const char *hex);
 
+/* Like hex2bin, but return a std::string.  */
+
+extern std::string hex2str (const char *hex, int count);
+
 /* Convert some bytes to a hexadecimal representation.  BIN holds the
    bytes to convert.  COUNT says how many bytes to convert.  The
    resulting characters are stored in HEX, followed by a NUL
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index cc930c6334..d673dbe7c1 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4291,17 +4291,17 @@ cleanup_target_stop (void *arg)
   target_continue_no_signal (*ptid);
 }
 
-static VEC(static_tracepoint_marker_p) *
+static std::vector<static_tracepoint_marker>
 linux_child_static_tracepoint_markers_by_strid (struct target_ops *self,
 						const char *strid)
 {
   char s[IPA_CMD_BUF_SIZE];
   struct cleanup *old_chain;
   int pid = ptid_get_pid (inferior_ptid);
-  VEC(static_tracepoint_marker_p) *markers = NULL;
-  struct static_tracepoint_marker *marker = NULL;
+  std::vector<static_tracepoint_marker> markers;
   const char *p = s;
   ptid_t ptid = ptid_build (pid, 0, 0);
+  static_tracepoint_marker marker;
 
   /* Pause all */
   target_stop (ptid);
@@ -4311,29 +4311,16 @@ linux_child_static_tracepoint_markers_by_strid (struct target_ops *self,
 
   agent_run_command (pid, s, strlen (s) + 1);
 
-  old_chain = make_cleanup (free_current_marker, &marker);
-  make_cleanup (cleanup_target_stop, &ptid);
+  old_chain = make_cleanup (cleanup_target_stop, &ptid);
 
   while (*p++ == 'm')
     {
-      if (marker == NULL)
-	marker = XCNEW (struct static_tracepoint_marker);
-
       do
 	{
-	  parse_static_tracepoint_marker_definition (p, &p, marker);
+	  parse_static_tracepoint_marker_definition (p, &p, &marker);
 
-	  if (strid == NULL || strcmp (strid, marker->str_id) == 0)
-	    {
-	      VEC_safe_push (static_tracepoint_marker_p,
-			     markers, marker);
-	      marker = NULL;
-	    }
-	  else
-	    {
-	      release_static_tracepoint_marker (marker);
-	      memset (marker, 0, sizeof (*marker));
-	    }
+	  if (strid == NULL || marker.str_id == strid)
+	    markers.push_back (std::move (marker));
 	}
       while (*p++ == ',');	/* comma-separated list */
 
diff --git a/gdb/remote.c b/gdb/remote.c
index 15d6c5bdbf..f030f93191 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3381,7 +3381,7 @@ remote_threads_extra_info (struct target_ops *self, struct thread_info *tp)
 }
 \f
 
-static int
+static bool
 remote_static_tracepoint_marker_at (struct target_ops *self, CORE_ADDR addr,
 				    struct static_tracepoint_marker *marker)
 {
@@ -3401,21 +3401,20 @@ remote_static_tracepoint_marker_at (struct target_ops *self, CORE_ADDR addr,
   if (*p++ == 'm')
     {
       parse_static_tracepoint_marker_definition (p, NULL, marker);
-      return 1;
+      return true;
     }
 
-  return 0;
+  return false;
 }
 
-static VEC(static_tracepoint_marker_p) *
+static std::vector<static_tracepoint_marker>
 remote_static_tracepoint_markers_by_strid (struct target_ops *self,
 					   const char *strid)
 {
   struct remote_state *rs = get_remote_state ();
-  VEC(static_tracepoint_marker_p) *markers = NULL;
-  struct static_tracepoint_marker *marker = NULL;
-  struct cleanup *old_chain;
+  std::vector<static_tracepoint_marker> markers;
   const char *p;
+  static_tracepoint_marker marker;
 
   /* Ask for a first packet of static tracepoint marker
      definition.  */
@@ -3425,28 +3424,14 @@ remote_static_tracepoint_markers_by_strid (struct target_ops *self,
   if (*p == 'E')
     error (_("Remote failure reply: %s"), p);
 
-  old_chain = make_cleanup (free_current_marker, &marker);
-
   while (*p++ == 'm')
     {
-      if (marker == NULL)
-	marker = XCNEW (struct static_tracepoint_marker);
-
       do
 	{
-	  parse_static_tracepoint_marker_definition (p, &p, marker);
+	  parse_static_tracepoint_marker_definition (p, &p, &marker);
 
-	  if (strid == NULL || strcmp (strid, marker->str_id) == 0)
-	    {
-	      VEC_safe_push (static_tracepoint_marker_p,
-			     markers, marker);
-	      marker = NULL;
-	    }
-	  else
-	    {
-	      release_static_tracepoint_marker (marker);
-	      memset (marker, 0, sizeof (*marker));
-	    }
+	  if (strid == NULL || marker.str_id == strid)
+	    markers.push_back (std::move (marker));
 	}
       while (*p++ == ',');	/* comma-separated list */
       /* Ask for another packet of static tracepoint definition.  */
@@ -3455,7 +3440,6 @@ remote_static_tracepoint_markers_by_strid (struct target_ops *self,
       p = rs->buf;
     }
 
-  do_cleanups (old_chain);
   return markers;
 }
 
diff --git a/gdb/target-debug.h b/gdb/target-debug.h
index d7fc001975..3f991cfdb0 100644
--- a/gdb/target-debug.h
+++ b/gdb/target-debug.h
@@ -118,8 +118,8 @@
   target_debug_do_print (host_address_to_string (X))
 #define target_debug_print_std_vector_mem_region(X) \
   target_debug_do_print (host_address_to_string (X.data ()))
-#define target_debug_print_VEC_static_tracepoint_marker_p_p(X)	\
-  target_debug_do_print (host_address_to_string (X))
+#define target_debug_print_std_vector_static_tracepoint_marker(X)	\
+  target_debug_do_print (host_address_to_string (X.data ()))
 #define target_debug_print_const_struct_target_desc_p(X)	\
   target_debug_do_print (host_address_to_string (X))
 #define target_debug_print_struct_bp_location_p(X)	\
@@ -136,7 +136,7 @@
   target_debug_do_print (host_address_to_string (X))
 #define target_debug_print_struct_uploaded_tsv_pp(X)	\
   target_debug_do_print (host_address_to_string (X))
-#define target_debug_print_struct_static_tracepoint_marker_p(X)	\
+#define target_debug_print_static_tracepoint_marker_p(X)	\
   target_debug_do_print (host_address_to_string (X))
 #define target_debug_print_struct_traceframe_info_p(X)	\
   target_debug_do_print (host_address_to_string (X))
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index c7ebdbfb5b..ed26e0bf9e 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -3315,23 +3315,23 @@ debug_set_permissions (struct target_ops *self)
   fputs_unfiltered (")\n", gdb_stdlog);
 }
 
-static int
-delegate_static_tracepoint_marker_at (struct target_ops *self, CORE_ADDR arg1, struct static_tracepoint_marker *arg2)
+static bool
+delegate_static_tracepoint_marker_at (struct target_ops *self, CORE_ADDR arg1, static_tracepoint_marker *arg2)
 {
   self = self->beneath;
   return self->to_static_tracepoint_marker_at (self, arg1, arg2);
 }
 
-static int
-tdefault_static_tracepoint_marker_at (struct target_ops *self, CORE_ADDR arg1, struct static_tracepoint_marker *arg2)
+static bool
+tdefault_static_tracepoint_marker_at (struct target_ops *self, CORE_ADDR arg1, static_tracepoint_marker *arg2)
 {
-  return 0;
+  return false;
 }
 
-static int
-debug_static_tracepoint_marker_at (struct target_ops *self, CORE_ADDR arg1, struct static_tracepoint_marker *arg2)
+static bool
+debug_static_tracepoint_marker_at (struct target_ops *self, CORE_ADDR arg1, static_tracepoint_marker *arg2)
 {
-  int result;
+  bool result;
   fprintf_unfiltered (gdb_stdlog, "-> %s->to_static_tracepoint_marker_at (...)\n", debug_target.to_shortname);
   result = debug_target.to_static_tracepoint_marker_at (&debug_target, arg1, arg2);
   fprintf_unfiltered (gdb_stdlog, "<- %s->to_static_tracepoint_marker_at (", debug_target.to_shortname);
@@ -3339,30 +3339,30 @@ debug_static_tracepoint_marker_at (struct target_ops *self, CORE_ADDR arg1, stru
   fputs_unfiltered (", ", gdb_stdlog);
   target_debug_print_CORE_ADDR (arg1);
   fputs_unfiltered (", ", gdb_stdlog);
-  target_debug_print_struct_static_tracepoint_marker_p (arg2);
+  target_debug_print_static_tracepoint_marker_p (arg2);
   fputs_unfiltered (") = ", gdb_stdlog);
-  target_debug_print_int (result);
+  target_debug_print_bool (result);
   fputs_unfiltered ("\n", gdb_stdlog);
   return result;
 }
 
-static VEC(static_tracepoint_marker_p) *
+static std::vector<static_tracepoint_marker>
 delegate_static_tracepoint_markers_by_strid (struct target_ops *self, const char *arg1)
 {
   self = self->beneath;
   return self->to_static_tracepoint_markers_by_strid (self, arg1);
 }
 
-static VEC(static_tracepoint_marker_p) *
+static std::vector<static_tracepoint_marker>
 tdefault_static_tracepoint_markers_by_strid (struct target_ops *self, const char *arg1)
 {
   tcomplain ();
 }
 
-static VEC(static_tracepoint_marker_p) *
+static std::vector<static_tracepoint_marker>
 debug_static_tracepoint_markers_by_strid (struct target_ops *self, const char *arg1)
 {
-  VEC(static_tracepoint_marker_p) * result;
+  std::vector<static_tracepoint_marker> result;
   fprintf_unfiltered (gdb_stdlog, "-> %s->to_static_tracepoint_markers_by_strid (...)\n", debug_target.to_shortname);
   result = debug_target.to_static_tracepoint_markers_by_strid (&debug_target, arg1);
   fprintf_unfiltered (gdb_stdlog, "<- %s->to_static_tracepoint_markers_by_strid (", debug_target.to_shortname);
@@ -3370,7 +3370,7 @@ debug_static_tracepoint_markers_by_strid (struct target_ops *self, const char *a
   fputs_unfiltered (", ", gdb_stdlog);
   target_debug_print_const_char_p (arg1);
   fputs_unfiltered (") = ", gdb_stdlog);
-  target_debug_print_VEC_static_tracepoint_marker_p_p (result);
+  target_debug_print_std_vector_static_tracepoint_marker (result);
   fputs_unfiltered ("\n", gdb_stdlog);
   return result;
 }
diff --git a/gdb/target.h b/gdb/target.h
index 83cf48575f..f0e64bd567 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -227,9 +227,6 @@ enum target_xfer_status
 extern const char *
   target_xfer_status_to_string (enum target_xfer_status status);
 
-typedef struct static_tracepoint_marker *static_tracepoint_marker_p;
-DEF_VEC_P(static_tracepoint_marker_p);
-
 typedef enum target_xfer_status
   target_xfer_partial_ftype (struct target_ops *ops,
 			     enum target_object object,
@@ -1084,14 +1081,16 @@ struct target_ops
       TARGET_DEFAULT_IGNORE ();
 
     /* Look for a static tracepoint marker at ADDR, and fill in MARKER
-       with its details.  Return 1 on success, 0 on failure.  */
-    int (*to_static_tracepoint_marker_at) (struct target_ops *, CORE_ADDR,
-					   struct static_tracepoint_marker *marker)
-      TARGET_DEFAULT_RETURN (0);
+       with its details.  Return true on success, false on failure.  */
+    bool (*to_static_tracepoint_marker_at) (struct target_ops *, CORE_ADDR,
+					    static_tracepoint_marker *marker)
+      TARGET_DEFAULT_RETURN (false);
 
     /* Return a vector of all tracepoints markers string id ID, or all
        markers if ID is NULL.  */
-    VEC(static_tracepoint_marker_p) *(*to_static_tracepoint_markers_by_strid) (struct target_ops *, const char *id)
+    std::vector<static_tracepoint_marker>
+      (*to_static_tracepoint_markers_by_strid) (struct target_ops *,
+						const char *id)
       TARGET_DEFAULT_NORETURN (tcomplain ());
 
     /* Return a traceframe info object describing the current
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 843041f739..e7fa9d416c 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -3674,21 +3674,6 @@ parse_tsv_definition (const char *line, struct uploaded_tsv **utsvp)
   utsv->name = xstrdup (buf);
 }
 
-void
-free_current_marker (void *arg)
-{
-  struct static_tracepoint_marker **marker_p
-    = (struct static_tracepoint_marker **) arg;
-
-  if (*marker_p != NULL)
-    {
-      release_static_tracepoint_marker (*marker_p);
-      xfree (*marker_p);
-    }
-  else
-    *marker_p = NULL;
-}
-
 /* Given a line of text defining a static tracepoint marker, parse it
    into a "static tracepoint marker" object.  Throws an error is
    parsing fails.  If PP is non-null, it points to one past the end of
@@ -3696,11 +3681,10 @@ free_current_marker (void *arg)
 
 void
 parse_static_tracepoint_marker_definition (const char *line, const char **pp,
-					   struct static_tracepoint_marker *marker)
+					   static_tracepoint_marker *marker)
 {
   const char *p, *endp;
   ULONGEST addr;
-  int end;
 
   p = line;
   p = unpack_varlen_hex (p, &addr);
@@ -3713,12 +3697,10 @@ parse_static_tracepoint_marker_definition (const char *line, const char **pp,
   if (endp == NULL)
     error (_("bad marker definition: %s"), line);
 
-  marker->str_id = (char *) xmalloc (endp - p + 1);
-  end = hex2bin (p, (gdb_byte *) marker->str_id, (endp - p + 1) / 2);
-  marker->str_id[end] = '\0';
+  marker->str_id = hex2str (p, (endp - p) / 2);
 
-  p += 2 * end;
-  p++;  /* skip a colon */
+  p = endp;
+  p++; /* skip a colon */
 
   /* This definition may be followed by another one, separated by a comma.  */
   int hex_len;
@@ -3728,30 +3710,17 @@ parse_static_tracepoint_marker_definition (const char *line, const char **pp,
   else
     hex_len = strlen (p);
 
-  marker->extra = (char *) xmalloc (hex_len / 2 + 1);
-  end = hex2bin (p, (gdb_byte *) marker->extra, hex_len / 2);
-  marker->extra[end] = '\0';
+  marker->extra = hex2str (p, hex_len / 2);
 
-  if (pp)
+  if (pp != nullptr)
     *pp = p + hex_len;
 }
 
-/* Release a static tracepoint marker's contents.  Note that the
-   object itself isn't released here.  There objects are usually on
-   the stack.  */
-
-void
-release_static_tracepoint_marker (struct static_tracepoint_marker *marker)
-{
-  xfree (marker->str_id);
-  marker->str_id = NULL;
-}
-
 /* Print MARKER to gdb_stdout.  */
 
 static void
 print_one_static_tracepoint_marker (int count,
-				    struct static_tracepoint_marker *marker)
+				    const static_tracepoint_marker &marker)
 {
   struct symbol *sym;
 
@@ -3761,9 +3730,9 @@ print_one_static_tracepoint_marker (int count,
   VEC(breakpoint_p) *tracepoints;
 
   symtab_and_line sal;
-  sal.pc = marker->address;
+  sal.pc = marker.address;
 
-  tracepoints = static_tracepoints_here (marker->address);
+  tracepoints = static_tracepoints_here (marker.address);
 
   ui_out_emit_tuple tuple_emitter (uiout, "marker");
 
@@ -3771,7 +3740,7 @@ print_one_static_tracepoint_marker (int count,
      identifier!  */
   uiout->field_int ("count", count);
 
-  uiout->field_string ("marker-id", marker->str_id);
+  uiout->field_string ("marker-id", marker.str_id.c_str ());
 
   uiout->field_fmt ("enabled", "%c",
 		    !VEC_empty (breakpoint_p, tracepoints) ? 'y' : 'n');
@@ -3779,17 +3748,17 @@ print_one_static_tracepoint_marker (int count,
 
   strcpy (wrap_indent, "                                   ");
 
-  if (gdbarch_addr_bit (marker->gdbarch) <= 32)
+  if (gdbarch_addr_bit (marker.gdbarch) <= 32)
     strcat (wrap_indent, "           ");
   else
     strcat (wrap_indent, "                   ");
 
   strcpy (extra_field_indent, "         ");
 
-  uiout->field_core_addr ("addr", marker->gdbarch, marker->address);
+  uiout->field_core_addr ("addr", marker.gdbarch, marker.address);
 
-  sal = find_pc_line (marker->address, 0);
-  sym = find_pc_sect_function (marker->address, NULL);
+  sal = find_pc_line (marker.address, 0);
+  sym = find_pc_sect_function (marker.address, NULL);
   if (sym)
     {
       uiout->text ("in ");
@@ -3827,7 +3796,7 @@ print_one_static_tracepoint_marker (int count,
   uiout->text ("\n");
   uiout->text (extra_field_indent);
   uiout->text (_("Data: \""));
-  uiout->field_string ("extra-data", marker->extra);
+  uiout->field_string ("extra-data", marker.extra.c_str ());
   uiout->text ("\"\n");
 
   if (!VEC_empty (breakpoint_p, tracepoints))
@@ -3861,11 +3830,9 @@ print_one_static_tracepoint_marker (int count,
 static void
 info_static_tracepoint_markers_command (const char *arg, int from_tty)
 {
-  VEC(static_tracepoint_marker_p) *markers;
-  struct cleanup *old_chain;
-  struct static_tracepoint_marker *marker;
   struct ui_out *uiout = current_uiout;
-  int i;
+  std::vector<static_tracepoint_marker> markers
+    = target_static_tracepoint_markers_by_strid (NULL);
 
   /* We don't have to check target_can_use_agent and agent's capability on
      static tracepoint here, in order to be compatible with older GDBserver.
@@ -3889,19 +3856,8 @@ info_static_tracepoint_markers_command (const char *arg, int from_tty)
 
   uiout->table_body ();
 
-  markers = target_static_tracepoint_markers_by_strid (NULL);
-  old_chain = make_cleanup (VEC_cleanup (static_tracepoint_marker_p), &markers);
-
-  for (i = 0;
-       VEC_iterate (static_tracepoint_marker_p,
-		    markers, i, marker);
-       i++)
-    {
-      print_one_static_tracepoint_marker (i + 1, marker);
-      release_static_tracepoint_marker (marker);
-    }
-
-  do_cleanups (old_chain);
+  for (int i = 0; i < markers.size (); i++)
+    print_one_static_tracepoint_marker (i + 1, markers[i]);
 }
 
 /* The $_sdata convenience variable is a bit special.  We don't know
diff --git a/gdb/tracepoint.h b/gdb/tracepoint.h
index 9f4596ecd1..c82b62aae6 100644
--- a/gdb/tracepoint.h
+++ b/gdb/tracepoint.h
@@ -214,14 +214,20 @@ struct uploaded_tsv
 
 struct static_tracepoint_marker
 {
-  struct gdbarch *gdbarch;
-  CORE_ADDR address;
+  DISABLE_COPY_AND_ASSIGN (static_tracepoint_marker);
+
+  static_tracepoint_marker () = default;
+  static_tracepoint_marker (static_tracepoint_marker &&) = default;
+  static_tracepoint_marker &operator= (static_tracepoint_marker &&) = default;
+
+  struct gdbarch *gdbarch = NULL;
+  CORE_ADDR address = 0;
 
   /* The string ID of the marker.  */
-  char *str_id;
+  std::string str_id;
 
   /* Extra target reported info associated with the marker.  */
-  char *extra;
+  std::string extra;
 };
 
 struct memrange
@@ -295,11 +301,9 @@ private:
   std::vector<std::string> m_computed;
 };
 
-extern void parse_static_tracepoint_marker_definition
-  (const char *line, const char **pp,
-   struct static_tracepoint_marker *marker);
-extern void release_static_tracepoint_marker (struct static_tracepoint_marker *);
-extern void free_current_marker (void *arg);
+extern void
+  parse_static_tracepoint_marker_definition (const char *line, const char **pp,
+					     static_tracepoint_marker *marker);
 
 /* A hook used to notify the UI of tracepoint operations.  */
 
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index ce224ed225..765a589279 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -104,6 +104,8 @@ class ui_out
   void field_core_addr (const char *fldname, struct gdbarch *gdbarch,
 			CORE_ADDR address);
   void field_string (const char *fldname, const char *string);
+  void field_string (const char *fldname, const std::string &string)
+  { field_string (fldname, string.c_str ()); }
   void field_stream (const char *fldname, string_file &stream);
   void field_skip (const char *fldname);
   void field_fmt (const char *fldname, const char *format, ...)
diff --git a/gdb/unittests/rsp-low-selftests.c b/gdb/unittests/rsp-low-selftests.c
index 74d106e9e6..3d1950079d 100644
--- a/gdb/unittests/rsp-low-selftests.c
+++ b/gdb/unittests/rsp-low-selftests.c
@@ -48,6 +48,16 @@ static void test_hex2bin_byte_vector ()
   SELF_CHECK (bv[1] == 0x23);
 }
 
+static void test_hex2str ()
+{
+  SELF_CHECK (hex2str ("666f6f") == "foo");
+  SELF_CHECK (hex2str ("666f6fa") == "foo");
+  SELF_CHECK (hex2str ("666f6f", 2) == "fo");
+  SELF_CHECK (hex2str ("666", 2) == "f");
+  SELF_CHECK (hex2str ("666", 6) == "f");
+  SELF_CHECK (hex2str ("") == "");
+}
+
 } /* namespace rsp_low */
 } /* namespace selftests */
 
@@ -56,4 +66,6 @@ _initialize_rsp_low_selftests ()
 {
   selftests::register_test ("hex2bin_byte_vector",
 			    selftests::rsp_low::test_hex2bin_byte_vector);
+  selftests::register_test ("hex2str",
+			    selftests::rsp_low::test_hex2str);
 }
diff --git a/gdb/unittests/tracepoint-selftests.c b/gdb/unittests/tracepoint-selftests.c
index 21ca3d0914..d5a884cab6 100644
--- a/gdb/unittests/tracepoint-selftests.c
+++ b/gdb/unittests/tracepoint-selftests.c
@@ -37,24 +37,24 @@ test_parse_static_tracepoint_marker_definition ()
   parse_static_tracepoint_marker_definition (start, &end, &marker);
 
   SELF_CHECK (marker.address == 0x1234);
-  SELF_CHECK (strcmp (marker.str_id, "marker1") == 0);
-  SELF_CHECK (strcmp (marker.extra, "extra stuff") == 0);
+  SELF_CHECK (marker.str_id == "marker1");
+  SELF_CHECK (marker.extra == "extra stuff");
   SELF_CHECK (end == strchr (start, ','));
 
   start = end + 1;
   parse_static_tracepoint_marker_definition (start, &end, &marker);
 
   SELF_CHECK (marker.address == 0xabba);
-  SELF_CHECK (strcmp (marker.str_id, "marker2") == 0);
-  SELF_CHECK (strcmp (marker.extra, "") == 0);
+  SELF_CHECK (marker.str_id == "marker2");
+  SELF_CHECK (marker.extra == "");
   SELF_CHECK (end == strchr (start, ','));
 
   start = end + 1;
   parse_static_tracepoint_marker_definition (start, &end, &marker);
 
   SELF_CHECK (marker.address == 0xcafe);
-  SELF_CHECK (strcmp (marker.str_id, "marker3") == 0);
-  SELF_CHECK (strcmp (marker.extra, "morestuff") == 0);
+  SELF_CHECK (marker.str_id == "marker3");
+  SELF_CHECK (marker.extra == "morestuff");
   SELF_CHECK (end == def + strlen (def));
 }
 
-- 
2.16.1

Re: [PATCH 1/2] Make parse_static_tracepoint_marker_definition work with multiple static tracepoint definitions

[Simon Marchi]

On 2018-02-18 22:35, Simon Marchi wrote:
> Since I modify the parse_static_tracepoint_marker_definition function 
> in
> the next patch, I wanted to write a unit test for it.  Doing so showed
> that it doesn't handle multiple consecutive static tracepoint
> definitions separated by commas.  However, the RSP documentation [1]
> states that servers may return multiple definitions, like:
> 
>   1234:6d61726b657231:6578747261207374756666,abba:6d61726b657232:
> 
> The problem is that the function uses strlen to compute the length of
> the last field (the extra field).  If there are additional definitions
> in addition to the one we are currently parsing, the returned length
> will include those definitions, and we'll try to hex-decode past the
> extra field.
> 
> This patch changes parse_static_tracepoint_marker_definition to 
> consider
> the case where the current definition is followed by a comma and more
> definitions.  It also adds the unit test that found the issue in the
> first place.
> 
> I don't think this causes any backwards compatibility issues, because
> the previous code only handled single static tracepoint definitions, 
> and
> the new code handles that correctly.

I just pushed these.

Simon