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.

The breakpoint infrastructure could use a little cleaning and 
refactoring i suppose. :-(

Also, i changed remote.c to not pass the "conditions" marker anymore.

Luis