This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH 3/4] Handle thread-specific breakpoints on remote targets


This patch lets remote targets evaluate thread-specific breakpoints,
instead of having to do the round trip to GDB.  It can potentially save
a lot of RSP messages, which can slow down execution considerably if the
number of hits by wrong threads is high.

The implementation is based on the existing target side evaluation of
condition.  A new "thread_id" agent expression bytecode is introduced and
used by GDB.  This bytecode tells the target to push the id of the
current thread on the stack, allowing GDB to craft agent expressions
comparing the current thread id with a constant (the thread specified by
breakpoint).  Therefore, remote targets only need to add support for
this bytecode to take advantage of the feature (and report "AxThreadID+"
in qSupported).  This patch does that for gdbserver.

gdb/ChangeLog:

	* NEWS: Mention new agent expression bytecode and qSupported
	reply.
	* ax-gdb.c (gen_thread_id): New function.
	(gen_expr): Handle OP_THREAD_ID.
	* breakpoint.c (make_thread_id_exp): New function.
	(expression_and): New function.
	(build_target_condition_list): Create and add thread-id check
	expression if the breakpoint is thread-specific.
	* common/ax.def (thread_id): New operator.
	* remote.c (PACKET_AXThreadId): New enum label.
	(remote_protocol_features): Add entry for AXThreadId.
	(remote_supports_thread_id_operator): New function.
	(init_remote_ops): Assign remote_supports_thread_id_operator
	to to_supports_thread_id_operator.
	(_initialize_remote): Add packet config command for AXThreadId.
	* std-operator.def (OP_THREAD_ID): New operator.
	* target-debug.h, target-delegates.c: Re-generate.
	* target.h (target_ops) <to_supports_thread_id_operator>: New
	field.
	(target_supports_thread_id_operator): New macro.

gdb/gdbserver/ChangeLog:

	* ax.c (gdb_eval_agent_expr): Handle thread_id bytecode.
	* server.c (handle_query): Send AXThreadId+ in qSupported reply.

gdb/doc/ChangeLog:

	* agentexpr.texi (Bytecode Descriptions): Document thread_id
	bytecode.
	* gdb.texinfo (General Query Packets): Document AXThreadId in
	the qSupported possible replies.
---
 gdb/NEWS               |   6 +++
 gdb/ax-gdb.c           |  15 ++++++-
 gdb/breakpoint.c       | 113 +++++++++++++++++++++++++++++++++++++++++++++++--
 gdb/common/ax.def      |   1 +
 gdb/doc/agentexpr.texi |   3 ++
 gdb/doc/gdb.texinfo    |   8 ++++
 gdb/gdbserver/ax.c     |  12 ++++++
 gdb/gdbserver/server.c |   1 +
 gdb/remote.c           |  17 ++++++++
 gdb/std-operator.def   |   4 ++
 gdb/target-debug.h     |   6 +++
 gdb/target-delegates.c |  31 ++++++++++++++
 gdb/target.h           |   9 ++++
 13 files changed, 221 insertions(+), 5 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index eb1a589c6d..5e9e5d4b7a 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,12 @@
 
 *** Changes since GDB 8.0
 
+* New Agent Expression bytecode "thread_id".
+
+  When executing this bytecode, the target will place the id of the current
+  thread at the top of the stack.  Targets which support this bytecode should
+  include AXThreadId+ in their reply to qSupported.
+
 *** Changes in GDB 8.0
 
 * GDB now supports access to the PKU register on GNU/Linux. The register is
diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
index fae2e2d483..eb83fc7eb5 100644
--- a/gdb/ax-gdb.c
+++ b/gdb/ax-gdb.c
@@ -1769,7 +1769,14 @@ gen_sizeof (struct expression *exp, union exp_element **pc,
   value->kind = axs_rvalue;
   value->type = size_type;
 }
-
+
+/* Emit a thread_id operator.  */
+
+static void
+gen_thread_id (struct agent_expr *ax)
+{
+  ax_simple (ax, aop_thread_id);
+}
 
 /* Generating bytecode from GDB expressions: general recursive thingy  */
 
