[RFC stub-side break conditions 3/5] GDB-side changes

[RFC stub-side break conditions 3/5] GDB-side changes

[Luis Machado]

[-- Attachment #1: Type: text/plain, Size: 2699 bytes --]

Hi,

This patch handles GDB-specific changes to support stub-side breakpoint 
conditions.

It adds machinery to detect locations that had their conditions changed. 
In case conditions change for any location, such changes need to be 
reported to/synched with the stub for evaluation, or else we may be left 
with a stub evaluating old conditionals.

This machinery works by marking locations modified through the 
"condition_changed" field. Conditions change whenever we create a new 
location, use the "condition" command or delete an old duplicate 
location. Furthermore, if a location at addr 0xdeadbeef had its 
condition modified, all duplicate locations at 0xdeadbeef are marked 
modified as well.

The modification detection takes place inside 
update_global_location_list (...). By calling 
force_breakpoint_reinsertion (...), we mark every location at address 
0xdeadbeef as modified. When consolidating the final updated list of 
locations, we detect these locations and mark the non-duplicate location 
at 0xdeafbeef as "needs_update".

The "needs_update" field is used during breakpoint insertion. It forces 
insertion of breakpoints even if they are marked as inserted already.

Now that we have information about locations that we should reinsert in 
the target, we proceed to build a list of hex-encoded agent expressions.

At this point, if any conditional expressions fail to parse to a valid 
agent expression, we assume that the conditions will be evaluated on 
GDB's side. Thus we don't send the conditions to the stub. Otherwise, we 
proceed to insert the breakpoints and the remote code now attaches the 
hex-encoded expressions to the z0/z1 packet.

Regarding always-inserted mode, if it is "on" we must send condition 
changes right away, or else the target will potentially miss breakpoint 
hits. If always-inserted is "off", this isn't too critical, so we just 
wait for the insertion call to send all the conditions to the stub. We 
will remove them when stopping anyway.

Other changes include handling of the new "condition-evaluation" 
breakpoint subcommand and showing the condition-evaluation mode in "info 
break" if it is "stub". We do not show the evaluation mode if it is "gdb".

I ran the testsuite and had no additional failures.

The location conditional modification detection machinery should be 
non-functional if GDB is doing all the condition evaluation work on its own.

I'd like to hear about the change to "info break" and also the way we 
should pass conditions down to the stub.  Currently i'm adding the agent 
expressions to a condition list in the target info field of bp locations.

Comments are much appreciated.

Luis
lgustavo@codesourcery.com

[-- Attachment #2: 0002-break_condition_bytecode.diff --]
[-- Type: text/x-patch, Size: 37489 bytes --]

2012-01-05  Luis Machado  <lgustavo@codesourcery.com>

	* remote.c (remote_supports_cond_breakpoints): New forward
	declaration.
	(remote_add_target_side_condition): New function.
	(remote_insert_breakpoint): Add target-side breakpoint
	conditional if supported.
	(remote_insert_hw_breakpoint): Likewise.
	(init_remote_ops): Set to_supports_breakpoint_condition hook.

	* target.c (update_current_target): Inherit
	to_supports_breakpoint_conditions.
	Default to_supports_breakpoint_conditions to return_zero.

	* target.h (struct target_ops) <to_supports_breakpoint_conditions>:
	New field.
	(target_supports_breakpoint_conditions): New #define.

	* breakpoint.c (get_first_locp_gte_addr): New forward declaration.
	(condition_evaluation_auto,
	condition_evaluation_gdb, condition_evaluation_stub,
	condition_evaluation_enums, condition_evaluation_mode): New
	static globals.
	(breakpoint_condition_evaluation_mode): New function.
	(gdb_evaluates_breakpoint_condition_p): New function.
	(show_condition_evaluation_mode): New function.
	(ALL_BP_LOCATIONS_AT_ADDR): New helper macro.
	(mark_breakpoint_modified): New function.
	(mark_breakpoint_location_modified): New function.
	(get_first_location_gte_addr): New helper function.
	(set_breakpoint_condition): Free condition bytecode if locations
	has become unconditional.  Call mark_breakpoint_modified (...).
	(condition_command): Call update_global_location_list (1) for
	breakpoints.
	(breakpoint_xfer_memory): Use is_breakpoint (...).
	(is_breakpoint): New function.
	(parse_cond_to_aexpr): New function.
	(build_target_condition_list): New function.
	(insert_bp_location): Handle target-side conditional
	breakpoints and call build_target_condition_list (...).
	(insert_breakpoint_locations): Handle target-side conditional
	breakpoints.
	(print_one_breakpoint_location): Print condition evaluation mode next
	to a breakpoint's condition string if it is not "gdb" mode.
	(bpstat_check_breakpoint_conditions): Add comment.
	(init_bp_location): Call mark_breakpoint_location_modified (...) for
	breakpoint location.
	(force_breakpoint_reinsertion): New functions.
	(update_global_location_list): Handle target-side breakpoint
	conditions.
	(bp_location_dtor): Free agent expression bytecode.
	(delete_breakpoint): Call update_global_location_list (...)
	with parameter 1 for breakpoints.
	(disable_breakpoint): Call mark_breakpoint_modified (...).
	Call update_global_location_list (...) with parameter 1 for breakpoints.
	(disable_command): Call mark_breakpoint_location_modified (...).
	Call update_global_location_list (...) with parameter 1 for breakpoints.
	(enable_breakpoint_disp): Call mark_breakpoint_modified (...).
	(enable_command): mark_breakpoint_location_modified (...).
	(_initialize_breakpoint): Update documentation and add
	condition-evaluation breakpoint subcommand.

	* breakpoint.h: Include ax.h.
	(condition_list): New data structure.
	(bp_target_info) <cond_list>: New field.
	(bp_location) <condition_changed, cond_bytecode>: New fields.
	(is_breakpoint): New prototype.

Index: gdb/gdb/remote.c
===================================================================
--- gdb.orig/gdb/remote.c	2012-01-04 14:03:00.810431999 -0200
+++ gdb/gdb/remote.c	2012-01-04 14:03:03.682431999 -0200
@@ -242,6 +242,8 @@ static int remote_read_description_p (st
 
 static void remote_console_output (char *msg);
 
+int remote_supports_cond_breakpoints (void);
+
 /* The non-stop remote protocol provisions for one pending stop reply.
    This is where we keep it until it is acknowledged.  */
 
@@ -7691,6 +7693,36 @@ extended_remote_create_inferior (struct
 }
 \f
 
+static int
+remote_add_target_side_condition (struct gdbarch *gdbarch,
+				  struct bp_target_info *bp_tgt, char *buf)
+{
+  struct agent_expr *aexpr = NULL;
+  int i;
+  char *pkt;
+  struct condition_list *cond, *tmp_cond;
+  char *buf_start = buf;
+
+  cond = bp_tgt->cond_list;
+
+  /* Send conditions to target and free the list.  */
+  while (cond != NULL)
+    {
+      aexpr = cond->expr;
+      sprintf (buf + strlen (buf), ":X%x,", aexpr->len);
+      pkt = buf + strlen (buf);
+      for (i = 0; i < aexpr->len; ++i)
+	pkt = pack_hex_byte (pkt, aexpr->buf[i]);
+      *pkt = '\0';
+      tmp_cond = cond;
+      cond = cond->next;
+      xfree (tmp_cond);
+    }
+
+  bp_tgt->cond_list = NULL;
+  return 0;
+}
+
 /* Insert a breakpoint.  On targets that have software breakpoint
    support, we ask the remote target to do the work; on targets
    which don't, we insert a traditional memory breakpoint.  */
@@ -7710,6 +7742,7 @@ remote_insert_breakpoint (struct gdbarch
       struct remote_state *rs;
       char *p;
       int bpsize;
+      struct condition_list *cond = NULL;
 
       gdbarch_remote_breakpoint_from_pc (gdbarch, &addr, &bpsize);
 
@@ -7723,6 +7756,10 @@ remote_insert_breakpoint (struct gdbarch
       p += hexnumstr (p, addr);
       sprintf (p, ",%d", bpsize);
 
+      if (bp_tgt->cond_list
+	  && remote_supports_cond_breakpoints ())
+	remote_add_target_side_condition (gdbarch, bp_tgt, p);
+
       putpkt (rs->buf);
       getpkt (&rs->buf, &rs->buf_size, 0);
 
@@ -7948,6 +7985,10 @@ remote_insert_hw_breakpoint (struct gdba
   p += hexnumstr (p, (ULONGEST) addr);
   sprintf (p, ",%x", bp_tgt->placed_size);
 
+  if (bp_tgt->cond_list
+      && remote_supports_cond_breakpoints ())
+    remote_add_target_side_condition (gdbarch, bp_tgt, p);
+
   putpkt (rs->buf);
   getpkt (&rs->buf, &rs->buf_size, 0);
 
@@ -10676,6 +10717,7 @@ Specify the serial device it is connecte
     = remote_supports_disable_randomization;
   remote_ops.to_supports_enable_disable_tracepoint = remote_supports_enable_disable_tracepoint;
   remote_ops.to_supports_string_tracing = remote_supports_string_tracing;
+  remote_ops.to_supports_breakpoint_conditions = remote_supports_cond_breakpoints;
   remote_ops.to_trace_init = remote_trace_init;
   remote_ops.to_download_tracepoint = remote_download_tracepoint;
   remote_ops.to_can_download_tracepoint = remote_can_download_tracepoint;
Index: gdb/gdb/target.c
===================================================================
--- gdb.orig/gdb/target.c	2012-01-04 14:02:33.774431998 -0200
+++ gdb/gdb/target.c	2012-01-04 14:03:03.682431999 -0200
@@ -698,6 +698,7 @@ update_current_target (void)
       INHERIT (to_static_tracepoint_markers_by_strid, t);
       INHERIT (to_traceframe_info, t);
       INHERIT (to_magic, t);
+      INHERIT (to_supports_breakpoint_conditions, t);
       /* Do not inherit to_memory_map.  */
       /* Do not inherit to_flash_erase.  */
       /* Do not inherit to_flash_done.  */
@@ -924,6 +925,9 @@ update_current_target (void)
   de_fault (to_traceframe_info,
 	    (struct traceframe_info * (*) (void))
 	    tcomplain);
+  de_fault (to_supports_breakpoint_conditions,
+	    (int (*) (void))
+	    return_zero);
   de_fault (to_execution_direction, default_execution_direction);
 
 #undef de_fault
Index: gdb/gdb/target.h
===================================================================
--- gdb.orig/gdb/target.h	2012-01-04 14:02:33.774431998 -0200
+++ gdb/gdb/target.h	2012-01-04 14:03:03.686431999 -0200
@@ -662,6 +662,9 @@ struct target_ops
     /* Does this target support the tracenz bytecode for string collection?  */
     int (*to_supports_string_tracing) (void);
 
+    /* Does this target support evaluation breakpoint conditions on its end?  */
+    int (*to_supports_breakpoint_conditions) (void);
+
     /* Determine current architecture of thread PTID.
 
        The target is supposed to determine the architecture of the code where
@@ -927,6 +930,12 @@ int target_supports_disable_randomizatio
 #define target_supports_string_tracing() \
   (*current_target.to_supports_string_tracing) ()
 
+/* Returns true if this target can handle breakpoint conditions
+   on its end.  */
+
+#define target_supports_breakpoint_conditions() \
+  (*current_target.to_supports_breakpoint_conditions) ()
+
 /* Invalidate all target dcaches.  */
 extern void target_dcache_invalidate (void);
 
Index: gdb/gdb/breakpoint.c
===================================================================
--- gdb.orig/gdb/breakpoint.c	2012-01-04 14:02:33.614431998 -0200
+++ gdb/gdb/breakpoint.c	2012-01-04 14:03:03.694431999 -0200
@@ -66,6 +66,7 @@
 #include "stack.h"
 #include "skip.h"
 #include "record.h"
+#include "ax-gdb.h"
 
 /* readline include files */
 #include "readline/readline.h"
@@ -238,6 +239,8 @@ static void trace_pass_command (char *,
 
 static int is_masked_watchpoint (const struct breakpoint *b);
 
+static struct bp_location **get_first_locp_gte_addr (CORE_ADDR address);
+
 /* Assuming we're creating a static tracepoint, does S look like a
    static tracepoint marker spec ("-m MARKER_ID")?  */
 #define is_marker_spec(s)						\
@@ -381,6 +384,63 @@ breakpoints_always_inserted_mode (void)
 	  && !RECORD_IS_USED);
 }
 
+/* Modes for breakpoint condition evaluation.  */
+static const char condition_evaluation_auto[] = "auto";
+static const char condition_evaluation_gdb[] = "gdb";
+static const char condition_evaluation_stub[] = "stub";
+static const char *condition_evaluation_enums[] = {
+  condition_evaluation_auto,
+  condition_evaluation_gdb,
+  condition_evaluation_stub,
+  NULL
+};
+
+/* Global that holds the current mode for breakpoint condition evaluation.  */
+static const char *condition_evaluation_mode = condition_evaluation_auto;
+
+/* Discovers what condition_evaluation_auto translates to.  */
+
+static const char *
+breakpoint_condition_evaluation_mode (void)
+{
+  if (condition_evaluation_mode == condition_evaluation_auto)
+    if (target_supports_breakpoint_conditions ())
+      return condition_evaluation_stub;
+    else
+      return condition_evaluation_gdb;
+  else
+    return condition_evaluation_mode;
+}
+
+/* Return true if GDB should evaluate breakpoint conditions or false
+   otherwise.  */
+
+static int
+gdb_evaluates_breakpoint_condition_p (void)
+{
+  const char *mode = breakpoint_condition_evaluation_mode ();
+
+  return (mode == condition_evaluation_gdb? 1 : 0);
+}
+
+/* Shows the current mode of breakpoint condition evaluation.  Explicitly shows
+   what "auto" is translating to.  */
+
+static void
+show_condition_evaluation_mode (struct ui_file *file, int from_tty,
+				struct cmd_list_element *c, const char *value)
+{
+  if (condition_evaluation_mode == condition_evaluation_auto)
+    fprintf_filtered (file,
+		      _("Breakpoint condition evaluation "
+			"mode is %s (currently %s).\n"),
+		      value,
+		      breakpoint_condition_evaluation_mode ());
+  else
+    fprintf_filtered (file, _("Breakpoint condition evaluation mode is %s.\n"),
+		      value);
+}
+
 void _initialize_breakpoint (void);
 
 /* Are we executing breakpoint commands?  */
@@ -412,6 +472,19 @@ int target_exact_watchpoints = 0;
 	     BP_TMP < bp_location + bp_location_count && (B = *BP_TMP);	\
 	     BP_TMP++)
 
+/* Iterates through locations with address ADDRESS.  BP_LOCP_TMP points to
+   each object.  BP_LOCP_START points to where the loop should start from.
+   If BP_LOCP_START is a NULL pointer, the macro automatically seeks the
+   appropriate location to start with.  */
+
+#define ALL_BP_LOCATIONS_AT_ADDR(BP_LOCP_TMP, BP_LOCP_START, ADDRESS)	\
+	if (!BP_LOCP_START)						\
+	  BP_LOCP_START = get_first_locp_gte_addr (ADDRESS);		\
+	for (BP_LOCP_TMP = BP_LOCP_START;				\
+	     (BP_LOCP_TMP < bp_location + bp_location_count		\
+	     && (*BP_LOCP_TMP)->address == ADDRESS);			\
+	     BP_LOCP_TMP++)
+
 /* Iterator for tracepoints only.  */
 
 #define ALL_TRACEPOINTS(B)  \
@@ -595,6 +668,68 @@ get_breakpoint (int num)
 
 \f
 
+/* Mark locations as "conditions have changed" in case
+   the target supports evaluating conditions on
+   its side.  */
+
+static void
+mark_breakpoint_modified (struct breakpoint *b)
+{
+  struct bp_location *loc;
+
+  /* This is only meaningful if the target is
+     evaluating conditions and if the user has
+     opted for condition evaluation on the target's
+     side.  */
+  if (gdb_evaluates_breakpoint_condition_p ()
+      || !target_supports_breakpoint_conditions ())
+    return;
+
+  if (!is_breakpoint (b))
+    return;
+
+  for (loc = b->loc; loc; loc = loc->next)
+    loc->condition_changed = 1;
+}
+
+/* Mark location as "conditions have changed" in case
+   the target supports evaluating conditions on
+   its side.  */
+
+static void
+mark_breakpoint_location_modified (struct bp_location *loc)
+{
+  /* This is only meaningful if the target is
+     evaluating conditions and if the user has
+     opted for condition evaluation on the target's
+     side.  */
+  if (gdb_evaluates_breakpoint_condition_p ()
+      || !target_supports_breakpoint_conditions ())
+
+    return;
+
+  if (!is_breakpoint (loc->owner))
+    return;
+
+  loc->condition_changed = 1;
+}
+
+/* Helper function to skip all bp_locations with addresses
+   less than ADDRESS.  It returns the first bp_location that
+   is greater than or equal to ADDRESS.  */
+
+static struct bp_location **
+get_first_locp_gte_addr (CORE_ADDR address)
+{
+  struct bp_location **locp = bp_location;
+
+  while (locp < bp_location + bp_location_count
+	 && (*locp)->address < address)
+    locp++;
+
+  return locp;
+}
+
 void
 set_breakpoint_condition (struct breakpoint *b, char *exp,
 			  int from_tty)
@@ -617,6 +752,10 @@ set_breakpoint_condition (struct breakpo
 	{
 	  xfree (loc->cond);
 	  loc->cond = NULL;
+
+	  /* No need to free the condition agent expression
+	     bytecode (if we have one).  We will handle this
+	     when we go through update_global_location_list (...).  */
 	}
     }
 
@@ -659,6 +798,8 @@ set_breakpoint_condition (struct breakpo
 	    }
 	}
     }
+  mark_breakpoint_modified (b);
+
   breakpoints_changed ();
   observer_notify_breakpoint_modified (b);
 }
@@ -692,6 +833,10 @@ condition_command (char *arg, int from_t
 	  error (_("Cannot set a condition where a Python 'stop' "
 		   "method has been defined in the breakpoint."));
 	set_breakpoint_condition (b, p, from_tty);
+
+	if (is_breakpoint (b))
+	  update_global_location_list (1);
+
 	return;
       }
 
@@ -1191,6 +1336,16 @@ breakpoint_xfer_memory (gdb_byte *readbu
 }
 \f
 
+/* Return true if BPT is of any breakpoint kind, hardware or
+   software.  */
+
+int
+is_breakpoint (const struct breakpoint *bpt)
+{
+  return (bpt->type == bp_breakpoint
+	  || bpt->type == bp_hardware_breakpoint);
+}
+
 /* Return true if BPT is of any hardware watchpoint kind.  */
 
 static int
@@ -1632,6 +1787,148 @@ unduplicated_should_be_inserted (struct
   return result;
 }
 
+/* Parses a conditional described by an expression COND into an
+   agent expression bytecode suitable for evaluation
+   by the bytecode interpreter.  Return NULL if there was
+   any error during parsing.  */
+
+static struct agent_expr *
+parse_cond_to_aexpr (CORE_ADDR scope, struct expression *cond)
+{
+  struct agent_expr *aexpr = NULL;
+  struct cleanup *old_chain = NULL;
+  volatile struct gdb_exception ex;
+
+  if (!cond)
+    return NULL;
+
+  /* We don't want to stop processing, so catch any errors
+     that may show up.  */
+  TRY_CATCH (ex, RETURN_MASK_ERROR)
+    {
+      aexpr = gen_eval_for_expr (scope, cond);
+    }
+
+  if (ex.reason < 0)
+    {
+      /* If we got here, it means the condition could not be parse to a valid
+	 bytecode expression and thus can't be evaluated on the target's side.
+	 It's no use iterating through the conditions.  */
+      return NULL;
+    }
+
+  /* We have a valid agent expression.  */
+  return aexpr;
+}
+
+/* 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
+   target will evaluate the list of conditions and will only notify GDB when
+   one of them is true.  */
+
+static int
+build_target_condition_list (struct bp_location *bl)
+{
+  struct bp_location **locp = NULL, **loc2p;
+  int null_condition_or_parse_error = 0;
+  int modified = bl->needs_update;
+  struct bp_location *loc;
+
+  /* This is only meaningful if the target is
+     evaluating conditions and if the user has
+     opted for condition evaluation on the target's
+     side.  */
+  if (gdb_evaluates_breakpoint_condition_p ()
+      || !target_supports_breakpoint_conditions ())
+    return 1;
+
+  /* Do a first pass to check for locations with no assigned
+     conditions or conditions that fail to parse to a valid agent expression
+     bytecode.  If any of these happen, then it's no use to send conditions
+     to the target since this location will always trigger and generate a
+     response back to GDB.  */
+  ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
+    {
+      loc = (*loc2p);
+      if (is_breakpoint (loc->owner))
+	{
+	  if (modified)
+	    {
+	      struct agent_expr *aexpr;
+
+	      /* 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.  */
+	      aexpr = parse_cond_to_aexpr (bl->address, loc->cond);
+	      loc->cond_bytecode = aexpr;
+
+	      /* Check if we managed to parse the conditional expression
+		 correctly.  If not, we will not send this condition
+		 to the target.  */
+	      if (aexpr)
+		continue;
+	    }
+
+	  /* If we have a NULL bytecode expression, it means something
+	     went wrong or we have a null condition expression.  */
+	  if (!loc->cond_bytecode)
+	    {
+	      null_condition_or_parse_error = 1;
+	      break;
+	    }
+	}
+    }
+
+  /* If any of these happened, it means we will have to evaluate the conditions
+     for the location's address on gdb's side.  It is no use keeping bytecodes
+     for all the other duplicate locations, thus we free all of them here.
+
+     This is so we have a finer control over which locations' conditions are
+     being evaluated by GDB or the remote stub.  */
+  if (null_condition_or_parse_error)
+    {
+      ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
+	{
+	  loc = (*loc2p);
+	  if (is_breakpoint (loc->owner))
+	    {
+	      if (loc->cond_bytecode)
+		{
+		  free_agent_expr (loc->cond_bytecode);
+		  loc->cond_bytecode = NULL;
+		}
+	      else
+		/* Only go as far as the first NULL bytecode is
+		   located.  */
+		return 1;
+	    }
+	}
+      /* We will never reach this point.  */
+    }
+
+  /* No NULL conditions or failed bytecode generation.  Build a condition list
+     for this location's address.  */
+  ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
+    {
+      loc = (*loc2p);
+      if (loc->cond
+	  && is_breakpoint (loc->owner)
+	  && loc->enabled)
+	{
+	  struct condition_list *new_cond;
+	  new_cond = (struct condition_list *) xmalloc (sizeof (*new_cond));
+
+	  new_cond->expr = loc->cond_bytecode;
+	  new_cond->next = bl->target_info.cond_list;
+	  bl->target_info.cond_list = new_cond;
+	}
+    }
+
+  return 0;
+}
+
 /* Insert a low-level "breakpoint" of some type.  BL is the breakpoint
    location.  Any error messages are printed to TMP_ERROR_STREAM; and
    DISABLED_BREAKS, and HW_BREAKPOINT_ERROR are used to report problems.
@@ -1648,7 +1945,7 @@ insert_bp_location (struct bp_location *
 {
   int val = 0;
 
-  if (!should_be_inserted (bl) || bl->inserted)
+  if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update))
     return 0;
 
   /* Initialize the target-specific information.  */
@@ -1657,6 +1954,18 @@ insert_bp_location (struct bp_location *
   bl->target_info.placed_address_space = bl->pspace->aspace;
   bl->target_info.length = bl->length;
 
+  /* When working with target-side conditions, we must pass all the conditions
+     for the same breakpoint address down to the target since GDB will not
+     insert those locations.  With a list of breakpoint conditions, the target
+     can decide when to stop and notify GDB.  */
+
+  if (is_breakpoint (bl->owner))
+    {
+      build_target_condition_list (bl);
+      /* Reset the condition modification marker.  */
+      bl->needs_update = 0;
+    }
+
   if (bl->loc_type == bp_loc_software_breakpoint
       || bl->loc_type == bp_loc_hardware_breakpoint)
     {
@@ -1988,7 +2297,7 @@ insert_breakpoint_locations (void)
 
   ALL_BP_LOCATIONS (bl, blp_tmp)
     {
-      if (!should_be_inserted (bl) || bl->inserted)
+      if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update))
 	continue;
 
       /* There is no point inserting thread-specific breakpoints if
@@ -3996,6 +4305,11 @@ bpstat_check_breakpoint_conditions (bpst
   b = bs->breakpoint_at;
   gdb_assert (b != NULL);
 
+  /* Even if the target evaluated the condition on its end and notified GDB, we
+     need to do so again since GDB does not know if we stopped due to a
+     breakpoint or a single step.  This will only be done when we single step
+     straight to a conditional breakpoint.  */
+
   if (frame_id_p (b->frame_id)
       && !frame_id_eq (b->frame_id, get_stack_frame_id (get_current_frame ())))
     bs->stop = 0;
@@ -4905,6 +5219,19 @@ print_one_breakpoint_location (struct br
       else
 	ui_out_text (uiout, "\tstop only if ");
       ui_out_field_string (uiout, "cond", b->cond_string);
+
+      /* Print whether the remote stub is doing the breakpoint's condition
+	 evaluation.  If GDB is doing the evaluation, don't print anything.  */
+      if (loc && is_breakpoint (b) && loc->cond_bytecode
+	  && breakpoint_condition_evaluation_mode ()
+	  != condition_evaluation_gdb)
+	{
+	  ui_out_text (uiout, " (");
+	  ui_out_field_string (uiout, "condeval",
+			       breakpoint_condition_evaluation_mode ());
+	  ui_out_text (uiout, ")");
+	}
+
       ui_out_text (uiout, "\n");
     }
 
@@ -5617,6 +5944,7 @@ init_bp_location (struct bp_location *lo
   loc->ops = ops;
   loc->owner = owner;
   loc->cond = NULL;
+  loc->cond_bytecode = NULL;
   loc->shlib_disabled = 0;
   loc->enabled = 1;
 
@@ -5644,9 +5972,11 @@ init_bp_location (struct bp_location *lo
     case bp_gnu_ifunc_resolver:
     case bp_gnu_ifunc_resolver_return:
       loc->loc_type = bp_loc_software_breakpoint;
+      mark_breakpoint_location_modified (loc);
       break;
     case bp_hardware_breakpoint:
       loc->loc_type = bp_loc_hardware_breakpoint;
+      mark_breakpoint_location_modified (loc);
       break;
     case bp_hardware_watchpoint:
     case bp_read_watchpoint:
@@ -10366,12 +10696,56 @@ swap_insertion (struct bp_location *left
 
   left->inserted = right->inserted;
   left->duplicate = right->duplicate;
+  left->needs_update = right->needs_update;
   left->target_info = right->target_info;
   right->inserted = left_inserted;
   right->duplicate = left_duplicate;
+  left->needs_update = right->needs_update;
   right->target_info = left_target_info;
 }
 
+/* Force the re-insertion of the locations at ADDRESS.  This is called
+   once a new/deleted/modified duplicate location is found and we are evaluating
+   conditions on the target's side.  Such conditions need to be updated on
+   the target.  */
+
+static void
+force_breakpoint_reinsertion (CORE_ADDR address)
+{
+  struct bp_location **locp = NULL, **loc2p;
+  struct bp_location *loc;
+
+  /* This is only meaningful if the target is
+     evaluating conditions and if the user has
+     opted for condition evaluation on the target's
+     side.  */
+  if (gdb_evaluates_breakpoint_condition_p ()
+      && !target_supports_breakpoint_conditions ())
+    return;
+
+  /* Flag all breakpoint locations with this address
+     as "its conditions has changed".  We need to
+     update the conditions on the target's side.  */
+  ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, address)
+    {
+      loc = *loc2p;
+
+      if (!is_breakpoint (loc->owner))
+	continue;
+
+      /* Flag the location appropriately.  */
+      loc->condition_changed = 1;
+
+      /* Free the agent expression bytecode as well.  We will compute
+	 it later on.  */
+      if (loc->cond_bytecode)
+	{
+	  free_agent_expr (loc->cond_bytecode);
+	  loc->cond_bytecode = NULL;
+	}
+    }
+}
+
 /* If SHOULD_INSERT is false, do not insert any breakpoint locations
    into the inferior, only remove already-inserted locations that no
    longer should be inserted.  Functions that delete a breakpoint or
@@ -10393,6 +10767,8 @@ update_global_location_list (int should_
   struct breakpoint *b;
   struct bp_location **locp, *loc;
   struct cleanup *cleanups;
+  /* Last breakpoint location address that was marked for update.  */
+  CORE_ADDR last_addr = 0;
 
   /* Used in the duplicates detection below.  When iterating over all
      bp_locations, points to the first bp_location of a given address.
@@ -10453,6 +10829,8 @@ update_global_location_list (int should_
       /* Tells if the location should remain inserted in the target.  */
       int keep_in_target = 0;
       int removed = 0;
+      /* Whether a breakpoint location's condition has been modified.  */
+      int modified = 0;
 
       /* Skip LOCP entries which will definitely never be needed.
 	 Stop either at or being the one matching OLD_LOC.  */
@@ -10465,13 +10843,32 @@ update_global_location_list (int should_
 	    && (*loc2p)->address == old_loc->address);
 	   loc2p++)
 	{
+	  modified = modified? 1 : (*loc2p)->condition_changed;
+
 	  if (*loc2p == old_loc)
 	    {
 	      found_object = 1;
-	      break;
+
+	      /* If we already marked this address for insertion,
+		 just break from the loop.  */
+	      if (last_addr == old_loc->address)
+		break;
+	      else
+		continue;
 	    }
 	}
 
+      /* Check if this is a new/deleted duplicated location or a duplicated
+	 location that had its condition modified.  If so,  we want to send its
+	 condition to the target if evaluation of conditions is taking
+	 place there.  */
+      if (last_addr != old_loc->address
+	  && is_breakpoint (old_loc->owner)
+	  && (modified || !found_object))
+	{
+	  last_addr = old_loc->address;
+	  force_breakpoint_reinsertion (old_loc->address);
+	}
       /* If this location is no longer present, and inserted, look if
 	 there's maybe a new location at the same address.  If so,
 	 mark that one inserted, and don't remove this one.  This is
@@ -10491,6 +10888,10 @@ update_global_location_list (int should_
 	    }
 	  else
 	    {
+	      /* This location still exists, but it won't be kept in the
+		 target since it may have been disabled.  We proceed to
+		 remove its target-side condition.  */
+
 	      /* The location is either no longer present, or got
 		 disabled.  See if there's another location at the
 		 same address, in which case we don't need to remove
@@ -10610,6 +11011,13 @@ update_global_location_list (int should_
 	    }
 	  else
 	    {
+	      if (last_addr != old_loc->address
+		  && is_breakpoint (old_loc->owner))
+		{
+		  force_breakpoint_reinsertion (old_loc->address);
+		  last_addr = old_loc->address;
+		}
+
 	      old_loc->owner = NULL;
 	      decref_bp_location (&old_loc);
 	    }
@@ -10643,7 +11051,15 @@ update_global_location_list (int should_
 	   never duplicated.  See the comments in field `duplicate' of
 	   `struct bp_location'.  */
 	  || is_tracepoint (b))
-	continue;
+	{
+	  /* This is not the first location, so unflag it.  We
+	     only want to keep the first location flagged.  */
+	  if (!gdb_evaluates_breakpoint_condition_p ()
+	      && target_supports_breakpoint_conditions ()
+	      && is_breakpoint (loc->owner))
+	    loc->condition_changed = 0;
+	  continue;
+	}
 
       /* Permanent breakpoint should always be inserted.  */
       if (b->enable_state == bp_permanent && ! loc->inserted)
@@ -10666,6 +11082,11 @@ update_global_location_list (int should_
 	{
 	  *loc_first_p = loc;
 	  loc->duplicate = 0;
+	  if (is_breakpoint (loc->owner) && loc->condition_changed)
+	    {
+	      loc->needs_update = 1;
+	      loc->condition_changed = 0;
+	    }
 	  continue;
 	}
 
@@ -10801,6 +11222,8 @@ static void
 bp_location_dtor (struct bp_location *self)
 {
   xfree (self->cond);
+  if (self->cond_bytecode)
+    free_agent_expr (self->cond_bytecode);
   xfree (self->function_name);
   xfree (self->source_file);
 }
@@ -11387,7 +11810,10 @@ delete_breakpoint (struct breakpoint *bp
      itself, since remove_breakpoint looks at location's owner.  It
      might be better design to have location completely
      self-contained, but it's not the case now.  */
-  update_global_location_list (0);
+  if (is_breakpoint (bpt))
+    update_global_location_list (1);
+  else
+    update_global_location_list (0);
 
   bpt->ops->dtor (bpt);
   /* On the chance that someone will soon try again to delete this
@@ -12243,6 +12669,9 @@ disable_breakpoint (struct breakpoint *b
 
   bpt->enable_state = bp_disabled;
 
+  /* Mark breakpoint locations modified.  */
+  mark_breakpoint_modified (bpt);
+
   if (target_supports_enable_disable_tracepoint ()
       && current_trace_status ()->running && is_tracepoint (bpt))
     {
@@ -12252,7 +12681,10 @@ disable_breakpoint (struct breakpoint *b
 	target_disable_tracepoint (location);
     }
 
-  update_global_location_list (0);
+  if (is_breakpoint (bpt))
+    update_global_location_list (1);
+  else
+    update_global_location_list (0);
 
   observer_notify_breakpoint_modified (bpt);
 }
@@ -12290,13 +12722,20 @@ disable_command (char *args, int from_tt
       struct bp_location *loc = find_location_by_number (args);
       if (loc)
 	{
-	  loc->enabled = 0;
+	  if (loc->enabled)
+	    {
+	      loc->enabled = 0;
+	      mark_breakpoint_location_modified (loc);
+	    }
 	  if (target_supports_enable_disable_tracepoint ()
 	      && current_trace_status ()->running && loc->owner
 	      && is_tracepoint (loc->owner))
 	    target_disable_tracepoint (loc);
 	}
-      update_global_location_list (0);
+      if (loc && is_breakpoint (loc->owner))
+	update_global_location_list (1);
+      else
+	update_global_location_list (0);
     }
   else
     map_breakpoint_numbers (args, do_map_disable_breakpoint, NULL);
@@ -12346,6 +12785,11 @@ enable_breakpoint_disp (struct breakpoin
   if (bpt->enable_state != bp_permanent)
     bpt->enable_state = bp_enabled;
 
+  bpt->enable_state = bp_enabled;
+
+  /* Mark breakpoint locations modified.  */
+  mark_breakpoint_modified (bpt);
+
   if (target_supports_enable_disable_tracepoint ()
       && current_trace_status ()->running && is_tracepoint (bpt))
     {
@@ -12404,7 +12848,11 @@ enable_command (char *args, int from_tty
       struct bp_location *loc = find_location_by_number (args);
       if (loc)
 	{
-	  loc->enabled = 1;
+	  if (!loc->enabled)
+	    {
+	      loc->enabled = 1;
+	      mark_breakpoint_location_modified (loc);
+	    }
 	  if (target_supports_enable_disable_tracepoint ()
 	      && current_trace_status ()->running && loc->owner
 	      && is_tracepoint (loc->owner))
@@ -13772,8 +14220,9 @@ The \"Type\" column indicates one of:\n\
 \twatchpoint     - watchpoint\n\
 The \"Disp\" column contains one of \"keep\", \"del\", or \"dis\" to indicate\n\
 the disposition of the breakpoint after it gets hit.  \"dis\" means that the\n\
-breakpoint will be disabled.  The \"Address\" and \"What\" columns indicate the\n\
-address and file/line number respectively.\n\
+breakpoint will be disabled.  The \"Address\" and \"What\" columns indicate\n\
+the address and file/line number respectively. The \"Cond Eval\" column\n\
+indicates where the breakpoint condition will be evaluated.\n\
 \n\
 Convenience variable \"$_\" and default examine address for \"x\"\n\
 are set to the address of the last breakpoint listed unless the command\n\
@@ -13789,8 +14238,9 @@ The \"Type\" column indicates one of:\n\
 \twatchpoint     - watchpoint\n\
 The \"Disp\" column contains one of \"keep\", \"del\", or \"dis\" to indicate\n\
 the disposition of the breakpoint after it gets hit.  \"dis\" means that the\n\
-breakpoint will be disabled.  The \"Address\" and \"What\" columns indicate the\n\
-address and file/line number respectively.\n\
+breakpoint will be disabled.  The \"Address\" and \"What\" columns indicate\n\
+the address and file/line number respectively. The \"Cond Eval\" column\n\
+indicates where the breakpoint condition will be evaluated.\n\
 \n\
 Convenience variable \"$_\" and default examine address for \"x\"\n\
 are set to the address of the last breakpoint listed unless the command\n\
@@ -13808,8 +14258,9 @@ The \"Type\" column indicates one of:\n\
 \twatchpoint     - watchpoint\n\
 The \"Disp\" column contains one of \"keep\", \"del\", or \"dis\" to indicate\n\
 the disposition of the breakpoint after it gets hit.  \"dis\" means that the\n\
-breakpoint will be disabled.  The \"Address\" and \"What\" columns indicate the\n\
-address and file/line number respectively.\n\
+breakpoint will be disabled.  The \"Address\" and \"What\" columns indicate\n\
+the address and file/line number respectively. The \"Cond Eval\" column\n\
+indicates where the breakpoint condition will be evaluated.\n\
 \n\
 Convenience variable \"$_\" and default examine address for \"x\"\n\
 are set to the address of the last breakpoint listed unless the command\n\
@@ -13828,8 +14279,9 @@ The \"Type\" column indicates one of:\n\
 \tfinish         - internal breakpoint used by the \"finish\" command\n\
 The \"Disp\" column contains one of \"keep\", \"del\", or \"dis\" to indicate\n\
 the disposition of the breakpoint after it gets hit.  \"dis\" means that the\n\
-breakpoint will be disabled.  The \"Address\" and \"What\" columns indicate the\n\
-address and file/line number respectively.\n\
+breakpoint will be disabled.  The \"Address\" and \"What\" columns indicate\n\
+the address and file/line number respectively. The \"Cond Eval\" column\n\
+indicates where the breakpoint condition will be evaluated.\n\
 \n\
 Convenience variable \"$_\" and default examine address for \"x\"\n\
 are set to the address of the last breakpoint listed unless the command\n\
@@ -14094,6 +14546,23 @@ inferior in all-stop mode, gdb behaves a
 			   &breakpoint_set_cmdlist,
 			   &breakpoint_show_cmdlist);
 
+  add_setshow_enum_cmd ("condition-evaluation", class_breakpoint,
+			condition_evaluation_enums,
+			&condition_evaluation_mode, _("\
+Set mode for breakpoint condition evaluation."), _("\
+Show mode for breakpoint condition evaluation."), _("\
+When this is set to \"gdb\", breakpoint conditions will be\n\
+evaluated on the host's side by GDB.  When it is set to \"stub\", breakpoint\n\
+conditions will be downloaded to the stub (if the stub supports such feature)\n\
+and conditions will be evaluated on the target's side by the remote stub. If\n\
+this is set to \"auto\" (default), this setting will be automatically set\n\
+to \"stub\" if said stub supports condition evaluation, otherwise it will\n\
+be set to \"gdb\""),
+			   NULL,
+			   &show_condition_evaluation_mode,
+			   &breakpoint_set_cmdlist,
+			   &breakpoint_show_cmdlist);
+
   add_com ("break-range", class_breakpoint, break_range_command, _("\
 Set a breakpoint for an address range.\n\
 break-range START-LOCATION, END-LOCATION\n\
Index: gdb/gdb/breakpoint.h
===================================================================
--- gdb.orig/gdb/breakpoint.h	2012-01-04 14:02:33.614431998 -0200
+++ gdb/gdb/breakpoint.h	2012-01-04 14:03:03.698431999 -0200
@@ -22,6 +22,7 @@
 #include "frame.h"
 #include "value.h"
 #include "vec.h"
+#include "ax.h"
 
 struct value;
 struct block;
@@ -213,6 +214,15 @@ enum target_hw_bp_type
   };
 
 
+/* List of conditions used to pass conditions down to the target.  */
+struct condition_list
+{
+  /* Breakpoint condition.  */
+  struct agent_expr *expr;
+  /* Pointer to the next condition.  */
+  struct condition_list *next;
+};
+
 /* Information used by targets to insert and remove breakpoints.  */
 
 struct bp_target_info
@@ -247,6 +257,10 @@ struct bp_target_info
      (e.g. if a remote stub handled the details).  We may still need
      the size to remove the breakpoint safely.  */
   int placed_size;
+
+  /* List of conditions the target should evaluate if it supports target-side
+     breakpoint conditions.  */
+  struct condition_list *cond_list;
 };
 
 /* GDB maintains two types of information about each breakpoint (or
@@ -313,6 +327,21 @@ struct bp_location
      the owner breakpoint object.  */
   struct expression *cond;
 
+  /* Conditional expression in agent expression
+     bytecode form.  This is used for stub-side breakpoint
+     condition evaluation.  */
+  struct agent_expr *cond_bytecode;
+
+  /* Signals that the condition has changed since the last time
+     we updated the global location list.  This means the condition
+     needs to be sent to the target again.  This is used together
+     with target-side breakpoint conditions.  */
+  int condition_changed;
+
+  /* Signals that breakpoint conditions need to be re-synched with the
+     target.  This has no use other than target-side breakpoints.  */
+  int needs_update;
+
   /* This location's address is in an unloaded solib, and so this
      location should not be inserted.  It will be automatically
      enabled when that solib is loaded.  */
@@ -683,6 +712,10 @@ struct watchpoint
   CORE_ADDR hw_wp_mask;
 };
 
+/* Returns true if BPT is a breakpoint of any kind.  */
+
+extern int is_breakpoint (const struct breakpoint *bpt);
+
 /* Returns true if BPT is really a watchpoint.  */
 
 extern int is_watchpoint (const struct breakpoint *bpt);

Re: [RFC stub-side break conditions 3/5] GDB-side changes

[Eli Zaretskii]

> Date: Thu, 05 Jan 2012 12:56:16 -0200
> From: Luis Machado <luis_gustavo@mentor.com>
> 
> This machinery works by marking locations modified through the 
> "condition_changed" field. Conditions change whenever we create a new 
> location, use the "condition" command or delete an old duplicate 
> location. Furthermore, if a location at addr 0xdeadbeef had its 
> condition modified, all duplicate locations at 0xdeadbeef are marked 
> modified as well.
> 
> The modification detection takes place inside 
> update_global_location_list (...). By calling 
> force_breakpoint_reinsertion (...), we mark every location at address 
> 0xdeadbeef as modified. When consolidating the final updated list of 
> locations, we detect these locations and mark the non-duplicate location 
> at 0xdeafbeef as "needs_update".
> 
> The "needs_update" field is used during breakpoint insertion. It forces 
> insertion of breakpoints even if they are marked as inserted already.
> 
> Now that we have information about locations that we should reinsert in 
> the target, we proceed to build a list of hex-encoded agent expressions.
> 
> At this point, if any conditional expressions fail to parse to a valid 
> agent expression, we assume that the conditions will be evaluated on 
> GDB's side. Thus we don't send the conditions to the stub. Otherwise, we 
> proceed to insert the breakpoints and the remote code now attaches the 
> hex-encoded expressions to the z0/z1 packet.
> 
> Regarding always-inserted mode, if it is "on" we must send condition 
> changes right away, or else the target will potentially miss breakpoint 
> hits. If always-inserted is "off", this isn't too critical, so we just 
> wait for the insertion call to send all the conditions to the stub. We 
> will remove them when stopping anyway.

WIBNI these details were somewhere in the source comments or (gasp!)
in gdbint.texinfo?

> +/* Shows the current mode of breakpoint condition evaluation.  Explicitly shows
> +   what "auto" is translating to.  */
> +
> +static void
> +show_condition_evaluation_mode (struct ui_file *file, int from_tty,
> +				struct cmd_list_element *c, const char *value)
> +{
> +  if (condition_evaluation_mode == condition_evaluation_auto)
> +    fprintf_filtered (file,
> +		      _("Breakpoint condition evaluation "
> +			"mode is %s (currently %s).\n"),
> +		      value,
> +		      breakpoint_condition_evaluation_mode ());
> +  else
> +    fprintf_filtered (file, _("Breakpoint condition evaluation mode is %s.\n"),
> +		      value);
> +}
> +

Is it a good idea to show "gdb" or "stub" rather than "auto"?  After
all, as you explained elsewhere, the translation is not 100% accurate,
depending on the specifics of each individual condition.

> +      /* Print whether the remote stub is doing the breakpoint's condition
> +	 evaluation.  If GDB is doing the evaluation, don't print anything.  */
> +      if (loc && is_breakpoint (b) && loc->cond_bytecode
> +	  && breakpoint_condition_evaluation_mode ()
> +	  != condition_evaluation_gdb)
> +	{
> +	  ui_out_text (uiout, " (");
> +	  ui_out_field_string (uiout, "condeval",
> +			       breakpoint_condition_evaluation_mode ());

I suggest "cond.eval." instead of "condeval".  Better yet, how about
"evaluated by"?

> -address and file/line number respectively.\n\
> +breakpoint will be disabled.  The \"Address\" and \"What\" columns indicate\n\
> +the address and file/line number respectively. The \"Cond Eval\" column\n\
                                                ^^
Need two spaces here.

> -address and file/line number respectively.\n\
> +breakpoint will be disabled.  The \"Address\" and \"What\" columns indicate\n\
> +the address and file/line number respectively. The \"Cond Eval\" column\n\

Likewise.

> -address and file/line number respectively.\n\
> +breakpoint will be disabled.  The \"Address\" and \"What\" columns indicate\n\
> +the address and file/line number respectively. The \"Cond Eval\" column\n\

Likewise.

> -address and file/line number respectively.\n\
> +breakpoint will be disabled.  The \"Address\" and \"What\" columns indicate\n\
> +the address and file/line number respectively. The \"Cond Eval\" column\n\

Likewise.

> +  add_setshow_enum_cmd ("condition-evaluation", class_breakpoint,
> +			condition_evaluation_enums,
> +			&condition_evaluation_mode, _("\
> +Set mode for breakpoint condition evaluation."), _("\

"Set mode of breakpoint condition evaluation".  "Of", not "for".  And
the same for "Show".

> +and conditions will be evaluated on the target's side by the remote stub. If\n\
                                                                           ^^
Two spaces.

> +this is set to \"auto\" (default), this setting will be automatically set\n\

"setting with be ... set" is not the best style.  Just "this will be
automatically set" is better.

Btw, what happens if I set the mode to "stub" and the sub does not
support this?  Do I get any feedback, and if so, at what time?

Thanks.

Re: [RFC stub-side break conditions 3/5] GDB-side changes

[Pedro Alves]

On 01/05/2012 02:56 PM, Luis Machado wrote:

> Regarding always-inserted mode, if it is "on" we must send condition changes right away, or else the target will potentially miss breakpoint hits.

Could you elaborate a bit more on how will it miss breakpoint hits?

 > I'd like to hear about the change to "info break"

As they say, a picture is worth a thousand words.  How does the new output look like?  What about MI?

 > and also the way we should pass conditions down to the stub.  Currently i'm adding the agent expressions to a condition list in the target info field of bp locations.

Sounds good at first sight.


I'm mostly wondering whether the assumption that we can re-insert
a breakpoint at the same address is solid.



 > +static int
 > +gdb_evaluates_breakpoint_condition_p (void)
 > +{
 > +  const char *mode = breakpoint_condition_evaluation_mode ();
 > +
 > +  return (mode == condition_evaluation_gdb? 1 : 0);

Missing space before `?'.  Or just write

   return (mode == condition_evaluation_gdb);

Same thing.

 > +}


 > +/* Iterates through locations with address ADDRESS.  BP_LOCP_TMP points to
 > +   each object.  BP_LOCP_START points to where the loop should start from.
 > +   If BP_LOCP_START is a NULL pointer, the macro automatically seeks the
 > +   appropriate location to start with.  */
 > +
 > +#define ALL_BP_LOCATIONS_AT_ADDR(BP_LOCP_TMP, BP_LOCP_START, ADDRESS)	\
 > +	if (!BP_LOCP_START)						\
 > +	  BP_LOCP_START = get_first_locp_gte_addr (ADDRESS);		\
 > +	for (BP_LOCP_TMP = BP_LOCP_START;				\
 > +	     (BP_LOCP_TMP<  bp_location + bp_location_count		\
 > +	&&  (*BP_LOCP_TMP)->address == ADDRESS);			\
 > +	     BP_LOCP_TMP++)
 > +

The address alone is not enough when you consider multi-process.
The users of this macro aren't considering that.  Several instances of
this problem.


 > +/* 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
 > +   target will evaluate the list of conditions and will only notify GDB when
 > +   one of them is true.  */

What does the return code mean?

 > +
 > +static int
 > +build_target_condition_list (struct bp_location *bl)
 > +{



On 01/05/2012 02:56 PM, Luis Machado wrote:
 > +	}
 > +      /* We will never reach this point.  */
 > +    }

Then by all means gdb_assert.  :-)




 > +  /* Even if the target evaluated the condition on its end and notified GDB, we
 > +     need to do so again since GDB does not know if we stopped due to a
 > +     breakpoint or a single step.

 > + This will only be done when we single step straight to a conditional breakpoint.  */

Where is the code that makes this true?

 > +
 >     if (frame_id_p (b->frame_id)
 >         &&  !frame_id_eq (b->frame_id, get_stack_frame_id (get_current_frame ())))
 >       bs->stop = 0;
 > @@ -4905,6 +5219,19 @@ print_one_breakpoint_location (struct br
 >         else
 >   	ui_out_text (uiout, "\tstop only if ");
 >         ui_out_field_string (uiout, "cond", b->cond_string);
 > +
 > +      /* Print whether the remote stub is doing the breakpoint's condition
 > +	 evaluation.  If GDB is doing the evaluation, don't print anything.  */
 > +      if (loc&&  is_breakpoint (b)&&  loc->cond_bytecode
 > +	&&  breakpoint_condition_evaluation_mode ()
 > +	  != condition_evaluation_gdb)

What if you set a conditional breakpoint, have it evaluate the in target,
and then, flip the setting to gdb-side, while the breakpoint's condition is
still being evaluated on the target side?  Shouldn't this always
print "target"/"stub" when loc->cond_bytecode is non-NULL?

 > +	{
 > +	  ui_out_text (uiout, " (");
 > +	  ui_out_field_string (uiout, "condeval",
 > +			       breakpoint_condition_evaluation_mode ());
 > +	  ui_out_text (uiout, ")");
 > +	}
 > +
 >         ui_out_text (uiout, "\n");
 >       }




 > @@ -10366,12 +10696,56 @@ swap_insertion (struct bp_location *left
 >
 >     left->inserted = right->inserted;
 >     left->duplicate = right->duplicate;
 > +  left->needs_update = right->needs_update;
 >     left->target_info = right->target_info;
 >     right->inserted = left_inserted;
 >     right->duplicate = left_duplicate;
 > +  left->needs_update = right->needs_update;

Doesn't look right.  Should probably be

   const int left_needs_update = left->needs_update;
   ...
   left->needs_update = right->needs_update;
   ...
   right->needs_update = left_needs_update;



 > +	  modified = modified? 1 : (*loc2p)->condition_changed;

Space before `?'.  Perhaps clearer alternatives are:

   modified = modified || (*loc2p)->condition_changed;

or even the common:

   if (!modified)
     modified = (*loc2p)->condition_changed;




 > +      /* Check if this is a new/deleted duplicated location or a duplicated
 > +	 location that had its condition modified.  If so,  we want to send its

Spurious space.


 > @@ -11387,7 +11810,10 @@ delete_breakpoint (struct breakpoint *bp
 >        itself, since remove_breakpoint looks at location's owner.  It
 >        might be better design to have location completely
 >        self-contained, but it's not the case now.  */
 > -  update_global_location_list (0);
 > +  if (is_breakpoint (bpt))
 > +    update_global_location_list (1);

The whole point of the update_global_location_list parameter is being able
to say "don't insert locations new locations because I'm just deleting a
breakpoint".  This was necessary for situations like following an exec, IIRC.

 > +  else
 > +    update_global_location_list (0);
 >


 > +/* List of conditions used to pass conditions down to the target.  */
 > +struct condition_list
 > +{
 > +  /* Breakpoint condition.  */
 > +  struct agent_expr *expr;
 > +  /* Pointer to the next condition.  */
 > +  struct condition_list *next;
 > +};

Can this be a VEC of `struct agent_expr *' instead?

Re: [RFC stub-side break conditions 3/5] GDB-side changes

[Tom Tromey]

>>>>> "Luis" == Luis Machado <luis_gustavo@mentor.com> writes:

Luis> This patch handles GDB-specific changes to support stub-side
Luis> breakpoint conditions.

Nice series, I quite like it.

Luis> I ran the testsuite and had no additional failures.

I think the patches should include some tests for the new feature.

Luis> I'd like to hear about the change to "info break" and also the way we
Luis> should pass conditions down to the stub.

I looked at this code and, while I think the output bits are fine, there
is something I don't understand:

+      /* Print whether the remote stub is doing the breakpoint's condition
+	 evaluation.  If GDB is doing the evaluation, don't print anything.  */
+      if (loc && is_breakpoint (b) && loc->cond_bytecode
+	  && breakpoint_condition_evaluation_mode ()
+	  != condition_evaluation_gdb)

(First, there should be parens around that last subexpression.)

If the user changes 'condition-evaluation', then the new setting might
affect what "info break" prints.  But this seems weird, since what is
relevant is what is actually going on under the hood.

Or, on the other hand, changing 'condition-evaluation' should change the
state of all breakpoints; but this requires additional code AFAICT.

Luis> +int remote_supports_cond_breakpoints (void);

Is it much uglier to rearrange things so no forward declaration is
needed?

Luis> +  /* List of conditions the target should evaluate if it supports target-side
Luis> +     breakpoint conditions.  */
Luis> +  struct condition_list *cond_list;

It seems that the callee must free these.  I think that should be
documented here.

Luis> +  /* Conditional expression in agent expression
Luis> +     bytecode form.  This is used for stub-side breakpoint
Luis> +     condition evaluation.  */
Luis> +  struct agent_expr *cond_bytecode;
Luis> +
Luis> +  /* Signals that the condition has changed since the last time
Luis> +     we updated the global location list.  This means the condition
Luis> +     needs to be sent to the target again.  This is used together
Luis> +     with target-side breakpoint conditions.  */
Luis> +  int condition_changed;
Luis> +
Luis> +  /* Signals that breakpoint conditions need to be re-synched with the
Luis> +     target.  This has no use other than target-side breakpoints.  */
Luis> +  int needs_update;

I still wish we had 'bool'.  Maybe this year.

pahole says there are holes in bp_location on my x86-64 box.  I think it
is somewhat preferable to fill the holes and to follow existing
practice, that is, use 'char' for boolean fields here.

My preference would be to use 'unsigned int : 1' for boolean fields, but
I don't know how others feel about this.

Tom

Re: [RFC stub-side break conditions 3/5] GDB-side changes

[Luis Machado]

On 01/06/2012 06:44 AM, Eli Zaretskii wrote:
>> Date: Thu, 05 Jan 2012 12:56:16 -0200
>> From: Luis Machado<luis_gustavo@mentor.com>
>>
>> This machinery works by marking locations modified through the
>> "condition_changed" field. Conditions change whenever we create a new
>> location, use the "condition" command or delete an old duplicate
>> location. Furthermore, if a location at addr 0xdeadbeef had its
>> condition modified, all duplicate locations at 0xdeadbeef are marked
>> modified as well.
>>
>> The modification detection takes place inside
>> update_global_location_list (...). By calling
>> force_breakpoint_reinsertion (...), we mark every location at address
>> 0xdeadbeef as modified. When consolidating the final updated list of
>> locations, we detect these locations and mark the non-duplicate location
>> at 0xdeafbeef as "needs_update".
>>
>> The "needs_update" field is used during breakpoint insertion. It forces
>> insertion of breakpoints even if they are marked as inserted already.
>>
>> Now that we have information about locations that we should reinsert in
>> the target, we proceed to build a list of hex-encoded agent expressions.
>>
>> At this point, if any conditional expressions fail to parse to a valid
>> agent expression, we assume that the conditions will be evaluated on
>> GDB's side. Thus we don't send the conditions to the stub. Otherwise, we
>> proceed to insert the breakpoints and the remote code now attaches the
>> hex-encoded expressions to the z0/z1 packet.
>>
>> Regarding always-inserted mode, if it is "on" we must send condition
>> changes right away, or else the target will potentially miss breakpoint
>> hits. If always-inserted is "off", this isn't too critical, so we just
>> wait for the insertion call to send all the conditions to the stub. We
>> will remove them when stopping anyway.
> WIBNI these details were somewhere in the source comments or (gasp!)
> in gdbint.texinfo?

I could probably improve source documentation if it is not enough. I did 
point out in some places that conditions need to be synched with the target.

Regarding the internal documentation, i can include such descriptions 
there, no problem.
>> +/* Shows the current mode of breakpoint condition evaluation.  Explicitly shows
>> +   what "auto" is translating to.  */
>> +
>> +static void
>> +show_condition_evaluation_mode (struct ui_file *file, int from_tty,
>> +				struct cmd_list_element *c, const char *value)
>> +{
>> +  if (condition_evaluation_mode == condition_evaluation_auto)
>> +    fprintf_filtered (file,
>> +		      _("Breakpoint condition evaluation "
>> +			"mode is %s (currently %s).\n"),
>> +		      value,
>> +		      breakpoint_condition_evaluation_mode ());
>> +  else
>> +    fprintf_filtered (file, _("Breakpoint condition evaluation mode is %s.\n"),
>> +		      value);
>> +}
>> +
> Is it a good idea to show "gdb" or "stub" rather than "auto"?  After
> all, as you explained elsewhere, the translation is not 100% accurate,
> depending on the specifics of each individual condition.

This will only show what the global setting is. If "auto", it will show 
"auto", but will also show what GDB is currently using given the 
available features.

If the user decides to switch to either "gdb" or "stub" (or "target"), 
this is what will be displayed by this function. This one does not 
relate directly to what is shown in "info break".

What is displayed in "info break" depends on the global setting and on 
the successful parsing of an expression into an agent expression, so it 
displays exactly where each location condition will be evaluated.

If "gdb" is set, every condition will be evaluated by GDB.  If "stub" 
(or "target") is set, there is no guarantee everything will be handled 
by the target (given the documented limitations), so the fallback is to 
evaluate conditions on GDB's side.

I'm regretting the "info break" change a little, it seems to obscure 
rather than clarify. :-)
>> +      /* Print whether the remote stub is doing the breakpoint's condition
>> +	 evaluation.  If GDB is doing the evaluation, don't print anything.  */
>> +      if (loc&&  is_breakpoint (b)&&  loc->cond_bytecode
>> +	&&  breakpoint_condition_evaluation_mode ()
>> +	  != condition_evaluation_gdb)
>> +	{
>> +	  ui_out_text (uiout, " (");
>> +	  ui_out_field_string (uiout, "condeval",
>> +			       breakpoint_condition_evaluation_mode ());
> I suggest "cond.eval." instead of "condeval".  Better yet, how about
> "evaluated by"?
Sounds good. I'll make that change.

Fixed all the other comments.
> Btw, what happens if I set the mode to "stub" and the sub does not
> support this?  Do I get any feedback, and if so, at what time?

If the target (stub) does not advertise support for condition 
evaluation, nothing will happen. We will fallback to "gdb" mode 
automatically.

Target-side condition evaluation only happens if:

1 - Target supports it (via ConditionalBreakpoints feature).
2 - breakpoint condition-evaluation is "auto" or "stub".

There isn't a warning message though.

I'll send an updated version of the patch with the fixes.

Thanks

--
Luis
lgustavo@codesourcery.com

Re: [RFC stub-side break conditions 3/5] GDB-side changes

[Eli Zaretskii]

> Date: Fri, 06 Jan 2012 20:37:14 -0200
> From: Luis Machado <luis_gustavo@mentor.com>
> CC: gdb-patches@sourceware.org
> 
> > Btw, what happens if I set the mode to "stub" and the sub does not
> > support this?  Do I get any feedback, and if so, at what time?
> 
> If the target (stub) does not advertise support for condition 
> evaluation, nothing will happen. We will fallback to "gdb" mode 
> automatically.
> 
> Target-side condition evaluation only happens if:
> 
> 1 - Target supports it (via ConditionalBreakpoints feature).
> 2 - breakpoint condition-evaluation is "auto" or "stub".
> 
> There isn't a warning message though.

Should there be some feedback somewhere?

Re: [RFC stub-side break conditions 3/5] GDB-side changes

[Luis Machado]

On 01/06/2012 02:23 PM, Pedro Alves wrote:
> On 01/05/2012 02:56 PM, Luis Machado wrote:
>
>> Regarding always-inserted mode, if it is "on" we must send condition 
>> changes right away, or else the target will potentially miss 
>> breakpoint hits.
>
> Could you elaborate a bit more on how will it miss breakpoint hits?

That is a bit misleading. Not actually the target, but GDB may not be 
notified of breakpoint hits that it should've seen.

For example. Suppose we have a conditional breakpoint location inserted 
at address foo. The inferior is running and stopping at the breakpoint 
correctly when its condition is true.

Suppose we insert a new unconditional location at foo. If we don't synch 
the breakpoint conditions of both those locations with the target, the 
target will keep ignoring any breakpoint hits at foo for which the 
condition of the first location is false.  Of course we may miss a few 
breakpoint hits during the condition synchronization operation, but GDB 
will eventually synch the conditions with the target instead of waiting 
for the next appropriate time to do so.

Another example is a set of 2 duplicate conditional breakpoint locations 
inserted at address foo. If we make one of them unconditional while the 
process is running (always-inserted mode on), the conditions need to be 
synched with the target as well. If we don't, GDB may miss some 
breakpoint triggers (since the target won't report every trigger, just 
the conditional ones).

>
> > I'd like to hear about the change to "info break"
>
> As they say, a picture is worth a thousand words.  How does the new 
> output look like?  What about MI?

An example:

2       breakpoint      keep y   0x080483dd in main at loop.c:12
     stop only if i==5 (stub)

The change in the output is just a (stub) right after the conditional 
expression.

This is being printed together with the conditional expression, so 
frontends will possibly pick that output up without requiring any 
additional changes.

>
> > and also the way we should pass conditions down to the stub.  
> Currently i'm adding the agent expressions to a condition list in the 
> target info field of bp locations.
>
> Sounds good at first sight.
>
>
> I'm mostly wondering whether the assumption that we can re-insert
> a breakpoint at the same address is solid.

Z packets should, in theory, be implemented in an idempotent way 
according to the documentation. So adding the same breakpoint location 
multiple times shouldn't be a problem.

I thought about potential issues with single-stepping breakpoints, but 
they will be inserted with no conditions (making the target remove any 
conditions that might be active on its end), thus i expect them to work.

> > +/* Iterates through locations with address ADDRESS.  BP_LOCP_TMP 
> points to
> > +   each object.  BP_LOCP_START points to where the loop should 
> start from.
> > +   If BP_LOCP_START is a NULL pointer, the macro automatically 
> seeks the
> > +   appropriate location to start with.  */
> > +
> > +#define ALL_BP_LOCATIONS_AT_ADDR(BP_LOCP_TMP, BP_LOCP_START, 
> ADDRESS)    \
> > +    if (!BP_LOCP_START)                        \
> > +      BP_LOCP_START = get_first_locp_gte_addr (ADDRESS);        \
> > +    for (BP_LOCP_TMP = BP_LOCP_START;                \
> > +         (BP_LOCP_TMP<  bp_location + bp_location_count        \
> > + &&  (*BP_LOCP_TMP)->address == ADDRESS);            \
> > +         BP_LOCP_TMP++)
> > +
>
> The address alone is not enough when you consider multi-process.
> The users of this macro aren't considering that.  Several instances of
> this problem.
>

IIUIC, we may have situations with breakpoint locations at the same 
address but on different processes.

If a breakpoint spans multiple processes, we must resynch conditions for 
both processes. If not, then we should only re-synch target conditions 
for the process that contains such a breakpoint/location.

Does that make sense?

>
> > +/* 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
> > +   target will evaluate the list of conditions and will only notify 
> GDB when
> > +   one of them is true.  */
>
> What does the return code mean?
>
> > +
> > +static int
> > +build_target_condition_list (struct bp_location *bl)
> > +{

Stale, it does not need one. Fixed.

> > +  /* Even if the target evaluated the condition on its end and 
> notified GDB, we
> > +     need to do so again since GDB does not know if we stopped due 
> to a
> > +     breakpoint or a single step.
>
> > + This will only be done when we single step straight to a 
> conditional breakpoint.  */
>
> Where is the code that makes this true?

The second sentence is a bit stale. GDB re-evaluates conditions 
everytime now, so we don't need to worry about what kind of event is 
associated with the breakpoint trigger.

Fixed.
>
> > +
> >     if (frame_id_p (b->frame_id)
> > &&  !frame_id_eq (b->frame_id, get_stack_frame_id (get_current_frame 
> ())))
> >       bs->stop = 0;
> > @@ -4905,6 +5219,19 @@ print_one_breakpoint_location (struct br
> >         else
> >       ui_out_text (uiout, "\tstop only if ");
> >         ui_out_field_string (uiout, "cond", b->cond_string);
> > +
> > +      /* Print whether the remote stub is doing the breakpoint's 
> condition
> > +     evaluation.  If GDB is doing the evaluation, don't print 
> anything.  */
> > +      if (loc&&  is_breakpoint (b)&&  loc->cond_bytecode
> > + &&  breakpoint_condition_evaluation_mode ()
> > +      != condition_evaluation_gdb)
>
> What if you set a conditional breakpoint, have it evaluate the in target,
> and then, flip the setting to gdb-side, while the breakpoint's 
> condition is
> still being evaluated on the target side?  Shouldn't this always
> print "target"/"stub" when loc->cond_bytecode is non-NULL?

I addressed this for Eli's comments as well. Now we prevent the user 
from setting "target" evaluation mode if the target does not support it. 
A warning is displayed.

The scenario you mentioned is taken care of now. Assume the target 
supports condition evaluation, if the user switches from "gdb" to 
"target" evaluation mode, GDB will send the breakpoint conditions to the 
target.

Similarly, if the user switches from "target" to "gdb", those conditions 
will be cleared.

>
>
> > @@ -11387,7 +11810,10 @@ delete_breakpoint (struct breakpoint *bp
> >        itself, since remove_breakpoint looks at location's owner.  It
> >        might be better design to have location completely
> >        self-contained, but it's not the case now.  */
> > -  update_global_location_list (0);
> > +  if (is_breakpoint (bpt))
> > +    update_global_location_list (1);
>
> The whole point of the update_global_location_list parameter is being 
> able
> to say "don't insert locations new locations because I'm just deleting a
> breakpoint".  This was necessary for situations like following an 
> exec, IIRC.

I'll double check this since the testsuite didn't give me any visible 
errors.

>
> > +  else
> > +    update_global_location_list (0);
> >
>
>
> > +/* List of conditions used to pass conditions down to the target.  */
> > +struct condition_list
> > +{
> > +  /* Breakpoint condition.  */
> > +  struct agent_expr *expr;
> > +  /* Pointer to the next condition.  */
> > +  struct condition_list *next;
> > +};
>
> Can this be a VEC of `struct agent_expr *' instead?

I'm using VEC now.

Fixed all the other issues.

I'll send a new version of the patch soon.

Thanks for reviewing.

-- 
Luis
lgustavo@codesourcery.com

Re: [RFC stub-side break conditions 3/5] GDB-side changes

[Luis Machado]

Hi Tom,

On 01/06/2012 07:44 PM, Tom Tromey wrote:
> Luis>  I ran the testsuite and had no additional failures.
>
> I think the patches should include some tests for the new feature.

Yes. I still need to think about those.

I'll probably add a few GDB-side tests for safety, but mostly 
target-side ones like the tracepoint tests.
> Luis>  I'd like to hear about the change to "info break" and also the way we
> Luis>  should pass conditions down to the stub.
>
> I looked at this code and, while I think the output bits are fine, there
> is something I don't understand:
>
> +      /* Print whether the remote stub is doing the breakpoint's condition
> +	 evaluation.  If GDB is doing the evaluation, don't print anything.  */
> +      if (loc&&  is_breakpoint (b)&&  loc->cond_bytecode
> +	&&  breakpoint_condition_evaluation_mode ()
> +	  != condition_evaluation_gdb)
>
> (First, there should be parens around that last subexpression.)
Fixed.
> If the user changes 'condition-evaluation', then the new setting might
> affect what "info break" prints.  But this seems weird, since what is
> relevant is what is actually going on under the hood.
>
> Or, on the other hand, changing 'condition-evaluation' should change the
> state of all breakpoints; but this requires additional code AFAICT.

This has been addressed according to Eli's and Pedro's comments.

If the user switches to a different mode, we will take appropriate 
action. Either we remove conditions or we add them to the target.
> Luis>  +int remote_supports_cond_breakpoints (void);
>
> Is it much uglier to rearrange things so no forward declaration is
> needed?

In this case, unfortunately, i think so.  I'd have to move the code that 
adds the conditions to the packet buffer to the far bottom of the file. 
If keeping similar functions apart is no issue, i can do this change.
> Luis>  +  /* List of conditions the target should evaluate if it supports target-side
> Luis>  +     breakpoint conditions.  */
> Luis>  +  struct condition_list *cond_list;
>
> It seems that the callee must free these.  I think that should be
> documented here.

This should be addressed now with the use of VEC's. We have a vector of 
agent expressions.
> Luis>  +  /* Conditional expression in agent expression
> Luis>  +     bytecode form.  This is used for stub-side breakpoint
> Luis>  +     condition evaluation.  */
> Luis>  +  struct agent_expr *cond_bytecode;
> Luis>  +
> Luis>  +  /* Signals that the condition has changed since the last time
> Luis>  +     we updated the global location list.  This means the condition
> Luis>  +     needs to be sent to the target again.  This is used together
> Luis>  +     with target-side breakpoint conditions.  */
> Luis>  +  int condition_changed;
> Luis>  +
> Luis>  +  /* Signals that breakpoint conditions need to be re-synched with the
> Luis>  +     target.  This has no use other than target-side breakpoints.  */
> Luis>  +  int needs_update;
>
> I still wish we had 'bool'.  Maybe this year.
>
> pahole says there are holes in bp_location on my x86-64 box.  I think it
> is somewhat preferable to fill the holes and to follow existing
> practice, that is, use 'char' for boolean fields here.
>
> My preference would be to use 'unsigned int : 1' for boolean fields, but
> I don't know how others feel about this.

I'm fine with either of those. I'll switch to char meanwhile, but will 
use bitfields if OK.

Thanks!

I'll send an updated patch soon, cc-ing everyone.

Luis
lgustavo@codesourcery.com

Re: [RFC stub-side break conditions 3/5] GDB-side changes

[Pedro Alves]

On 01/10/2012 10:39 AM, Luis Machado wrote:
> On 01/06/2012 02:23 PM, Pedro Alves wrote:
>> On 01/05/2012 02:56 PM, Luis Machado wrote:
>>
>>> Regarding always-inserted mode, if it is "on" we must send condition changes right away, or else the target will potentially miss breakpoint hits.
>>
>> Could you elaborate a bit more on how will it miss breakpoint hits?
> 
> That is a bit misleading. Not actually the target, but GDB may not be notified of breakpoint hits that it should've seen.

Ah, that makes more sense.

> For example. Suppose we have a conditional breakpoint location inserted at address foo. The inferior is running and stopping at the breakpoint correctly when its condition is true.
> 
> Suppose we insert a new unconditional location at foo. If we don't synch the breakpoint conditions of both those locations with the target, the target will keep ignoring any breakpoint hits at foo for which the condition of the first location is false.  Of course we may miss a few breakpoint hits during the condition synchronization operation, but GDB will eventually synch the conditions with the target instead of waiting for the next appropriate time to do so.

Right.

> Another example is a set of 2 duplicate conditional breakpoint locations inserted at address foo. If we make one of them unconditional while the process is running (always-inserted mode on), the conditions need to be synched with the target as well. If we don't, GDB may miss some breakpoint triggers (since the target won't report every trigger, just the conditional ones).
> 
>>
>> > I'd like to hear about the change to "info break"
>>
>> As they say, a picture is worth a thousand words.  How does the new output look like?  What about MI?
> 
> An example:
> 
> 2       breakpoint      keep y   0x080483dd in main at loop.c:12
>     stop only if i==5 (stub)
> 
> The change in the output is just a (stub) right after the conditional expression.
> 
> This is being printed together with the conditional expression, so frontends will possibly pick that output up without requiring any additional changes.

Might that be confused with a function call, like?

     stop only if i==foo (stub)

What does the output look like in MI ?  The new MI field should be documented in the manual.

> 
>>
>> > and also the way we should pass conditions down to the stub.  Currently i'm adding the agent expressions to a condition list in the target info field of bp locations.
>>
>> Sounds good at first sight.
>>
>>
>> I'm mostly wondering whether the assumption that we can re-insert
>> a breakpoint at the same address is solid.
> 
> Z packets should, in theory, be implemented in an idempotent way according to the documentation. So adding the same breakpoint location multiple times shouldn't be a problem.

Indeed they are.  Good then.

> I thought about potential issues with single-stepping breakpoints, but they will be inserted with no conditions (making the target remove any conditions that might be active on its end), thus i expect them to work.
> 
>> > +/* Iterates through locations with address ADDRESS.  BP_LOCP_TMP points to
>> > +   each object.  BP_LOCP_START points to where the loop should start from.
>> > +   If BP_LOCP_START is a NULL pointer, the macro automatically seeks the
>> > +   appropriate location to start with.  */
>> > +
>> > +#define ALL_BP_LOCATIONS_AT_ADDR(BP_LOCP_TMP, BP_LOCP_START, ADDRESS)    \
>> > +    if (!BP_LOCP_START)                        \
>> > +      BP_LOCP_START = get_first_locp_gte_addr (ADDRESS);        \
>> > +    for (BP_LOCP_TMP = BP_LOCP_START;                \
>> > +         (BP_LOCP_TMP<  bp_location + bp_location_count        \
>> > + &&  (*BP_LOCP_TMP)->address == ADDRESS);            \
>> > +         BP_LOCP_TMP++)
>> > +
>>
>> The address alone is not enough when you consider multi-process.
>> The users of this macro aren't considering that.  Several instances of
>> this problem.
>>
> 
> IIUIC, we may have situations with breakpoint locations at the same address but on different processes.

Yes.

> 
> If a breakpoint spans multiple processes, we must resynch conditions for both processes. If not, then we should only re-synch target conditions for the process that contains such a breakpoint/location.
> 
> Does that make sense?

Yes, though at this level, we think about program and address spaces instead of processes.

>> > @@ -11387,7 +11810,10 @@ delete_breakpoint (struct breakpoint *bp
>> >        itself, since remove_breakpoint looks at location's owner.  It
>> >        might be better design to have location completely
>> >        self-contained, but it's not the case now.  */
>> > -  update_global_location_list (0);
>> > +  if (is_breakpoint (bpt))
>> > +    update_global_location_list (1);
>>
>> The whole point of the update_global_location_list parameter is being able
>> to say "don't insert locations new locations because I'm just deleting a
>> breakpoint".  This was necessary for situations like following an exec, IIRC.
> 
> I'll double check this since the testsuite didn't give me any visible errors.

See http://sourceware.org/ml/gdb-patches/2008-07/msg00026.html .

> 
>>
>> > +  else
>> > +    update_global_location_list (0);
>> >
>>
>>
>> > +/* List of conditions used to pass conditions down to the target.  */
>> > +struct condition_list
>> > +{
>> > +  /* Breakpoint condition.  */
>> > +  struct agent_expr *expr;
>> > +  /* Pointer to the next condition.  */
>> > +  struct condition_list *next;
>> > +};
>>
>> Can this be a VEC of `struct agent_expr *' instead?
> 
> I'm using VEC now.
> 
> Fixed all the other issues.
> 
> I'll send a new version of the patch soon.

Looking forward.

Re: [RFC stub-side break conditions 3/5] GDB-side changes

[Pedro Alves]

On 01/06/2012 10:37 PM, Luis Machado wrote:
> On 01/06/2012 06:44 AM, Eli Zaretskii wrote:

>>> +      /* Print whether the remote stub is doing the breakpoint's condition
>>> +     evaluation.  If GDB is doing the evaluation, don't print anything.  */
>>> +      if (loc&&  is_breakpoint (b)&&  loc->cond_bytecode
>>> +    &&  breakpoint_condition_evaluation_mode ()
>>> +      != condition_evaluation_gdb)
>>> +    {
>>> +      ui_out_text (uiout, " (");
>>> +      ui_out_field_string (uiout, "condeval",
>>> +                   breakpoint_condition_evaluation_mode ());
>> I suggest "cond.eval." instead of "condeval".  Better yet, how about
>> "evaluated by"?
> Sounds good. I'll make that change.

Isn't that only visible by MI?  Are spaces valid in MI field names?

Re: [RFC stub-side break conditions 3/5] GDB-side changes

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>>> I suggest "cond.eval." instead of "condeval".  Better yet, how about
>>> "evaluated by"?

>> Sounds good. I'll make that change.

Pedro> Isn't that only visible by MI?  Are spaces valid in MI field names?

Yes, it is only visible to MI.

The MI spec does not rule out spaces AFAICT, but I would rather we stick
to names similar to those we already use.  On this basis, "condeval" is
better than "cond.eval.".

Tom

Re: [RFC stub-side break conditions 3/5] GDB-side changes

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Cc: "Gustavo\, Luis" <luis_gustavo@mentor.com>, Eli Zaretskii <eliz@gnu.org>,
>         gdb-patches@sourceware.org
> Date: Thu, 12 Jan 2012 14:41:32 -0700
> 
> >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> >>> I suggest "cond.eval." instead of "condeval".  Better yet, how about
> >>> "evaluated by"?
> 
> >> Sounds good. I'll make that change.
> 
> Pedro> Isn't that only visible by MI?  Are spaces valid in MI field names?
> 
> Yes, it is only visible to MI.
> 
> The MI spec does not rule out spaces AFAICT, but I would rather we stick
> to names similar to those we already use.  On this basis, "condeval" is
> better than "cond.eval.".

How about "evaluated-by"?

Re: [RFC stub-side break conditions 3/5] GDB-side changes

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>> The MI spec does not rule out spaces AFAICT, but I would rather we stick
>> to names similar to those we already use.  On this basis, "condeval" is
>> better than "cond.eval.".

Eli> How about "evaluated-by"?

Works for me.

Tom

Re: [RFC stub-side break conditions 3/5] GDB-side changes

[Pedro Alves]

On 01/13/2012 02:29 PM, Tom Tromey wrote:
>>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> 
>>> The MI spec does not rule out spaces AFAICT, but I would rather we stick
>>> to names similar to those we already use.  On this basis, "condeval" is
>>> better than "cond.eval.".
> 
> Eli> How about "evaluated-by"?
> 
> Works for me.

FAOD, work for me too.

-- 
Pedro Alves

Re: [RFC stub-side break conditions 3/5] GDB-side changes

[Yao Qi]

On 01/10/2012 07:12 PM, Luis Machado wrote:
>> I think the patches should include some tests for the new feature.
> 
> Yes. I still need to think about those.
> 
> I'll probably add a few GDB-side tests for safety, but mostly
> target-side ones like the tracepoint tests.

This patch is about adding evaluating breakpoints' conditions in target
side, so existing test cases for condition breakpoints should be
sufficient for testing.  We only need to force gdb to evaluate condition
in target side somewhere in test suite scripts.  User can define
target_info in his/her board file to set where to evaluate condition,
host or target.

If we add new test cases, they should be used for both cases (condition
evaluated on host side vs. target side).

-- 
Yao (齐尧)

Re: [RFC stub-side break conditions 3/5] GDB-side changes

[Pedro Alves]

On 01/13/2012 02:43 PM, Yao Qi wrote:
> On 01/10/2012 07:12 PM, Luis Machado wrote:
>>> I think the patches should include some tests for the new feature.
>>
>> Yes. I still need to think about those.
>>
>> I'll probably add a few GDB-side tests for safety, but mostly
>> target-side ones like the tracepoint tests.
> 
> This patch is about adding evaluating breakpoints' conditions in target
> side, so existing test cases for condition breakpoints should be
> sufficient for testing.  We only need to force gdb to evaluate condition
> in target side somewhere in test suite scripts.  User can define
> target_info in his/her board file to set where to evaluate condition,
> host or target.

You can just run the testsuite with GDBFLAGS set to "-ex \"set condition-evaluation target\"".

> If we add new test cases, they should be used for both cases (condition
> evaluated on host side vs. target side).

Re: [RFC stub-side break conditions 3/5] GDB-side changes

[Yao Qi]

On 01/14/2012 01:23 AM, Pedro Alves wrote:
>> > This patch is about adding evaluating breakpoints' conditions in target
>> > side, so existing test cases for condition breakpoints should be
>> > sufficient for testing.  We only need to force gdb to evaluate condition
>> > in target side somewhere in test suite scripts.  User can define
>> > target_info in his/her board file to set where to evaluate condition,
>> > host or target.
> You can just run the testsuite with GDBFLAGS set to "-ex \"set condition-evaluation target\"".
> 

Yes, that works.  Thanks for this tip.

-- 
Yao (齐尧)