[PATCH 0/6] C++-ify and simplify agent expressions

[PATCH 0/6] C++-ify and simplify agent expressions

[Tom Tromey]

This series applies some straightforward C++-ification and
simplifications to the agent expression code in gdb.

Regression tested on x86-64 Fedora 36.

---
Tom Tromey (6):
      Remove mem2hex
      Use gdb::byte_vector in agent_expr
      Use std::vector<bool> for agent_expr::reg_mask
      Simplify agent_expr constructor
      Use bool for agent_expr::tracing
      Make aop_map 'static'

 gdb/ax-gdb.c     |  38 ++++++------
 gdb/ax-general.c | 184 ++++++++++++++++++++++---------------------------------
 gdb/ax.h         |  66 ++++----------------
 gdb/dwarf2/loc.c |   4 +-
 gdb/remote.c     |  14 ++---
 gdb/tracepoint.c |  57 ++++-------------
 6 files changed, 125 insertions(+), 238 deletions(-)
---
base-commit: e11a2ebb8e44b75fad8c520a170dcce6254c5164
change-id: 20230619-ax-new-9cb41cbddf34

Best regards,
-- 
Tom Tromey <tom@tromey.com>

[PATCH 1/6] Remove mem2hex

[Tom Tromey]

tracepoint.c has a 'mem2hex' function that is functionally equivalent
to bin2hex.  This patch removes the redundancy.
---
 gdb/tracepoint.c | 30 ++----------------------------
 1 file changed, 2 insertions(+), 28 deletions(-)

diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index f297cea5b43..0af7404aef1 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -69,7 +69,7 @@
    the worst case of maximum length for each of the pieces of a
    continuation packet.
 
-   NOTE: expressions get mem2hex'ed otherwise this would be twice as
+   NOTE: expressions get bin2hex'ed otherwise this would be twice as
    large.  (400 - 31)/2 == 184 */
 #define MAX_AGENT_EXPR_LEN	184
 
@@ -156,7 +156,6 @@ static std::string trace_stop_notes;
 /* support routines */
 
 struct collection_list;
-static char *mem2hex (gdb_byte *, char *, int);
 
 static counted_command_line all_tracepoint_actions (struct breakpoint *);
 
@@ -1226,7 +1225,7 @@ collection_list::stringify ()
       end += 10;		/* 'X' + 8 hex digits + ',' */
       count += 10;
 
-      end = mem2hex (m_aexprs[i]->buf, end, m_aexprs[i]->len);
+      end += 2 * bin2hex (m_aexprs[i]->buf, end, m_aexprs[i]->len);
       count += 2 * m_aexprs[i]->len;
     }
 
@@ -2888,31 +2887,6 @@ set_trace_stop_notes (const char *args, int from_tty,
     warning (_("Target does not support trace notes, stop note ignored"));
 }
 
