Re: [PATCH] Fix MI output for multi-location breakpoints

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

* Simon Marchi <simon.marchi@ericsson.com> [2019-01-11 00:15:34 +0000]:

> [CCing Pedro because we had some discussions earlier about that offline]
> 
> Various MI commands or events related to breakpoints output invalid MI
> records when printing information about a multi-location breakpoint.
> For example:
> 
>     -break-insert allo
>     ^done,bkpt={...,addr="<MULTIPLE>",...},{number="1.1",...},{number="1.2",...}
> 
> The problem is that according to the syntax [1], the top-level elements
> are of type "result" and should be of the form "variable=value".
> 
> This patch changes the output to wrap the locations in a list:
> 
>     ^done,bkpt={...,addr="<MULTIPLE>",...},locations=[{number="1.1",...},{number="1.2",...}]
> 
> The events =breakpoint-created, =breakpoint-modified, as well as the
> -break-info command also suffer from this (and maybe others I didn't
> find).
> 
> Since this is a breaking change for MI, we have to deal somehow with
> backwards compatibility.  The approach taken by this patch is to bump
> the MI version, use the new syntax in MI3 while retaining the old syntax
> in MI2.  Frontends are expected to use a precise MI version (-i=mi2), so
> if they do that they should be unaffected.
> 
> My previous attempts of this patch (not published) introduced new
> commands (-break-insert2, -break-info2) or a flag to the original
> commands to make them output the fixed syntax.  That doesn't really work
> because async events need to be fixed too.
> 
> Another solution would be to have a setting or command
> (-use-fixed-breakpoint-output) to enable the fixed output, keeping the
> broken one by default.  The only thing I don't like about that is that
> we will keep the broken behavior by default, which means that somebody
> writing a frontend who is not aware of that gotcha will go through the
> trouble of stumbling on broken MI output, and then hopefully discover
> that there is a command to un-break it.  I also don't see an easy way to
> deprecate and remove the old output over time using this strategy.  With
> a new MI version, somebody starting from scratch will use the latest MI
> version available, and therefore the fixed version.  Also, we can
> deprecate and then remove old MI versions after, let's say, 5 years of a
> newer version being available.

I think fixing issues like this is a good thing, but I wonder if we
should move a little slower.

With the next release only days away, could we not include this fix
for the release, but leave MI2 as the default.

MI3 would still exist, and include the fix, but would be documented as
unstable, or under-development, with the caveat that things could
change in the MI3 output.

Then between the next release (few days) and the release after (~6
months?) we can go crazy looking for MI fixes.  Then before the next
release we bump the default version to MI3.

UI developers can still use MI2 until they switch to MI3, but, when
they do switch to MI3 they get more fixes than just one item.

Further, I think we should make the lives of UI developers easier, but
having an explicit list in the documentation of what changed between
MI versions (starting from MI2 -> MI3) including examples.  Yes we
have the NEWS file, but (my personal opinion) the docs should stand on
their own for these sort of changes.

The above only really makes sense if we think there might be other
issues that could benefit from being fixed in the MI.  If we really
feel this is the only bug out there, then maybe we should just push
ahead with this patch as is...

In summary, I think we should:

  1. Merge this patch, but leave MI2 the default, add a new section to
  the docs listing "Changes Between MI2 and MI3".

  2. Change GDB so that when a user starts with -i=mi3 they get a
  warning that this version of the MI is under development, and liable
  to change, this should be documented too.

  3. Between now and the next + 1 release we should find and fix as
  many MI bugs as possible, documenting each.

  4. Just before the next + 1 release we should make MI3 the default,
  create MI4, and mark MI4 as "unstable".

  5. (Optional) Maybe, officially mark MI1 as deprecated, and note
  that it will be removed at some point.

Thoughts?

Thanks,
Andrew


