[PATCH v2 0/6] Fix tracepoint register limitations

[PATCH v2 0/6] Fix tracepoint register limitations

[Pedro Franco de Carvalho]

This is V2 of: https://sourceware.org/ml/gdb-patches/2018-06/msg00501.html

The goal of this series is to address internal limitations with
tracepoints that showed up when I tried to enable more powerpc
registers.

I added two new patches to the series, one that changes
collection_list in tracepoint.c to only set remote register numbers in
the register mask, and one that fixes another limitation from adding
more powerpc registers, which caused a buffer overrun when saving a
.ctf file.

Pedro Franco de Carvalho (6):
  Fix indentation in remote_target::download_tracepoint
  Remove trailing '-' from the last QTDP action packet
  Use get_remote_packet_size in download_tracepoint
  Use remote register numbers in tracepoint mask
  Variable size for regs mask in collection list
  Allow larger regblock sizes when saving tracefiles

 gdb/remote.c     | 161 ++++++++++++++++++++++++++++++-------------
 gdb/tracefile.c  |  42 +++++++-----
 gdb/tracepoint.c | 206 ++++++++++++++++++++++++++++++++-----------------------
 gdb/tracepoint.h |  13 ++--
 4 files changed, 267 insertions(+), 155 deletions(-)

-- 
2.13.6

[PATCH v2 3/6] Use get_remote_packet_size in download_tracepoint

[Pedro Franco de Carvalho]

This patch changes the remote target to use the remote packet size to
build QTDP packets, and to check if there is enough room for the
packet.

I changed the function to raise an error if the packet is too small,
instead of aborting gdb (through xsnprintf).  It isn't clear if gdb
will be in a consistent state with respect to the stub after this,
since it's possible that some packets will be sent but not others, and
there could be an incomplete tracepoint on the stub.

The char array used to build the packets is changed to a
gdb::char_vector and sized with the result from
get_remote_packet_size.

When checking if the buffer is large enough to hold the tracepoint
condition agent expression, the length of the expression is multiplied
by two, since it is encoded with two hex digits per expression
byte.  For simplicity, I assume that the result won't overflow, which
can happen for very long condition expressions.

gdb/ChangeLog:
YYYY-MM-DD  Pedro Franco de Carvalho  <pedromfc@linux.ibm.com>

	* remote.c (remote_target::download_tracepoint): Remove BUF_SIZE.
	Replace array buf with gdb::char_vector buf, of size
	get_remote_packet_size ().  Replace references to buf and BUF_SIZE
	to buf.data () and buf.size ().  Replace strcpy, strcat and
	pack_hex_byte with snprintf.  Raise errors if the buffer is too
	small.
---
 gdb/remote.c | 135 ++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 101 insertions(+), 34 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index e3180923ee..5c8b6861fc 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -12832,26 +12832,35 @@ remote_target::remote_download_command_source (int num, ULONGEST addr,
 void
 remote_target::download_tracepoint (struct bp_location *loc)
 {
-#define BUF_SIZE 2048
-
   CORE_ADDR tpaddr;
   char addrbuf[40];
-  char buf[BUF_SIZE];
   std::vector<std::string> tdp_actions;
   std::vector<std::string> stepping_actions;
   char *pkt;
   struct breakpoint *b = loc->owner;
   struct tracepoint *t = (struct tracepoint *) b;
   struct remote_state *rs = get_remote_state ();
+  int ret;
+  char *err_msg = _("Tracepoint packet too large for target.");
+  size_t size_left;
+
+  /* We use a buffer other than rs->buf because we'll build strings
+     across multiple statements, and other statements in between could
+     modify rs->buf.  */
+  gdb::char_vector buf (get_remote_packet_size ());
 
   encode_actions_rsp (loc, &tdp_actions, &stepping_actions);
 
   tpaddr = loc->address;
   sprintf_vma (addrbuf, tpaddr);
-  xsnprintf (buf, BUF_SIZE, "QTDP:%x:%s:%c:%lx:%x", b->number,
-	     addrbuf, /* address */
-	     (b->enable_state == bp_enabled ? 'E' : 'D'),
-	     t->step_count, t->pass_count);
+  ret = snprintf (buf.data (), buf.size (), "QTDP:%x:%s:%c:%lx:%x",
+		  b->number, addrbuf, /* address */
+		  (b->enable_state == bp_enabled ? 'E' : 'D'),
+		  t->step_count, t->pass_count);
+
+  if (ret < 0 || ret >= buf.size ())
+    error (err_msg);
+
   /* Fast tracepoints are mostly handled by the target, but we can
      tell the target how big of an instruction block should be moved
      around.  */
@@ -12863,8 +12872,15 @@ remote_target::download_tracepoint (struct bp_location *loc)
 	{
 	  if (gdbarch_fast_tracepoint_valid_at (loc->gdbarch, tpaddr,
 						NULL))
-	    xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":F%x",
-		       gdb_insn_length (loc->gdbarch, tpaddr));
+	    {
+	      size_left = buf.size () - strlen (buf.data ());
+	      ret = snprintf (buf.data () + strlen (buf.data ()),
+			      size_left, ":F%x",
+			      gdb_insn_length (loc->gdbarch, tpaddr));
+
+	      if (ret < 0 || ret >= size_left)
+		error (err_msg);
+	    }
 	  else
 	    /* If it passed validation at definition but fails now,
 	       something is very wrong.  */
@@ -12888,7 +12904,15 @@ remote_target::download_tracepoint (struct bp_location *loc)
 	  struct static_tracepoint_marker marker;
 
 	  if (target_static_tracepoint_marker_at (tpaddr, &marker))
-	    strcat (buf, ":S");
+	    {
+	      size_left = buf.size () - strlen (buf.data ());
+	      ret = snprintf (buf.data () + strlen (buf.data ()),
+			      size_left, ":F%x",
+			      gdb_insn_length (loc->gdbarch, tpaddr));
+
+	      if (ret < 0 || ret >= size_left)
+		error (err_msg);
+	    }
 	  else
 	    error (_("Static tracepoint not valid during download"));
 	}
@@ -12906,10 +12930,26 @@ remote_target::download_tracepoint (struct bp_location *loc)
 	 capabilities at definition time.  */
       if (remote_supports_cond_tracepoints ())
 	{
-	  agent_expr_up aexpr = gen_eval_for_expr (tpaddr, loc->cond.get ());
-	  xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":X%x,",
-		     aexpr->len);
-	  pkt = buf + strlen (buf);
+	  agent_expr_up aexpr = gen_eval_for_expr (tpaddr,
+						   loc->cond.get ());
+
+	  size_left = buf.size () - strlen (buf.data ());
+
+	  ret = snprintf (buf.data () + strlen (buf.data ()),
+			  size_left, ":X%x,", aexpr->len);
+
+	  if (ret < 0 || ret >= size_left)
+	    error (err_msg);
+
+	  size_left = buf.size () - strlen (buf.data ());
+
+	  /* Two bytes to encode each aexpr byte, plus the terminating
+	     null byte.  */
+	  if (aexpr->len * 2 + 1 > size_left)
+	    error (err_msg);
+
+	  pkt = buf.data () + strlen (buf.data ());
+
 	  for (int ndx = 0; ndx < aexpr->len; ++ndx)
 	    pkt = pack_hex_byte (pkt, aexpr->buf[ndx]);
 	  *pkt = '\0';
@@ -12920,8 +12960,17 @@ remote_target::download_tracepoint (struct bp_location *loc)
     }
 
   if (b->commands || *default_collect)
-    strcat (buf, "-");
-  putpkt (buf);
+    {
+      size_left = buf.size () - strlen (buf.data ());
+
+      ret = snprintf (buf.data () + strlen (buf.data ()),
+		      size_left, "-");
+
+      if (ret < 0 || ret >= size_left)
+	error (err_msg);
+    }
+
+  putpkt (buf.data ());
   remote_get_noisy_reply ();
   if (strcmp (rs->buf, "OK"))
     error (_("Target does not support tracepoints."));
@@ -12935,11 +12984,15 @@ remote_target::download_tracepoint (struct bp_location *loc)
       bool has_more = ((action_it + 1) != tdp_actions.end ()
 		       || !stepping_actions.empty ());
 
-      xsnprintf (buf, BUF_SIZE, "QTDP:-%x:%s:%s%c",
-		 b->number, addrbuf, /* address */
-		 action_it->c_str (),
-		 has_more ? '-' : 0);
-      putpkt (buf);
+      ret = snprintf (buf.data (), buf.size (), "QTDP:-%x:%s:%s%c",
+		      b->number, addrbuf, /* address */
+		      action_it->c_str (),
+		      has_more ? '-' : 0);
+
+      if (ret < 0 || ret >= buf.size ())
+	error (err_msg);
+
+      putpkt (buf.data ());
       remote_get_noisy_reply ();
       if (strcmp (rs->buf, "OK"))
 	error (_("Error on target while setting tracepoints."));
@@ -12953,12 +13006,16 @@ remote_target::download_tracepoint (struct bp_location *loc)
       bool is_first = action_it == stepping_actions.begin ();
       bool has_more = (action_it + 1) != stepping_actions.end ();
 
-      xsnprintf (buf, BUF_SIZE, "QTDP:-%x:%s:%s%s%s",
-		 b->number, addrbuf, /* address */
-		 is_first ? "S" : "",
-		 action_it->c_str (),
-		 has_more ? "-" : "");
-      putpkt (buf);
+      ret = snprintf (buf.data (), buf.size (), "QTDP:-%x:%s:%s%s%s",
+		      b->number, addrbuf, /* address */
+		      is_first ? "S" : "",
+		      action_it->c_str (),
+		      has_more ? "-" : "");
+
+      if (ret < 0 || ret >= buf.size ())
+	error (err_msg);
+
+      putpkt (buf.data ());
       remote_get_noisy_reply ();
       if (strcmp (rs->buf, "OK"))
 	error (_("Error on target while setting tracepoints."));
