[PATCH 4/4] Variable size for regs mask in collection list

[PATCH 4/4] 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 with
num_regs + num_pseudoregs. Although no pseudoreg numbers should be set
in the mask, some parts of collection_list still do this.

The mask is also resized as needed when new registers are added to the
mask.

YYYY-MM-DD  Pedro Franco de Carvalho  <pedromfc@linux.vnet.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
	in using the number of registers from the arch.
	(collection_list::add_register): Resize m_regs_mask instead of
	throwing error when regno is too large.
	(collection_list::stringify): Use size () instead of sizeof. Use
	xsnprintf instead of sprintf.
---
 gdb/tracepoint.c | 28 +++++++++++++++++++++-------
 gdb/tracepoint.h |  2 +-
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 5af3cfe202..2cffc0251e 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -818,9 +818,10 @@ collection_list::add_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);
+
+  if (regno / 8 >= m_regs_mask.size ())
+    m_regs_mask.resize ((regno / 8) + 1);
+
   m_regs_mask[regno / 8] |= 1 << (regno % 8);
 }
 
@@ -1086,9 +1087,21 @@ collection_list::add_static_trace_data ()
 }
 
 collection_list::collection_list ()
-  : m_regs_mask (),
-    m_strace_data (false)
+  : m_strace_data (false)
 {
+  /* Guess the number of bytes needed for the register mask. If it's
+     too low, it will be expanded in add_register. If it's too high,
+     stringify will ignore the extra bytes.
+
+     The mask shouldn't include pseudoreg numbers, but
+     encode_actions_1 currently doesn't handle remote register numbers
+     and pseudoregs properly for tracepoint actions that don't
+     generate an AX (e.g. "collect $<pseudoreg>").  */
+  int num_regs = gdbarch_num_regs (target_gdbarch ())
+    + gdbarch_num_pseudo_regs (target_gdbarch ());
+
+  m_regs_mask.resize ((num_regs / 8) + 1);
+
   m_memranges.reserve (128);
   m_aexprs.reserve (128);
 }
@@ -1113,7 +1126,7 @@ collection_list::stringify ()
       str_list.emplace_back (temp_buf, end - temp_buf);
     }
 
-  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.  */
@@ -1127,7 +1140,8 @@ collection_list::stringify ()
 	  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]);
+	  xsnprintf (end, sizeof (temp_buf) - (end - temp_buf),
+		     "%02X", m_regs_mask[i]);
 	  end += 2;
 	}
       str_list.emplace_back (temp_buf);
diff --git a/gdb/tracepoint.h b/gdb/tracepoint.h
index 42e413018a..59279a73ef 100644
--- a/gdb/tracepoint.h
+++ b/gdb/tracepoint.h
@@ -289,7 +289,7 @@ public:
 
 private:
   /* room for up to 256 regs */
-  unsigned char m_regs_mask[32];
+  std::vector<unsigned char> m_regs_mask;
 
   std::vector<memrange> m_memranges;
 
-- 
2.13.6

[PATCH 1/4] Fix indentation in remote_target::download_tracepoint

[Pedro Franco de Carvalho]

gdb/ChangeLog:
YYYY-MM-DD  Pedro Franco de Carvalho  <pedromfc@linux.vnet.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 7cef0cf19f..ff6280a47d 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -12906,24 +12906,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

[PATCH 0/4] Allow larger sizes for tracepoint register masks

[Pedro Franco de Carvalho]

Currently, the size of the register mask for tracepoint action packets
is limited to 32 bytes, due to the size of an internal array. This
patch series changes the array to a vector so that it can be sized
according to the number of registers in the arch.

The motivation for this is that I'm working on enabling additional
registers for powerpc, and there are more registers that can be
represented with the current mask size, so actions like "collect
$regs" don't work.

The series also changes the remote target to use the remote packet
size when sending tracepoint packets, because with larger masks it no
longer can be assumed that the mask will fit in a packet.

Patches 1-2 fix minor issues in remote_target::download_tracepoint, so
that patch 3 can change the method to use the remote packet
size. Patch 1 fixes an indentation error, and patch 2 fixes an error
that caused the last QTDP action packet to include the '-' trailing
indicator for additional action packets.