-/* Convert the memory pointed to by mem into hex, placing result in buf.
- * Return a pointer to the last char put in buf (null)
- * "stolen" from sparc-stub.c
- */
-
-static const char hexchars[] = "0123456789abcdef";
-
-static char *
-mem2hex (gdb_byte *mem, char *buf, int count)
-{
-  gdb_byte ch;
-
-  while (count-- > 0)
-    {
-      ch = *mem++;
-
-      *buf++ = hexchars[ch >> 4];
-      *buf++ = hexchars[ch & 0xf];
-    }
-
-  *buf = 0;
-
-  return buf;
-}
-
 int
 get_traceframe_number (void)
 {

-- 
2.39.2

[PATCH 2/6] Use gdb::byte_vector in agent_expr

[Tom Tromey]

This changes agent_expr to use gdb::byte_vector rather than manually
reimplementing a vector.
---
 gdb/ax-gdb.c     |  28 +++++++--------
 gdb/ax-general.c | 101 ++++++++++++++++++-------------------------------------
 gdb/ax.h         |   8 +----
 gdb/dwarf2/loc.c |   4 +--
 gdb/remote.c     |  14 ++++----
 gdb/tracepoint.c |  13 +++----
 6 files changed, 63 insertions(+), 105 deletions(-)

diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
index 927dfc6337e..b0bde62f465 100644
--- a/gdb/ax-gdb.c
+++ b/gdb/ax-gdb.c
@@ -818,7 +818,6 @@ static int
 is_nontrivial_conversion (struct type *from, struct type *to)
 {
   agent_expr_up ax (new agent_expr (NULL, 0));
-  int nontrivial;
 
   /* Actually generate the code, and see if anything came out.  At the
      moment, it would be trivial to replicate the code in
@@ -827,8 +826,7 @@ is_nontrivial_conversion (struct type *from, struct type *to)
      way allows this function to be independent of the logic in
      gen_conversion.  */
   gen_conversion (ax.get (), from, to);
-  nontrivial = ax->len > 0;
-  return nontrivial;
+  return !ax->buf.empty ();
 }
 
 
@@ -1719,10 +1717,10 @@ ternop_cond_operation::do_generate_ax (struct expression *exp,
   std::get<1> (m_storage)->generate_ax (exp, ax, &value2);
   gen_usual_unary (ax, &value2);
   end = ax_goto (ax, aop_goto);
-  ax_label (ax, if1, ax->len);
+  ax_label (ax, if1, ax->buf.size ());
   std::get<2> (m_storage)->generate_ax (exp, ax, &value3);
   gen_usual_unary (ax, &value3);
-  ax_label (ax, end, ax->len);
+  ax_label (ax, end, ax->buf.size ());
   /* This is arbitrary - what if B and C are incompatible types? */
   value->type = value2.type;
   value->kind = value2.kind;
@@ -1811,12 +1809,12 @@ unop_sizeof_operation::do_generate_ax (struct expression *exp,
      only way to find an expression's type is to generate code for it.
      So we generate code for the operand, and then throw it away,
      replacing it with code that simply pushes its size.  */
-  int start = ax->len;
+  int start = ax->buf.size ();
 
   std::get<0> (m_storage)->generate_ax (exp, ax, value);
 
   /* Throw away the code we just generated.  */
-  ax->len = start;
+  ax->buf.resize (start);
 
   ax_const_l (ax, value->type->length ());
   value->kind = axs_rvalue;
@@ -2033,18 +2031,18 @@ logical_and_operation::do_generate_ax (struct expression *exp,
   gen_usual_unary (ax, &value1);
   if1 = ax_goto (ax, aop_if_goto);
   go1 = ax_goto (ax, aop_goto);
-  ax_label (ax, if1, ax->len);
+  ax_label (ax, if1, ax->buf.size ());
   std::get<1> (m_storage)->generate_ax (exp, ax, &value2);
   gen_usual_unary (ax, &value2);
   if2 = ax_goto (ax, aop_if_goto);
   go2 = ax_goto (ax, aop_goto);
-  ax_label (ax, if2, ax->len);
+  ax_label (ax, if2, ax->buf.size ());
   ax_const_l (ax, 1);
   end = ax_goto (ax, aop_goto);
-  ax_label (ax, go1, ax->len);
-  ax_label (ax, go2, ax->len);
+  ax_label (ax, go1, ax->buf.size ());
+  ax_label (ax, go2, ax->buf.size ());
   ax_const_l (ax, 0);
-  ax_label (ax, end, ax->len);
+  ax_label (ax, end, ax->buf.size ());
   value->kind = axs_rvalue;
   value->type = builtin_type (ax->gdbarch)->builtin_int;
 }
@@ -2067,10 +2065,10 @@ logical_or_operation::do_generate_ax (struct expression *exp,
   if2 = ax_goto (ax, aop_if_goto);
   ax_const_l (ax, 0);
   end = ax_goto (ax, aop_goto);
-  ax_label (ax, if1, ax->len);
-  ax_label (ax, if2, ax->len);
+  ax_label (ax, if1, ax->buf.size ());
+  ax_label (ax, if2, ax->buf.size ());
   ax_const_l (ax, 1);
-  ax_label (ax, end, ax->len);
+  ax_label (ax, end, ax->buf.size ());
   value->kind = axs_rvalue;
   value->type = builtin_type (ax->gdbarch)->builtin_int;
 }
diff --git a/gdb/ax-general.c b/gdb/ax-general.c
index e43e7732260..d5f4c51e65d 100644
--- a/gdb/ax-general.c
+++ b/gdb/ax-general.c
@@ -28,8 +28,6 @@
 #include "value.h"
 #include "user-regs.h"
 
-static void grow_expr (struct agent_expr *x, int n);
-
 static void append_const (struct agent_expr *x, LONGEST val, int n);
 
 static LONGEST read_const (struct agent_expr *x, int o, int n);
@@ -40,11 +38,6 @@ static void generic_ext (struct agent_expr *x, enum agent_op op, int n);
 
 agent_expr::agent_expr (struct gdbarch *gdbarch, CORE_ADDR scope)
 {
-  this->len = 0;
-  this->size = 1;		/* Change this to a larger value once
-				   reallocation code is tested.  */
-  this->buf = (unsigned char *) xmalloc (this->size);
-
   this->gdbarch = gdbarch;
   this->scope = scope;
 
@@ -58,25 +51,9 @@ agent_expr::agent_expr (struct gdbarch *gdbarch, CORE_ADDR scope)
 
 agent_expr::~agent_expr ()
 {
-  xfree (this->buf);
   xfree (this->reg_mask);
 }
 
-/* Make sure that X has room for at least N more bytes.  This doesn't
-   affect the length, just the allocated size.  */
-static void
-grow_expr (struct agent_expr *x, int n)
-{
-  if (x->len + n > x->size)
-    {
-      x->size *= 2;
-      if (x->size < x->len + n)
-	x->size = x->len + n + 10;
-      x->buf = (unsigned char *) xrealloc (x->buf, x->size);
-    }
-}
-
-
 /* Append the low N bytes of VAL as an N-byte integer to the
    expression X, in big-endian order.  */
 static void
@@ -84,13 +61,11 @@ append_const (struct agent_expr *x, LONGEST val, int n)
 {
   int i;
 
-  grow_expr (x, n);
   for (i = n - 1; i >= 0; i--)
     {
-      x->buf[x->len + i] = val & 0xff;
+      x->buf.push_back (val & 0xff);
       val >>= 8;
     }
-  x->len += n;
 }
 
 
@@ -103,7 +78,7 @@ read_const (struct agent_expr *x, int o, int n)
   LONGEST accum = 0;
 
   /* Make sure we're not reading off the end of the expression.  */
-  if (o + n > x->len)
+  if (o + n > x->buf.size ())
     error (_("GDB bug: ax-general.c (read_const): incomplete constant"));
 
   for (i = 0; i < n; i++)
@@ -117,8 +92,7 @@ read_const (struct agent_expr *x, int o, int n)
 void
 ax_raw_byte (struct agent_expr *x, gdb_byte byte)
 {
-  grow_expr (x, 1);
-  x->buf[x->len++] = byte;
+  x->buf.push_back (byte);
 }
 
 /* Append a simple operator OP to EXPR.  */
@@ -154,9 +128,8 @@ generic_ext (struct agent_expr *x, enum agent_op op, int n)
     error (_("GDB bug: ax-general.c (generic_ext): "
 	     "opcode has inadequate range"));
 
-  grow_expr (x, 2);
-  x->buf[x->len++] = op;
-  x->buf[x->len++] = n;
+  x->buf.push_back (op);
+  x->buf.push_back (n);
 }
 
 
@@ -185,9 +158,8 @@ ax_trace_quick (struct agent_expr *x, int n)
     error (_("GDB bug: ax-general.c (ax_trace_quick): "
 	     "size out of range for trace_quick"));
 
-  grow_expr (x, 2);
-  x->buf[x->len++] = aop_trace_quick;
-  x->buf[x->len++] = n;
+  x->buf.push_back (aop_trace_quick);
+  x->buf.push_back (n);
 }
 
 
@@ -200,12 +172,10 @@ ax_trace_quick (struct agent_expr *x, int n)
 int
 ax_goto (struct agent_expr *x, enum agent_op op)
 {
-  grow_expr (x, 3);
-  x->buf[x->len + 0] = op;
-  x->buf[x->len + 1] = 0xff;
-  x->buf[x->len + 2] = 0xff;
-  x->len += 3;
-  return x->len - 2;
+  x->buf.push_back (op);
+  x->buf.push_back (0xff);
+  x->buf.push_back (0xff);
+  return x->buf.size () - 2;
 }
 
 /* Suppose a given call to ax_goto returns some value PATCH.  When you
@@ -294,11 +264,9 @@ ax_reg (struct agent_expr *x, int reg)
       if (reg < 0 || reg > 0xffff)
 	error (_("GDB bug: ax-general.c (ax_reg): "
 		 "register number out of range"));
-      grow_expr (x, 3);
-      x->buf[x->len] = aop_reg;
-      x->buf[x->len + 1] = (reg >> 8) & 0xff;
-      x->buf[x->len + 2] = (reg) & 0xff;
-      x->len += 3;
+      x->buf.push_back (aop_reg);
+      x->buf.push_back ((reg >> 8) & 0xff);
+      x->buf.push_back ((reg) & 0xff);
     }
 }
 
@@ -312,11 +280,9 @@ ax_tsv (struct agent_expr *x, enum agent_op op, int num)
     internal_error (_("ax-general.c (ax_tsv): variable "
 		      "number is %d, out of range"), num);
 
-  grow_expr (x, 3);
-  x->buf[x->len] = op;
-  x->buf[x->len + 1] = (num >> 8) & 0xff;
-  x->buf[x->len + 2] = (num) & 0xff;
-  x->len += 3;
+  x->buf.push_back (op);
+  x->buf.push_back ((num >> 8) & 0xff);
+  x->buf.push_back ((num) & 0xff);
 }
 
 /* Append a string to the expression.  Note that the string is going
@@ -334,12 +300,11 @@ ax_string (struct agent_expr *x, const char *str, int slen)
     internal_error (_("ax-general.c (ax_string): string "
 		      "length is %d, out of allowed range"), slen);
 
-  grow_expr (x, 2 + slen + 1);
-  x->buf[x->len++] = ((slen + 1) >> 8) & 0xff;
-  x->buf[x->len++] = (slen + 1) & 0xff;
+  x->buf.push_back (((slen + 1) >> 8) & 0xff);
+  x->buf.push_back ((slen + 1) & 0xff);
   for (i = 0; i < slen; ++i)
-    x->buf[x->len++] = str[i];
-  x->buf[x->len++] = '\0';
+    x->buf.push_back (str[i]);
+  x->buf.push_back ('\0');
 }
 \f
 
@@ -375,7 +340,7 @@ ax_print (struct ui_file *f, struct agent_expr *x)
       != aop_last)
     error (_("GDB bug: ax-general.c (ax_print): opcode map out of sync"));
 
-  for (i = 0; i < x->len;)
+  for (i = 0; i < x->buf.size ();)
     {
       enum agent_op op = (enum agent_op) x->buf[i];
 
@@ -386,7 +351,7 @@ ax_print (struct ui_file *f, struct agent_expr *x)
 	  i++;
 	  continue;
 	}
-      if (i + 1 + aop_map[op].op_size > x->len)
+      if (i + 1 + aop_map[op].op_size > x->buf.size ())
 	{
 	  gdb_printf (f, _("%3d  <incomplete opcode %s>\n"),
 		      i, aop_map[op].name);
@@ -471,28 +436,28 @@ ax_reqs (struct agent_expr *ax)
 
   /* Jump target table.  targets[i] is non-zero iff we have found a
      jump to offset i.  */
-  char *targets = (char *) alloca (ax->len * sizeof (targets[0]));
+  char *targets = (char *) alloca (ax->buf.size () * sizeof (targets[0]));
 
   /* Instruction boundary table.  boundary[i] is non-zero iff our scan
      has reached an instruction starting at offset i.  */
-  char *boundary = (char *) alloca (ax->len * sizeof (boundary[0]));
+  char *boundary = (char *) alloca (ax->buf.size () * sizeof (boundary[0]));
 
   /* Stack height record.  If either targets[i] or boundary[i] is
      non-zero, heights[i] is the height the stack should have before
      executing the bytecode at that point.  */
-  int *heights = (int *) alloca (ax->len * sizeof (heights[0]));
+  int *heights = (int *) alloca (ax->buf.size () * sizeof (heights[0]));
 
   /* Pointer to a description of the present op.  */
   struct aop_map *op;
 
-  memset (targets, 0, ax->len * sizeof (targets[0]));
-  memset (boundary, 0, ax->len * sizeof (boundary[0]));
+  memset (targets, 0, ax->buf.size () * sizeof (targets[0]));
+  memset (boundary, 0, ax->buf.size () * sizeof (boundary[0]));
 
   ax->max_height = ax->min_height = height = 0;
   ax->flaw = agent_flaw_none;
   ax->max_data_size = 0;
 
-  for (i = 0; i < ax->len; i += 1 + op->op_size)
+  for (i = 0; i < ax->buf.size (); i += 1 + op->op_size)
     {
       if (ax->buf[i] > (sizeof (aop_map) / sizeof (aop_map[0])))
 	{
@@ -508,7 +473,7 @@ ax_reqs (struct agent_expr *ax)
 	  return;
 	}
 
-      if (i + 1 + op->op_size > ax->len)
+      if (i + 1 + op->op_size > ax->buf.size ())
 	{
 	  ax->flaw = agent_flaw_incomplete_instruction;
 	  return;
@@ -543,7 +508,7 @@ ax_reqs (struct agent_expr *ax)
 	  || aop_if_goto == op - aop_map)
 	{
 	  int target = read_const (ax, i + 1, 2);
-	  if (target < 0 || target >= ax->len)
+	  if (target < 0 || target >= ax->buf.size ())
 	    {
 	      ax->flaw = agent_flaw_bad_jump;
 	      return;
@@ -568,7 +533,7 @@ ax_reqs (struct agent_expr *ax)
       /* For unconditional jumps with a successor, check that the
 	 successor is a target, and pick up its stack height.  */
       if (aop_goto == op - aop_map
-	  && i + 3 < ax->len)
+	  && i + 3 < ax->buf.size ())
 	{
 	  if (!targets[i + 3])
 	    {
@@ -589,7 +554,7 @@ ax_reqs (struct agent_expr *ax)
     }
 
   /* Check that all the targets are on boundaries.  */
-  for (i = 0; i < ax->len; i++)
+  for (i = 0; i < ax->buf.size (); i++)
     if (targets[i] && !boundary[i])
       {
 	ax->flaw = agent_flaw_bad_jump;
diff --git a/gdb/ax.h b/gdb/ax.h
index 9c47a8d0b63..608af4c8c68 100644
--- a/gdb/ax.h
+++ b/gdb/ax.h
@@ -86,13 +86,7 @@ struct agent_expr
     ~agent_expr ();
 
     /* The bytes of the expression.  */
-    unsigned char *buf;
-
-    /* The number of bytecode in the expression.  */
-    int len;
-
-    /* Allocated space available currently.  */
-    int size;
+    gdb::byte_vector buf;
 
     /* The target architecture assumed to be in effect.  */
     struct gdbarch *gdbarch;
diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index e50e7f1e47e..5b2d58ab44e 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -2399,7 +2399,7 @@ dwarf2_compile_expr_to_ax (struct agent_expr *expr, struct axs_value *loc,
       int64_t offset;
       int i;
 
-      offsets[op_ptr - base] = expr->len;
+      offsets[op_ptr - base] = expr->buf.size ();
       ++op_ptr;
 
       /* Our basic approach to code generation is to map DWARF
@@ -2734,7 +2734,7 @@ dwarf2_compile_expr_to_ax (struct agent_expr *expr, struct axs_value *loc,
 	  ax_const_l (expr, 0);
 	  ax_simple (expr, aop_swap);
 	  ax_simple (expr, aop_sub);
-	  ax_label (expr, i, expr->len);
+	  ax_label (expr, i, expr->buf.size ());
 	  break;
 
 	case DW_OP_neg:
diff --git a/gdb/remote.c b/gdb/remote.c
index 02ff3a12bdb..5d09fa1758a 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -10693,9 +10693,9 @@ remote_add_target_side_condition (struct gdbarch *gdbarch,
   /* Send conditions to the target.  */
   for (agent_expr *aexpr : bp_tgt->conditions)
     {
-      xsnprintf (buf, buf_end - buf, "X%x,", aexpr->len);
+      xsnprintf (buf, buf_end - buf, "X%x,", (int) aexpr->buf.size ());
       buf += strlen (buf);
-      for (int i = 0; i < aexpr->len; ++i)
+      for (int i = 0; i < aexpr->buf.size (); ++i)
 	buf = pack_hex_byte (buf, aexpr->buf[i]);
       *buf = '\0';
     }
@@ -10718,9 +10718,9 @@ remote_add_target_side_commands (struct gdbarch *gdbarch,
      cmds parameter.  */
   for (agent_expr *aexpr : bp_tgt->tcommands)
     {
-      sprintf (buf, "X%x,", aexpr->len);
+      sprintf (buf, "X%x,", (int) aexpr->buf.size ());
       buf += strlen (buf);
-      for (int i = 0; i < aexpr->len; ++i)
+      for (int i = 0; i < aexpr->buf.size (); ++i)
 	buf = pack_hex_byte (buf, aexpr->buf[i]);
       *buf = '\0';
     }
@@ -13386,7 +13386,7 @@ remote_target::download_tracepoint (struct bp_location *loc)
 	  size_left = buf.size () - strlen (buf.data ());
 
 	  ret = snprintf (buf.data () + strlen (buf.data ()),
-			  size_left, ":X%x,", aexpr->len);
+			  size_left, ":X%x,", (int) aexpr->buf.size ());
 
 	  if (ret < 0 || ret >= size_left)
 	    error ("%s", err_msg);
@@ -13395,12 +13395,12 @@ remote_target::download_tracepoint (struct bp_location *loc)
 
 	  /* Two bytes to encode each aexpr byte, plus the terminating
 	     null byte.  */
-	  if (aexpr->len * 2 + 1 > size_left)
+	  if (aexpr->buf.size () * 2 + 1 > size_left)
 	    error ("%s", err_msg);
 
 	  pkt = buf.data () + strlen (buf.data ());
 
-	  for (int ndx = 0; ndx < aexpr->len; ++ndx)
+	  for (int ndx = 0; ndx < aexpr->buf.size (); ++ndx)
 	    pkt = pack_hex_byte (pkt, aexpr->buf[ndx]);
 	  *pkt = '\0';
 	}
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 0af7404aef1..335df997c05 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -618,7 +618,7 @@ finalize_tracepoint_aexpr (struct agent_expr *aexpr)
 {
   ax_reqs (aexpr);
 
-  if (aexpr->len > MAX_AGENT_EXPR_LEN)
+  if (aexpr->buf.size () > MAX_AGENT_EXPR_LEN)
     error (_("Expression is too complicated."));
 
   report_agent_reqs_errors (aexpr);
@@ -885,7 +885,7 @@ collection_list::add_local_register (struct gdbarch *gdbarch,
 	 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)
+      if (aexpr->buf.size () > 0)
 	add_aexpr (std::move (aexpr));
     }
 }
@@ -1215,18 +1215,19 @@ collection_list::stringify ()
   for (i = 0; i < m_aexprs.size (); i++)
     {
       QUIT;			/* Allow user to bail out with ^C.  */
-      if ((count + 10 + 2 * m_aexprs[i]->len) > MAX_AGENT_EXPR_LEN)
+      if ((count + 10 + 2 * m_aexprs[i]->buf.size ()) > MAX_AGENT_EXPR_LEN)
 	{
 	  str_list.emplace_back (temp_buf.data (), count);
 	  count = 0;
 	  end = temp_buf.data ();
 	}
-      sprintf (end, "X%08X,", m_aexprs[i]->len);
+      sprintf (end, "X%08X,", (int) m_aexprs[i]->buf.size ());
       end += 10;		/* 'X' + 8 hex digits + ',' */
       count += 10;
 
-      end += 2 * bin2hex (m_aexprs[i]->buf, end, m_aexprs[i]->len);
-      count += 2 * m_aexprs[i]->len;
+      end += 2 * bin2hex (m_aexprs[i]->buf.data (), end,
+			  m_aexprs[i]->buf.size ());
+      count += 2 * m_aexprs[i]->buf.size ();
     }
 
   if (count != 0)

-- 
2.39.2

[PATCH 3/6] Use std::vector<bool> for agent_expr::reg_mask

[Tom Tromey]

agent_expr::reg_mask implements its own packed boolean vector.  This
patch replaces it with a std::vector<bool>, simplifying the code.
---
 gdb/ax-general.c | 30 ++++++------------------------
 gdb/ax.h         | 15 +++++----------
 gdb/tracepoint.c | 16 +++++-----------
 3 files changed, 16 insertions(+), 45 deletions(-)

diff --git a/gdb/ax-general.c b/gdb/ax-general.c
index d5f4c51e65d..89e297eddc6 100644
--- a/gdb/ax-general.c
+++ b/gdb/ax-general.c
@@ -41,17 +41,12 @@ agent_expr::agent_expr (struct gdbarch *gdbarch, CORE_ADDR scope)
   this->gdbarch = gdbarch;
   this->scope = scope;
 
-  /* Bit vector for registers used.  */
-  this->reg_mask_len = 1;
-  this->reg_mask = XCNEWVEC (unsigned char, this->reg_mask_len);
-
   this->tracing = 0;
   this->trace_string = 0;
 }
 
 agent_expr::~agent_expr ()
 {
-  xfree (this->reg_mask);
 }
 
 /* Append the low N bytes of VAL as an N-byte integer to the
@@ -330,8 +325,9 @@ ax_print (struct ui_file *f, struct agent_expr *x)
 
   gdb_printf (f, _("Scope: %s\n"), paddress (x->gdbarch, x->scope));
   gdb_printf (f, _("Reg mask:"));
-  for (i = 0; i < x->reg_mask_len; ++i)
-    gdb_printf (f, _(" %02x"), x->reg_mask[i]);
+  for (i = 0; i < x->reg_mask.size (); ++i)
+    if (x->reg_mask[i])
+      gdb_printf (f, _(" %02x"), i);
   gdb_printf (f, _("\n"));
 
   /* Check the size of the name array against the number of entries in
@@ -401,28 +397,14 @@ ax_reg_mask (struct agent_expr *ax, int reg)
     }
   else
     {
-      int byte;
-
       /* Get the remote register number.  */
       reg = gdbarch_remote_register_number (ax->gdbarch, reg);
-      byte = reg / 8;
 
       /* Grow the bit mask if necessary.  */
-      if (byte >= ax->reg_mask_len)
-	{
-	  /* It's not appropriate to double here.  This isn't a
-	     string buffer.  */
-	  int new_len = byte + 1;
-	  unsigned char *new_reg_mask
-	    = XRESIZEVEC (unsigned char, ax->reg_mask, new_len);
-
-	  memset (new_reg_mask + ax->reg_mask_len, 0,
-		  (new_len - ax->reg_mask_len) * sizeof (ax->reg_mask[0]));
-	  ax->reg_mask_len = new_len;
-	  ax->reg_mask = new_reg_mask;
-	}
+      if (reg >= ax->reg_mask.size ())
+	ax->reg_mask.resize (reg);
 
-      ax->reg_mask[byte] |= 1 << (reg % 8);
+      ax->reg_mask[reg] = true;
     }
 }
 
diff --git a/gdb/ax.h b/gdb/ax.h
index 608af4c8c68..b0eb20daf75 100644
--- a/gdb/ax.h
+++ b/gdb/ax.h
@@ -110,14 +110,10 @@ struct agent_expr
     int max_data_size;
 
     /* Bit vector of registers needed.  Register R is needed iff
-
-       reg_mask[R / 8] & (1 << (R % 8))
-
-       is non-zero.  Note!  You may not assume that this bitmask is long
-       enough to hold bits for all the registers of the machine; the
-       agent expression code has no idea how many registers the machine
-       has.  However, the bitmask is reg_mask_len bytes long, so the
-       valid register numbers run from 0 to reg_mask_len * 8 - 1.
+       reg_mask[R] is non-zero.  Note!  You may not assume that this
+       bitmask is long enough to hold bits for all the registers of
+       the machine; the agent expression code has no idea how many
+       registers the machine has.
 
        Also note that this mask may contain registers that are needed
        for the original collection expression to work, but that are
@@ -126,8 +122,7 @@ struct agent_expr
        compiler sets the mask bit and skips generating a bytecode whose
        result is going to be discarded anyway.
     */
-    int reg_mask_len;
-    unsigned char *reg_mask;
+    std::vector<bool> reg_mask;
 
     /* For the data tracing facility, we need to insert `trace' bytecodes
        before each data fetch; this records all the memory that the
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 335df997c05..205380476b3 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -835,19 +835,13 @@ collection_list::add_remote_register (unsigned int regno)
 void
 collection_list::add_ax_registers (struct agent_expr *aexpr)
 {
-  if (aexpr->reg_mask_len > 0)
+  for (int ndx1 = 0; ndx1 < aexpr->reg_mask.size (); ndx1++)
     {
-      for (int ndx1 = 0; ndx1 < aexpr->reg_mask_len; ndx1++)
+      QUIT;	/* Allow user to bail out with ^C.  */
+      if (aexpr->reg_mask[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);
-	    }
+	  /* It's used -- record it.  */
+	  add_remote_register (ndx1);
 	}
     }
 }