@@ -12968,22 +13025,32 @@ remote_target::download_tracepoint (struct bp_location *loc)
     {
       if (b->location != NULL)
 	{
-	  strcpy (buf, "QTDPsrc:");
+	  ret = snprintf (buf.data (), buf.size (), "QTDPsrc:");
+
+	  if (ret < 0 || ret >= buf.size ())
+	    error (err_msg);
+
 	  encode_source_string (b->number, loc->address, "at",
 				event_location_to_string (b->location.get ()),
-				buf + strlen (buf), 2048 - strlen (buf));
-	  putpkt (buf);
+				buf.data () + strlen (buf.data ()),
+				buf.size () - strlen (buf.data ()));
+	  putpkt (buf.data ());
 	  remote_get_noisy_reply ();
 	  if (strcmp (rs->buf, "OK"))
 	    warning (_("Target does not support source download."));
 	}
       if (b->cond_string)
 	{
-	  strcpy (buf, "QTDPsrc:");
+	  ret = snprintf (buf.data (), buf.size (), "QTDPsrc:");
+
+	  if (ret < 0 || ret >= buf.size ())
+	    error (err_msg);
+
 	  encode_source_string (b->number, loc->address,
-				"cond", b->cond_string, buf + strlen (buf),
-				2048 - strlen (buf));
-	  putpkt (buf);
+				"cond", b->cond_string,
+				buf.data () + strlen (buf.data ()),
+				buf.size () - strlen (buf.data ()));
+	  putpkt (buf.data ());
 	  remote_get_noisy_reply ();
 	  if (strcmp (rs->buf, "OK"))
 	    warning (_("Target does not support source download."));
-- 
2.13.6

Re: [PATCH v2 3/6] Use get_remote_packet_size in download_tracepoint

[Ulrich Weigand]

Pedro Franco de Carvalho wrote:

> 	* remote.c (remote_target::download_tracepoint): Remove BUF_SIZE.
> 	Replace array buf with gdb::char_vector buf, of size
> 	get_remote_packet_size ().  Replace references to buf and BUF_SIZE
> 	to buf.data () and buf.size ().  Replace strcpy, strcat and
> 	pack_hex_byte with snprintf.  Raise errors if the buffer is too
> 	small.

>  	  if (target_static_tracepoint_marker_at (tpaddr, &marker))
> -	    strcat (buf, ":S");
> +	    {
> +	      size_left = buf.size () - strlen (buf.data ());
> +	      ret = snprintf (buf.data () + strlen (buf.data ()),
> +			      size_left, ":F%x",
> +			      gdb_insn_length (loc->gdbarch, tpaddr));

This changes the message -- I guess that should remain :S?

Otherwise this looks good to me.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH v2 3/6] Use get_remote_packet_size in download_tracepoint

[Pedro Franco de Carvalho]

Ulrich Weigand <uweigand@de.ibm.com> writes:

> This changes the message -- I guess that should remain :S?

Oops! Must have been a careless copy-paste. Good catch.

See below for the updated patch. Let me know if you prefer that I resend
the full series in a V3.

--
Pedro Franco de Carvalho

[PATCH v2 3/6] Use get_remote_packet_size in download_tracepoint

[Pedro Franco de Carvalho]

This patch changes the remote target to use the remote packet size to
build QTDP packets, and to check if there is enough room for the
packet.

I changed the function to raise an error if the packet is too small,
instead of aborting gdb (through xsnprintf).  It isn't clear if gdb
will be in a consistent state with respect to the stub after this,
since it's possible that some packets will be sent but not others, and
there could be an incomplete tracepoint on the stub.

The char array used to build the packets is changed to a
gdb::char_vector and sized with the result from
get_remote_packet_size.

When checking if the buffer is large enough to hold the tracepoint
condition agent expression, the length of the expression is multiplied
by two, since it is encoded with two hex digits per expression
byte.  For simplicity, I assume that the result won't overflow, which
can happen for very long condition expressions.

gdb/ChangeLog:
YYYY-MM-DD  Pedro Franco de Carvalho  <pedromfc@linux.ibm.com>

	* remote.c (remote_target::download_tracepoint): Remove BUF_SIZE.
	Replace array buf with gdb::char_vector buf, of size
	get_remote_packet_size ().  Replace references to buf and BUF_SIZE
	to buf.data () and buf.size ().  Replace strcpy, strcat and
	pack_hex_byte with snprintf.  Raise errors if the buffer is too
	small.
---
 gdb/remote.c | 134 ++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 100 insertions(+), 34 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index e3180923ee..4974c2e8f0 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -12832,26 +12832,35 @@ remote_target::remote_download_command_source (int num, ULONGEST addr,
 void
 remote_target::download_tracepoint (struct bp_location *loc)
 {
-#define BUF_SIZE 2048
-
   CORE_ADDR tpaddr;
   char addrbuf[40];
-  char buf[BUF_SIZE];
   std::vector<std::string> tdp_actions;
   std::vector<std::string> stepping_actions;
   char *pkt;
   struct breakpoint *b = loc->owner;
   struct tracepoint *t = (struct tracepoint *) b;
   struct remote_state *rs = get_remote_state ();
+  int ret;
+  char *err_msg = _("Tracepoint packet too large for target.");
+  size_t size_left;
+
+  /* We use a buffer other than rs->buf because we'll build strings
+     across multiple statements, and other statements in between could
+     modify rs->buf.  */
+  gdb::char_vector buf (get_remote_packet_size ());
 
   encode_actions_rsp (loc, &tdp_actions, &stepping_actions);
 
   tpaddr = loc->address;
   sprintf_vma (addrbuf, tpaddr);
-  xsnprintf (buf, BUF_SIZE, "QTDP:%x:%s:%c:%lx:%x", b->number,
-	     addrbuf, /* address */
-	     (b->enable_state == bp_enabled ? 'E' : 'D'),
-	     t->step_count, t->pass_count);
+  ret = snprintf (buf.data (), buf.size (), "QTDP:%x:%s:%c:%lx:%x",
+		  b->number, addrbuf, /* address */
+		  (b->enable_state == bp_enabled ? 'E' : 'D'),
+		  t->step_count, t->pass_count);
+
+  if (ret < 0 || ret >= buf.size ())
+    error (err_msg);
+
   /* Fast tracepoints are mostly handled by the target, but we can
      tell the target how big of an instruction block should be moved
      around.  */
@@ -12863,8 +12872,15 @@ remote_target::download_tracepoint (struct bp_location *loc)
 	{
 	  if (gdbarch_fast_tracepoint_valid_at (loc->gdbarch, tpaddr,
 						NULL))
-	    xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":F%x",
-		       gdb_insn_length (loc->gdbarch, tpaddr));
+	    {
+	      size_left = buf.size () - strlen (buf.data ());
+	      ret = snprintf (buf.data () + strlen (buf.data ()),
+			      size_left, ":F%x",
+			      gdb_insn_length (loc->gdbarch, tpaddr));
+
+	      if (ret < 0 || ret >= size_left)
+		error (err_msg);
+	    }
 	  else
 	    /* If it passed validation at definition but fails now,
 	       something is very wrong.  */
@@ -12888,7 +12904,14 @@ remote_target::download_tracepoint (struct bp_location *loc)
 	  struct static_tracepoint_marker marker;
 
 	  if (target_static_tracepoint_marker_at (tpaddr, &marker))
-	    strcat (buf, ":S");
+	    {
+	      size_left = buf.size () - strlen (buf.data ());
+	      ret = snprintf (buf.data () + strlen (buf.data ()),
+			      size_left, ":S");
+
+	      if (ret < 0 || ret >= size_left)
+		error (err_msg);
+	    }
 	  else
 	    error (_("Static tracepoint not valid during download"));
 	}
@@ -12906,10 +12929,26 @@ remote_target::download_tracepoint (struct bp_location *loc)
 	 capabilities at definition time.  */
       if (remote_supports_cond_tracepoints ())
 	{
-	  agent_expr_up aexpr = gen_eval_for_expr (tpaddr, loc->cond.get ());
-	  xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":X%x,",
-		     aexpr->len);
-	  pkt = buf + strlen (buf);
+	  agent_expr_up aexpr = gen_eval_for_expr (tpaddr,
+						   loc->cond.get ());
+
+	  size_left = buf.size () - strlen (buf.data ());
+
+	  ret = snprintf (buf.data () + strlen (buf.data ()),
+			  size_left, ":X%x,", aexpr->len);
+
+	  if (ret < 0 || ret >= size_left)
+	    error (err_msg);
+
+	  size_left = buf.size () - strlen (buf.data ());
+
+	  /* Two bytes to encode each aexpr byte, plus the terminating
+	     null byte.  */
+	  if (aexpr->len * 2 + 1 > size_left)
+	    error (err_msg);
+
+	  pkt = buf.data () + strlen (buf.data ());
+
 	  for (int ndx = 0; ndx < aexpr->len; ++ndx)
 	    pkt = pack_hex_byte (pkt, aexpr->buf[ndx]);
 	  *pkt = '\0';
@@ -12920,8 +12959,17 @@ remote_target::download_tracepoint (struct bp_location *loc)
     }
 
   if (b->commands || *default_collect)
-    strcat (buf, "-");
-  putpkt (buf);
+    {
+      size_left = buf.size () - strlen (buf.data ());
+
+      ret = snprintf (buf.data () + strlen (buf.data ()),
+		      size_left, "-");
+
+      if (ret < 0 || ret >= size_left)
+	error (err_msg);
+    }
+
+  putpkt (buf.data ());
   remote_get_noisy_reply ();
   if (strcmp (rs->buf, "OK"))
     error (_("Target does not support tracepoints."));
@@ -12935,11 +12983,15 @@ remote_target::download_tracepoint (struct bp_location *loc)
       bool has_more = ((action_it + 1) != tdp_actions.end ()
 		       || !stepping_actions.empty ());
 
-      xsnprintf (buf, BUF_SIZE, "QTDP:-%x:%s:%s%c",
-		 b->number, addrbuf, /* address */
-		 action_it->c_str (),
-		 has_more ? '-' : 0);
-      putpkt (buf);
+      ret = snprintf (buf.data (), buf.size (), "QTDP:-%x:%s:%s%c",
+		      b->number, addrbuf, /* address */
+		      action_it->c_str (),
+		      has_more ? '-' : 0);
+
+      if (ret < 0 || ret >= buf.size ())
+	error (err_msg);
+
+      putpkt (buf.data ());
       remote_get_noisy_reply ();
       if (strcmp (rs->buf, "OK"))
 	error (_("Error on target while setting tracepoints."));
@@ -12953,12 +13005,16 @@ remote_target::download_tracepoint (struct bp_location *loc)
       bool is_first = action_it == stepping_actions.begin ();
       bool has_more = (action_it + 1) != stepping_actions.end ();
 