@@ -2232,6 +2239,12 @@ gen_expr (struct expression *exp, union exp_element **pc,
     case OP_DECLTYPE:
       error (_("Attempt to use a type name as an expression."));
 
+    case OP_THREAD_ID:
+      gen_thread_id (ax);
+      value->kind = axs_rvalue;
+      value->type = int_type;
+      break;
+
     default:
       error (_("Unsupported operator %s (%d) in expression."),
 	     op_name (exp, op), op);
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 4940ec271b..12d2766a67 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2282,6 +2282,77 @@ parse_cond_to_aexpr (CORE_ADDR scope, struct expression *cond)
   return aexpr;
 }
 
+/* Build an expression comparing the THREAD_ID operator (current thread id at
+   the moment the expression is evaluated) with the thread id of THREAD.  */
+
+static expression_up
+make_thread_id_exp (const thread_info *thread, struct gdbarch *gdbarch,
+		    const struct language_defn *lang)
+{
+  /* We need 6 elements to build this expression:
+
+       - BINOP_EQUAL (1)
+         - OP_LONG <type> <thread id> OP_LONG (4)
+         - OP_THREAD_ID (1)  */
+  const int nelts = 6;
+  size_t sz = sizeof (expression) + nelts * sizeof (exp_element);
+  expression_up exp (XCNEWVAR (expression, sz));
+  ptid_t ptid = thread->ptid;
+  int tid;
+
+  if (ptid_lwp_p (ptid))
+    tid = ptid_get_lwp (ptid);
+  else if (ptid_tid_p (ptid))
+    tid = ptid_get_tid (ptid);
+  else
+    tid = ptid_get_pid (ptid);
+
+  exp->language_defn = lang;
+  exp->gdbarch = gdbarch;
+  exp->nelts = nelts;
+
+  exp->elts[0].opcode = BINOP_EQUAL;
+
+  exp->elts[1].opcode = OP_LONG;
+  exp->elts[2].type = builtin_type (exp->gdbarch)->builtin_int;
+  exp->elts[3].longconst = tid;
+  exp->elts[4].opcode = OP_LONG;
+
+  exp->elts[5].opcode = OP_THREAD_ID;
+
+  return exp;
+}
+
+/* Build an expression that is a logical AND of EXP1 and EXP2.  */
+
+static expression_up
+expression_and (expression *exp1, expression *exp2)
+{
+  gdb_assert (exp1 != NULL);
+  gdb_assert (exp2 != NULL);
+
+  /* The expressions must be compatible.  */
+  gdb_assert (exp1->gdbarch == exp2->gdbarch);
+  gdb_assert (exp1->language_defn == exp2->language_defn);
+
+  /* We'll need enough elements to fit both expressions, plus one for the
+     BINOP_LOGICAL_AND.  */
+  const int nelts = exp1->nelts + exp2->nelts + 1;
+  size_t sz = sizeof (expression) + nelts * sizeof (exp_element);
+  expression_up exp (XCNEWVAR (expression, sz));
+
+  exp->language_defn = exp1->language_defn;
+  exp->gdbarch = exp1->gdbarch;
+  exp->nelts = nelts;
+
+  exp->elts[0].opcode = BINOP_LOGICAL_AND;
+  memcpy (&exp->elts[1], exp1->elts, sizeof (exp_element) * exp1->nelts);
+  memcpy (&exp->elts[exp1->nelts + 1], exp2->elts,
+	  sizeof (exp_element) * exp2->nelts);
+
+  return exp;
+}
+
 /* Based on location BL, create a list of breakpoint conditions to be
    passed on to the target.  If we have duplicated locations with different
    conditions, we will add such conditions to the list.  The idea is that the
@@ -2322,9 +2393,43 @@ build_target_condition_list (struct bp_location *bl)
 	      /* Re-parse the conditions since something changed.  In that
 		 case we already freed the condition bytecodes (see
 		 force_breakpoint_reinsertion).  We just
-		 need to parse the condition to bytecodes again.  */
-	      loc->cond_bytecode = parse_cond_to_aexpr (bl->address,
-							loc->cond.get ());
+		 need to parse the condition to bytecodes again.
+
+		 If the breakpoint is thread-specific, build an conditional
+		 expression allowing the target itself to verify if it's the
+		 right thread that hit the breakpoint.  If the breakpoint
+		 already has a condition, the two expressions are merged with a
+		 logical and.  */
+
+	      /* The temporary thread-checking expression we'll build.  */
+	      expression_up thread_exp;
+	      /* The original breakpoint condition expression.  */
+	      expression *exp = loc->cond.get ();
+	      int thread_num = loc->owner->thread;
+
+	      if (thread_num != -1 && target_supports_thread_id_operator ())
+		{
+		  thread_info *thread = find_thread_global_id (thread_num);
+
+		  /* Thread-specific breakpoints can only be created for
+		     existing threads, and are deleted when the corresponding
+		     thread exits.  */
+		  gdb_assert (thread != NULL);
+
+		  const language_defn *lang
+		    = exp != NULL ? exp->language_defn : NULL;
+		  gdbarch *gdbarch
+		    = exp != NULL ? exp->gdbarch : target_gdbarch ();
+
+		  thread_exp = make_thread_id_exp (thread, gdbarch, lang);
+
+		  if (exp != NULL)
+		    thread_exp = expression_and (exp, thread_exp.get ());
+
+		  exp = thread_exp.get ();
+		}
+
+	      loc->cond_bytecode = parse_cond_to_aexpr (bl->address, exp);
 	    }
 
 	  /* If we have a NULL bytecode expression, it means something
@@ -2365,7 +2470,7 @@ build_target_condition_list (struct bp_location *bl)
   ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
     {
       loc = (*loc2p);
-      if (loc->cond
+      if (loc->cond_bytecode.get () != NULL
 	  && is_breakpoint (loc->owner)
 	  && loc->pspace->num == bl->pspace->num
 	  && loc->owner->enable_state == bp_enabled
diff --git a/gdb/common/ax.def b/gdb/common/ax.def
index d0c696f1a5..ff7f6d9b04 100644
--- a/gdb/common/ax.def
+++ b/gdb/common/ax.def
@@ -95,3 +95,4 @@ DEFOP (pick, 1, 0, 0, 1, 0x32)
 DEFOP (rot, 0, 0, 3, 3, 0x33)
 /* Both the argument and consumed numbers are dynamic for this one.  */
 DEFOP (printf, 0, 0, 0, 0, 0x34)