Patch 3 uses the remote packet size for the buffer used to build the
QTDP packets and changes all writes to this buffer to xsnprintf.

Patch 4 changes collection_list to build a variable-sized register
mask.

I also noticed that collection_list sometimes uses local register
numbers, and sometimes remote register numbers to set the bits in the
register mask, and allows pseudoregister numbers to be set in the
mask, but his issue isn't fixed in this series.

Pedro Franco de Carvalho (4):
  Fix indentation in remote_target::download_tracepoint
  Remove trailing '-' from the last QTDP action packet
  Use get_remote_packet_size in download_tracepoint
  Variable size for regs mask in collection list

 gdb/remote.c     | 102 ++++++++++++++++++++++++++++++++-----------------------
 gdb/tracepoint.c |  28 +++++++++++----
 gdb/tracepoint.h |   2 +-
 3 files changed, 82 insertions(+), 50 deletions(-)

-- 
2.13.6

[PATCH 3/4] 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.

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. All writes to the buffer are changed to use
xsnprintf, so that no assumptions on the size of the action strings
are needed.

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

	* remote.c (remote_target::download_tracepoint): Remove BUF_SIZE
	and pkt. 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 xsnprintf.
---
 gdb/remote.c | 68 ++++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 43 insertions(+), 25 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 04dc67fdc0..f5fbb7e7ce 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -12793,23 +12793,24 @@ 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 ();
 
+  /* 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,
+  xsnprintf (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);
@@ -12824,8 +12825,9 @@ 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));
+	    xsnprintf (buf.data () + strlen (buf.data ()),
+		       buf.size () - strlen (buf.data ()),
+		       ":F%x", gdb_insn_length (loc->gdbarch, tpaddr));
 	  else
 	    /* If it passed validation at definition but fails now,
 	       something is very wrong.  */
@@ -12849,7 +12851,8 @@ remote_target::download_tracepoint (struct bp_location *loc)
 	  struct static_tracepoint_marker marker;
 
 	  if (target_static_tracepoint_marker_at (tpaddr, &marker))
-	    strcat (buf, ":S");
+	    xsnprintf (buf.data () + strlen (buf.data ()),
+		       buf.size () - strlen (buf.data ()), ":S");
 	  else
 	    error (_("Static tracepoint not valid during download"));
 	}
@@ -12868,12 +12871,23 @@ remote_target::download_tracepoint (struct bp_location *loc)
       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,",
+	  xsnprintf (buf.data () + strlen (buf.data ()),
+		     buf.size () - strlen (buf.data ()), ":X%x,",
 		     aexpr->len);
-	  pkt = buf + strlen (buf);
+
+	  char *end = buf.data () + strlen (buf.data ());
+	  long size_left = buf.size () - strlen (buf.data ());
+	  char hex_byte[3];
+	  hex_byte[2] = '\0';
+
 	  for (int ndx = 0; ndx < aexpr->len; ++ndx)