-      xsnprintf (buf, BUF_SIZE, "QTDP:-%x:%s:%s%s%s",
-		 b->number, addrbuf, /* address */
-		 is_first ? "S" : "",
-		 action_it->c_str (),
-		 has_more ? "-" : "");
-      putpkt (buf);
+      ret = snprintf (buf.data (), buf.size (), "QTDP:-%x:%s:%s%s%s",
+		      b->number, addrbuf, /* address */
+		      is_first ? "S" : "",
+		      action_it->c_str (),
+		      has_more ? "-" : "");
+
+      if (ret < 0 || ret >= buf.size ())
+	error (err_msg);
+
+      putpkt (buf.data ());
       remote_get_noisy_reply ();
       if (strcmp (rs->buf, "OK"))
 	error (_("Error on target while setting tracepoints."));
@@ -12968,22 +13024,32 @@ remote_target::download_tracepoint (struct bp_location *loc)
     {
       if (b->location != NULL)
 	{
-	  strcpy (buf, "QTDPsrc:");
+	  ret = snprintf (buf.data (), buf.size (), "QTDPsrc:");
+
+	  if (ret < 0 || ret >= buf.size ())
+	    error (err_msg);
+
 	  encode_source_string (b->number, loc->address, "at",
 				event_location_to_string (b->location.get ()),
-				buf + strlen (buf), 2048 - strlen (buf));
-	  putpkt (buf);
+				buf.data () + strlen (buf.data ()),
+				buf.size () - strlen (buf.data ()));
+	  putpkt (buf.data ());
 	  remote_get_noisy_reply ();
 	  if (strcmp (rs->buf, "OK"))
 	    warning (_("Target does not support source download."));
 	}
       if (b->cond_string)
 	{
-	  strcpy (buf, "QTDPsrc:");
+	  ret = snprintf (buf.data (), buf.size (), "QTDPsrc:");
+
+	  if (ret < 0 || ret >= buf.size ())
+	    error (err_msg);
+
 	  encode_source_string (b->number, loc->address,
-				"cond", b->cond_string, buf + strlen (buf),
-				2048 - strlen (buf));
-	  putpkt (buf);
+				"cond", b->cond_string,
+				buf.data () + strlen (buf.data ()),
+				buf.size () - strlen (buf.data ()));
+	  putpkt (buf.data ());
 	  remote_get_noisy_reply ();
 	  if (strcmp (rs->buf, "OK"))
 	    warning (_("Target does not support source download."));
-- 
2.13.6

Re: [PATCH v2 3/6] Use get_remote_packet_size in download_tracepoint

[Ulrich Weigand]

Pedro Franco de Carvalho wrote:

> 	* remote.c (remote_target::download_tracepoint): Remove BUF_SIZE.
> 	Replace array buf with gdb::char_vector buf, of size
> 	get_remote_packet_size ().  Replace references to buf and BUF_SIZE
> 	to buf.data () and buf.size ().  Replace strcpy, strcat and
> 	pack_hex_byte with snprintf.  Raise errors if the buffer is too
> 	small.

This is OK.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH v2 3/6] Use get_remote_packet_size in download_tracepoint

[Szabolcs Nagy]