+DEFOP (thread_id, 0, 0, 0, 1, 0x35)
diff --git a/gdb/doc/agentexpr.texi b/gdb/doc/agentexpr.texi
index 5668e9c1e4..6e43c1a32f 100644
--- a/gdb/doc/agentexpr.texi
+++ b/gdb/doc/agentexpr.texi
@@ -515,6 +515,9 @@ stack.  If the purpose of the expression was to compute an lvalue or a
 range of memory, then the next-to-top of the stack is the lvalue's
 address, and the top of the stack is the lvalue's size, in bytes.
 
+@item @code{thread_id} (0x35): @result{} @var{tid}
+Push the id the current thread to the top of the stack.
+
 @end table
 
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 300d78eefb..8813f83caf 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -37233,6 +37233,11 @@ These are the currently defined stub features and their properties:
 @tab @samp{-}
 @tab No
 
+@item @samp{AXThreadId}
+@tab No
+@tab @samp{-}
+@tab No
+
 @end multitable
 
 These are the currently defined stub features, in more detail:
@@ -37455,6 +37460,9 @@ The remote stub understands the @samp{QThreadEvents} packet.
 @item no-resumed
 The remote stub reports the @samp{N} stop reply.
 
+@item AXThreadId
+The remote stub supports the thread_id agent expression bytecode.
+
 @end table
 
 @item qSymbol::
diff --git a/gdb/gdbserver/ax.c b/gdb/gdbserver/ax.c
index 87b9359591..60b1e548f5 100644
--- a/gdb/gdbserver/ax.c
+++ b/gdb/gdbserver/ax.c
@@ -21,6 +21,7 @@
 #include "format.h"
 #include "tracepoint.h"
 #include "rsp-low.h"