-	    pkt = pack_hex_byte (pkt, aexpr->buf[ndx]);
-	  *pkt = '\0';
+	    {
+	      pack_hex_byte (hex_byte, aexpr->buf[ndx]);
+	      xsnprintf (end, size_left, hex_byte);
+
+	      size_left -= strlen (end);
+	      end += strlen (end);
+	    }
 	}
       else
 	warning (_("Target does not support conditional tracepoints, "
@@ -12881,8 +12895,10 @@ remote_target::download_tracepoint (struct bp_location *loc)
     }
 
   if (b->commands || *default_collect)
-    strcat (buf, "-");
-  putpkt (buf);
+    xsnprintf (buf.data () + strlen (buf.data ()),
+	       buf.size () - strlen (buf.data ()), "-");
+
+  putpkt (buf.data ());
   remote_get_noisy_reply ();
   if (strcmp (rs->buf, "OK"))
     error (_("Target does not support tracepoints."));
@@ -12896,11 +12912,11 @@ 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",
+      xsnprintf (buf.data (), buf.size (), "QTDP:-%x:%s:%s%c",
 		 b->number, addrbuf, /* address */
 		 action_it->c_str (),
 		 has_more ? '-' : 0);
-      putpkt (buf);
+      putpkt (buf.data ());
       remote_get_noisy_reply ();
       if (strcmp (rs->buf, "OK"))
 	error (_("Error on target while setting tracepoints."));
@@ -12914,12 +12930,12 @@ 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",
+      xsnprintf (buf.data (), buf.size (), "QTDP:-%x:%s:%s%s%s",
 		 b->number, addrbuf, /* address */
 		 is_first ? "S" : "",
 		 action_it->c_str (),
 		 has_more ? "-" : "");
-      putpkt (buf);
+      putpkt (buf.data ());
       remote_get_noisy_reply ();
       if (strcmp (rs->buf, "OK"))
 	error (_("Error on target while setting tracepoints."));
@@ -12929,22 +12945,24 @@ remote_target::download_tracepoint (struct bp_location *loc)
     {
       if (b->location != NULL)
 	{
-	  strcpy (buf, "QTDPsrc:");
+	  xsnprintf (buf.data (), buf.size (), "QTDPsrc:");
 	  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:");
+	  xsnprintf (buf.data (), buf.size (), "QTDPsrc:");
 	  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

[PATCH 2/4] 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.vnet.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 ff6280a47d..04dc67fdc0 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -12893,7 +12893,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",
@@ -12912,7 +12912,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 1/4] 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 OK.

Thanks,
Ulrich

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

Re: [PATCH 2/4] 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 OK.

Thanks,
Ulrich

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

Re: [PATCH 3/4] Use get_remote_packet_size in download_tracepoint

[Ulrich Weigand]

Pedro Franco de Carvalho wrote:

> 	* remote.c (remote_target::download_tracepoint): Remove BUF_SIZE
> 	and pkt. 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 xsnprintf.

This makes sense in general, but I'm not sure it makes sense to use
a separate hex_byte buffer here:

>  	{
>  	  agent_expr_up aexpr = gen_eval_for_expr (tpaddr, loc->cond.get ());
> -	  xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":X%x,",
> +	  xsnprintf (buf.data () + strlen (buf.data ()),
> +		     buf.size () - strlen (buf.data ()), ":X%x,",
>  		     aexpr->len);
> -	  pkt = buf + strlen (buf);
> +
> +	  char *end = buf.data () + strlen (buf.data ());
> +	  long size_left = buf.size () - strlen (buf.data ());
> +	  char hex_byte[3];
> +	  hex_byte[2] = '\0';
> +
>  	  for (int ndx = 0; ndx < aexpr->len; ++ndx)
> -	    pkt = pack_hex_byte (pkt, aexpr->buf[ndx]);
> -	  *pkt = '\0';
> +	    {
> +	      pack_hex_byte (hex_byte, aexpr->buf[ndx]);
> +	      xsnprintf (end, size_left, hex_byte);
> +
> +	      size_left -= strlen (end);
> +	      end += strlen (end);
> +	    }
>  	}

You know from the beginning that the agent expression will take
(2 * aexpr->len) bytes, so it should be OK to only check this
once, ahead of time.  In fact, sending a partial agent expression
seems to be worse than sending none, so if the agent expression
is too long, I think it should be just omitted (and the user
warned).

Bye,
Ulrich

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

Re: [PATCH 4/4] 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
> 	in using the number of registers from the arch.
> 	(collection_list::add_register): Resize m_regs_mask instead of
> 	throwing error when regno is too large.
> 	(collection_list::stringify): Use size () instead of sizeof. Use
> 	xsnprintf instead of sprintf.

This is OK, but:

>  private:
>    /* room for up to 256 regs */
> -  unsigned char m_regs_mask[32];
> +  std::vector<unsigned char> m_regs_mask;

The comment is now wrong, so please remove it as well.

Bye,
Ulrich

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

Re: [PATCH 3/4] Use get_remote_packet_size in download_tracepoint

[Pedro Franco de Carvalho]

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

> You know from the beginning that the agent expression will take
> (2 * aexpr->len) bytes, so it should be OK to only check this
> once, ahead of time.  In fact, sending a partial agent expression
> seems to be worse than sending none, so if the agent expression
> is too long, I think it should be just omitted (and the user
> warned).

I don't think a partial agent expression would be sent in this case,
since this is before the first putpkt is called in the function. But I
can still issue the warning and ignore the condition expression instead
of failing on the assertion. Otherwise I can check the size once and
call a gdb_assert if its too small, like the rest of the function. Which
is better?

Would something like one of the two alternative below be ok for checking
the size only once?

The second one looks complicated, but my goal was to avoid overflows in
2 * aexpr->len, since that length ultimately comes from the condition
expression the user supplies.

I am also assuming throughout this function that size_t and
gdb::char_vector::size_type are compatible (since buf.size () returns
the latter and xsnprintf takes a size_t). Is this ok?

Thanks!

1:

       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);
-	  for (int ndx = 0; ndx < aexpr->len; ++ndx)
-	    pkt = pack_hex_byte (pkt, aexpr->buf[ndx]);
-	  *pkt = '\0';
+
+	  int cond_str_size = snprintf (NULL, 0, ":X%x,", aexpr->len);
+	  gdb_assert (cond_str_size >= 0);
+
+	  cond_str_size += aexpr->len * 2;
+
+	  if (cond_str_size < buf.size () - strlen (buf.data ()))
+	    {
+	      sprintf (buf.data () + strlen (buf.data ()),
+		       ":X%x,", aexpr->len);
+
+	      pkt = buf.data () + strlen (buf.data ());
+
+	      for (int ndx = 0; ndx < aexpr->len; ++ndx)
+		pkt = pack_hex_byte (pkt, aexpr->buf[ndx]);
+	      *pkt = '\0';
+	    }
+	  else
+	    warning (_("Condition expression too long, "
+		       "ignoring tp %d cond"), b->number);
 	}
       else
 	warning (_("Target does not support conditional tracepoints, "

2:

       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);
-	  for (int ndx = 0; ndx < aexpr->len; ++ndx)
-	    pkt = pack_hex_byte (pkt, aexpr->buf[ndx]);
-	  *pkt = '\0';
+
+	  int cond_str_size = snprintf (NULL, 0, ":X%x,", aexpr->len);
+	  gdb_assert (cond_str_size >= 0);
+
+	  size_t size_left = buf.size () - strlen (buf.data ());
+
+	  bool size_ok = (cond_str_size < size_left);
+
+	  if (size_ok)
+	    {
+	      size_left -= cond_str_size;
+
+	      size_ok = (size_left/2 > aexpr->len);
+
+	      if (size_ok)
+		{
+		  sprintf (buf.data () + strlen (buf.data ()),
+			   ":X%x,", aexpr->len);
+
+		  pkt = buf.data () + strlen (buf.data ());
+
+		  for (int ndx = 0; ndx < aexpr->len; ++ndx)
+		    pkt = pack_hex_byte (pkt, aexpr->buf[ndx]);
+		  *pkt = '\0';
+		}
+	    }
+
+	  if (!size_ok)
+	    warning (_("Condition expression too long, "
+		       "ignoring tp %d cond"), b->number);
 	}