On 03/08/18 22:41, Pedro Franco de Carvalho wrote:
> YYYY-MM-DD  Pedro Franco de Carvalho  <pedromfc@linux.ibm.com>
> 
> 	* remote.c (remote_target::download_tracepoint): Remove BUF_SIZE.
> 	Replace array buf with gdb::char_vector buf, of size
> 	get_remote_packet_size ().  Replace references to buf and BUF_SIZE
> 	to buf.data () and buf.size ().  Replace strcpy, strcat and
> 	pack_hex_byte with snprintf.  Raise errors if the buffer is too
> 	small.
> ---
>   gdb/remote.c | 134 ++++++++++++++++++++++++++++++++++++++++++++---------------
>   1 file changed, 100 insertions(+), 34 deletions(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index e3180923ee..4974c2e8f0 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -12832,26 +12832,35 @@ remote_target::remote_download_command_source (int num, ULONGEST addr,
>   void
>   remote_target::download_tracepoint (struct bp_location *loc)
>   {
> -#define BUF_SIZE 2048
> -
>     CORE_ADDR tpaddr;
>     char addrbuf[40];
> -  char buf[BUF_SIZE];
>     std::vector<std::string> tdp_actions;
>     std::vector<std::string> stepping_actions;
>     char *pkt;
>     struct breakpoint *b = loc->owner;
>     struct tracepoint *t = (struct tracepoint *) b;
>     struct remote_state *rs = get_remote_state ();
> +  int ret;
> +  char *err_msg = _("Tracepoint packet too large for target.");
> +  size_t size_left;
> +

on baremetal arm targets i see

In file included from /S/gdb/common/common-defs.h:86:0,
                  from /S/gdb/defs.h:28,
                  from /S/gdb/remote.c:22:
/S/gdb/remote.c: In member function 'virtual void remote_target::download_tracepoint(bp_location*)':
/S/gdb/common/gdb_locale.h:35:27: error: deprecated conversion from string constant to 'char*' [-Werror=write-strings]
  # define _(String) (String)
                            ^
/S/gdb/remote.c:12844:19: note: in expansion of macro '_'
    char *err_msg = _("Tracepoint packet too large for target.");
                    ^
cc1plus: all warnings being treated as errors
make[1]: *** [Makefile:1611: remote.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make[1]: Leaving directory '/B/gdb'
make: *** [Makefile:9446: all-gdb] Error 2

[committed] Fix gdb/remote.c build failure

[Szabolcs Nagy]

Add const qualifier to fix

/S/gdb/common/gdb_locale.h:35:27: error: deprecated conversion from string constant to 'char*' [-Werror=write-strings]
  # define _(String) (String)
                            ^
/S/gdb/remote.c:12844:19: note: in expansion of macro '_'
    char *err_msg = _("Tracepoint packet too large for target.");
                    ^
gdb/ChangeLog:

	* remote.c (remote_target::download_tracepoint): Change char* to
	const char*.
---
  gdb/ChangeLog | 5 +++++
  gdb/remote.c  | 2 +-
  2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 6702b445b7..2f5d1ef8b7 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2018-08-08  Szabolcs Nagy  <szabolcs.nagy@arm.com>
+
+	* remote.c (remote_target::download_tracepoint): Change char* to
+	const char*.
+
  2018-08-07  Simon Marchi  <simon.marchi@polymtl.ca>

  	* target.h (target_options_to_string): Return an std::string.
diff --git a/gdb/remote.c b/gdb/remote.c
index 33f6cd57aa..3b19da75ea 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -12841,7 +12841,7 @@ remote_target::download_tracepoint (struct bp_location *loc)
    struct tracepoint *t = (struct tracepoint *) b;
    struct remote_state *rs = get_remote_state ();
    int ret;
-  char *err_msg = _("Tracepoint packet too large for target.");
+  const char *err_msg = _("Tracepoint packet too large for target.");
    size_t size_left;

    /* We use a buffer other than rs->buf because we'll build strings
-- 
2.14.1

Re: [committed] Fix gdb/remote.c build failure

[Ulrich Weigand]

Szabolcs Nagy wrote:

> 	* remote.c (remote_target::download_tracepoint): Change char* to
> 	const char*.

Ah, I missed that.  Sorry.  The fix looks obviously good to me.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH v2 3/6] Use get_remote_packet_size in download_tracepoint

[Pedro Franco de Carvalho]

Szabolcs Nagy <szabolcs.nagy@arm.com> writes:

> on baremetal arm targets i see
>
> In file included from /S/gdb/common/common-defs.h:86:0,
>                   from /S/gdb/defs.h:28,
>                   from /S/gdb/remote.c:22:
> /S/gdb/remote.c: In member function 'virtual void remote_target::download_tracepoint(bp_location*)':
> /S/gdb/common/gdb_locale.h:35:27: error: deprecated conversion from string constant to 'char*' [-Werror=write-strings]
>   # define _(String) (String)
>                             ^
> /S/gdb/remote.c:12844:19: note: in expansion of macro '_'
>     char *err_msg = _("Tracepoint packet too large for target.");

Sorry, and thanks for the fix!

--
Pedro Franco de Carvalho

[PATCH v2 6/6] Allow larger regblock sizes when saving tracefiles

[Pedro Franco de Carvalho]

The tracefile.c:trace_save function assumes trace_regblock_size won't
be larger than the MAX_TRACE_UPLOAD constant, used to size the buffer
which holds trace data.  This can cause buffer overruns when this is
not the case.  This patch changes this function so that the larger
size is used to size the buffer.

gdb/ChangeLog:
YYYY-MM-DD  Pedro Franco de Carvalho  <pedromfc@linux.ibm.com>

	* tracepoint.c: Include common/byte-vector.h.
	(trace_save): Change type of buf to gdb::byte_vector.  Initialize
	with trace_regblock_size if needed.  Update uses of buf.
---
 gdb/tracefile.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/gdb/tracefile.c b/gdb/tracefile.c
index ecd2f5c678..b367f6e403 100644
--- a/gdb/tracefile.c
+++ b/gdb/tracefile.c
@@ -22,6 +22,7 @@
 #include "ctf.h"
 #include "exec.h"
 #include "regcache.h"
+#include "common/byte-vector.h"
 
 /* Helper macros.  */
 
@@ -67,7 +68,7 @@ trace_save (const char *filename, struct trace_file_writer *writer,
 
   ULONGEST offset = 0;
 #define MAX_TRACE_UPLOAD 2000
-  gdb_byte buf[MAX_TRACE_UPLOAD];
+  gdb::byte_vector buf (std::max (MAX_TRACE_UPLOAD, trace_regblock_size));
   enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch ());
 
   /* If the target is to save the data to a file on its own, then just
@@ -144,7 +145,7 @@ trace_save (const char *filename, struct trace_file_writer *writer,
 	  /* We ask for big blocks, in the hopes of efficiency, but
 	     will take less if the target has packet size limitations
 	     or some such.  */
-	  gotten = target_get_raw_trace_data (buf, offset,
+	  gotten = target_get_raw_trace_data (buf.data (), offset,
 					      MAX_TRACE_UPLOAD);
 	  if (gotten < 0)
 	    error (_("Failure to get requested trace buffer data"));
@@ -152,7 +153,7 @@ trace_save (const char *filename, struct trace_file_writer *writer,
 	  if (gotten == 0)
 	    break;
 
-	  writer->ops->write_trace_buffer (writer, buf, gotten);
+	  writer->ops->write_trace_buffer (writer, buf.data (), gotten);
 
 	  offset += gotten;
 	}
@@ -163,7 +164,7 @@ trace_save (const char *filename, struct trace_file_writer *writer,
 	  /* Parse the trace buffers according to how data are stored
 	     in trace buffer in GDBserver.  */
 
-	  gotten = target_get_raw_trace_data (buf, offset, 6);
+	  gotten = target_get_raw_trace_data (buf.data (), offset, 6);
 
 	  if (gotten == 0)
 	    break;
@@ -171,10 +172,10 @@ trace_save (const char *filename, struct trace_file_writer *writer,
 	  /* Read the first six bytes in, which is the tracepoint
 	     number and trace frame size.  */
 	  tp_num = (uint16_t)
-	    extract_unsigned_integer (&buf[0], 2, byte_order);
+	    extract_unsigned_integer (&((buf.data ())[0]), 2, byte_order);
 
 	  tf_size = (uint32_t)
-	    extract_unsigned_integer (&buf[2], 4, byte_order);
+	    extract_unsigned_integer (&((buf.data ())[2]), 4, byte_order);
 
 	  writer->ops->frame_ops->start (writer, tp_num);
 	  gotten = 6;
@@ -192,7 +193,8 @@ trace_save (const char *filename, struct trace_file_writer *writer,
 		  /* We'll fetch one block each time, in order to
 		     handle the extremely large 'M' block.  We first
 		     fetch one byte to get the type of the block.  */
-		  gotten = target_get_raw_trace_data (buf, offset, 1);
+		  gotten = target_get_raw_trace_data (buf.data (),
+						      offset, 1);
 		  if (gotten < 1)
 		    error (_("Failure to get requested trace buffer data"));
 
@@ -205,13 +207,13 @@ trace_save (const char *filename, struct trace_file_writer *writer,
 		    {
 		    case 'R':
 		      gotten
-			= target_get_raw_trace_data (buf, offset,
+			= target_get_raw_trace_data (buf.data (), offset,
 						     trace_regblock_size);
 		      if (gotten < trace_regblock_size)
 			error (_("Failure to get requested trace"
 				 " buffer data"));
 
-		      TRACE_WRITE_R_BLOCK (writer, buf,
+		      TRACE_WRITE_R_BLOCK (writer, buf.data (),
 					   trace_regblock_size);
 		      break;
 		    case 'M':
@@ -221,7 +223,8 @@ trace_save (const char *filename, struct trace_file_writer *writer,
 			LONGEST t;
 			int j;
 
-			t = target_get_raw_trace_data (buf,offset, 10);
+			t = target_get_raw_trace_data (buf.data (),
+						       offset, 10);
 			if (t < 10)
 			  error (_("Failure to get requested trace"
 				   " buffer data"));
@@ -231,10 +234,10 @@ trace_save (const char *filename, struct trace_file_writer *writer,
 
 			gotten = 0;
 			addr = (ULONGEST)
-			  extract_unsigned_integer (buf, 8,
+			  extract_unsigned_integer (buf.data (), 8,
 						    byte_order);
 			mlen = (unsigned short)
-			  extract_unsigned_integer (&buf[8], 2,
+			  extract_unsigned_integer (&((buf.data ())[8]), 2,
 						    byte_order);
 
 			TRACE_WRITE_M_BLOCK_HEADER (writer, addr,
@@ -252,14 +255,15 @@ trace_save (const char *filename, struct trace_file_writer *writer,
 			    else
 			      read_length = mlen - j;
 
-			    t = target_get_raw_trace_data (buf,
+			    t = target_get_raw_trace_data (buf.data (),
 							   offset + j,
 							   read_length);
 			    if (t < read_length)
 			      error (_("Failure to get requested"
 				       " trace buffer data"));
 
-			    TRACE_WRITE_M_BLOCK_MEMORY (writer, buf,
+			    TRACE_WRITE_M_BLOCK_MEMORY (writer,
+							buf.data (),
 							read_length);
 
 			    j += read_length;
@@ -274,18 +278,18 @@ trace_save (const char *filename, struct trace_file_writer *writer,
 			LONGEST val;
 
 			gotten
-			  = target_get_raw_trace_data (buf, offset,
-						       12);
+			  = target_get_raw_trace_data (buf.data (),
+						       offset, 12);
 			if (gotten < 12)
 			  error (_("Failure to get requested"
 				   " trace buffer data"));
 
-			vnum  = (int) extract_signed_integer (buf,
+			vnum  = (int) extract_signed_integer (buf.data (),
 							      4,
 							      byte_order);
 			val
-			  = extract_signed_integer (&buf[4], 8,
-						    byte_order);
+			  = extract_signed_integer (&((buf.data ())[4]),
+						    8, byte_order);
 
 			TRACE_WRITE_V_BLOCK (writer, vnum, val);
 		      }
-- 
2.13.6

Re: [PATCH v2 6/6] Allow larger regblock sizes when saving tracefiles

[Ulrich Weigand]

Pedro Franco de Carvalho wrote:

> 	* tracepoint.c: Include common/byte-vector.h.
> 	(trace_save): Change type of buf to gdb::byte_vector.  Initialize
> 	with trace_regblock_size if needed.  Update uses of buf.

This is OK.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

[PATCH v2 5/6] Variable size for regs mask in collection list

[Pedro Franco de Carvalho]

This patch changes collection_list to allow larger register masks.

The mask is changed from an array to a vector and is initialized to
hold the maximum possible remote register number.  The stringify
method is updated to resize temp_buf if needed.

gdb/ChangeLog:
YYYY-MM-DD  Pedro Franco de Carvalho  <pedromfc@linux.ibm.com>

	* tracepoint.h (collection_list) <m_regs_mask>: Change type to
	std::vector<unsigned char>.
	* tracepoint.c (collection_list::collection_list): Remove
	m_regs_mask initializer from initializer list.  Resize m_regs_mask
	using the largest remote register number.
	(collection_list::add_remote_register): Remove size check on
	m_regs_mask.  Use at to access element.
	(collection_list::stringify): Change type of temp_buf to
	gdb::char_vector.  Update uses of temp_buf.  Resize if needed to
	stringify the register mask.  Use pack_hex_byte for the register
	mask.
---
 gdb/tracepoint.c | 64 +++++++++++++++++++++++++++++++++++++-------------------
 gdb/tracepoint.h |  5 +++--
 2 files changed, 45 insertions(+), 24 deletions(-)

diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index f06386158a..fffc9cc173 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -818,10 +818,8 @@ collection_list::add_remote_register (unsigned int regno)
 {
   if (info_verbose)
     printf_filtered ("collect register %d\n", regno);
-  if (regno >= (8 * sizeof (m_regs_mask)))
-    error (_("Internal: register number %d too large for tracepoint"),
-	   regno);
-  m_regs_mask[regno / 8] |= 1 << (regno % 8);
+
+  m_regs_mask.at (regno / 8) |= 1 << (regno % 8);
 }
 
 /* Add all the registers from the mask in AEXPR to the mask in the
@@ -1122,9 +1120,20 @@ collection_list::add_static_trace_data ()
 }
 
 collection_list::collection_list ()
-  : m_regs_mask (),
-    m_strace_data (false)
+  : m_strace_data (false)
 {
+  int max_remote_regno = 0;
+  for (int i = 0; i < gdbarch_num_regs (target_gdbarch ()); i++)
+    {
+      int remote_regno = (gdbarch_remote_register_number
+			  (target_gdbarch (), i));
+
+      if (remote_regno >= 0 && remote_regno > max_remote_regno)
+	max_remote_regno = remote_regno;
+    }
+
+  m_regs_mask.resize ((max_remote_regno / 8) + 1);
+
   m_memranges.reserve (128);
   m_aexprs.reserve (128);
 }
@@ -1134,7 +1143,8 @@ collection_list::collection_list ()
 std::vector<std::string>
 collection_list::stringify ()
 {
-  char temp_buf[2048];
+  gdb::char_vector temp_buf (2048);
+
   int count;
   char *end;
   long i;
@@ -1144,35 +1154,45 @@ collection_list::stringify ()
     {
       if (info_verbose)
 	printf_filtered ("\nCollecting static trace data\n");
-      end = temp_buf;
+      end = temp_buf.data ();
       *end++ = 'L';
-      str_list.emplace_back (temp_buf, end - temp_buf);
+      str_list.emplace_back (temp_buf.data (), end - temp_buf.data ());
     }
 
-  for (i = sizeof (m_regs_mask) - 1; i > 0; i--)
+  for (i = m_regs_mask.size () - 1; i > 0; i--)
     if (m_regs_mask[i] != 0)    /* Skip leading zeroes in regs_mask.  */
       break;
   if (m_regs_mask[i] != 0)	/* Prepare to send regs_mask to the stub.  */
     {
       if (info_verbose)
 	printf_filtered ("\nCollecting registers (mask): 0x");
-      end = temp_buf;
+
+      /* One char for 'R', one for the null terminator and two per
+	 mask byte.  */
+      std::size_t new_size = (i + 1) * 2 + 2;
+      if (new_size > temp_buf.size ())
+	temp_buf.resize (new_size);
+
+      end = temp_buf.data ();
       *end++ = 'R';
       for (; i >= 0; i--)
 	{
 	  QUIT;			/* Allow user to bail out with ^C.  */
 	  if (info_verbose)
 	    printf_filtered ("%02X", m_regs_mask[i]);
-	  sprintf (end, "%02X", m_regs_mask[i]);
-	  end += 2;
+
+	  end = pack_hex_byte (end, m_regs_mask[i]);
 	}
-      str_list.emplace_back (temp_buf);
+      *end = '\0';
+
+      str_list.emplace_back (temp_buf.data ());
     }
   if (info_verbose)
     printf_filtered ("\n");
   if (!m_memranges.empty () && info_verbose)
     printf_filtered ("Collecting memranges: \n");
-  for (i = 0, count = 0, end = temp_buf; i < m_memranges.size (); i++)
+  for (i = 0, count = 0, end = temp_buf.data ();
+       i < m_memranges.size (); i++)
     {
       QUIT;			/* Allow user to bail out with ^C.  */
       if (info_verbose)
@@ -1186,9 +1206,9 @@ collection_list::stringify ()
 	}
       if (count + 27 > MAX_AGENT_EXPR_LEN)
 	{
-	  str_list.emplace_back (temp_buf, count);
+	  str_list.emplace_back (temp_buf.data (), count);
 	  count = 0;
-	  end = temp_buf;
+	  end = temp_buf.data ();
 	}
 
       {
@@ -1208,7 +1228,7 @@ collection_list::stringify ()
       }
 
       count += strlen (end);
-      end = temp_buf + count;
+      end = temp_buf.data () + count;
     }
 
   for (i = 0; i < m_aexprs.size (); i++)
@@ -1216,9 +1236,9 @@ collection_list::stringify ()
       QUIT;			/* Allow user to bail out with ^C.  */
       if ((count + 10 + 2 * m_aexprs[i]->len) > MAX_AGENT_EXPR_LEN)
 	{
-	  str_list.emplace_back (temp_buf, count);
+	  str_list.emplace_back (temp_buf.data (), count);
 	  count = 0;
-	  end = temp_buf;
+	  end = temp_buf.data ();
 	}
       sprintf (end, "X%08X,", m_aexprs[i]->len);
       end += 10;		/* 'X' + 8 hex digits + ',' */
@@ -1230,9 +1250,9 @@ collection_list::stringify ()
 
   if (count != 0)
     {
-      str_list.emplace_back (temp_buf, count);
+      str_list.emplace_back (temp_buf.data (), count);
       count = 0;
-      end = temp_buf;
+      end = temp_buf.data ();
     }
 
   return str_list;
diff --git a/gdb/tracepoint.h b/gdb/tracepoint.h
index 8bdad3567e..c185672cc1 100644
--- a/gdb/tracepoint.h
+++ b/gdb/tracepoint.h
@@ -292,8 +292,9 @@ public:
   { return m_computed; }
 
 private:
-  /* room for up to 256 regs */
-  unsigned char m_regs_mask[32];
+  /* We need the allocator zero-initialize the mask, so we don't use
+     gdb::byte_vector.  */
+  std::vector<unsigned char> m_regs_mask;
 
   std::vector<memrange> m_memranges;
 
-- 
2.13.6

Re: [PATCH v2 5/6] Variable size for regs mask in collection list

[Ulrich Weigand]

Pedro Franco de Carvalho wrote:

> 	* tracepoint.h (collection_list) <m_regs_mask>: Change type to
> 	std::vector<unsigned char>.
> 	* tracepoint.c (collection_list::collection_list): Remove
> 	m_regs_mask initializer from initializer list.  Resize m_regs_mask
> 	using the largest remote register number.
> 	(collection_list::add_remote_register): Remove size check on
> 	m_regs_mask.  Use at to access element.
> 	(collection_list::stringify): Change type of temp_buf to
> 	gdb::char_vector.  Update uses of temp_buf.  Resize if needed to
> 	stringify the register mask.  Use pack_hex_byte for the register
> 	mask.

This is OK.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

[PATCH v2 4/6] Use remote register numbers in tracepoint mask

[Pedro Franco de Carvalho]

Currently, tracepoint register masks in the QTDP packets include both
internal and remote register numbers, as well as pseudo-register
numbers.

This patch changes this so that the mask only includes remote register
numbers.

Register numbers from agent expressions are already set in the mask
using remote numbers.  Other tracepoint actions used internal numbers,
e.g. "collect $regs" or "collect $<pseudoreg>".  To handle pseudoreg
numbers, an empty agent expression is created and ax_reg_mask is
called for this expression and the pseudoreg.  This will cause the ax
to set its mask with the corresponding remote raw register
numbers (using gdbarch_ax_pseudo_register_collect).

This assumes that all gdbarch_ax_pseudo_register_collect
implementations only use ax_reg_mask themselves to set the mask, and
don't generate more complicated agent expressions to collect
pseudoregs.  This seems to be the case, and seems to be the intended
use of gdbarch_ax_pseudo_register_collect, despite the gdbarch.h
comment for this function.

gdb/ChangeLog:
YYYY-MM-DD  Pedro Franco de Carvalho  <pedromfc@linux.ibm.com>

	* tracepoint.h (class collection_list) <add_register>: Remove.
	<add_remote_register, add_ax_registers, add_local_register>:
	Declare.
	<add_memrange>: Add scope parameter.
	* tracepoint.c (collection_list::add_register): Rename to ...
	(collection_list::add_remote_register): ... this.  Update comment.
	(collection_list::add_ax_registers, add_local_register): New
	methods.
	(collection_list::add_memrange): Add scope parameter.  Call
	add_local_register instead of add_register.
	(collection_list::collect_symbol): Update calls to
	add_memrange.  Call add_local_register instead of add_register.
	(collection_list::collect_symbol): Call add_ax_registers.
	(encode_actions_1): Get remote regnos for $reg action.  Call
	add_remote_register, add_ax_registers, and add_local_register.
	Update call to add_memrange.
---
 gdb/tracepoint.c | 142 +++++++++++++++++++++++++++++++------------------------
 gdb/tracepoint.h |   8 +++-
 2 files changed, 85 insertions(+), 65 deletions(-)

diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 74d1183386..f06386158a 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -811,10 +811,10 @@ memrange_sortmerge (std::vector<memrange> &memranges)
     }
 }
 
-/* Add a register to a collection list.  */
+/* Add remote register number REGNO to the collection list mask.  */
 
 void
-collection_list::add_register (unsigned int regno)
+collection_list::add_remote_register (unsigned int regno)
 {
   if (info_verbose)
     printf_filtered ("collect register %d\n", regno);
@@ -824,12 +824,62 @@ collection_list::add_register (unsigned int regno)
   m_regs_mask[regno / 8] |= 1 << (regno % 8);
 }
 
+/* Add all the registers from the mask in AEXPR to the mask in the
+   collection list.  Registers in the AEXPR mask are already remote
+   register numbers.  */
+
+void
+collection_list::add_ax_registers (struct agent_expr *aexpr)
+{
+  if (aexpr->reg_mask_len > 0)
+    {
+      for (int ndx1 = 0; ndx1 < aexpr->reg_mask_len; ndx1++)
+	{
+	  QUIT;	/* Allow user to bail out with ^C.  */
+	  if (aexpr->reg_mask[ndx1] != 0)
+	    {
+	      /* Assume chars have 8 bits.  */
+	      for (int ndx2 = 0; ndx2 < 8; ndx2++)
+		if (aexpr->reg_mask[ndx1] & (1 << ndx2))
+		  /* It's used -- record it.  */
+		  add_remote_register (ndx1 * 8 + ndx2);
+	    }
+	}
+    }
+}
+
+/* If REGNO is raw, add its corresponding remote register number to
+   the mask.  If REGNO is a pseudo-register, figure out the necessary
+   registers using a temporary agent expression.  */
+
+void
+collection_list::add_local_register (struct gdbarch *gdbarch,
+				     unsigned int regno,
+				     CORE_ADDR scope)
+{
+  if (regno < gdbarch_num_regs (gdbarch))
+    {
+      int remote_regno = gdbarch_remote_register_number (gdbarch, regno);
+
+      if (remote_regno < 0)
+	error (_("Can't collect register %d"), regno);
+
+      add_remote_register (remote_regno);
+    }
+  else
+    {
+      struct agent_expr aexpr (gdbarch, scope);
+      ax_reg_mask (&aexpr, regno);
+      add_ax_registers (&aexpr);
+    }
+}
+
 /* Add a memrange to a collection list.  */
 
 void
 collection_list::add_memrange (struct gdbarch *gdbarch,
 			       int type, bfd_signed_vma base,
-			       unsigned long len)
+			       unsigned long len, CORE_ADDR scope)
 {
   if (info_verbose)
     printf_filtered ("(%d,%s,%ld)\n", type, paddress (gdbarch, base), len);
@@ -840,7 +890,7 @@ collection_list::add_memrange (struct gdbarch *gdbarch,
   m_memranges.emplace_back (type, base, base + len);
 
   if (type != memrange_absolute)    /* Better collect the base register!  */
-    add_register (type);
+    add_local_register (gdbarch, type, scope);
 }
 
 /* Add a symbol to a collection list.  */
@@ -882,19 +932,19 @@ collection_list::collect_symbol (struct symbol *sym,
       if (TYPE_CODE (SYMBOL_TYPE (sym)) == TYPE_CODE_STRUCT)
 	treat_as_expr = 1;
       else
-	add_memrange (gdbarch, memrange_absolute, offset, len);
+	add_memrange (gdbarch, memrange_absolute, offset, len, scope);
       break;
     case LOC_REGISTER:
       reg = SYMBOL_REGISTER_OPS (sym)->register_number (sym, gdbarch);
       if (info_verbose)
 	printf_filtered ("LOC_REG[parm] %s: ", 
 			 SYMBOL_PRINT_NAME (sym));
-      add_register (reg);
+      add_local_register (gdbarch, reg, scope);
       /* Check for doubles stored in two registers.  */
       /* FIXME: how about larger types stored in 3 or more regs?  */
       if (TYPE_CODE (SYMBOL_TYPE (sym)) == TYPE_CODE_FLT &&
 	  len > register_size (gdbarch, reg))
-	add_register (reg + 1);
+	add_local_register (gdbarch, reg + 1, scope);
       break;
     case LOC_REF_ARG:
       printf_filtered ("Sorry, don't know how to do LOC_REF_ARG yet.\n");
@@ -911,7 +961,7 @@ collection_list::collect_symbol (struct symbol *sym,
 			   SYMBOL_PRINT_NAME (sym), len,
 			   paddress (gdbarch, offset), reg);
 	}
-      add_memrange (gdbarch, reg, offset, len);
+      add_memrange (gdbarch, reg, offset, len, scope);
       break;
     case LOC_REGPARM_ADDR:
       reg = SYMBOL_VALUE (sym);
@@ -923,7 +973,7 @@ collection_list::collect_symbol (struct symbol *sym,
 			   SYMBOL_PRINT_NAME (sym), len,
 			   paddress (gdbarch, offset), reg);
 	}
-      add_memrange (gdbarch, reg, offset, len);
+      add_memrange (gdbarch, reg, offset, len, scope);
       break;
     case LOC_LOCAL:
       reg = frame_regno;
@@ -935,7 +985,7 @@ collection_list::collect_symbol (struct symbol *sym,
 			   SYMBOL_PRINT_NAME (sym), len,
 			   paddress (gdbarch, offset), reg);
 	}
-      add_memrange (gdbarch, reg, offset, len);
+      add_memrange (gdbarch, reg, offset, len, scope);
       break;
 
     case LOC_UNRESOLVED:
@@ -973,21 +1023,7 @@ collection_list::collect_symbol (struct symbol *sym,
       report_agent_reqs_errors (aexpr.get ());
 
       /* Take care of the registers.  */
-      if (aexpr->reg_mask_len > 0)
-	{
-	  for (int ndx1 = 0; ndx1 < aexpr->reg_mask_len; ndx1++)
-	    {
-	      QUIT;	/* Allow user to bail out with ^C.  */
-	      if (aexpr->reg_mask[ndx1] != 0)
-		{
-		  /* Assume chars have 8 bits.  */
-		  for (int ndx2 = 0; ndx2 < 8; ndx2++)
-		    if (aexpr->reg_mask[ndx1] & (1 << ndx2))
-		      /* It's used -- record it.  */
-		      add_register (ndx1 * 8 + ndx2);
-		}
-	    }
-	}
+      add_ax_registers (aexpr.get ());
 
       add_aexpr (std::move (aexpr));
     }
@@ -1257,8 +1293,18 @@ encode_actions_1 (struct command_line *action,
 
 	      if (0 == strncasecmp ("$reg", action_exp, 4))
 		{
-		  for (i = 0; i < gdbarch_num_regs (target_gdbarch ()); i++)
-		    collect->add_register (i);
+		  for (i = 0; i < gdbarch_num_regs (target_gdbarch ());
+		       i++)
+		    {
+		      int remote_regno = (gdbarch_remote_register_number
+					  (target_gdbarch (), i));
+
+		      /* Ignore arch regnos without a corresponding
+			 remote regno.  This can happen for regnos not
+			 in the tdesc.  */
+		      if (remote_regno >= 0)
+			collect->add_remote_register (remote_regno);
+		    }
 		  action_exp = strchr (action_exp, ',');	/* more? */
 		}
 	      else if (0 == strncasecmp ("$arg", action_exp, 4))
@@ -1292,23 +1338,7 @@ encode_actions_1 (struct command_line *action,
 		  report_agent_reqs_errors (aexpr.get ());
 
 		  /* take care of the registers */
-		  if (aexpr->reg_mask_len > 0)
-		    {
-		      for (int ndx1 = 0; ndx1 < aexpr->reg_mask_len; ndx1++)
-			{
-			  QUIT;	/* allow user to bail out with ^C */
-			  if (aexpr->reg_mask[ndx1] != 0)
-			    {
-			      /* assume chars have 8 bits */
-			      for (int ndx2 = 0; ndx2 < 8; ndx2++)
-				if (aexpr->reg_mask[ndx1] & (1 << ndx2))
-				  {
-				    /* It's used -- record it.  */
-				    collect->add_register (ndx1 * 8 + ndx2);
-				  }
-			    }
-			}
-		    }
+		  collect->add_ax_registers (aexpr.get ());
 
 		  collect->add_aexpr (std::move (aexpr));
 		  action_exp = strchr (action_exp, ',');	/* more? */
@@ -1340,7 +1370,8 @@ encode_actions_1 (struct command_line *action,
 					  name);
 			if (info_verbose)
 			  printf_filtered ("OP_REGISTER: ");
-			collect->add_register (i);
+			collect->add_local_register (target_gdbarch (),
+						     i, tloc->address);
 			break;
 		      }
 
@@ -1352,7 +1383,8 @@ encode_actions_1 (struct command_line *action,
 		      check_typedef (exp->elts[1].type);
 		      collect->add_memrange (target_gdbarch (),
 					     memrange_absolute, addr,
-					     TYPE_LENGTH (exp->elts[1].type));
+					     TYPE_LENGTH (exp->elts[1].type),
+					     tloc->address);
 		      collect->append_exp (exp.get ());
 		      break;
 
@@ -1381,23 +1413,7 @@ encode_actions_1 (struct command_line *action,
 		      report_agent_reqs_errors (aexpr.get ());
 
 		      /* Take care of the registers.  */
-		      if (aexpr->reg_mask_len > 0)
-			{
-			  for (int ndx1 = 0; ndx1 < aexpr->reg_mask_len; ndx1++)
-			    {
-			      QUIT;	/* Allow user to bail out with ^C.  */
-			      if (aexpr->reg_mask[ndx1] != 0)
-				{
-				  /* Assume chars have 8 bits.  */
-				  for (int ndx2 = 0; ndx2 < 8; ndx2++)
-				    if (aexpr->reg_mask[ndx1] & (1 << ndx2))
-				      {
-					/* It's used -- record it.  */
-					collect->add_register (ndx1 * 8 + ndx2);
-				      }
-				}
-			    }
-			}
+		      collect->add_ax_registers (aexpr.get ());
 
 		      collect->add_aexpr (std::move (aexpr));
 		      collect->append_exp (exp.get ());
diff --git a/gdb/tracepoint.h b/gdb/tracepoint.h
index 42e413018a..8bdad3567e 100644
--- a/gdb/tracepoint.h
+++ b/gdb/tracepoint.h
@@ -262,10 +262,14 @@ public:
   /* Add AEXPR to the list, taking ownership.  */
   void add_aexpr (agent_expr_up aexpr);
 
-  void add_register (unsigned int regno);
+  void add_remote_register (unsigned int regno);
+  void add_ax_registers (struct agent_expr *aexpr);
+  void add_local_register (struct gdbarch *gdbarch,
+			   unsigned int regno,
+			   CORE_ADDR scope);
   void add_memrange (struct gdbarch *gdbarch,
 		     int type, bfd_signed_vma base,
-		     unsigned long len);
+		     unsigned long len, CORE_ADDR scope);
   void collect_symbol (struct symbol *sym,
 		       struct gdbarch *gdbarch,
 		       long frame_regno, long frame_offset,
-- 
2.13.6

Re: [PATCH v2 4/6] Use remote register numbers in tracepoint mask

[Ulrich Weigand]

Pedro Franco de Carvalho wrote:

> Currently, tracepoint register masks in the QTDP packets include both
> internal and remote register numbers, as well as pseudo-register
> numbers.

Yes, this looks like a bug to me.

> Register numbers from agent expressions are already set in the mask
> using remote numbers.  Other tracepoint actions used internal numbers,
> e.g. "collect $regs" or "collect $<pseudoreg>".  To handle pseudoreg
> numbers, an empty agent expression is created and ax_reg_mask is
> called for this expression and the pseudoreg.  This will cause the ax
> to set its mask with the corresponding remote raw register
> numbers (using gdbarch_ax_pseudo_register_collect).
> 
> This assumes that all gdbarch_ax_pseudo_register_collect
> implementations only use ax_reg_mask themselves to set the mask, and
> don't generate more complicated agent expressions to collect
> pseudoregs.  This seems to be the case, and seems to be the intended
> use of gdbarch_ax_pseudo_register_collect, despite the gdbarch.h
> comment for this function.

It would be good to update that comment then, if this behavior
is now an actual requirement.

The alternative I guess would be to have collect_symbol fall back to
the "treat_as_expr" case for collecting pseudo registers; and then
possibly implementing as optimization a check whether the agent
expression is actually empty except for the register mask ...
Something similar would then also have to be done in encode_actions_1.

Do you think this would be doable?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH v2 4/6] Use remote register numbers in tracepoint mask

[Pedro Franco de Carvalho]

Ulrich Weigand <uweigand@de.ibm.com> writes:

> The alternative I guess would be to have collect_symbol fall back to
> the "treat_as_expr" case for collecting pseudo registers; and then
> possibly implementing as optimization a check whether the agent
> expression is actually empty except for the register mask ...
> Something similar would then also have to be done in encode_actions_1.
>
> Do you think this would be doable?

I tried doing something like this but in add_local_register (this way
even if add_memranges uses a pseudoregister for some reason, it should
also work). See the next message for the updated patch.

I just check if ax_regs_mask adds anything to the expression other than
the mask, and add the expression to the collection_list.

I'm not sure that this is the best approach, would it make sense to
create a full expression, and generate an ax from that?

This means that if a pseudo-register does something more complicated
like reading memory, the ax it generates through
gdbarch_ax_pseudo_register_collect should include the trace bytecodes
for the memory it needs.

I don't think this was the original intent, and it isn't clear that the
rest of the ax code was made to work with that, but I tested this with a
pseudoreg that reads the backchain from r1 (in powerpc), and it works.

Alternatively, I can just raise an error in add_local_register if the
length of the ax after ax_regs_mask is non-zero.

I also refactored the lines that check if an ax is valid into a
function, which is now also called from add_local_register too, since a
full ax might be added.

Previously only validate_actionline checked that the expression was not
too long, now every place that generates an ax does this as well.

Thanks for the review!

--
Pedro Franco de Carvalho

[PATCH v2 4/6] Use remote register numbers in tracepoint mask

[Pedro Franco de Carvalho]

Currently, tracepoint register masks in the QTDP packets include both
internal and remote register numbers, as well as pseudo-register
numbers.

This patch changes this so that the mask only includes remote register
numbers.

Register numbers from agent expressions are already set in the mask
using remote numbers.  Other tracepoint actions used internal numbers,
e.g. "collect $regs" or "collect $<pseudoreg>".  To handle pseudoreg
numbers, an empty agent expression is created and ax_reg_mask is
called for this expression and the pseudoreg.  This will cause the ax
to set its mask with the corresponding remote raw register
numbers (using ax_regs_mask, which calls
gdbarch_ax_pseudo_register_collect).

If ax_regs_mask and gdbarch_ax_pseudo_register_collect also generate
more ax bytecode, the ax is also appended to the collection list.  It
isn't clear that this was the original intent for
gdbarch_ax_pseudo_register_collect, and none of the arches seem to do
this, but if this changes in the future, it should work.

The patch also refactors the code used by validate_action line to
validate axs into a function that is now called from every place that
generates axs.  Previously, some parts of tracepoint.c that generated
axs didn't check if the ax length was greater than MAX_AGENT_EXPR_LEN.

gdb/ChangeLog:
YYYY-MM-DD  Pedro Franco de Carvalho  <pedromfc@linux.ibm.com>

	* tracepoint.h (class collection_list) <add_register>: Remove.
	<add_remote_register, add_ax_registers, add_local_register>:
	Declare.
	<add_memrange>: Add scope parameter.
	* tracepoint.c (encode_actions_1): Likewise.
	(collection_list::add_register): Rename to ...
	(collection_list::add_remote_register): ... this.  Update comment.
	(collection_list::add_ax_registers, add_local_register): New
	methods.
	(collection_list::add_memrange): Add scope parameter.  Call
	add_local_register instead of add_register.
	(finalize_tracepoint_aexpr): New function.
	(collection_list::collect_symbol): Update calls to add_memrange.
	Call add_local_register instead of add_register.  Call
	add_ax_registers. Call finalize_tracepoint_aexpr.
	(encode_actions_1): Get remote regnos for $reg action.  Call
	add_remote_register, add_ax_registers, and add_local_register.
	Update call to add_memrange. Call finalize_tracepoint_aexpr.
	(validate_actionline): Call finalize_tracepoint_aexpr.
---
 gdb/tracepoint.c | 194 +++++++++++++++++++++++++++++++------------------------
 gdb/tracepoint.h |   8 ++-
 2 files changed, 116 insertions(+), 86 deletions(-)

diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 74d1183386..55ab13f952 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -615,6 +615,19 @@ report_agent_reqs_errors (struct agent_expr *aexpr)
     error (_("Expression is too complicated."));
 }
 
+/* Call ax_reqs on AEXPR and raise an error if something is wrong.  */
+
+static void
+finalize_tracepoint_aexpr (struct agent_expr *aexpr)
+{
+  ax_reqs (aexpr);
+
+  if (aexpr->len > MAX_AGENT_EXPR_LEN)
+    error (_("Expression is too complicated."));
+
+  report_agent_reqs_errors (aexpr);
+}
+
 /* worker function */
 void
 validate_actionline (const char *line, struct breakpoint *b)
@@ -699,12 +712,7 @@ validate_actionline (const char *line, struct breakpoint *b)
 							exp.get (),
 							trace_string);
 
-	      if (aexpr->len > MAX_AGENT_EXPR_LEN)
-		error (_("Expression is too complicated."));
-
-	      ax_reqs (aexpr.get ());
-
-	      report_agent_reqs_errors (aexpr.get ());
+	      finalize_tracepoint_aexpr (aexpr.get ());
 	    }
 	}
       while (p && *p++ == ',');
@@ -731,11 +739,7 @@ validate_actionline (const char *line, struct breakpoint *b)
 		 long.  */
 	      agent_expr_up aexpr = gen_eval_for_expr (loc->address, exp.get ());
 
-	      if (aexpr->len > MAX_AGENT_EXPR_LEN)
-		error (_("Expression is too complicated."));
-
-	      ax_reqs (aexpr.get ());
-	      report_agent_reqs_errors (aexpr.get ());
+	      finalize_tracepoint_aexpr (aexpr.get ());
 	    }
 	}
       while (p && *p++ == ',');
@@ -811,10 +815,10 @@ memrange_sortmerge (std::vector<memrange> &memranges)
     }
 }
 
-/* Add a register to a collection list.  */
+/* Add remote register number REGNO to the collection list mask.  */
 
 void
-collection_list::add_register (unsigned int regno)
+collection_list::add_remote_register (unsigned int regno)
 {
   if (info_verbose)
     printf_filtered ("collect register %d\n", regno);
@@ -824,12 +828,74 @@ collection_list::add_register (unsigned int regno)
   m_regs_mask[regno / 8] |= 1 << (regno % 8);
 }
 
+/* Add all the registers from the mask in AEXPR to the mask in the
+   collection list.  Registers in the AEXPR mask are already remote
+   register numbers.  */
+
+void
+collection_list::add_ax_registers (struct agent_expr *aexpr)
+{
+  if (aexpr->reg_mask_len > 0)
+    {
+      for (int ndx1 = 0; ndx1 < aexpr->reg_mask_len; ndx1++)
+	{
+	  QUIT;	/* Allow user to bail out with ^C.  */
+	  if (aexpr->reg_mask[ndx1] != 0)
+	    {
+	      /* Assume chars have 8 bits.  */
+	      for (int ndx2 = 0; ndx2 < 8; ndx2++)
+		if (aexpr->reg_mask[ndx1] & (1 << ndx2))
+		  /* It's used -- record it.  */
+		  add_remote_register (ndx1 * 8 + ndx2);
+	    }
+	}
+    }
+}
+
+/* If REGNO is raw, add its corresponding remote register number to
+   the mask.  If REGNO is a pseudo-register, figure out the necessary
+   registers using a temporary agent expression, and add it to the
+   list if it needs more than just a mask.  */
+
+void
+collection_list::add_local_register (struct gdbarch *gdbarch,
+				     unsigned int regno,
+				     CORE_ADDR scope)
+{
+  if (regno < gdbarch_num_regs (gdbarch))
+    {
+      int remote_regno = gdbarch_remote_register_number (gdbarch, regno);
+
+      if (remote_regno < 0)
+	error (_("Can't collect register %d"), regno);
+
+      add_remote_register (remote_regno);
+    }
+  else
+    {
+      agent_expr_up aexpr (new agent_expr (gdbarch, scope));
+
+      ax_reg_mask (aexpr.get (), regno);
+
+      finalize_tracepoint_aexpr (aexpr.get ());
+
+      add_ax_registers (aexpr.get ());
+
+      /* Usually ax_reg_mask for a pseudo-regiser only sets the
+	 corresponding raw registers in the ax mask, but if this isn't
+	 the case add the expression that is generated to the
+	 collection list.  */
+      if (aexpr->len > 0)
+	add_aexpr (std::move (aexpr));
+    }
+}
+
 /* Add a memrange to a collection list.  */
 
 void
 collection_list::add_memrange (struct gdbarch *gdbarch,
 			       int type, bfd_signed_vma base,
-			       unsigned long len)
+			       unsigned long len, CORE_ADDR scope)
 {
   if (info_verbose)
     printf_filtered ("(%d,%s,%ld)\n", type, paddress (gdbarch, base), len);
@@ -840,7 +906,7 @@ collection_list::add_memrange (struct gdbarch *gdbarch,
   m_memranges.emplace_back (type, base, base + len);
 
   if (type != memrange_absolute)    /* Better collect the base register!  */
-    add_register (type);
+    add_local_register (gdbarch, type, scope);
 }
 
 /* Add a symbol to a collection list.  */
@@ -882,19 +948,19 @@ collection_list::collect_symbol (struct symbol *sym,
       if (TYPE_CODE (SYMBOL_TYPE (sym)) == TYPE_CODE_STRUCT)
 	treat_as_expr = 1;
       else
-	add_memrange (gdbarch, memrange_absolute, offset, len);
+	add_memrange (gdbarch, memrange_absolute, offset, len, scope);
       break;
     case LOC_REGISTER:
       reg = SYMBOL_REGISTER_OPS (sym)->register_number (sym, gdbarch);
       if (info_verbose)
 	printf_filtered ("LOC_REG[parm] %s: ", 
 			 SYMBOL_PRINT_NAME (sym));
-      add_register (reg);
+      add_local_register (gdbarch, reg, scope);
       /* Check for doubles stored in two registers.  */
       /* FIXME: how about larger types stored in 3 or more regs?  */
       if (TYPE_CODE (SYMBOL_TYPE (sym)) == TYPE_CODE_FLT &&
 	  len > register_size (gdbarch, reg))
-	add_register (reg + 1);
+	add_local_register (gdbarch, reg + 1, scope);
       break;
     case LOC_REF_ARG:
       printf_filtered ("Sorry, don't know how to do LOC_REF_ARG yet.\n");
@@ -911,7 +977,7 @@ collection_list::collect_symbol (struct symbol *sym,
 			   SYMBOL_PRINT_NAME (sym), len,
 			   paddress (gdbarch, offset), reg);
 	}
-      add_memrange (gdbarch, reg, offset, len);
+      add_memrange (gdbarch, reg, offset, len, scope);
       break;
     case LOC_REGPARM_ADDR:
       reg = SYMBOL_VALUE (sym);
@@ -923,7 +989,7 @@ collection_list::collect_symbol (struct symbol *sym,
 			   SYMBOL_PRINT_NAME (sym), len,
 			   paddress (gdbarch, offset), reg);
 	}
-      add_memrange (gdbarch, reg, offset, len);
+      add_memrange (gdbarch, reg, offset, len, scope);
       break;
     case LOC_LOCAL:
       reg = frame_regno;
@@ -935,7 +1001,7 @@ collection_list::collect_symbol (struct symbol *sym,
 			   SYMBOL_PRINT_NAME (sym), len,
 			   paddress (gdbarch, offset), reg);
 	}
-      add_memrange (gdbarch, reg, offset, len);
+      add_memrange (gdbarch, reg, offset, len, scope);
       break;
 
     case LOC_UNRESOLVED:
@@ -968,26 +1034,10 @@ collection_list::collect_symbol (struct symbol *sym,
 	  return;
 	}
 
-      ax_reqs (aexpr.get ());
-
-      report_agent_reqs_errors (aexpr.get ());
+      finalize_tracepoint_aexpr (aexpr.get ());
 
       /* Take care of the registers.  */
-      if (aexpr->reg_mask_len > 0)
-	{
-	  for (int ndx1 = 0; ndx1 < aexpr->reg_mask_len; ndx1++)
-	    {
-	      QUIT;	/* Allow user to bail out with ^C.  */
-	      if (aexpr->reg_mask[ndx1] != 0)
-		{
-		  /* Assume chars have 8 bits.  */
-		  for (int ndx2 = 0; ndx2 < 8; ndx2++)
-		    if (aexpr->reg_mask[ndx1] & (1 << ndx2))
-		      /* It's used -- record it.  */
-		      add_register (ndx1 * 8 + ndx2);
-		}
-	    }
-	}
+      add_ax_registers (aexpr.get ());
 
       add_aexpr (std::move (aexpr));
     }
@@ -1257,8 +1307,18 @@ encode_actions_1 (struct command_line *action,
 
 	      if (0 == strncasecmp ("$reg", action_exp, 4))
 		{
-		  for (i = 0; i < gdbarch_num_regs (target_gdbarch ()); i++)
-		    collect->add_register (i);
+		  for (i = 0; i < gdbarch_num_regs (target_gdbarch ());
+		       i++)
+		    {
+		      int remote_regno = (gdbarch_remote_register_number
+					  (target_gdbarch (), i));
+
+		      /* Ignore arch regnos without a corresponding
+			 remote regno.  This can happen for regnos not
+			 in the tdesc.  */
+		      if (remote_regno >= 0)
+			collect->add_remote_register (remote_regno);
+		    }
 		  action_exp = strchr (action_exp, ',');	/* more? */
 		}
 	      else if (0 == strncasecmp ("$arg", action_exp, 4))
@@ -1288,27 +1348,10 @@ encode_actions_1 (struct command_line *action,
 						    target_gdbarch (),
 						    trace_string);
 
-		  ax_reqs (aexpr.get ());
-		  report_agent_reqs_errors (aexpr.get ());
+		  finalize_tracepoint_aexpr (aexpr.get ());
 
 		  /* take care of the registers */
-		  if (aexpr->reg_mask_len > 0)
-		    {
-		      for (int ndx1 = 0; ndx1 < aexpr->reg_mask_len; ndx1++)
-			{
-			  QUIT;	/* allow user to bail out with ^C */
-			  if (aexpr->reg_mask[ndx1] != 0)
-			    {
-			      /* assume chars have 8 bits */
-			      for (int ndx2 = 0; ndx2 < 8; ndx2++)
-				if (aexpr->reg_mask[ndx1] & (1 << ndx2))
-				  {
-				    /* It's used -- record it.  */
-				    collect->add_register (ndx1 * 8 + ndx2);
-				  }
-			    }
-			}
-		    }
+		  collect->add_ax_registers (aexpr.get ());
 
 		  collect->add_aexpr (std::move (aexpr));
 		  action_exp = strchr (action_exp, ',');	/* more? */
@@ -1340,7 +1383,8 @@ encode_actions_1 (struct command_line *action,
 					  name);
 			if (info_verbose)
 			  printf_filtered ("OP_REGISTER: ");
-			collect->add_register (i);
+			collect->add_local_register (target_gdbarch (),
+						     i, tloc->address);
 			break;
 		      }
 
@@ -1352,7 +1396,8 @@ encode_actions_1 (struct command_line *action,
 		      check_typedef (exp->elts[1].type);
 		      collect->add_memrange (target_gdbarch (),
 					     memrange_absolute, addr,
-					     TYPE_LENGTH (exp->elts[1].type));
+					     TYPE_LENGTH (exp->elts[1].type),
+					     tloc->address);
 		      collect->append_exp (exp.get ());
 		      break;
 
@@ -1376,28 +1421,10 @@ encode_actions_1 (struct command_line *action,
 								exp.get (),
 								trace_string);
 
-		      ax_reqs (aexpr.get ());
-
-		      report_agent_reqs_errors (aexpr.get ());
+		      finalize_tracepoint_aexpr (aexpr.get ());
 
 		      /* Take care of the registers.  */
-		      if (aexpr->reg_mask_len > 0)
-			{
-			  for (int ndx1 = 0; ndx1 < aexpr->reg_mask_len; ndx1++)
-			    {
-			      QUIT;	/* Allow user to bail out with ^C.  */
-			      if (aexpr->reg_mask[ndx1] != 0)
-				{
-				  /* Assume chars have 8 bits.  */
-				  for (int ndx2 = 0; ndx2 < 8; ndx2++)
-				    if (aexpr->reg_mask[ndx1] & (1 << ndx2))
-				      {
-					/* It's used -- record it.  */
-					collect->add_register (ndx1 * 8 + ndx2);
-				      }
-				}
-			    }
-			}
+		      collect->add_ax_registers (aexpr.get ());
 
 		      collect->add_aexpr (std::move (aexpr));
 		      collect->append_exp (exp.get ());
@@ -1422,8 +1449,7 @@ encode_actions_1 (struct command_line *action,
 		  agent_expr_up aexpr = gen_eval_for_expr (tloc->address,
 							   exp.get ());
 
-		  ax_reqs (aexpr.get ());
-		  report_agent_reqs_errors (aexpr.get ());
+		  finalize_tracepoint_aexpr (aexpr.get ());
 
 		  /* Even though we're not officially collecting, add
 		     to the collect list anyway.  */
diff --git a/gdb/tracepoint.h b/gdb/tracepoint.h
index 42e413018a..8bdad3567e 100644
--- a/gdb/tracepoint.h
+++ b/gdb/tracepoint.h
@@ -262,10 +262,14 @@ public:
   /* Add AEXPR to the list, taking ownership.  */
   void add_aexpr (agent_expr_up aexpr);
 
-  void add_register (unsigned int regno);
+  void add_remote_register (unsigned int regno);
+  void add_ax_registers (struct agent_expr *aexpr);
+  void add_local_register (struct gdbarch *gdbarch,
+			   unsigned int regno,
+			   CORE_ADDR scope);
   void add_memrange (struct gdbarch *gdbarch,
 		     int type, bfd_signed_vma base,
-		     unsigned long len);
+		     unsigned long len, CORE_ADDR scope);
   void collect_symbol (struct symbol *sym,
 		       struct gdbarch *gdbarch,
 		       long frame_regno, long frame_offset,
-- 
2.13.6

Re: [PATCH v2 4/6] Use remote register numbers in tracepoint mask

[Ulrich Weigand]

Pedro Franco de Carvalho wrote:

> If ax_regs_mask and gdbarch_ax_pseudo_register_collect also generate
> more ax bytecode, the ax is also appended to the collection list.  It
> isn't clear that this was the original intent for
> gdbarch_ax_pseudo_register_collect, and none of the arches seem to do
> this, but if this changes in the future, it should work.

OK, this makes sense to me.

> 	* tracepoint.h (class collection_list) <add_register>: Remove.
> 	<add_remote_register, add_ax_registers, add_local_register>:
> 	Declare.
> 	<add_memrange>: Add scope parameter.
> 	* tracepoint.c (encode_actions_1): Likewise.
> 	(collection_list::add_register): Rename to ...
> 	(collection_list::add_remote_register): ... this.  Update comment.
> 	(collection_list::add_ax_registers, add_local_register): New
> 	methods.
> 	(collection_list::add_memrange): Add scope parameter.  Call
> 	add_local_register instead of add_register.
> 	(finalize_tracepoint_aexpr): New function.
> 	(collection_list::collect_symbol): Update calls to add_memrange.
> 	Call add_local_register instead of add_register.  Call
> 	add_ax_registers. Call finalize_tracepoint_aexpr.
> 	(encode_actions_1): Get remote regnos for $reg action.  Call
> 	add_remote_register, add_ax_registers, and add_local_register.
> 	Update call to add_memrange. Call finalize_tracepoint_aexpr.
> 	(validate_actionline): Call finalize_tracepoint_aexpr.

This is OK.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH v2 4/6] Use remote register numbers in tracepoint mask

[Pedro Franco de Carvalho]

Ulrich Weigand <uweigand@de.ibm.com> writes:

> This is OK.
>
> Thanks,
> Ulrich

Thanks for the review. I've checked these in now.

--
Pedro Franco de Carvalho

[PATCH v2 1/6] Fix indentation in remote_target::download_tracepoint

[Pedro Franco de Carvalho]

gdb/ChangeLog:
YYYY-MM-DD  Pedro Franco de Carvalho  <pedromfc@linux.ibm.com>

	* remote.c (remote_target::download_tracepoint): Fix indentation
	in for block.
---
 gdb/remote.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index a61680d242..088efaad12 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -12945,24 +12945,24 @@ remote_target::download_tracepoint (struct bp_location *loc)
 	error (_("Error on target while setting tracepoints."));
     }
 
-    for (auto action_it = stepping_actions.begin ();
-	 action_it != stepping_actions.end (); action_it++)
-      {
-	QUIT;	/* Allow user to bail out with ^C.  */
-
-	bool is_first = action_it == stepping_actions.begin ();
-	bool has_more = action_it != stepping_actions.end ();
-
-	xsnprintf (buf, BUF_SIZE, "QTDP:-%x:%s:%s%s%s",
-		   b->number, addrbuf, /* address */
-		   is_first ? "S" : "",
-		   action_it->c_str (),
-		   has_more ? "-" : "");
-	putpkt (buf);
-	remote_get_noisy_reply ();
-	if (strcmp (rs->buf, "OK"))
-	  error (_("Error on target while setting tracepoints."));
-      }
+  for (auto action_it = stepping_actions.begin ();
+       action_it != stepping_actions.end (); action_it++)
+    {
+      QUIT;	/* Allow user to bail out with ^C.  */
+
+      bool is_first = action_it == stepping_actions.begin ();
+      bool has_more = action_it != stepping_actions.end ();
+
+      xsnprintf (buf, BUF_SIZE, "QTDP:-%x:%s:%s%s%s",
+		 b->number, addrbuf, /* address */
+		 is_first ? "S" : "",
+		 action_it->c_str (),
+		 has_more ? "-" : "");
+      putpkt (buf);
+      remote_get_noisy_reply ();
+      if (strcmp (rs->buf, "OK"))
+	error (_("Error on target while setting tracepoints."));
+    }
 
   if (packet_support (PACKET_TracepointSource) == PACKET_ENABLE)
     {
-- 
2.13.6

Re: [PATCH v2 1/6] Fix indentation in remote_target::download_tracepoint

[Ulrich Weigand]

Pedro Franco de Carvalho wrote:

> 	* remote.c (remote_target::download_tracepoint): Fix indentation
> 	in for block.

This is (still) OK.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

[PATCH v2 2/6] Remove trailing '-' from the last QTDP action packet

[Pedro Franco de Carvalho]

The has_more predicate in remote_target::download_tracepoint always
evaluates to true, so the last action packet will be sent with a
trailing '-'.  This patch changes the predicate to remove the last
trailing '-'.

gdb/ChangeLog:
YYYY-MM-DD  Pedro Franco de Carvalho  <pedromfc@linux.ibm.com>

	* remote.c (remote_target::download_tracepoint): Fix the has_more
	predicate in the QTDP action list iteration.
---
 gdb/remote.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 088efaad12..e3180923ee 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -12932,7 +12932,7 @@ remote_target::download_tracepoint (struct bp_location *loc)
     {
       QUIT;	/* Allow user to bail out with ^C.  */
 
-      bool has_more = (action_it != tdp_actions.end ()
+      bool has_more = ((action_it + 1) != tdp_actions.end ()
 		       || !stepping_actions.empty ());
 
       xsnprintf (buf, BUF_SIZE, "QTDP:-%x:%s:%s%c",
@@ -12951,7 +12951,7 @@ remote_target::download_tracepoint (struct bp_location *loc)
       QUIT;	/* Allow user to bail out with ^C.  */
 
       bool is_first = action_it == stepping_actions.begin ();
-      bool has_more = action_it != stepping_actions.end ();
+      bool has_more = (action_it + 1) != stepping_actions.end ();
 
       xsnprintf (buf, BUF_SIZE, "QTDP:-%x:%s:%s%s%s",
 		 b->number, addrbuf, /* address */
-- 
2.13.6

Re: [PATCH v2 2/6] Remove trailing '-' from the last QTDP action packet

[Ulrich Weigand]

Pedro Franco de Carvalho wrote:

> 	* remote.c (remote_target::download_tracepoint): Fix the has_more
> 	predicate in the QTDP action list iteration.

This is also still OK.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com