+#include "inferiors.h"
 
 static void ax_vdebug (const char *, ...) ATTRIBUTE_PRINTF (1, 2);
 
@@ -1336,6 +1337,17 @@ gdb_eval_agent_expr (struct eval_agent_expr_context *ctx,
 	     option of ignoring.  */
 	  return expr_eval_unhandled_opcode;
 
+	case gdb_agent_op_thread_id:
+	  stack[sp++] = top;
+
+#ifdef IN_PROCESS_AGENT
+	  /* The thread_id opcode is not supported in the IPA yet.  */
+	  top = 0;
+#else
+	  top = current_ptid.lwp ();
+#endif
+	  break;
+
 	default:
 	  ax_debug ("Agent expression op 0x%x not recognized", op);
 	  /* Don't struggle on, things will just get worse.  */
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 69fcab128b..d16aa1c461 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -2349,6 +2349,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
 	  || target_supports_software_single_step () )
 	{
 	  strcat (own_buf, ";ConditionalBreakpoints+");
+	  strcat (own_buf, ";AXThreadId+");
 	}
       strcat (own_buf, ";BreakpointCommands+");
 
diff --git a/gdb/remote.c b/gdb/remote.c
index 694897df1d..440d579252 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1526,6 +1526,9 @@ enum {
   /* Support TARGET_WAITKIND_NO_RESUMED.  */
   PACKET_no_resumed,
 
+  /* Support for the thread_id agent expression bytecode operator.  */
+  PACKET_AXThreadId,
+
   PACKET_MAX
 };
 
@@ -4698,6 +4701,8 @@ static const struct protocol_feature remote_protocol_features[] = {
   { "vContSupported", PACKET_DISABLE, remote_supported_packet, PACKET_vContSupported },
   { "QThreadEvents", PACKET_DISABLE, remote_supported_packet, PACKET_QThreadEvents },
   { "no-resumed", PACKET_DISABLE, remote_supported_packet, PACKET_no_resumed },
+  { "AXThreadId", PACKET_DISABLE, remote_supported_packet,
+    PACKET_AXThreadId },
 };
 
 static char *remote_support_xml;
@@ -12201,6 +12206,12 @@ remote_can_run_breakpoint_commands (struct target_ops *self)
   return packet_support (PACKET_BreakpointCommands) == PACKET_ENABLE;
 }
 
+static bool
+remote_supports_thread_id_operator (struct target_ops *self)
+{
+  return packet_support (PACKET_AXThreadId) == PACKET_ENABLE;
+}
+
 static void
 remote_trace_init (struct target_ops *self)
 {
@@ -13653,6 +13664,8 @@ Specify the serial device it is connected to\n\
   remote_ops.to_insert_exec_catchpoint = remote_insert_exec_catchpoint;
   remote_ops.to_remove_exec_catchpoint = remote_remove_exec_catchpoint;
   remote_ops.to_execution_direction = remote_execution_direction;
+  remote_ops.to_supports_thread_id_operator
+    = remote_supports_thread_id_operator;
 }
 
 /* Set up the extended remote vector by making a copy of the standard
@@ -14334,6 +14347,10 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
   add_packet_config_cmd (&remote_protocol_packets[PACKET_no_resumed],
 			 "N stop reply", "no-resumed-stop-reply", 0);
 
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_AXThreadId],
+			 "AXThreadId",
+			 "ax-thread-id", 0);
+
   /* Assert that we've registered "set remote foo-packet" commands
      for all packet configs.  */
   {
diff --git a/gdb/std-operator.def b/gdb/std-operator.def
index 465072616f..852f076c5e 100644
--- a/gdb/std-operator.def
+++ b/gdb/std-operator.def
@@ -320,3 +320,7 @@ OP (OP_TYPEID)
 /* This is used for the Rust [expr; N] form of array construction.  It
    takes two expression arguments.  */
 OP (OP_RUST_ARRAY)
+
+/* A special operator that yields the value of the current target thread id
+   (not the GDB thread number).  */
+OP (OP_THREAD_ID)
diff --git a/gdb/target-debug.h b/gdb/target-debug.h
index 6923608afd..10669d3e21 100644
--- a/gdb/target-debug.h
+++ b/gdb/target-debug.h
@@ -206,4 +206,10 @@ target_debug_print_signals (unsigned char *sigs)
   fputs_unfiltered (" }", gdb_stdlog);
 }
 