Re: [PATCH 3/4] Use get_remote_packet_size in download_tracepoint

[Ulrich Weigand]

Pedro Franco de Carvalho wrote:
> Ulrich Weigand <uweigand@de.ibm.com> writes:
> 
> > You know from the beginning that the agent expression will take
> > (2 * aexpr->len) bytes, so it should be OK to only check this
> > once, ahead of time.  In fact, sending a partial agent expression
> > seems to be worse than sending none, so if the agent expression
> > is too long, I think it should be just omitted (and the user
> > warned).
> 
> I don't think a partial agent expression would be sent in this case,
> since this is before the first putpkt is called in the function. But I
> can still issue the warning and ignore the condition expression instead
> of failing on the assertion. Otherwise I can check the size once and
> call a gdb_assert if its too small, like the rest of the function. Which
> is better?

Ah, you right -- I missed that xsnprintf acually aborts.  In this case,
I agree that just checking once and assserting would be fine.

> I am also assuming throughout this function that size_t and
> gdb::char_vector::size_type are compatible (since buf.size () returns
> the latter and xsnprintf takes a size_t). Is this ok?

I'm not sure I fully understand the C++ standard on this question,
but it seems a reasonable assumption to me.  Maybe some of the C++
experts can chime in here?

Bye,
Ulrich

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

Re: [PATCH 3/4] Use get_remote_packet_size in download_tracepoint

[Pedro Alves]

