Breakpoint locations, enableness and MI's =breakpoint-modified event
Mon May 28 04:51:00 GMT 2018
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
> commit e766ec361a76749f14e63abb6ac588ebed59ad12
> Author: Jan Vrany <email@example.com>
> Date: Wed May 23 10:27:16 2018 +0100
> MI: emit =breakpoint-modified when breakpoint location enableness
> When a CLI command enabled/disables a breakpoint, MI frontend is
> via a =breakpoint-modified event. However, if only a particular
> is enabled/disabled, this event was not emitted which may cause
> to be out of sync (displaying wrong information to the user and so
> 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:
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. */
> -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 *>
> /* 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 *>
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
extern observable<struct breakpoint * /* b */, struct bp_location * /*
bl */> ...
I would be fine with that. Also, make sure to wrap to 80 columns.
More information about the Gdb-patches