Re: Breakpoint locations, enableness and MI's =breakpoint-modified event

[Simon Marchi <simon dot marchi at polymtl dot ca>]

Hi Jan,

A tip for next times, your mail client has wrapped the lines of your 
patch, making it very difficult to apply.  The easiest and safest way to 
send a patch is to use git-send-email.  Could you give it a try next 
time?

> commit e766ec361a76749f14e63abb6ac588ebed59ad12
> Author: Jan Vrany <jan.vrany@fit.cvut.cz>
> Date:   Wed May 23 10:27:16 2018 +0100
>
>     MI: emit =breakpoint-modified when breakpoint location enableness
> changes
>
>     When a CLI command enabled/disables a breakpoint, MI frontend is
> notified
>     via a =breakpoint-modified event. However, if only a particular
> location
>     is enabled/disabled, this event was not emitted which may cause
> frontend
>     to be out of sync (displaying wrong information to the user and so
> on).
>     This commit fixes this by emmiting =breakpoint-modified event with
> only the
>     location that has changed.

I am not sure it's a good idea to only emit the location that changed.  
When a location to a breakpoint is added or removed, do we simply emit a 
=breakpoint-modified event including all locations post-modification?  
If so, emitting only the location that changed could lead frontends to 
think that all other locations vanished and there is only one location 
remaining.  It would probably be simpler (though a bit less efficient) 
to emit all the details about the breakpoint, and let the frontend 
figure out what changed.

In any case, the patch will need to be accompanied by a ChangeLog entry:

https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_Formatted_GNU_ChangeLog

Some general GNU-style formatting comments:

- Put a space before the parenthesis when declaring/calling functions.
- Wrap to 80 characters.

> @@ -6443,10 +6443,19 @@ breakpoint_address_bits (struct breakpoint *b)
>  /* See breakpoint.h.  */
>
>  void
> -print_breakpoint (breakpoint *b)
> +print_breakpoint (breakpoint *b, bp_location *loc)
>  {
>    struct bp_location *dummy_loc = NULL;
> -  print_one_breakpoint (b, &dummy_loc, 0);
> +  if (!loc)

When comparing pointers, use "loc != NULL" or "loc != nullptr".

> @@ -14225,6 +14234,8 @@ enable_disable_bp_num_loc (int bp_num, int
> loc_num, bool enable)
>  	target_disable_tracepoint (loc);
>      }
>    update_global_location_list (UGLL_DONT_INSERT);
> +
> +  gdb::observers::breakpoint_modified.notify (loc->owner, loc);

Do we need to send an event if loc == NULL here?


> diff --git a/gdb/observable.h b/gdb/observable.h
> index 7a3cb39268..d36e73d569 100644
> --- a/gdb/observable.h
> +++ b/gdb/observable.h
> @@ -149,7 +149,7 @@ extern observable<struct breakpoint *>
> breakpoint_deleted;
>
>  /* A breakpoint has been modified in some way.  The argument b
>     is the modified breakpoint.  */
> -extern observable<struct breakpoint *> breakpoint_modified;
> +extern observable<struct breakpoint *, struct bp_location *>
> breakpoint_modified;

Could you update the doc to reflect the new argument?

When converting observers to C++, we lost the argument names.  It was 
suggested somewhere to add comments to give the "names" to the template 
arguments:

extern observable<struct breakpoint * /* b */, struct bp_location * /* 
bl */> ...

I would be fine with that.  Also, make sure to wrap to 80 columns.

Thanks,

Simon