-- 
2.39.2

Re: [PATCH 3/6] Use std::vector<bool> for agent_expr::reg_mask

[John Baldwin]

On 6/19/23 2:06 PM, Tom Tromey wrote:
> agent_expr::reg_mask implements its own packed boolean vector.  This
> patch replaces it with a std::vector<bool>, simplifying the code.
> ---
>   gdb/ax-general.c | 30 ++++++------------------------
>   gdb/ax.h         | 15 +++++----------
>   gdb/tracepoint.c | 16 +++++-----------
>   3 files changed, 16 insertions(+), 45 deletions(-)
> 
> diff --git a/gdb/ax-general.c b/gdb/ax-general.c
> index d5f4c51e65d..89e297eddc6 100644
> --- a/gdb/ax-general.c
> +++ b/gdb/ax-general.c
> @@ -330,8 +325,9 @@ ax_print (struct ui_file *f, struct agent_expr *x)
>   
>     gdb_printf (f, _("Scope: %s\n"), paddress (x->gdbarch, x->scope));
>     gdb_printf (f, _("Reg mask:"));
> -  for (i = 0; i < x->reg_mask_len; ++i)
> -    gdb_printf (f, _(" %02x"), x->reg_mask[i]);
> +  for (i = 0; i < x->reg_mask.size (); ++i)
> +    if (x->reg_mask[i])
> +      gdb_printf (f, _(" %02x"), i);
>     gdb_printf (f, _("\n"));