On 06/25/2018 09:51 PM, Pedro Franco de Carvalho wrote:
> Ulrich Weigand <uweigand@de.ibm.com> writes:
> 
>> You know from the beginning that the agent expression will take
>> (2 * aexpr->len) bytes, so it should be OK to only check this
>> once, ahead of time.  In fact, sending a partial agent expression
>> seems to be worse than sending none, so if the agent expression
>> is too long, I think it should be just omitted (and the user
>> warned).
> 
> I don't think a partial agent expression would be sent in this case,
> since this is before the first putpkt is called in the function. But I
> can still issue the warning and ignore the condition expression instead
> of failing on the assertion. Otherwise I can check the size once and
> call a gdb_assert if its too small, like the rest of the function. Which
> is better?

I'm not sure I understand the details or the suggestions below (the
patches don't seem to be meant to apply on top of current master, but
they're using buf.data()), but I'd just like to point out that ideally
GDB should not gdb_assert or abort on user input or remote stub
limitations (small remote packet size), since neither are a GDB bug.

> 
> Would something like one of the two alternative below be ok for checking
> the size only once?
> 
> The second one looks complicated, but my goal was to avoid overflows in
> 2 * aexpr->len, since that length ultimately comes from the condition
> expression the user supplies.
> 
> I am also assuming throughout this function that size_t and
> gdb::char_vector::size_type are compatible (since buf.size () returns
> the latter and xsnprintf takes a size_t). Is this ok?

It is.

Thanks,
Pedro Alves

Re: [PATCH 4/4] Variable size for regs mask in collection list

[Pedro Alves]

Hi,

I quickly skimmed this one and noticed a couple formatting nits,
mentioned below.

On 06/20/2018 10:08 PM, Pedro Franco de Carvalho wrote:

> @@ -1086,9 +1087,21 @@ collection_list::add_static_trace_data ()
>  }
>  
>  collection_list::collection_list ()
> -  : m_regs_mask (),
> -    m_strace_data (false)
> +  : m_strace_data (false)
>  {
> +  /* Guess the number of bytes needed for the register mask. If it's
> +     too low, it will be expanded in add_register. If it's too high,
> +     stringify will ignore the extra bytes.

Double-space after the periods of the first two sentences.

> +
> +     The mask shouldn't include pseudoreg numbers, but
> +     encode_actions_1 currently doesn't handle remote register numbers
> +     and pseudoregs properly for tracepoint actions that don't
> +     generate an AX (e.g. "collect $<pseudoreg>").  */
> +  int num_regs = gdbarch_num_regs (target_gdbarch ())
> +    + gdbarch_num_pseudo_regs (target_gdbarch ());

Wrap in parens so that emacs tab-indents the second line
under gdbarch:

     int num_regs = (gdbarch_num_regs (target_gdbarch ())
                     + gdbarch_num_pseudo_regs (target_gdbarch ()));

Thanks,
Pedro Alves

Re: [PATCH 3/4] Use get_remote_packet_size in download_tracepoint

[Pedro Franco de Carvalho]

Pedro Alves <palves@redhat.com> writes:

> I'm not sure I understand the details or the suggestions below (the
> patches don't seem to be meant to apply on top of current master, but
> they're using buf.data()), but I'd just like to point out that ideally
> GDB should not gdb_assert or abort on user input or remote stub
> limitations (small remote packet size), since neither are a GDB bug.

Sorry, I messed up the formatting.

I used the assert because other places in remote.c also use xsnprintf
with the remote packet size and so fail with an assert if this size is
too small.

But I agree that it makes more sense not to abort, so I can change all
uses of xsnprintf in remote_target::download_tracepoint to snprintf and
raise an error if the return value indicates the packet size is too
small.

>> I am also assuming throughout this function that size_t and
>> gdb::char_vector::size_type are compatible (since buf.size () returns
>> the latter and xsnprintf takes a size_t). Is this ok?
>
> It is.

Thanks!

--
Pedro Franco de Carvalho

Re: [PATCH 4/4] Variable size for regs mask in collection list

[Pedro Franco de Carvalho]

Pedro Alves <palves@redhat.com> writes:

> Hi,
>
> I quickly skimmed this one and noticed a couple formatting nits,
> mentioned below.

Thanks! I will change these.