> 
> [1] https://sourceware.org/gdb/onlinedocs/gdb/GDB_002fMI-Output-Syntax.html#GDB_002fMI-Output-Syntax
> 
> gdb/ChangeLog:
> 
> 	* NEWS: Mention that the new default MI version is 3.  Mention
> 	changes to the output of commands and events that deal with
> 	multi-location breakpoints.
> 	* breakpoint.c: Include "mi/mi-out.h".
> 	(print_one_breakpoint): Change output syntax if using MI version
> 	>= 3.
> 	* mi/mi-interp.c (mi_interp::init): Change default MI version to
> 	3.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* mi-breakpooint-location-ena-dis.exp: Rename to ...
> 	* mi-breakpooint-multiple-locations.exp: ... this.
> 	(make_breakpoints_pattern): New proc.
> 	(do_test): Add mi_version parameter, test -break-insert,
> 	-break-info and	=breakpoint-created.
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (Choosing Modes): Mention mi3.
> 	(Command Interpreters): Likewise.
> ---
>  gdb/NEWS                                      |  11 ++
>  gdb/breakpoint.c                              |  12 ++-
>  gdb/doc/gdb.texinfo                           |  20 ++--
>  gdb/mi/mi-interp.c                            |   5 +-
>  .../gdb.mi/mi-breakpoint-location-ena-dis.exp |  56 ----------
>  ...cc => mi-breakpoint-multiple-locations.cc} |   0
>  .../mi-breakpoint-multiple-locations.exp      | 100 ++++++++++++++++++
>  7 files changed, 134 insertions(+), 70 deletions(-)
>  delete mode 100644 gdb/testsuite/gdb.mi/mi-breakpoint-location-ena-dis.exp
>  rename gdb/testsuite/gdb.mi/{mi-breakpoint-location-ena-dis.cc => mi-breakpoint-multiple-locations.cc} (100%)
>  create mode 100644 gdb/testsuite/gdb.mi/mi-breakpoint-multiple-locations.exp
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index eaef6aa3849..f9c68871c93 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -165,6 +165,8 @@ set style address intensity VALUE
>  
>  * MI changes
>  
> +  ** The default version of the MI interpreter is now 3 (-i=mi3).
> +
>    ** The '-data-disassemble' MI command now accepts an '-a' option to
>       disassemble the whole function surrounding the given program
>       counter value or function name.  Support for this feature can be
> @@ -174,6 +176,15 @@ set style address intensity VALUE
>    ** Command responses and notifications that include a frame now include
>       the frame's architecture in a new "arch" attribute.
>  
> +  ** The output of information about multi-location breakpoints (which is
> +     syntactically incorrect in MI 2) has changed in MI 3.  This affects
> +     the following commands and events:
> +
> +	- -break-insert
> +	- -break-info
> +	- =breakpoint-created
> +	- =breakpoint-modified
> +
>  * New native configurations
>  
>  GNU/Linux/RISC-V		riscv*-*-linux*
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 2ab8a76326c..059f2d97419 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -69,6 +69,7 @@
>  #include "thread-fsm.h"
>  #include "tid-parse.h"
>  #include "cli/cli-style.h"
> +#include "mi/mi-out.h"
>  
>  /* readline include files */
>  #include "readline/readline.h"
> @@ -6384,10 +6385,15 @@ print_one_breakpoint (struct breakpoint *b,
>  	  && !is_hardware_watchpoint (b)
>  	  && (b->loc->next || !b->loc->enabled))
>  	{
> -	  struct bp_location *loc;
> -	  int n = 1;
> +	  gdb::optional<ui_out_emit_list> locations_list;
>  
> -	  for (loc = b->loc; loc; loc = loc->next, ++n)
> +	  /* For MI version <= 2, keep the behavior where GDB outputs an invalid MI
> +	     record.  For later versions, place breakpoint locations in a list.  */
> +	  if (uiout->is_mi_like_p () && mi_version (uiout) >= 3)
> +	    locations_list.emplace (uiout, "locations");
> +
> +	  int n = 1;
> +	  for (bp_location *loc = b->loc; loc != NULL; loc = loc->next, ++n)
>  	    {
>  	      ui_out_emit_tuple tuple_emitter (uiout, NULL);
>  	      print_one_breakpoint_location (b, loc, n, last_loc, allflag);
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 4a00834d0bf..a422a395c47 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -1268,12 +1268,11 @@ program or device.  This option is meant to be set by programs which
>  communicate with @value{GDBN} using it as a back end.
>  @xref{Interpreters, , Command Interpreters}.
>  
> -@samp{--interpreter=mi} (or @samp{--interpreter=mi2}) causes
> +@samp{--interpreter=mi} (or @samp{--interpreter=mi3}) causes
>  @value{GDBN} to use the @dfn{@sc{gdb/mi} interface} (@pxref{GDB/MI, ,
> -The @sc{gdb/mi} Interface}) included since @value{GDBN} version 6.0.  The
> -previous @sc{gdb/mi} interface, included in @value{GDBN} version 5.3 and
> -selected with @samp{--interpreter=mi1}, is deprecated.  Earlier
> -@sc{gdb/mi} interfaces are no longer supported.
> +The @sc{gdb/mi} Interface}).  The @sc{gdb/mi} interfaces 1 and 2 are
> +available, but deprecated.  Earlier @sc{gdb/mi} interfaces are no
> +longer available.
>  
>  @item -write
>  @cindex @code{--write}
> @@ -26501,18 +26500,23 @@ used interpreter with @value{GDBN}. With no interpreter specified at runtime,
>  
>  @item mi
>  @cindex mi interpreter
> -The newest @sc{gdb/mi} interface (currently @code{mi2}).  Used primarily
> +The newest @sc{gdb/mi} interface (currently @code{mi3}).  Used primarily
>  by programs wishing to use @value{GDBN} as a backend for a debugger GUI
>  or an IDE.  For more information, see @ref{GDB/MI, ,The @sc{gdb/mi}
>  Interface}.
>  
> +@item mi3
> +@cindex mi3 interpreter
> +The @sc{gdb/mi} interface introduced in @value{GDBN} 9.  It is the latest
> +@sc{gdb/mi} version.
> +
>  @item mi2
>  @cindex mi2 interpreter
> -The current @sc{gdb/mi} interface.
> +The @sc{gdb/mi} interface introduced in @value{GDBN} 6.0.
>  
>  @item mi1
>  @cindex mi1 interpreter
> -The @sc{gdb/mi} interface included in @value{GDBN} 5.1, 5.2, and 5.3.
> +The @sc{gdb/mi} interface introduced in @value{GDBN} 5.1.
>  
>  @end table
>  
> diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
> index d4baa485210..d51d3d72553 100644
> --- a/gdb/mi/mi-interp.c
> +++ b/gdb/mi/mi-interp.c
> @@ -129,10 +129,9 @@ mi_interp::init (bool top_level)
>    mi->targ = new mi_console_file (mi->raw_stdout, "@", '"');
>    mi->event_channel = new mi_console_file (mi->raw_stdout, "=", 0);
>  
> -  /* INTERP_MI selects the most recent released version.  "mi2" was
> -     released as part of GDB 6.0.  */
> +  /* INTERP_MI selects the most recent released version.  */
>    if (strcmp (name (), INTERP_MI) == 0)
> -    mi_version = 2;
> +    mi_version = 3;
>    else if (strcmp (name (), INTERP_MI1) == 0)
>      mi_version = 1;
>    else if (strcmp (name (), INTERP_MI2) == 0)
> diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-location-ena-dis.exp b/gdb/testsuite/gdb.mi/mi-breakpoint-location-ena-dis.exp
> deleted file mode 100644
> index eb781490fe7..00000000000
> --- a/gdb/testsuite/gdb.mi/mi-breakpoint-location-ena-dis.exp
> +++ /dev/null
> @@ -1,56 +0,0 @@
> -# Copyright 2018-2019 Free Software Foundation, Inc.
> -
> -# This program is free software; you can redistribute it and/or modify
> -# it under the terms of the GNU General Public License as published by
> -# the Free Software Foundation; either version 3 of the License, or
> -# (at your option) any later version.
> -#
> -# This program is distributed in the hope that it will be useful,
> -# but WITHOUT ANY WARRANTY; without even the implied warranty of
> -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> -# GNU General Public License for more details.
> -#
> -# You should have received a copy of the GNU General Public License
> -# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> -
> -# Tests whether =breakpoint=modified notification is sent when a single
> -# breakpoint location is enabled or disabled via CLI.
> -
> -load_lib mi-support.exp
> -set MIFLAGS "-i=mi"
> -
> -gdb_exit
> -if {[mi_gdb_start]} {
> -    continue
> -}
> -
> -#
> -# Start here
> -#
> -standard_testfile .cc
> -
> -if {[gdb_compile "$srcdir/$subdir/$srcfile" $binfile executable {debug c++}] != "" } {
> -    return -1
> -}
> -
> -mi_run_to_main
> -
> -mi_gdb_test "break add" \
> -	{(&.*)*.*~"Breakpoint 2 at.*\\n".*=breakpoint-created,bkpt=\{number="2",type="breakpoint".*\},\{number="2.1",enabled="y".*\}.*\n\^done} \
> -	"break add"
> -
> -# Modify enableness through MI commands shouldn't trigger MI
> -# notification.
> -mi_gdb_test "-break-disable 2.2" "\\^done" "-break-disable 2.2"
> -mi_gdb_test "-break-enable 2.2"  "\\^done" "-break-enable 2.2"
> -
> -# Modify enableness through CLI commands should trigger MI
> -# notification.
> -mi_gdb_test "dis 2.2" \
> -	{.*=breakpoint-modified,bkpt=\{number="2",type="breakpoint".*\},\{number="2.1",enabled="y".*\},\{number="2.2",enabled="n".*\}.*\n\^done} \
> -	"dis 2.2"
> -mi_gdb_test "en 2.2" \
> -	{.*=breakpoint-modified,bkpt=\{number="2",type="breakpoint".*\},\{number="2.1",enabled="y".*\},\{number="2.2",enabled="y".*\}.*\n\^done} \
> -	"en 2.2"
> -
> -mi_gdb_exit
> diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-location-ena-dis.cc b/gdb/testsuite/gdb.mi/mi-breakpoint-multiple-locations.cc
> similarity index 100%
> rename from gdb/testsuite/gdb.mi/mi-breakpoint-location-ena-dis.cc
> rename to gdb/testsuite/gdb.mi/mi-breakpoint-multiple-locations.cc
> diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-multiple-locations.exp b/gdb/testsuite/gdb.mi/mi-breakpoint-multiple-locations.exp
> new file mode 100644
> index 00000000000..4d61859d2f0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-breakpoint-multiple-locations.exp
> @@ -0,0 +1,100 @@
> +# Copyright 2018-2019 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Tests various things related to breakpoints with multiple locations.
> +
> +load_lib mi-support.exp
> +standard_testfile .cc
> +
> +if {[gdb_compile "$srcdir/$subdir/$srcfile" $binfile executable {debug c++}] != "" } {
> +    return -1
> +}
> +
> +# Generate the regexp pattern used to match the breakpoint description emitted
> +# in the various breakpoint command results/events, as expected for the given
> +# MI_VERSION.
> +#
> +# - BP_NUM is the expected breakpoint number
> +# - LOC1_EN and LOC2_EN are the expected value of the enabled field, for the
> +#   two locations.
> +
> +
> +proc make_breakpoints_pattern { mi_version bp_num loc1_en loc2_en } {
> +    if { $mi_version == "" || $mi_version >= 3 } {
> +	return "bkpt=\{number=\"${bp_num}\",type=\"breakpoint\",.*\},locations=\\\[\{number=\"${bp_num}\\.1\",enabled=\"${loc1_en}\",.*\},\{number=\"${bp_num}\\.2\",enabled=\"${loc2_en}\",.*\}\\\]"
> +    } else {
> +	return "bkpt=\{number=\"${bp_num}\",type=\"breakpoint\",.*\},\{number=\"${bp_num}\\.1\",enabled=\"${loc1_en}\",.*\},\{number=\"${bp_num}\\.2\",enabled=\"${loc2_en}\",.*\}"
> +    }
> +}
> +
> +# Run the test against the given version of the MI interpreter.
> +
> +proc do_test { mi_version } {
> +    with_test_prefix "mi_version=${mi_version}" {
> +	global MIFLAGS decimal
> +	set MIFLAGS "-i=mi${mi_version}"
> +
> +	gdb_exit
> +	if {[mi_gdb_start]} {
> +	    return
> +	}
> +
> +	mi_run_to_main
> +
> +	# Check the breakpoint-created event.
> +	set pattern [make_breakpoints_pattern $mi_version 2 y y]
> +	mi_gdb_test "break add" \
> +	    [multi_line "&\"break add\\\\n\"" \
> +			"~\"Breakpoint ${decimal} at.*\\(2 locations\\)\\\\n\"" \
> +			"=breakpoint-created,${pattern}" \
> +			"\\^done" ] \
> +	    "break add"
> +
> +	# Check the -break-info output.
> +	mi_gdb_test "-break-info" \
> +	    "\\^done,BreakpointTable=\{.*,body=\\\[${pattern}\\\]\}" \
> +	    "-break-info"
> +
> +	# Check the -break-insert response.
> +	set pattern [make_breakpoints_pattern $mi_version 3 y y]
> +	mi_gdb_test "-break-insert add" "\\^done,${pattern}" "insert breakpoint with MI command"
> +
> +	# Modify enableness through MI commands shouldn't trigger MI
> +	# notification.
> +	mi_gdb_test "-break-disable 2.2" "\\^done" "-break-disable 2.2"
> +	mi_gdb_test "-break-enable 2.2"  "\\^done" "-break-enable 2.2"
> +
> +	# Modify enableness through CLI commands should trigger MI
> +	# notification.
> +	set pattern [make_breakpoints_pattern $mi_version 2 y n]
> +	mi_gdb_test "dis 2.2" \
> +	    [multi_line "&\"dis 2.2\\\\n\"" \
> +			"=breakpoint-modified,${pattern}" \
> +			"\\^done" ] \
> +	    "dis 2.2"
> +	set pattern [make_breakpoints_pattern $mi_version 2 y y]
> +	mi_gdb_test "en 2.2" \
> +	    [multi_line "&\"en 2.2\\\\n\"" \
> +			"=breakpoint-modified,${pattern}" \
> +			"\\^done" ] \
> +	    "en 2.2"
> +
> +	mi_gdb_exit
> +    }
> +}
> +
> +do_test ""
> +do_test 2
> +do_test 3
> -- 
> 2.20.1
>