This was previously printing the bytes of the raw bitmask so that the mask was
printed in packed hex.  Now it is printing each bit as a 2 character hex value
(so you now end up with 01 01 00 00 00 01 00 00 instead of c4 for example).
Re-synthesizing the packed hex output may not be useful, but perhaps you could
print it as binary instead by only printing 0 or 1 for each bit without spaces
(or maybe only spaces between each 8 bits?).  Something like:

    for (i = 0; i < x->reg_mask.size (); ++i)
      {
        if (i % 8 == 0)
          gdb_printf(f, _(" "));
        gdb_printf (f, _("%u"), x->reg_mask[i]);
      }

-- 
John Baldwin

Re: [PATCH 3/6] Use std::vector<bool> for agent_expr::reg_mask

[Tom Tromey]

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> Re-synthesizing the packed hex output may not be useful, but perhaps you could
John> print it as binary instead by only printing 0 or 1 for each bit without spaces
John> (or maybe only spaces between each 8 bits?).  Something like:

John>    for (i = 0; i < x->reg_mask.size (); ++i)
John>      {
John>        if (i % 8 == 0)
John>          gdb_printf(f, _(" "));
John>        gdb_printf (f, _("%u"), x->reg_mask[i]);
John>      }

Makes sense, I used this approach.

Tom

