Re: [rfc target-side break conditions 3/5 v2] GDB-side changes

[Luis Gustavo <luis_gustavo@mentor.com>]

Fixed all the other comments...

On 02/08/2012 05:33 PM, Pedro Alves wrote:
>>   /* Delete a breakpoint and clean up all traces of it in the data
>> -   structures.  */
>> +   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)
>>   {
>>     struct breakpoint *b;
>>
>> @@ -11955,8 +12451,12 @@ delete_breakpoint (struct breakpoint *bp
>>        belong to this breakpoint.  Do this before freeing the breakpoint
>>        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);
>> +     self-contained, but it's not the case now.
>> +
>> +     If we have conditions being evaluated on the target, we will also
>> +     synchronize the list of conditions due to a deleted duplicate
>> +     breakpoint.  */
>> +  update_global_location_list (update);
>>
>>     bpt->ops->dtor (bpt);
>>     /* On the chance that someone will soon try again to delete this
>> @@ -11965,6 +12465,24 @@ delete_breakpoint (struct breakpoint *bp
>>     xfree (bpt);
>>   }
>>
>> +/* Delete a breakpoint but don't update the list of locations.  */
>> +
>> +void
>> +delete_breakpoint (struct breakpoint *bpt)
>> +{
>> +  delete_breakpoint_1 (bpt, 0);
>> +}
>> +
>> +/* Delete a breakpoint and update the list of locations.  We use
>> +   this one to synchronize the list of conditions with the target
>> +   in case the target is responsible for evaluating them.  */
>> +
>> +void
>> +delete_breakpoint_with_update (struct breakpoint *bpt)
>> +{
>> +  delete_breakpoint_1 (bpt, 1);
>> +}
>> +
>>   static void
>>   do_delete_breakpoint_cleanup (void *b)
>>   {
>> @@ -12017,7 +12535,10 @@ iterate_over_related_breakpoints (struct
>>   static void
>>   do_delete_breakpoint (struct breakpoint *b, void *ignore)
>>   {
>> -  delete_breakpoint (b);
>> +  if (is_breakpoint (b))
>> +    delete_breakpoint_with_update (b);
>> +  else
>> +    delete_breakpoint (b);
>>   }
>>
>>   /* A callback for map_breakpoint_numbers that calls
>> @@ -12056,7 +12577,7 @@ delete_command (char *arg, int from_tty)
>>   	{
>>   	  ALL_BREAKPOINTS_SAFE (b, b_tmp)
>>   	    if (user_breakpoint_p (b))
>> -	      delete_breakpoint (b);
>> +	      do_delete_breakpoint (b, NULL);
>>   	}
>>       }
>>     else
>> @@ -12840,6 +13361,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))
>>       {
>> @@ -12849,7 +13373,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);
>
> 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.

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.