+static void
+target_debug_print_bool (bool value)
+{
+  fprintf_unfiltered (gdb_stdlog, "%s", value ? "true" : "false");
+}
+
 #endif /* TARGET_DEBUG_H */
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 88e3e0b994..be1e9ae302 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -4148,6 +4148,33 @@ debug_done_generating_core (struct target_ops *self)
   fputs_unfiltered (")\n", gdb_stdlog);
 }
 
+static bool
+delegate_supports_thread_id_operator (struct target_ops *self)
+{
+  self = self->beneath;
+  return self->to_supports_thread_id_operator (self);
+}
+
+static bool
+tdefault_supports_thread_id_operator (struct target_ops *self)
+{
+  return false;
+}
+
+static bool
+debug_supports_thread_id_operator (struct target_ops *self)
+{
+  bool result;
+  fprintf_unfiltered (gdb_stdlog, "-> %s->to_supports_thread_id_operator (...)\n", debug_target.to_shortname);
+  result = debug_target.to_supports_thread_id_operator (&debug_target);
+  fprintf_unfiltered (gdb_stdlog, "<- %s->to_supports_thread_id_operator (", debug_target.to_shortname);
+  target_debug_print_struct_target_ops_p (&debug_target);
+  fputs_unfiltered (") = ", gdb_stdlog);
+  target_debug_print_bool (result);
+  fputs_unfiltered ("\n", gdb_stdlog);
+  return result;
+}
+
 static void
 install_delegators (struct target_ops *ops)
 {
@@ -4459,6 +4486,8 @@ install_delegators (struct target_ops *ops)
     ops->to_prepare_to_generate_core = delegate_prepare_to_generate_core;
   if (ops->to_done_generating_core == NULL)
     ops->to_done_generating_core = delegate_done_generating_core;
+  if (ops->to_supports_thread_id_operator == NULL)
+    ops->to_supports_thread_id_operator = delegate_supports_thread_id_operator;
 }
 
 static void
@@ -4618,6 +4647,7 @@ install_dummy_methods (struct target_ops *ops)
   ops->to_get_tailcall_unwinder = tdefault_get_tailcall_unwinder;
   ops->to_prepare_to_generate_core = tdefault_prepare_to_generate_core;
   ops->to_done_generating_core = tdefault_done_generating_core;
+  ops->to_supports_thread_id_operator = tdefault_supports_thread_id_operator;
 }
 
 static void
@@ -4777,4 +4807,5 @@ init_debug_target (struct target_ops *ops)
   ops->to_get_tailcall_unwinder = debug_get_tailcall_unwinder;
   ops->to_prepare_to_generate_core = debug_prepare_to_generate_core;
   ops->to_done_generating_core = debug_done_generating_core;
+  ops->to_supports_thread_id_operator = debug_supports_thread_id_operator;
 }
diff --git a/gdb/target.h b/gdb/target.h
index a971adf8c6..a090c79f2e 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1260,6 +1260,9 @@ struct target_ops
     void (*to_done_generating_core) (struct target_ops *)
       TARGET_DEFAULT_IGNORE ();
 
+    bool (*to_supports_thread_id_operator) (struct target_ops *)
+      TARGET_DEFAULT_RETURN (false);
+
     int to_magic;
     /* Need sub-structure for target machine related rather than comm related?
      */
@@ -1436,6 +1439,12 @@ int target_supports_disable_randomization (void);
 #define target_can_run_breakpoint_commands() \
   (*current_target.to_can_run_breakpoint_commands) (&current_target)
 
+/* Returns true if this target supports the thread_id agent expression bytecode
+   operator.  */
+
+#define target_supports_thread_id_operator() \
+  (*current_target.to_supports_thread_id_operator) (&current_target)
+
 extern int target_read_string (CORE_ADDR, char **, int, int *);
 
 /* For target_read_memory see target/target.h.  */
-- 
2.11.0


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]