[PATCH 4/6] Simplify agent_expr constructor

[Tom Tromey]

This simplifies the agent_expr constructor a bit, and removes the
destructor entirely.
---
 gdb/ax-general.c | 13 -------------
 gdb/ax.h         | 10 ++++++----
 2 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/gdb/ax-general.c b/gdb/ax-general.c
index 89e297eddc6..7a37aff3d70 100644
--- a/gdb/ax-general.c
+++ b/gdb/ax-general.c
@@ -36,19 +36,6 @@ static void generic_ext (struct agent_expr *x, enum agent_op op, int n);
 \f
 /* Functions for building expressions.  */
 
-agent_expr::agent_expr (struct gdbarch *gdbarch, CORE_ADDR scope)
-{
-  this->gdbarch = gdbarch;
-  this->scope = scope;
-
-  this->tracing = 0;
-  this->trace_string = 0;
-}
-
-agent_expr::~agent_expr ()
-{
-}
-
 /* Append the low N bytes of VAL as an N-byte integer to the
    expression X, in big-endian order.  */
 static void
diff --git a/gdb/ax.h b/gdb/ax.h
index b0eb20daf75..67ad3145349 100644
--- a/gdb/ax.h
+++ b/gdb/ax.h
@@ -81,9 +81,11 @@ enum agent_flaws
 struct agent_expr
   {
     /* Construct an empty agent expression.  */
-    explicit agent_expr (struct gdbarch *gdbarch, CORE_ADDR scope);
-
-    ~agent_expr ();
+    agent_expr (struct gdbarch *gdbarch, CORE_ADDR scope)
+      : gdbarch (gdbarch),
+	scope (scope),
+	tracing (0)
+    { }
 
     /* The bytes of the expression.  */
     gdb::byte_vector buf;
@@ -139,7 +141,7 @@ struct agent_expr
        tracenz bytecode to record nonzero bytes, up to a length that
        is the value of trace_string.  */
 
-    int trace_string;
+    int trace_string = 0;
   };
 
 /* An agent_expr owning pointer.  */

