On 02/23/2012 03:24 PM, Pedro Alves wrote:
> On 02/22/2012 03:24 PM, Luis Gustavo wrote:
>> On 02/09/2012 10:56 AM, Pedro Alves wrote:
>>> On 02/08/2012 08:56 PM, Luis Gustavo wrote:
>>>
>>>>> I'm confused.  How does this (and all similar places) address the issued I
>>>>> pointed out before?  If you're passing one to update_global_location_list
>>>>> when deleting a breakpoint, you're pretty much defeating the whole
>>>>> purpose of the update_global_location_list's argument in the first place.
>>>>>
>>>>
>>>> Updates will only take place when the user explicitly removes/disables a breakpoint. Functions that are deleting breakpoints (like remove_thread_event_breakpoints) won't cause insertion of any breakpoints since they go through the "delete_breakpoint" path, not the delete_breakpoint_with_update one.
>>>
>>> Ah, got it now.  Thanks.  But please:
>>>
>>>> +   structures.  If UPDATE is true, proceed to update the list of
>>>> +   locations, otherwise don't update it.  */
>>>>
>>>> -void
>>>> -delete_breakpoint (struct breakpoint *bpt)
>>>> +static void
>>>> +delete_breakpoint_1 (struct breakpoint *bpt, int update)
>>>
>>> "update" here is quite confusing, because that's not what is happening.  Or at
>>> least, update is so overloaded that I'm not understanding it the way you are.
>>> The list of locations is still updated/regenerated, and we do delete locations
>>> from the target, but, we don't allow new insertions.  So, could we
>>> s/update/somethingelse/ please?  Even pointing at update_global_location_list's
>>> description of its parameter would be good.
>>>
>>> Or maybe, I just had an idea --- I wonder if we made update_global_location_list
>>> still re-insert _only_ the _already inserted_ locations when the
>>> condition changes, then we'd be good.  That is, something like:
>>>
>>> update_global_location_list:
>>> ...
>>> -  if (breakpoints_always_inserted_mode ()&&   should_insert
>>> -&&   (have_live_inferiors ()
>>> -      || (gdbarch_has_global_breakpoints (target_gdbarch))))
>>> -    insert_breakpoint_locations ();
>>> +  if (breakpoints_always_inserted_mode ()
>>> +&&   (have_live_inferiors ()
>>> +      || (gdbarch_has_global_breakpoints (target_gdbarch))))
>>> +      {
>>> +     if (should_insert)
>>> +          insert_breakpoint_locations ();
>>> +        else
>>> +          update_inserted_breakpoint_locations ();
>>> +      }
>>>
>>> The problem is that a delete_breakpoint would trigger insertions of all
>>> _other_ breakpoints.  But if we're allow "reinserting" breakpoints that
>>> are _already_ inserted, I think we're fine.
>>
>> I implemented this change. update_inserted_breakpoint_locations is a simpler version of insert_breakpoint_locations. Its purpose is to synch conditions of already-inserted breakpoint locations with the target.
>>
>>>
>>>> Is disabling breakpoints also something we would like to do without triggering insertions? If so, i'm inclined to go with the same solution as for deleting user breakpoints.
>>>
>>> Maybe, not sure.  Better be safe, I think.
>>>
>>> It'd be nice to come up with a better way to solve the initial problem that
>>> led to update_global_location_list having an argument in the first place.  :-/
>>
>> I've confirmed this works with both delete and disable. Looks like a good solution for now.
>>
>
> Yes, looks much better.  Thanks.  The patch looks mostly good to me now.  Only a
> few comments below.
>
>> The breakpoint infrastructure could use a little cleaning and refactoring i suppose. :-(
>
> What doesn't? :-)
>
>>
>> Also, i changed remote.c to not pass the "conditions" marker anymore.
>>
>> Luis
>>
>> 0002-break_condition_bytecode.diff
>>
>>
>> 2012-02-22  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_evaluation_of_breakpoint_conditions
>> 	hook.
>>
>> 	* target.c (update_current_target): Inherit
>> 	to_supports_evaluation_of_breakpoint_conditions.
>> 	Default to_supports_evaluation_of_breakpoint_conditions to return_zero.
>>
>> 	* target.h (struct target_ops)
>> 	<to_supports_evaluation_of_breakpoint_conditions>: New field.
>> 	(target_supports_evaluation_of_breakpoint_conditions): New #define.
>>
>> 	* breakpoint.c (get_first_locp_gte_addr): New forward declaration.
>> 	(condition_evaluation_both, condition_evaluation_auto,
>> 	condition_evaluation_host, condition_evaluation_target,
>> 	condition_evaluation_enums, condition_evaluation_mode_1,
>> 	condition_evaluation_mode): New	static globals.
>> 	(translate_condition_evaluation_mode): New function.
>> 	(breakpoint_condition_evaluation_mode): New function.
>> 	(gdb_evaluates_breakpoint_condition_p): New function.
>> 	(ALL_BP_LOCATIONS_AT_ADDR): New helper macro.
>> 	(mark_breakpoint_modified): New function.
>> 	(mark_breakpoint_location_modified): New function.
>> 	(set_condition_evaluation_mode): New function.
>> 	(show_condition_evaluation_mode): 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 (...).
>> 	(update_inserted_breakpoint_locations): New function.
>> 	(insert_breakpoint_locations): Handle target-side conditional
>> 	breakpoints.
>> 	(bpstat_check_breakpoint_conditions): Add comment.
>> 	(bp_condition_evaluator): New function.
>> 	(bp_location_condition_evaluator): New function.
>> 	(print_breakpoint_location): Print information on where the condition
>> 	will be evaluated.
>> 	(print_one_breakpoint_location): Likewise.
>> 	(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.
>> 	Reinsert locations that are already inserted if conditions have
>> 	changed.
>> 	(bp_location_dtor): Free agent expression bytecode.
>> 	(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.
>> 	(condition_status): New enum.
>> 	(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-02-22 13:22:16.930553985 -0200
>> +++ gdb/gdb/remote.c	2012-02-22 13:22:49.174553987 -0200
>> @@ -242,6 +242,8 @@ static int remote_read_description_p (st
>>
>>   static void remote_console_output (char *msg);
>>
>> +static 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.  */
>>
>> @@ -7729,6 +7731,43 @@ extended_remote_create_inferior (struct
>>   }
>>
>>
>> +/* Given a location's target info BP_TGT and the packet buffer BUF,  output
>> +   the list of conditions (in agent expression bytecode format), if any, the
>> +   target needs to evaluate.  The output is placed into the packet buffer
>> +   BUF.  */
>> +
>> +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, ix;
>> +  char *pkt;
>> +  char *buf_start = buf;
>> +
>> +  if (VEC_length (agent_expr_p, bp_tgt->conditions))
>
> if (!VEC_empty...)
>
> if better:
>
>
>    if (VEC_empty...)
>      return 0;
>
>    sprintf (buf + strlen (buf), "%s", ";");
>    ...
>
>> +    {
>> +      sprintf (buf + strlen (buf), "%s", ";");
>> +    }
>> +  else
>> +    return 0;
>> +
>
>    and all these repeated strlen calls are unnecessary:
>
>    buf += strlen (buf);
>    sprintf (buf, "%s", ";");
>
>
>> +  /* Send conditions to the target and free the vector.  */
>> +  for (ix = 0;
>> +       VEC_iterate (agent_expr_p, bp_tgt->conditions, ix, aexpr);
>> +       ix++)
>> +    {
>> +      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';
>
>        sprintf (buf, "X%x,", aexpr->len);
>        buf += strlen (buf);
>        for (i = 0; i<  aexpr->len; ++i)
> 	buf = pack_hex_byte (buf, aexpr->buf[i]);
>        *buf = '\0';
>
>
>> +    }
>> +
>> +  VEC_free (agent_expr_p, bp_tgt->conditions);
>> +  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.  */
>> @@ -7748,6 +7787,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);
>>
>> @@ -7761,6 +7801,9 @@ remote_insert_breakpoint (struct gdbarch
>>         p += hexnumstr (p, addr);
>>         sprintf (p, ",%d", bpsize);
>>
>> +      if (remote_supports_cond_breakpoints ())
>> +	remote_add_target_side_condition (gdbarch, bp_tgt, p);
>> +
>>         putpkt (rs->buf);
>>         getpkt (&rs->buf,&rs->buf_size, 0);
>>
>> @@ -7986,6 +8029,9 @@ remote_insert_hw_breakpoint (struct gdba
>>     p += hexnumstr (p, (ULONGEST) addr);
>>     sprintf (p, ",%x", bp_tgt->placed_size);
>>
>> +  if (remote_supports_cond_breakpoints ())
>> +    remote_add_target_side_condition (gdbarch, bp_tgt, p);
>> +
>>     putpkt (rs->buf);
>>     getpkt (&rs->buf,&rs->buf_size, 0);
>>
>> @@ -10781,6 +10827,7 @@ Specify the serial device it is connecte
>>     remote_ops.to_fileio_readlink = remote_hostio_readlink;
>>     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_evaluation_of_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-02-22 13:22:16.946553985 -0200
>> +++ gdb/gdb/target.c	2012-02-22 13:22:49.174553987 -0200
>> @@ -699,6 +699,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_evaluation_of_breakpoint_conditions, t);
>>         /* Do not inherit to_memory_map.  */
>>         /* Do not inherit to_flash_erase.  */
>>         /* Do not inherit to_flash_done.  */
>> @@ -925,6 +926,9 @@ update_current_target (void)
>>     de_fault (to_traceframe_info,
>>   	    (struct traceframe_info * (*) (void))
>>   	    tcomplain);
>> +  de_fault (to_supports_evaluation_of_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-02-22 13:22:16.894553984 -0200
>> +++ gdb/gdb/target.h	2012-02-22 13:22:49.178553986 -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?  */
>
> either "evaluation of breakpoint conditions", or "evaluating breakpoint conditions"?
>
>
>> +    int (*to_supports_evaluation_of_breakpoint_conditions) (void);
>> +
>>       /* Determine current architecture of thread PTID.
>>
>>          The target is supposed to determine the architecture of the code where
>> @@ -968,6 +971,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_evaluation_of_breakpoint_conditions() \
>> +  (*current_target.to_supports_evaluation_of_breakpoint_conditions) ()
>> +
>>   /* Invalidate all target dcaches.  */
>>   extern void target_dcache_invalidate (void);
>>
>> Index: gdb/gdb/breakpoint.c
>> ===================================================================
>> --- gdb.orig/gdb/breakpoint.c	2012-02-22 13:22:16.882553985 -0200
>> +++ gdb/gdb/breakpoint.c	2012-02-22 13:22:58.038553985 -0200
>> @@ -66,6 +66,7 @@
>>   #include "skip.h"
>>   #include "record.h"
>>   #include "gdb_regex.h"
>> +#include "ax-gdb.h"
>>
>>   /* readline include files */
>>   #include "readline/readline.h"
>> @@ -258,6 +259,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);
>> +
>>   /* Return 1 if B refers to a static tracepoint set by marker ("-m"), zero
>>      otherwise.  */
>>
>> @@ -406,6 +409,64 @@ breakpoints_always_inserted_mode (void)
>>   	&&  !RECORD_IS_USED);
>>   }
>>
>> +static const char condition_evaluation_both[] = "host or target";
>> +
>> +/* Modes for breakpoint condition evaluation.  */
>> +static const char condition_evaluation_auto[] = "auto";
>> +static const char condition_evaluation_host[] = "host";
>> +static const char condition_evaluation_target[] = "target";
>> +static const char *const condition_evaluation_enums[] = {
>> +  condition_evaluation_auto,
>> +  condition_evaluation_host,
>> +  condition_evaluation_target,
>> +  NULL
>> +};
>> +
>> +/* Global that holds the current mode for breakpoint condition evaluation.  */
>> +static const char *condition_evaluation_mode_1 = condition_evaluation_auto;
>> +
>> +/* Global that we use to display information to the user (gets its value from
>> +   condition_evaluation_mode_1.  */
>> +static const char *condition_evaluation_mode = condition_evaluation_auto;
>> +
>> +/* Translate a condition evaluation mode MODE into either "gdb"
>
> It's "host" now, no longer "gdb".
>
>> +   or "target".  This is used mostly to translate from "auto" to the
>> +   real setting that is being used.  It returns the translated
>> +   evaluation mode.  */
>> +
>> +static const char *
>> +translate_condition_evaluation_mode (const char *mode)
>> +{
>> +  if (mode == condition_evaluation_auto)
>> +    {
>> +      if (target_supports_evaluation_of_breakpoint_conditions ())
>> +	return condition_evaluation_target;
>> +      else
>> +	return condition_evaluation_host;
>> +    }
>> +  else
>> +    return mode;
>> +}
>> +
>> +/* Discovers what condition_evaluation_auto translates to.  */
>> +
>> +static const char *
>> +breakpoint_condition_evaluation_mode (void)
>> +{
>> +  return translate_condition_evaluation_mode (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_host);
>> +}
>> +
>>   void _initialize_breakpoint (void);
>>
>>   /* Are we executing breakpoint commands?  */
>> @@ -437,6 +498,19 @@ int target_exact_watchpoints = 0;
>>   	     BP_TMP<  bp_location + bp_location_count&&  (B = *BP_TMP);	\
>>   	     BP_TMP++)
>>
>> +/* Iterates through locations with address ADDRESS for the currently selected
>> +   program space.  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)	\
>> +	for (BP_LOCP_START = BP_LOCP_START == NULL ? get_first_locp_gte_addr (ADDRESS) : BP_LOCP_START, \
>> +	     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)  \
>> @@ -620,6 +694,144 @@ get_breakpoint (int num)
>>
>>
>>
>> +/* 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_evaluation_of_breakpoint_conditions ())
>> +    return;
>> +
>> +  if (!is_breakpoint (b))
>> +    return;
>> +
>> +  for (loc = b->loc; loc; loc = loc->next)
>> +    loc->condition_changed = condition_modified;
>> +}
>> +
>> +/* 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_evaluation_of_breakpoint_conditions ())
>> +
>> +    return;
>> +
>> +  if (!is_breakpoint (loc->owner))
>> +    return;
>> +
>> +  loc->condition_changed = condition_modified;
>> +}
>> +
>> +/* Sets the condition-evaluation mode using the static global
>> +   condition_evaluation_mode.  */
>> +
>> +static void
>> +set_condition_evaluation_mode (char *args, int from_tty,
>> +			       struct cmd_list_element *c)
>> +{
>> +  struct breakpoint *b;
>> +  const char *old_mode, *new_mode;
>> +
>> +  if ((condition_evaluation_mode_1 == condition_evaluation_target)
>> +&&  !target_supports_evaluation_of_breakpoint_conditions ())
>> +    {
>> +      condition_evaluation_mode_1 = condition_evaluation_mode;
>> +      warning (_("Target does not support breakpoint condition evaluation.\n"
>> +		 "Using GDB evaluation mode instead."));
>
> s/GDB/host/ ?
>
>> +      return;
>> +    }
>> +
>> +  new_mode = translate_condition_evaluation_mode (condition_evaluation_mode_1);
>> +  old_mode = translate_condition_evaluation_mode (condition_evaluation_mode);
>> +
>> +  /* Only update the mode if the user picked a different one.  */
>> +  if (new_mode != old_mode)
>> +    {
>> +      struct bp_location *loc, **loc_tmp;
>> +      /* If the user switched to a different evaluation mode, we
>> +	 need to synch the changes with the target as follows:
>> +
>> +	 "gdb" ->  "target": Send all (valid) conditions to the target.
>> +	 "target" ->  "gdb": Remove all the conditions from the target.
>
> s/gdb/host/ ?
>
>> +      */
>> +
>> +      /* Flip the switch.  */
>> +      condition_evaluation_mode = condition_evaluation_mode_1;
>> +
>> +      if (new_mode == condition_evaluation_target)
>> +	{
>> +	  /* Mark everything modified and to synch conditions with the
>> +	     target.  */
>> +	  ALL_BP_LOCATIONS (loc, loc_tmp)
>> +	    mark_breakpoint_location_modified (loc);
>> +  	}
>> +      else
>> +	{
>> +	  /* Manually mark non-duplicate locations to synch conditions
>> +	     with the target.  We do this to remove all the conditions the
>> +	     target knows about.  */
>> +	  ALL_BP_LOCATIONS (loc, loc_tmp)
>> +	    if (is_breakpoint (loc->owner)&&  loc->inserted)
>> +	      loc->needs_update = 1;
>> +	}
>> +
>> +      /* Do the update.  */
>> +      update_global_location_list (1);
>> +    }
>> +
>> +  return;
>> +}
>> +
>> +/* 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);
>> +}
>> +
>> +/* 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)
>> @@ -642,6 +854,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.  */
>>   	}
>>       }
>>
>> @@ -684,6 +900,8 @@ set_breakpoint_condition (struct breakpo
>>   	    }
>>   	}
>>       }
>> +  mark_breakpoint_modified (b);
>> +
>>     breakpoints_changed ();
>>     observer_notify_breakpoint_modified (b);
>>   }
>> @@ -717,6 +935,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;
>>         }
>>
>> @@ -1216,6 +1438,15 @@ breakpoint_xfer_memory (gdb_byte *readbu
>>   }
>>
>>
>> +/* Return true if BPT is either a software breakpoint or a hardware
>> +   breakpoint.  */
>> +int
>
> Space between comment and function, please.
>
>> +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
>> @@ -1658,6 +1889,143 @@ 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 parsed 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 void
>> +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_evaluation_of_breakpoint_conditions ())
>> +    return;
>> +
>> +  /* 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)&&  loc->pspace == bl->pspace)
>> +	{
>> +	  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)&&  loc->pspace == bl->pspace)
>> +	    {
>> +	      /* Only go as far as the first NULL bytecode is
>> +		 located.  */
>> +	      if (!loc->cond_bytecode)
>> +		return;
>> +
>> +	      free_agent_expr (loc->cond_bytecode);
>> +	      loc->cond_bytecode = NULL;
>> +	    }
>> +	}
>> +    }
>> +
>> +  /* 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->pspace == bl->pspace
>> +	&&  loc->owner->enable_state == bp_enabled
>> +	&&  loc->enabled)
>> +	/* Add the condition to the vector.  This will be used later to send the
>> +	   conditions to the target.  */
>> +	VEC_safe_push (agent_expr_p, bl->target_info.conditions,
>> +		       loc->cond_bytecode);
>> +    }
>> +
>> +  return;
>> +}
>> +
>>   /* 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.
>> @@ -1674,7 +2042,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.  */
>> @@ -1683,6 +2051,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)
>>       {
>> @@ -1991,6 +2371,66 @@ insert_breakpoints (void)
>>       insert_breakpoint_locations ();
>>   }
>>
>> +/* This is used when we need to synch breakpoint conditions between GDB and the
>> +   target.  It is the case with deleting and disabling of breakpoints when using
>> +   always-inserted mode.  */
>> +
>> +static void
>> +update_inserted_breakpoint_locations (void)
>> +{
>> +  struct bp_location *bl, **blp_tmp;
>> +  int error_flag = 0;
>> +  int val = 0;
>> +  int disabled_breaks = 0;
>> +  int hw_breakpoint_error = 0;
>> +
>> +  struct ui_file *tmp_error_stream = mem_fileopen ();
>> +  struct cleanup *cleanups = make_cleanup_ui_file_delete (tmp_error_stream);
>> +
>> +  /* Explicitly mark the warning -- this will only be printed if
>> +     there was an error.  */
>> +  fprintf_unfiltered (tmp_error_stream, "Warning:\n");
>> +
>> +  save_current_space_and_thread ();
>> +
>> +  ALL_BP_LOCATIONS (bl, blp_tmp)
>> +    {
>> +      /* We only want to update software breakpoints and hardware
>> +	 breakpoints.  */
>> +      if (!is_breakpoint (bl->owner))
>> +	continue;
>> +
>> +      /* We only want to update locations that are already inserted
>> +	 and need updating.  This is to avoid unwanted insertion during
>> +	 deletion of breakpoints.  */
>> +      if (!bl->inserted || (bl->inserted&&  !bl->needs_update))
>> +	continue;
>> +
>> +      switch_to_program_space_and_thread (bl->pspace);
>> +
>> +      /* For targets that support global breakpoints, there's no need
>> +	 to select an inferior to insert breakpoint to.  In fact, even
>> +	 if we aren't attached to any process yet, we should still
>> +	 insert breakpoints.  */
>> +      if (!gdbarch_has_global_breakpoints (target_gdbarch)
>> +	&&  ptid_equal (inferior_ptid, null_ptid))
>> +	continue;
>> +
>> +      val = insert_bp_location (bl, tmp_error_stream,&disabled_breaks,
>> +				&hw_breakpoint_error);
>> +      if (val)
>> +	error_flag = val;
>> +    }
>> +
>> +  if (error_flag)
>> +    {
>> +      target_terminal_ours_for_output ();
>> +      error_stream (tmp_error_stream);
>> +    }
>> +
>> +  do_cleanups (cleanups);
>> +}
>> +
>>   /* Used when starting or continuing the program.  */
>>
>>   static void
>> @@ -2014,7 +2454,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
>> @@ -4092,6 +4532,10 @@ 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 breakpoint.  */
>> +
>>     if (frame_id_p (b->frame_id)
>>         &&  !frame_id_eq (b->frame_id, get_stack_frame_id (get_current_frame ())))
>>       bs->stop = 0;
>> @@ -4669,6 +5113,66 @@ wrap_indent_at_field (struct ui_out *uio
>>     return NULL;
>>   }
>>
>> +/* Determine if the locations of this breakpoint will have their conditions
>> +   evaluated by the target, host or a mix of both.  Returns the following:
>> +
>> +    "host": Host evals condition.
>> +    "host or target": Host or Target evals condition.
>> +    "target": Target evals condition.
>> +*/
>> +
>> +static const char *
>> +bp_condition_evaluator (struct breakpoint *b)
>> +{
>> +  struct bp_location *bl;
>> +  char host_evals = 0;
>> +  char target_evals = 0;
>> +
>> +  if (!b)
>> +    return NULL;
>> +
>> +  if (!is_breakpoint (b))
>> +    return NULL;
>> +
>> +  if (gdb_evaluates_breakpoint_condition_p ()
>> +      || !target_supports_evaluation_of_breakpoint_conditions ())
>> +    return condition_evaluation_host;
>> +
>> +  for (bl = b->loc; bl; bl = bl->next)
>> +    {
>> +      if (bl->cond_bytecode)
>> +	target_evals++;
>> +      else
>> +	host_evals++;
>> +    }
>> +
>> +  if (host_evals&&  target_evals)
>> +    return condition_evaluation_both;
>> +  else if (target_evals)
>> +    return condition_evaluation_target;
>> +  else
>> +    return condition_evaluation_host;
>> +}
>> +
>> +/* Determine the breakpoint location's condition evaluator.  This is
>> +   similar to bp_condition_evaluator, but for locations.  */
>> +
>> +static const char *
>> +bp_location_condition_evaluator (struct bp_location *bl)
>> +{
>> +  if (bl&&  !is_breakpoint (bl->owner))
>> +    return NULL;
>> +
>> +  if (gdb_evaluates_breakpoint_condition_p ()
>> +      || !target_supports_evaluation_of_breakpoint_conditions ())
>> +    return condition_evaluation_host;
>> +
>> +  if (bl&&  bl->cond_bytecode)
>> +    return condition_evaluation_target;
>> +  else
>> +    return condition_evaluation_host;
>> +}
>> +
>>   /* Print the LOC location out of the list of B->LOC locations.  */
>>
>>   static void
>> @@ -4727,6 +5231,16 @@ print_breakpoint_location (struct breakp
>>     else
>>       ui_out_field_string (uiout, "pending", b->addr_string);
>>
>> +  if (loc&&  is_breakpoint (b)
>> +&&  breakpoint_condition_evaluation_mode () == condition_evaluation_target
>> +&&  bp_condition_evaluator (b) == condition_evaluation_both)
>> +    {
>> +      ui_out_text (uiout, " (");
>> +      ui_out_field_string (uiout, "evaluated-by",
>> +			   bp_location_condition_evaluator (loc));
>> +      ui_out_text (uiout, ")");
>> +    }
>> +
>>     do_cleanups (old_chain);
>>   }
>>
>> @@ -5002,6 +5516,18 @@ 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 target is doing the breakpoint's condition
>> +	 evaluation.  If GDB is doing the evaluation, don't print anything.  */
>> +      if (is_breakpoint (b)
>> +	&&  breakpoint_condition_evaluation_mode ()
>> +	  == condition_evaluation_target)
>> +	{
>> +	  ui_out_text (uiout, " (");
>> +	  ui_out_field_string (uiout, "evaluated-by",
>> +			       bp_condition_evaluator (b));
>> +	  ui_out_text (uiout, " evals)");
>> +	}
>>         ui_out_text (uiout, "\n");
>>       }
>>
>> @@ -5731,6 +6257,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;
>>
>> @@ -5758,9 +6285,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:
>> @@ -10717,6 +11246,7 @@ swap_insertion (struct bp_location *left
>>   {
>>     const int left_inserted = left->inserted;
>>     const int left_duplicate = left->duplicate;
>> +  const int left_needs_update = left->needs_update;
>>     const struct bp_target_info left_target_info = left->target_info;
>>
>>     /* Locations of tracepoints can never be duplicated.  */
>> @@ -10727,12 +11257,67 @@ 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;
>> +  right->needs_update = left_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 (struct bp_location *bl)
>> +{
>> +  struct bp_location **locp = NULL, **loc2p;
>> +  struct bp_location *loc;
>> +  CORE_ADDR address = 0;
>> +  struct program_space *pspace = NULL;
>> +
>> +  address = bl->address;
>> +  pspace = bl->pspace;
>> +
>> +  /* 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_evaluation_of_breakpoint_conditions ())
>> +    return;
>> +
>> +  /* Flag all breakpoint locations with this address and
>> +     the same program space as the location
>> +     as "its condition has changed".  We need to
>> +     update the conditions on the target's side.  */
>> +  ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, address)
>
> It's unfortunate that we're introducing these extra iterations over
> all locations.  I think we can easily get rid of them by using
> bsearch'ing for the initial location at address (and then rewinding
> a bit for the first location that matches).  I'm not making it
> a requirement for acceptance, but I think it may be a good idea.
>

Yes, sounds like a good optimization. What do you think of the following 
patch?

>> +    {
>> +      loc = *loc2p;
>> +
>> +      if (!is_breakpoint (loc->owner)
>> +	  || pspace != loc->pspace)
>> +	continue;
>> +
>> +      /* Flag the location appropriately.  We use a different number to
>
> number?  I think this was referring to before you had enums, right?
> s/number/state/ would work, I think.
>
>> +	 let everyone know that we already updated the set of locations
>> +	 with addr bl->address and program space bl->pspace.  This is so
>> +	 we don't have to keep calling this functions just to mark locations
>> +	 that have already been marked.  */
>> +      loc->condition_changed = condition_updated;
>> +
>> +      /* 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
>> @@ -10754,6 +11339,10 @@ 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;
>> +  /* Last breakpoint location program space that was marked for updated.  */
>
> "for update".
>
>> +  struct program_space *last_pspace = NULL;
>>
>>     /* Used in the duplicates detection below.  When iterating over all
>>        bp_locations, points to the first bp_location of a given address.
>> @@ -10826,13 +11415,27 @@ update_global_location_list (int should_
>>   	&&  (*loc2p)->address == old_loc->address);
>>   	   loc2p++)
>>   	{
>> +	  /* Check if this is a new/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 ((*loc2p)->condition_changed == condition_modified
>> +	&&  last_addr != old_loc->address)
>> +	    force_breakpoint_reinsertion ((*loc2p));
>
> Unnecessary double parens '(('.
>
> I think we need to check pspaces as well, in case the addresses are
> the same, but the pspaces aren't.
>

Fixed this. We're checking pspace numbers now.

>> +
>>   	  if (*loc2p == old_loc)
>> -	    {
>> -	      found_object = 1;
>> -	      break;
>> -	    }
>> +	    found_object = 1;
>>   	}
>>
>> +      /* We have already handled this address, update it so that we don't
>> +	 have to go through updates again.  */
>> +      last_addr = old_loc->address;
>> +
>> +      /* Target-side condition evaluation: Handle deleted locations.  */
>> +      if (!found_object)
>> +	force_breakpoint_reinsertion (old_loc);
>> +
>>         /* 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
>> @@ -10852,6 +11455,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
>> @@ -11004,7 +11611,11 @@ update_global_location_list (int should_
>>   	   never duplicated.  See the comments in field `duplicate' of
>>   	   `struct bp_location'.  */
>>   	  || is_tracepoint (b))
>> -	continue;
>> +	{
>> +	  /* Clear the condition modification flag.  */
>> +	  loc->condition_changed = condition_unchanged;
>> +	  continue;
>> +	}
>>
>>         /* Permanent breakpoint should always be inserted.  */
>>         if (b->enable_state == bp_permanent&&  ! loc->inserted)
>> @@ -11027,6 +11638,13 @@ 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;
>> +	      /* Clear the condition modification flag.  */
>> +	      loc->condition_changed = condition_unchanged;
>> +	    }
>>   	  continue;
>>   	}
>>
>> @@ -11038,6 +11656,9 @@ update_global_location_list (int should_
>>   	swap_insertion (loc, *loc_first_p);
>>         loc->duplicate = 1;
>>
>> +      /* Clear the condition modification flag.  */
>> +      loc->condition_changed = condition_unchanged;
>> +
>>         if ((*loc_first_p)->owner->enable_state == bp_permanent&&  loc->inserted
>>   	&&  b->enable_state != bp_permanent)
>>   	internal_error (__FILE__, __LINE__,
>> @@ -11045,10 +11666,21 @@ update_global_location_list (int should_
>>   			"a permanent breakpoint"));
>>       }
>>
>> -  if (breakpoints_always_inserted_mode ()&&  should_insert
>> +  if (breakpoints_always_inserted_mode ()
>>         &&  (have_live_inferiors ()
>>   	  || (gdbarch_has_global_breakpoints (target_gdbarch))))
>> -    insert_breakpoint_locations ();
>> +    {
>> +      if (should_insert)
>> +	insert_breakpoint_locations ();
>> +      else
>> +	{
>> +	  /* Though should_insert is false, we may need to update conditions
>> +	     on the target's side if it is evaluating such conditions.  We
>> +	     only update conditions for locations that are marked
>> +	     "needs_update".  */
>> +	  update_inserted_breakpoint_locations ();
>> +	}
>> +    }
>>
>>     if (should_insert)
>>       download_tracepoint_locations ();
>> @@ -11162,6 +11794,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);
>>   }
>> @@ -12855,6 +13489,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))
>>       {
>> @@ -12902,7 +13539,11 @@ 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))
>> @@ -12959,6 +13600,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))
>>       {
>> @@ -13018,7 +13664,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))
>> @@ -14459,8 +15109,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\
>> @@ -14476,8 +15127,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\
>> @@ -14495,8 +15147,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\
>> @@ -14515,8 +15168,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\
>
> It didn't look like it is really true that we have a new "Cond Eval" column?
> Can you show an example of what the new output looks like?
>
>
>>   \n\
>>   Convenience variable \"$_\" and default examine address for \"x\"\n\
>>   are set to the address of the last breakpoint listed unless the command\n\
>> @@ -14793,6 +15447,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_1, _("\
>> +Set mode of breakpoint condition evaluation."), _("\
>> +Show mode of 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 \"target\",\n\
>> +breakpoint conditions will be downloaded to the target (if the target\n\
>> +supports such feature) and conditions will be evaluated on the target's side.\n\
>> +If this is set to \"auto\" (default), this will be automatically set to\n\
>> +\"target\" if it supports condition evaluation, otherwise it will\n\
>> +be set to \"gdb\""),
>> +			&set_condition_evaluation_mode,
>> +			&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-02-22 13:22:16.906553986 -0200
>> +++ gdb/gdb/breakpoint.h	2012-02-22 13:22:49.190553987 -0200
>
>> +/* Returns true if BPT is a breakpoint of any kind.  */
>> +
>> +extern int is_breakpoint (const struct breakpoint *bpt);
>
> I see you've updated the comment on the .c side.  Please update it here as
> well (to be more specific).
>

Fixed all the other comments and addressed a few typos i've found as well.

Thanks,
Luis