-- 
2.39.2

[PATCH 5/6] Use bool for agent_expr::tracing

[Tom Tromey]

This changese agent_expr::tracing to have type bool, allowing inline
initialization and cleaning up the code a little.q
---
 gdb/ax-gdb.c | 10 +++++-----
 gdb/ax.h     |  7 +++----
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
index b0bde62f465..55450bd2979 100644
--- a/gdb/ax-gdb.c
+++ b/gdb/ax-gdb.c
@@ -2335,7 +2335,7 @@ gen_trace_for_var (CORE_ADDR scope, struct gdbarch *gdbarch,
   agent_expr_up ax (new agent_expr (gdbarch, scope));
   struct axs_value value;
 
-  ax->tracing = 1;
+  ax->tracing = true;
   ax->trace_string = trace_string;
   gen_var_ref (ax.get (), &value, var);
 
@@ -2368,7 +2368,7 @@ gen_trace_for_expr (CORE_ADDR scope, struct expression *expr,
   agent_expr_up ax (new agent_expr (expr->gdbarch, scope));
   struct axs_value value;
 
-  ax->tracing = 1;
+  ax->tracing = true;
   ax->trace_string = trace_string;
   value.optimized_out = 0;
   expr->op->generate_ax (expr, ax.get (), &value);
@@ -2395,7 +2395,7 @@ gen_eval_for_expr (CORE_ADDR scope, struct expression *expr)
   agent_expr_up ax (new agent_expr (expr->gdbarch, scope));
   struct axs_value value;
 
-  ax->tracing = 0;
+  ax->tracing = false;
   value.optimized_out = 0;
   expr->op->generate_ax (expr, ax.get (), &value);
 
@@ -2414,7 +2414,7 @@ gen_trace_for_return_address (CORE_ADDR scope, struct gdbarch *gdbarch,
   agent_expr_up ax (new agent_expr (gdbarch, scope));
   struct axs_value value;
 
-  ax->tracing = 1;
+  ax->tracing = true;
   ax->trace_string = trace_string;
 
   gdbarch_gen_return_address (gdbarch, ax.get (), &value, scope);
@@ -2443,7 +2443,7 @@ gen_printf (CORE_ADDR scope, struct gdbarch *gdbarch,
   int tem;
 
   /* We're computing values, not doing side effects.  */
-  ax->tracing = 0;
+  ax->tracing = false;
 
   /* Evaluate and push the args on the stack in reverse order,
      for simplicity of collecting them on the target side.  */
diff --git a/gdb/ax.h b/gdb/ax.h
index 67ad3145349..0e82ed9e313 100644
--- a/gdb/ax.h
+++ b/gdb/ax.h
@@ -83,8 +83,7 @@ struct agent_expr
     /* Construct an empty agent expression.  */
     agent_expr (struct gdbarch *gdbarch, CORE_ADDR scope)
       : gdbarch (gdbarch),
-	scope (scope),
-	tracing (0)
+	scope (scope)
     { }
 
     /* The bytes of the expression.  */
@@ -132,10 +131,10 @@ struct agent_expr
        be available when the user later tries to evaluate the expression
        in GDB.
 
-       Setting the flag 'tracing' to non-zero enables the code that
+       Setting the flag 'tracing' to true enables the code that
        emits the trace bytecodes at the appropriate points.  */
 
-    unsigned int tracing : 1;
+    bool tracing = false;
 
     /* This indicates that pointers to chars should get an added
        tracenz bytecode to record nonzero bytes, up to a length that

-- 
2.39.2

Re: [PATCH 5/6] Use bool for agent_expr::tracing

[John Baldwin]

On 6/19/23 2:06 PM, Tom Tromey wrote:
> This changese agent_expr::tracing to have type bool, allowing inline
> initialization and cleaning up the code a little.q
> ---

Nit: stray 'q' at the end of the log

-- 
John Baldwin

Re: [PATCH 5/6] Use bool for agent_expr::tracing

[Tom Tromey]

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> On 6/19/23 2:06 PM, Tom Tromey wrote:
>> This changese agent_expr::tracing to have type bool, allowing inline
>> initialization and cleaning up the code a little.q
>> ---

John> Nit: stray 'q' at the end of the log

Thanks, I fixed this.

Tom

[PATCH 6/6] Make aop_map 'static'

[Tom Tromey]

This changes aop_map to be 'static'.

It also changes a runtime assertion into a static assert.  I'm not
sure if this assert provides much value -- by construction, it can't
really fail -- but it's clearly more useful as a static assert than a
runtime one.
---
 gdb/ax-general.c | 40 +++++++++++++++++++++++++++++++++-------
 gdb/ax.h         | 30 ------------------------------
 2 files changed, 33 insertions(+), 37 deletions(-)

diff --git a/gdb/ax-general.c b/gdb/ax-general.c
index 7a37aff3d70..b1ee2753bd8 100644
--- a/gdb/ax-general.c
+++ b/gdb/ax-general.c
@@ -294,7 +294,36 @@ ax_string (struct agent_expr *x, const char *str, int slen)
 /* Functions for disassembling agent expressions, and otherwise
    debugging the expression compiler.  */
 
-struct aop_map aop_map[] =
+/* An entry in the opcode map.  */
+struct aop_map
+  {
+
+    /* The name of the opcode.  Null means that this entry is not a
+       valid opcode --- a hole in the opcode space.  */
+    const char *name;
+
+    /* All opcodes take no operands from the bytecode stream, or take
+       unsigned integers of various sizes.  If this is a positive number
+       n, then the opcode is followed by an n-byte operand, which should
+       be printed as an unsigned integer.  If this is zero, then the
+       opcode takes no operands from the bytecode stream.
+
+       If we get more complicated opcodes in the future, don't add other
+       magic values of this; that's a crock.  Add an `enum encoding'
+       field to this, or something like that.  */
+    int op_size;
+
+    /* The size of the data operated upon, in bits, for bytecodes that
+       care about that (ref and const).  Zero for all others.  */
+    int data_size;
+
+    /* Number of stack elements consumed, and number produced.  */
+    int consumed, produced;
+  };
+
+/* Map of the bytecodes, indexed by bytecode number.  */
+
+static struct aop_map aop_map[] =
 {
   {0, 0, 0, 0, 0}
 #define DEFOP(NAME, SIZE, DATA_SIZE, CONSUMED, PRODUCED, VALUE) \
@@ -303,6 +332,9 @@ struct aop_map aop_map[] =
 #undef DEFOP
 };
 
+/* Check the size of the name array against the number of entries in
+   the enum, to catch additions that people didn't sync.  */
+gdb_static_assert ((sizeof (aop_map) / sizeof (aop_map[0])) == aop_last);
 
 /* Disassemble the expression EXPR, writing to F.  */
 void
@@ -317,12 +349,6 @@ ax_print (struct ui_file *f, struct agent_expr *x)
       gdb_printf (f, _(" %02x"), i);
   gdb_printf (f, _("\n"));
 
-  /* Check the size of the name array against the number of entries in
-     the enum, to catch additions that people didn't sync.  */
-  if ((sizeof (aop_map) / sizeof (aop_map[0]))
-      != aop_last)
-    error (_("GDB bug: ax-general.c (ax_print): opcode map out of sync"));
-
   for (i = 0; i < x->buf.size ();)
     {
       enum agent_op op = (enum agent_op) x->buf[i];
diff --git a/gdb/ax.h b/gdb/ax.h
index 0e82ed9e313..1fdecb2f9e5 100644
--- a/gdb/ax.h
+++ b/gdb/ax.h
@@ -221,36 +221,6 @@ extern void ax_string (struct agent_expr *x, const char *str, int slen);
 /* Disassemble the expression EXPR, writing to F.  */
 extern void ax_print (struct ui_file *f, struct agent_expr * EXPR);
 
-/* An entry in the opcode map.  */
-struct aop_map
-  {
-
-    /* The name of the opcode.  Null means that this entry is not a
-       valid opcode --- a hole in the opcode space.  */
-    const char *name;
-
-    /* All opcodes take no operands from the bytecode stream, or take
-       unsigned integers of various sizes.  If this is a positive number
-       n, then the opcode is followed by an n-byte operand, which should
-       be printed as an unsigned integer.  If this is zero, then the
-       opcode takes no operands from the bytecode stream.
-
-       If we get more complicated opcodes in the future, don't add other
-       magic values of this; that's a crock.  Add an `enum encoding'
-       field to this, or something like that.  */
-    int op_size;
-
-    /* The size of the data operated upon, in bits, for bytecodes that
-       care about that (ref and const).  Zero for all others.  */
-    int data_size;
-
-    /* Number of stack elements consumed, and number produced.  */
-    int consumed, produced;
-  };
-
-/* Map of the bytecodes, indexed by bytecode number.  */
-extern struct aop_map aop_map[];
-
 /* Given an agent expression AX, analyze and update its requirements.  */
 
 extern void ax_reqs (struct agent_expr *ax);

-- 
2.39.2

Re: [PATCH 6/6] Make aop_map 'static'

[John Baldwin]

On 6/19/23 2:06 PM, Tom Tromey wrote:
> This changes aop_map to be 'static'.
> 
> It also changes a runtime assertion into a static assert.  I'm not
> sure if this assert provides much value -- by construction, it can't
> really fail -- but it's clearly more useful as a static assert than a
> runtime one.
> ---
>   gdb/ax-general.c | 40 +++++++++++++++++++++++++++++++++-------
>   gdb/ax.h         | 30 ------------------------------
>   2 files changed, 33 insertions(+), 37 deletions(-)
> 
> diff --git a/gdb/ax-general.c b/gdb/ax-general.c
> index 7a37aff3d70..b1ee2753bd8 100644
> --- a/gdb/ax-general.c
> +++ b/gdb/ax-general.c
> @@ -294,7 +294,36 @@ ax_string (struct agent_expr *x, const char *str, int slen)
>   /* Functions for disassembling agent expressions, and otherwise
>      debugging the expression compiler.  */
>   
> -struct aop_map aop_map[] =
> +/* An entry in the opcode map.  */
> +struct aop_map
> +  {
> +
> +    /* The name of the opcode.  Null means that this entry is not a
> +       valid opcode --- a hole in the opcode space.  */
> +    const char *name;
> +
> +    /* All opcodes take no operands from the bytecode stream, or take
> +       unsigned integers of various sizes.  If this is a positive number
> +       n, then the opcode is followed by an n-byte operand, which should
> +       be printed as an unsigned integer.  If this is zero, then the
> +       opcode takes no operands from the bytecode stream.
> +
> +       If we get more complicated opcodes in the future, don't add other
> +       magic values of this; that's a crock.  Add an `enum encoding'
> +       field to this, or something like that.  */
> +    int op_size;
> +
> +    /* The size of the data operated upon, in bits, for bytecodes that
> +       care about that (ref and const).  Zero for all others.  */
> +    int data_size;
> +
> +    /* Number of stack elements consumed, and number produced.  */
> +    int consumed, produced;
> +  };
> +
> +/* Map of the bytecodes, indexed by bytecode number.  */
> +
> +static struct aop_map aop_map[] =
>   {
>     {0, 0, 0, 0, 0}
>   #define DEFOP(NAME, SIZE, DATA_SIZE, CONSUMED, PRODUCED, VALUE) \
> @@ -303,6 +332,9 @@ struct aop_map aop_map[] =
>   #undef DEFOP
>   };
>   
> +/* Check the size of the name array against the number of entries in
> +   the enum, to catch additions that people didn't sync.  */
> +gdb_static_assert ((sizeof (aop_map) / sizeof (aop_map[0])) == aop_last);

Maybe use ARRAY_SIZE here while you are at it?

Looking around in ax-general.c more, it seems we already do runtime
validation of potential indices before indexing the array, so I'm not sure
the assertion adds much value and I'd be tempted to remove aop_last entirely.

BTW, the various other places in ax-general.c that do the assertion are
all using the expanded form of ARRAY_SIZE and would be a bit more readable
perhaps if they used ARRAY_SIZE instead.

-- 
John Baldwin

Re: [PATCH 6/6] Make aop_map 'static'

[Tom Tromey]

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> Looking around in ax-general.c more, it seems we already do runtime
John> validation of potential indices before indexing the array, so I'm not sure
John> the assertion adds much value and I'd be tempted to remove aop_last entirely.

Yeah, that was my feeling as well.  I've changed this patch to just make
aop_map static, and split out another patch to remove aop_last.

John> BTW, the various other places in ax-general.c that do the assertion are
John> all using the expanded form of ARRAY_SIZE and would be a bit more readable
John> perhaps if they used ARRAY_SIZE instead.

I tacked on a patch to do this.  This caught a bug, one of the checks
was using '>' rather than '>='.

Tom