Fix MI output for multi-location breakpoints

  [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.

[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
  

> From: Simon Marchi <simon.marchi@ericsson.com>
> CC: "palves@redhat.com" <palves@redhat.com>, Simon Marchi	<simon.marchi@ericsson.com>
> Date: Fri, 11 Jan 2019 00:15:34 +0000

The documentation changes are approved with these comments:

> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (Choosing Modes): Mention mi3.
> 	(Command Interpreters): Likewise.

You seem to be using the chapter/section names in the parentheses.
You should be using node names instead (or use Emacs, which will do
that for you ;-).

> 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.

Please don't remove the version information from this text.  At the
very least, we should tell what GDB version introduced the latest mi3
syntax.  We should tell this here, and not only in the "Interpreters"
node, because this section is a concise list of invocation options,
and should include the important information without sending the
reader to read the more detailed parts.

> +@item mi3
> +@cindex mi3 interpreter
> +The @sc{gdb/mi} interface introduced in @value{GDBN} 9.  It is the latest
                                           ^^^^^^^^^^^^^^
GDB 9.1, presumably?

>  @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.

I think the old text is better.

Thanks.

P.S. I wonder how did we let this problem slip through the cracks when
multiple-location breakpoints were introduced?  Maybe we should do
something to avoid such mistakes in the future.  We really shouldn't
be changing the MI syntax in incompatible ways so late into GDB
development cycle.
  

* 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
>
  

On 01/11/2019 12:34 PM, Andrew Burgess wrote:
> * Simon Marchi <simon.marchi@ericsson.com> [2019-01-11 00:15:34 +0000]:
> 
>> [CCing Pedro because we had some discussions earlier about that offline]


Thanks.  This was also recently-ish discussed in PR9659.

https://sourceware.org/bugzilla/show_bug.cgi?id=9659


>>
>> 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.


My original concern with MI bumps for individual MI fixes is that they
force an all-or-nothing approach on the frontends.  Let me expand.

Suppose a frontend developer only cares about the multi-location
fix, and not any of the other (supposed) fixes that go into MI3 that
make it backwards incompatible.  It was with that in mind that I had
suggested at <https://sourceware.org/bugzilla/show_bug.cgi?id=9659#c20>
to consider going with the "-fix-break-list-bug" solution first.
That would be usable with MI2 and also be enabled by default with
MI3 (with no way to disable).  Then later on, when we get rid of
MI2, the "-fix-break-list-bug" setting disappears.

But I suppose that that's really an unnecessary complication if we're
not really going to massively change MI every other release, and if
migrating a frontend to a new MI version isn't expected to be that
complicated.  We probably aren't and it probably isn't.

So all things considered, it's fine with me to go your route directly
without a "-fix-break-list-bug" step.

I agree with Andrew below though.  Bumping the MI version this late in
the cycle is probably not a good idea.  

If we want to fix this bug for 8.3, we could merge the fix while
leaving MI2 as the default, declare MI3 stable, and then bump the
WIP MI version to MI4.  I.e., the comments in the code that talk
about things to fix for MI3 should become references to MI4 instead.

Thanks,
Pedro Alves

> 
> 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?
>
  

On 2019-01-11 3:04 a.m., Eli Zaretskii wrote:
>> From: Simon Marchi <simon.marchi@ericsson.com>

>> CC: "palves@redhat.com" <palves@redhat.com>, Simon Marchi	<simon.marchi@ericsson.com>

>> Date: Fri, 11 Jan 2019 00:15:34 +0000

> 

> The documentation changes are approved with these comments:

> 

>> gdb/doc/ChangeLog:

>>

>> 	* gdb.texinfo (Choosing Modes): Mention mi3.

>> 	(Command Interpreters): Likewise.

> 

> You seem to be using the chapter/section names in the parentheses.

> You should be using node names instead (or use Emacs, which will do

> that for you ;-).


Oh, that's what I have always done!  Is this better?

	* gdb.texinfo (Mode Options): Mention mi3.
	(Interpreters): Likewise.

> 

>> 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.

> 

> Please don't remove the version information from this text.  At the

> very least, we should tell what GDB version introduced the latest mi3

> syntax.  We should tell this here, and not only in the "Interpreters"

> node, because this section is a concise list of invocation options,

> and should include the important information without sending the

> reader to read the more detailed parts.


The reason I thought this was not really appropriate here is that this manual
applies to a particular version of GDB (e.g. it's the manual shipped with
GDB 9.1).  So if you are using that version of GDB, it doesn't really matter
which version of GDB introduced mi3, all that matters is that it exists now.

As you said, this section is a concise list of invocation options.  But I don't
consider this very important information to have at the fingertips.

As Andrew suggested, I think we should have a list or table of all mi versions,
when they were introduced, and what the breaking changes are.  This would help
people writing or upgrading their MI frontend.   But I don't think that the
casual user looking for the possible command line flags will need to know the
history of MI.

Finally, the goal is to reduce the duplication of information, so there is less
things to update when releasing a new MI version (therefore less chance of having
outdated information).

>> +@item mi3

>> +@cindex mi3 interpreter

>> +The @sc{gdb/mi} interface introduced in @value{GDBN} 9.  It is the latest

>                                            ^^^^^^^^^^^^^^

> GDB 9.1, presumably?


Right.

>>  @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.

> 

> I think the old text is better.


The problem I see with the current wording is that it implies that mi1 is included
in version 5.1, 5.2 and 5.3 only.  In reality, it's included in all versions from
5.1 and up.  So it seems odd to list 5.2 and 5.3 specifically.  Do you have a
suggestion to address this?

> Thanks.

> 

> P.S. I wonder how did we let this problem slip through the cracks when

> multiple-location breakpoints were introduced?  Maybe we should do

> something to avoid such mistakes in the future.  We really shouldn't

> be changing the MI syntax in incompatible ways so late into GDB

> development cycle.


Well it's been known for a long time, as shows this bug from 2008 (AFAIU,
multiple location breakpoints were introduced around 2007?):

https://sourceware.org/bugzilla/show_bug.cgi?id=9659

I've been meaning to fix this for a few years, but always put it aside (until now)
because of the headache introducing backwards-incompatible MI changes represents.

Note that I intend to merge this change (or whatever solution we come up with) after
the 8.3 branch is created, so it will be part of the next release cycle.  We should
have plenty of time to iron out any bug we find, as well as introduce other MI-breaking
changes, while at it.

Simon
  

On 2019-01-11 7:34 a.m., Andrew Burgess wrote:
> * 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.


That's what I intended (I should have noted it in my original message), to
have this merge post-8.3.  It,s been broken for 12 years, nobody will cry
if 8.3 still has this bug.

> 

> 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.


Agreed, it's documentation that should be kept across releases (so, in the
doc), because frontend developers may upgrade from mi2 to mi3 a few versions
in the future, and we want the info about the backwards-incompatible changes
all in one place.

> 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...


As an example, I would be considering getting rid of BreakpointTable in the
-break-list output.

> 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.


Yes that makes sense.  Except that I would merge this patch after the 8.3
branch creation (until there's a compelling reason to have it in 8.3).

Simon
  

> From: Simon Marchi <simon.marchi@ericsson.com>
> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>,
> 	"palves@redhat.com" <palves@redhat.com>
> Date: Fri, 11 Jan 2019 20:21:22 +0000
> 
> >> gdb/doc/ChangeLog:
> >>
> >> 	* gdb.texinfo (Choosing Modes): Mention mi3.
> >> 	(Command Interpreters): Likewise.
> > 
> > You seem to be using the chapter/section names in the parentheses.
> > You should be using node names instead (or use Emacs, which will do
> > that for you ;-).
> 
> Oh, that's what I have always done!  Is this better?
> 
> 	* gdb.texinfo (Mode Options): Mention mi3.
> 	(Interpreters): Likewise.

Yes, of course.

> >> -@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.
> > 
> > Please don't remove the version information from this text.  At the
> > very least, we should tell what GDB version introduced the latest mi3
> > syntax.  We should tell this here, and not only in the "Interpreters"
> > node, because this section is a concise list of invocation options,
> > and should include the important information without sending the
> > reader to read the more detailed parts.
> 
> The reason I thought this was not really appropriate here is that this manual
> applies to a particular version of GDB (e.g. it's the manual shipped with
> GDB 9.1).  So if you are using that version of GDB, it doesn't really matter
> which version of GDB introduced mi3, all that matters is that it exists now.

People read newer manuals even if they have older versions of GDB
available.  E.g., I have on my development system all the versions of
GDB since v6.3, but only one version of the manual, the latest one.
Since Texinfo doesn't provide a way of installing several versions of
the same manual side by side, we should assume that my situation is
quite typical.

So let's not assume that when one reads a manual for GDB X.Y they are
interested only in that GDB version.

> As you said, this section is a concise list of invocation options.  But I don't
> consider this very important information to have at the fingertips.

I respectfully disagree.  A manual is more often than not read as a
reference, where the reader wants to quickly find the information they
are after, and stop reading right after that.  We should cater to such
usage of the manual, since it's IME the most important use.

> As Andrew suggested, I think we should have a list or table of all mi versions,
> when they were introduced, and what the breaking changes are.  This would help
> people writing or upgrading their MI frontend.   But I don't think that the
> casual user looking for the possible command line flags will need to know the
> history of MI.

I'm okay with adding a detailed table elsewhere, but I don't think
it's a good idea to remove the above information from the description
of the -mi command-line switch.

> Finally, the goal is to reduce the duplication of information, so there is less
> things to update when releasing a new MI version (therefore less chance of having
> outdated information).

That is not the main goal, though.  The main goal is to produce a
useful manual that allows finding the information one needs quickly
and efficiently.

> >> +@item mi3
> >> +@cindex mi3 interpreter
> >> +The @sc{gdb/mi} interface introduced in @value{GDBN} 9.  It is the latest
> >                                            ^^^^^^^^^^^^^^
> > GDB 9.1, presumably?
> 
> Right.

Then let's say that.

> >>  @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.
> > 
> > I think the old text is better.
> 
> The problem I see with the current wording is that it implies that mi1 is included
> in version 5.1, 5.2 and 5.3 only.

That's not my interpretation of "included in".

> In reality, it's included in all versions from 5.1 and up.  So it
> seems odd to list 5.2 and 5.3 specifically.  Do you have a
> suggestion to address this?

Say "used by" instead of "included in"?

> > P.S. I wonder how did we let this problem slip through the cracks when
> > multiple-location breakpoints were introduced?  Maybe we should do
> > something to avoid such mistakes in the future.  We really shouldn't
> > be changing the MI syntax in incompatible ways so late into GDB
> > development cycle.
> 
> Well it's been known for a long time, as shows this bug from 2008 (AFAIU,
> multiple location breakpoints were introduced around 2007?):
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=9659
> 
> I've been meaning to fix this for a few years, but always put it aside (until now)
> because of the headache introducing backwards-incompatible MI changes represents.

That's not what bothered me.  What bothered me was that we released a
GDB with this MI syntax without fixing it first.  I'm wondering how to
prevent such mistakes in the future.
  

On 2019-01-11 1:39 p.m., Pedro Alves wrote:
> On 01/11/2019 12:34 PM, Andrew Burgess wrote:

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

>>

>>> [CCing Pedro because we had some discussions earlier about that offline]

> 

> 

> Thanks.  This was also recently-ish discussed in PR9659.

> 

> https://sourceware.org/bugzilla/show_bug.cgi?id=9659


Ahh thanks for the reference, I couldn't remember where you had already
wrote about that.

> My original concern with MI bumps for individual MI fixes is that they

> force an all-or-nothing approach on the frontends.  Let me expand.

> 

> Suppose a frontend developer only cares about the multi-location

> fix, and not any of the other (supposed) fixes that go into MI3 that

> make it backwards incompatible.  It was with that in mind that I had

> suggested at <https://sourceware.org/bugzilla/show_bug.cgi?id=9659#c20>

> to consider going with the "-fix-break-list-bug" solution first.


I agree this would be nice.

> That would be usable with MI2 and also be enabled by default with

> MI3 (with no way to disable).  Then later on, when we get rid of

> MI2, the "-fix-break-list-bug" setting disappears.


Well this addresses my concern that frontends won't need to use
-fix-break-list-bug until the end of time, so I am ok with it.

> But I suppose that that's really an unnecessary complication if we're

> not really going to massively change MI every other release, and if

> migrating a frontend to a new MI version isn't expected to be that

> complicated.  We probably aren't and it probably isn't.


I'll at least give it a try, implementing it is probably not hard.  If it doesn't
add too much maintenance burden, I'm not against it.  If I do it for this bug, it
will pave the path for future bug fixes, so hopefully it will be smoother next time.

> So all things considered, it's fine with me to go your route directly

> without a "-fix-break-list-bug" step.


As I said, I'll give it a try.  I intend to name it -fix-multi-location-breakpoint-output.

> I agree with Andrew below though.  Bumping the MI version this late in

> the cycle is probably not a good idea.


I agree, I intend to merge a fix for this after 8.3 has branched.

> If we want to fix this bug for 8.3, we could merge the fix while

> leaving MI2 as the default, declare MI3 stable, and then bump the

> WIP MI version to MI4.  I.e., the comments in the code that talk

> about things to fix for MI3 should become references to MI4 instead.


Yes, although I would wait until 8.3 is branched before merging it.

Btw I realized the output with this patch is not good.  For -break-list with two multi-location
breakpoints, it results in something like:

body=[
  bkpt={ ... },
  locations={ ... },
  bkpt={ ... },
  locations={ ... },
]

Where I was aiming for:

body=[
  bkpt={
    ...,
    locations={ ... },
  },
  bkpt={
    ...,
    locations={ ... },
  },
]

The next version will fix this.

Simon
  

* Simon Marchi <simon.marchi@ericsson.com> [2019-01-11 23:36:16 +0000]:

> On 2019-01-11 1:39 p.m., Pedro Alves wrote:
> > On 01/11/2019 12:34 PM, Andrew Burgess wrote:
> >> * Simon Marchi <simon.marchi@ericsson.com> [2019-01-11 00:15:34 +0000]:
> >>
> >>> [CCing Pedro because we had some discussions earlier about that offline]
> > 
> > 
> > Thanks.  This was also recently-ish discussed in PR9659.
> > 
> > https://sourceware.org/bugzilla/show_bug.cgi?id=9659
> 
> Ahh thanks for the reference, I couldn't remember where you had already
> wrote about that.
> 
> > My original concern with MI bumps for individual MI fixes is that they
> > force an all-or-nothing approach on the frontends.  Let me expand.
> > 
> > Suppose a frontend developer only cares about the multi-location
> > fix, and not any of the other (supposed) fixes that go into MI3 that
> > make it backwards incompatible.  It was with that in mind that I had
> > suggested at <https://sourceware.org/bugzilla/show_bug.cgi?id=9659#c20>
> > to consider going with the "-fix-break-list-bug" solution first.
> 
> I agree this would be nice.
> 
> > That would be usable with MI2 and also be enabled by default with
> > MI3 (with no way to disable).  Then later on, when we get rid of
> > MI2, the "-fix-break-list-bug" setting disappears.
> 
> Well this addresses my concern that frontends won't need to use
> -fix-break-list-bug until the end of time, so I am ok with it.
> 
> > But I suppose that that's really an unnecessary complication if we're
> > not really going to massively change MI every other release, and if
> > migrating a frontend to a new MI version isn't expected to be that
> > complicated.  We probably aren't and it probably isn't.
> 
> I'll at least give it a try, implementing it is probably not hard.  If it doesn't
> add too much maintenance burden, I'm not against it.  If I do it for this bug, it
> will pave the path for future bug fixes, so hopefully it will be smoother next time.
> 
> > So all things considered, it's fine with me to go your route directly
> > without a "-fix-break-list-bug" step.
> 
> As I said, I'll give it a try.  I intend to name it -fix-multi-location-breakpoint-output.
>

Instead of adding a flag for this specific issue, should we consider
adding a generic mechanism that allows single commands to be run with
a different MI version?

My first thought was to add (in mi-parse.c:mi_parse) a new flag
'--mi', like we already have '--thread' and '--language', which would
let you pick a different MI version just for this command.  So you
could say:

  -break-insert --mi 3 LOCATION

And get MI3 for this command, even if you are currently running at MI2
by default.  Conversely, if a UI developer has mostly moved to MI3,
but break has not been updated yet, they could (assuming their default
is now MI3) do this:

  -break-insert --mi 2 LOCATION

and get the old behaviour.

The problem with the above, is that a user can also do:

  break LOCATION

and run the console command, but also get the formatted output.

I'm slightly tempted so say that we could ignore this case.  If you
use a CLI command then you get whatever the default is, only pure MI
commands would allow per-command switching...

An alternative, but similarly generic approach would be to allow
recursive MI invocation, with something like this (assuming MI2 is the
current default):

  -interpreter-exec mi3 "-break-insert LOCATION"

Again, this would allow the interpreter to be switched up and down as
needed on a command-by-command basis.  The problem with the second
approach is that it currently segfaults, I assume we don't currently
expect recursive MI invocation.

I started working on a patch for the first approach before realising
the problem with CLI commands.  I haven't looked at the cause of the
segfault in the second approach yet.

Do you think there's any benefit to adopting a more general solution
to this issue?

Thanks,
Andrew





> > I agree with Andrew below though.  Bumping the MI version this late in
> > the cycle is probably not a good idea.
> 
> I agree, I intend to merge a fix for this after 8.3 has branched.
> 
> > If we want to fix this bug for 8.3, we could merge the fix while
> > leaving MI2 as the default, declare MI3 stable, and then bump the
> > WIP MI version to MI4.  I.e., the comments in the code that talk
> > about things to fix for MI3 should become references to MI4 instead.
> 
> Yes, although I would wait until 8.3 is branched before merging it.
> 
> Btw I realized the output with this patch is not good.  For -break-list with two multi-location
> breakpoints, it results in something like:
> 
> body=[
>   bkpt={ ... },
>   locations={ ... },
>   bkpt={ ... },
>   locations={ ... },
> ]
> 
> Where I was aiming for:
> 
> body=[
>   bkpt={
>     ...,
>     locations={ ... },
>   },
>   bkpt={
>     ...,
>     locations={ ... },
>   },
> ]
> 
> The next version will fix this.
> 
> Simon
  

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Instead of adding a flag for this specific issue, should we consider
Andrew> adding a generic mechanism that allows single commands to be run with
Andrew> a different MI version?
[...]
Andrew>   -break-insert --mi 3 LOCATION

I tend to think the feature approach makes life easier for us and for
front end developers when bumping MI versions.

What I mean by this is that, if we ship MI3, then after some period of
time -- probably several years, given gdb's conservative approach --
we'd like to remove MI2.  This would let us clean up various hacks.

But consider the current bug.  The output is wrong in MI2, so a front
end developer would send:

    -break-insert --mi 3 blah blah

Now, we ship a gdb with, say, MI4.  Now we either have to support
backward compatibility here, and front ends will have to adapt whenever
MI3 is dropped.

With the feature flag, though, we can drop MI3 in this situation, but
keep accepting -fix-bug-NNN commands, and up-to-date front ends can just
keep working.

I think the difference is that a feature flag has a ratchet effect: you
can't go backward, only forward.

Tom
  

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>> In reality, it's included in all versions from 5.1 and up.  So it
>> seems odd to list 5.2 and 5.3 specifically.  Do you have a
>> suggestion to address this?

Eli> Say "used by" instead of "included in"?

Yeah, I think that would be clearer.  The current text makes it sounds
as though MI1 were limited to just these releases, whereas it's actually
the case that these releases included MI1 but not anything later.

Speaking of, it seems like we could remove MI1.

Eli> That's not what bothered me.  What bothered me was that we released a
Eli> GDB with this MI syntax without fixing it first.  I'm wondering how to
Eli> prevent such mistakes in the future.

Yeah, this is unfortunate.

Also, I think it is hard to see a programmatic way around this, at least
given gdb's current ui-out approach.  As I see it, the problem is that
at a given point, one can make either a tuple emitter or a list emitter,
but otherwise there's no distinction in the source.  If you choose the
wrong one this is hard to see in review, and there's really nothing else
checking the grammar.

Maybe the MI ui-out object could somehow enforce the constraint, by
asserting if the wrong thing is done.  Though it would need a bypass
anyhow because there are already bugs.

Tom
  

>>>> -@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.

>>>

>>> Please don't remove the version information from this text.  At the

>>> very least, we should tell what GDB version introduced the latest mi3

>>> syntax.  We should tell this here, and not only in the "Interpreters"

>>> node, because this section is a concise list of invocation options,

>>> and should include the important information without sending the

>>> reader to read the more detailed parts.

>>

>> The reason I thought this was not really appropriate here is that this manual

>> applies to a particular version of GDB (e.g. it's the manual shipped with

>> GDB 9.1).  So if you are using that version of GDB, it doesn't really matter

>> which version of GDB introduced mi3, all that matters is that it exists now.

> 

> People read newer manuals even if they have older versions of GDB

> available.  E.g., I have on my development system all the versions of

> GDB since v6.3, but only one version of the manual, the latest one.

> Since Texinfo doesn't provide a way of installing several versions of

> the same manual side by side, we should assume that my situation is

> quite typical.

> 

> So let's not assume that when one reads a manual for GDB X.Y they are

> interested only in that GDB version.


But we could say that for every single feature.  We introduced (or rather, will
introduce) "set index-cache" in GDB 8.3, and we don't mention in the doc that it
exists since 8.3.

>> As you said, this section is a concise list of invocation options.  But I don't

>> consider this very important information to have at the fingertips.

> 

> I respectfully disagree.  A manual is more often than not read as a

> reference, where the reader wants to quickly find the information they

> are after, and stop reading right after that.  We should cater to such

> usage of the manual, since it's IME the most important use.


I agree that the general use case of the manual is not to read it from cover to cover.

>> As Andrew suggested, I think we should have a list or table of all mi versions,

>> when they were introduced, and what the breaking changes are.  This would help

>> people writing or upgrading their MI frontend.   But I don't think that the

>> casual user looking for the possible command line flags will need to know the

>> history of MI.

> 

> I'm okay with adding a detailed table elsewhere, but I don't think

> it's a good idea to remove the above information from the description

> of the -mi command-line switch.


Would a cross-reference to the table be good enough?  If we add a detailed table
of MI versions (which I think is required), we will have the information about which
GDB version introduced each MI versions at three separate places in the manual...
I think this just adds unnecessary overhead for when we want to add a new MI version
(I know this doesn't happen often, but still).

Actually, there is already a cross reference just above.  I would consider rewording
things a bit to be clear that "to know more about the available interpreters, see
Command Interpreters".

>> Finally, the goal is to reduce the duplication of information, so there is less

>> things to update when releasing a new MI version (therefore less chance of having

>> outdated information).

> 

> That is not the main goal, though.  The main goal is to produce a

> useful manual that allows finding the information one needs quickly

> and efficiently.


Right, I meant "my goal", not the goal of the GDB manual.  I believe a cross-reference
would allow people find that information quickly and efficiently.

>>>>  @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.

>>>

>>> I think the old text is better.

>>

>> The problem I see with the current wording is that it implies that mi1 is included

>> in version 5.1, 5.2 and 5.3 only.

> 

> That's not my interpretation of "included in".

>>> In reality, it's included in all versions from 5.1 and up.  So it

>> seems odd to list 5.2 and 5.3 specifically.  Do you have a

>> suggestion to address this?

> 

> Say "used by" instead of "included in"?


Ok, so there are two things bothering me in the current text, and I am not sure if
we are in disagreement with both or just one.

- mi1 can be used by any GDB version between 5.1 and the present.  Why do we list
  specifically 5.1, 5.2 and 5.3?  It's not untrue to say that mi1 is available in
  5.1, 5.2 and 5.3, but it's ambiguous whether this is an exhaustive list or not.
  Better just say it's 5.1 and up.
- The term "included in", or "used by", regardless of whether it is coupled with "5.1"
  or 5.1, 5.2 and 5.3".  Since mi1 can be used with any version between 5.1 and the
  latest release, I think we should be looking for a phrasing that conveys that it's
  an half-open interval.  Saying that "mi1 is used by GDB 5.1" just tells you about
  5.1.  Saying that "mi1 is available starting with GDB 5.1" tells you about all
  versions between 5.1 and the current one.

Hopefully that clears up why I see the need to re-word this.


>>> P.S. I wonder how did we let this problem slip through the cracks when

>>> multiple-location breakpoints were introduced?  Maybe we should do

>>> something to avoid such mistakes in the future.  We really shouldn't

>>> be changing the MI syntax in incompatible ways so late into GDB

>>> development cycle.

>>

>> Well it's been known for a long time, as shows this bug from 2008 (AFAIU,

>> multiple location breakpoints were introduced around 2007?):

>>

>> https://sourceware.org/bugzilla/show_bug.cgi?id=9659

>>

>> I've been meaning to fix this for a few years, but always put it aside (until now)

>> because of the headache introducing backwards-incompatible MI changes represents.

> 

> That's not what bothered me.  What bothered me was that we released a

> GDB with this MI syntax without fixing it first.  I'm wondering how to

> prevent such mistakes in the future.


The problem here is that we had a tuple without name/id in a context where this
should be forbidden.  Maybe we could add some assert in ui_out::begin to catch
this.

Also, these MI commands and async events weren't properly tested in the testsuite.
But even if they were, our tests aren't bulletproof, since we match MI output textually.
We don't confirm that the output has valid MI syntax.

Instead of matching the MI output textually, we should have a proper MI output parser
written in TCL.  This would mean that any output produced by GDB in MI could be ran
through this parser, and any non-conforming output would cause an error.

I don't know if anybody is ready to write such a parser.  An idea out there would be
for GDB to use JSON for MI, instead of its own format (which is close to JSON anyway).
Then we could probably use an existing library, such as

  https://core.tcl.tk/tcllib/doc/trunk/embedded/www/tcllib/files/modules/json/json.html

making the process much easier.

Thanks,

Simon
  

On 2019-01-11 8:40 p.m., Andrew Burgess wrote:
> Do you think there's any benefit to adopting a more general solution

> to this issue?


I think that these are nice technical solutions, but now I fear the feature
creep in what started out as just fixing a simple bug :).

Perhaps something like that could be useful at some point, but for the moment
I don't think it's worth going through that much trouble and give that much
flexibility.  The reality is that stepping from MI2 to MI3 will be easy (at
least that's what I expect).

Simon
  

> From: Simon Marchi <simon.marchi@ericsson.com>
> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>,
> 	"palves@redhat.com" <palves@redhat.com>
> Date: Sun, 13 Jan 2019 05:09:29 +0000
> 
> > People read newer manuals even if they have older versions of GDB
> > available.  E.g., I have on my development system all the versions of
> > GDB since v6.3, but only one version of the manual, the latest one.
> > Since Texinfo doesn't provide a way of installing several versions of
> > the same manual side by side, we should assume that my situation is
> > quite typical.
> > 
> > So let's not assume that when one reads a manual for GDB X.Y they are
> > interested only in that GDB version.
> 
> But we could say that for every single feature.  We introduced (or rather, will
> introduce) "set index-cache" in GDB 8.3, and we don't mention in the doc that it
> exists since 8.3.

Doing this for every single feature is indeed overkill.  But changes
in the version of MI are rare and backward-incompatible, so they are a
different story, IMO.

> > I'm okay with adding a detailed table elsewhere, but I don't think
> > it's a good idea to remove the above information from the description
> > of the -mi command-line switch.
> 
> Would a cross-reference to the table be good enough?

Not in this particular case, no.

> If we add a detailed table
> of MI versions (which I think is required), we will have the information about which
> GDB version introduced each MI versions at three separate places in the manual...

Which is the third place?  I thought we have it only in two places.
The detailed table could come instead of that second place we have
now, not in addition to it.  But let's wait with that decision until
we actually see the proposed change for that table.

> - mi1 can be used by any GDB version between 5.1 and the present.  Why do we list
>   specifically 5.1, 5.2 and 5.3?

So that readers who have those versions installed knew they can only
use mi1.

> - The term "included in", or "used by", regardless of whether it is coupled with "5.1"
>   or 5.1, 5.2 and 5.3".  Since mi1 can be used with any version between 5.1 and the
>   latest release, I think we should be looking for a phrasing that conveys that it's
>   an half-open interval.  Saying that "mi1 is used by GDB 5.1" just tells you about
>   5.1.  Saying that "mi1 is available starting with GDB 5.1" tells you about all
>   versions between 5.1 and the current one.

The purpose of that text is to say that those versions can only use
mi1.

> > That's not what bothered me.  What bothered me was that we released a
> > GDB with this MI syntax without fixing it first.  I'm wondering how to
> > prevent such mistakes in the future.
> 
> The problem here is that we had a tuple without name/id in a context where this
> should be forbidden.  Maybe we could add some assert in ui_out::begin to catch
> this.

Something that enforces our own syntax rules would be nice, indeed.

> Instead of matching the MI output textually, we should have a proper MI output parser
> written in TCL.  This would mean that any output produced by GDB in MI could be ran
> through this parser, and any non-conforming output would cause an error.

Something like that, yes.

Thanks.
  

> Doing this for every single feature is indeed overkill.  But changes
> in the version of MI are rare and backward-incompatible, so they are a
> different story, IMO.

Just adding a new MI version is not a backward-incompatible change.  
Adding MI3 does not change anything for you if you use MI2.  What is 
backward-incompatible may be which version of MI is selected if you use 
"--interpreter=mi" (without an explicit version).  Is it what we are 
trying to document here, which version of the interpreter you end up 
with if you use "--interpreter=mi"?  Or we are trying to document the 
possible arguments to pass to "--interpreter"?

For the former, wouldn't it be clear to say that you end up with the 
latest stable release of the MI interpreter (and cross-reference to the 
table)?

For the latter, a cross-reference to the table gives would point you to 
the relevant info, without overloading this section as the number of MI 
releases grow.

>> > I'm okay with adding a detailed table elsewhere, but I don't think
>> > it's a good idea to remove the above information from the description
>> > of the -mi command-line switch.
>> 
>> Would a cross-reference to the table be good enough?
> 
> Not in this particular case, no.

Can you expand on why?  I really don't expect that many users to come to 
the manual often to know about MI versions.  Only a handful of people 
care about that (people who implement frontends), and they will surely 
dig much more in the manual (especially the GDB/MI section) to achieve 
what they want to do.

>> If we add a detailed table
>> of MI versions (which I think is required), we will have the 
>> information about which
>> GDB version introduced each MI versions at three separate places in 
>> the manual...
> 
> Which is the third place?  I thought we have it only in two places.
> The detailed table could come instead of that second place we have
> now, not in addition to it.  But let's wait with that decision until
> we actually see the proposed change for that table.

1. In "@node Mode Options"
2. In "@node Interpreters"
3. In the hypothetical table of MI versions.

>> - mi1 can be used by any GDB version between 5.1 and the present.  Why 
>> do we list
>>   specifically 5.1, 5.2 and 5.3?
> 
> So that readers who have those versions installed knew they can only
> use mi1.
>> - The term "included in", or "used by", regardless of whether it is 
>> coupled with "5.1"
>>   or 5.1, 5.2 and 5.3".  Since mi1 can be used with any version 
>> between 5.1 and the
>>   latest release, I think we should be looking for a phrasing that 
>> conveys that it's
>>   an half-open interval.  Saying that "mi1 is used by GDB 5.1" just 
>> tells you about
>>   5.1.  Saying that "mi1 is available starting with GDB 5.1" tells you 
>> about all
>>   versions between 5.1 and the current one.
> 
> The purpose of that text is to say that those versions can only use
> mi1.

Well, IMO it's clear from the fact that mi2, documented just above, is 
said to have been introduced by version 6.0 (and that 5.3 < 6.0).

But anyhow, I can live with the "used by" formulation if you and Tom 
think it's clear enough.  For MI2, however, we won't list all versions, 
since there are too many.  Could we switch them both to ranges for 
consistency?

- mi3: The gdb/mi interface used by GDB versions 9.1 and later.  This is 
the latest gdb/mi version.
- mi2: The gdb/mi interface used by GDB versions 6.0 to 8.3 
(inclusively).
- mi1: The gdb/mi interface used by GDB versions 5.1 to 5.3 
(inclusively).

Thanks,

Simon
  

> Date: Sun, 13 Jan 2019 11:17:27 -0500
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Cc: Simon Marchi <simon.marchi@ericsson.com>, gdb-patches@sourceware.org,
>         palves@redhat.com
> 
> > Doing this for every single feature is indeed overkill.  But changes
> > in the version of MI are rare and backward-incompatible, so they are a
> > different story, IMO.
> 
> Just adding a new MI version is not a backward-incompatible change.  
> Adding MI3 does not change anything for you if you use MI2.  What is 
> backward-incompatible may be which version of MI is selected if you use 
> "--interpreter=mi" (without an explicit version).  Is it what we are 
> trying to document here, which version of the interpreter you end up 
> with if you use "--interpreter=mi"?  Or we are trying to document the 
> possible arguments to pass to "--interpreter"?

We are trying to do both.

> For the former, wouldn't it be clear to say that you end up with the 
> latest stable release of the MI interpreter (and cross-reference to the 
> table)?

It's a different issue.  Saying that you get the latest stable release
says nothing about the version of MI supported by older GDB versions.

> For the latter, a cross-reference to the table gives would point you to 
> the relevant info, without overloading this section as the number of MI 
> releases grow.

Well, a single sentence is hardly an overload.  And I already
explained why I don't think a cross-reference here will do: many
people read this section as a man page, when they want to read about
GDB invocation.

> >> > I'm okay with adding a detailed table elsewhere, but I don't think
> >> > it's a good idea to remove the above information from the description
> >> > of the -mi command-line switch.
> >> 
> >> Would a cross-reference to the table be good enough?
> > 
> > Not in this particular case, no.
> 
> Can you expand on why?

See above.  My personal experience with simple information which I
need to find by following links is that it's only justified when the
description is long enough.  This is not such a case.

> I really don't expect that many users to come to the manual often to
> know about MI versions.  Only a handful of people care about that
> (people who implement frontends), and they will surely dig much more
> in the manual (especially the GDB/MI section) to achieve what they
> want to do.

I don't think the number of people who need some information is a
factor we should consider in such cases.  We should instead try to see
this from the POV of a single reader who does need it.

> > Which is the third place?  I thought we have it only in two places.
> > The detailed table could come instead of that second place we have
> > now, not in addition to it.  But let's wait with that decision until
> > we actually see the proposed change for that table.
> 
> 1. In "@node Mode Options"
> 2. In "@node Interpreters"
> 3. In the hypothetical table of MI versions.

We don't need to consider hypothetical issues.  When it becomes real,
we can discuss whether it will replace what's currently in
"Interpreters" or be an addition.

> But anyhow, I can live with the "used by" formulation if you and Tom 
> think it's clear enough.  For MI2, however, we won't list all versions, 
> since there are too many.  Could we switch them both to ranges for 
> consistency?

Fine with me (but I don't think we must be consistent in these
matters).

Thanks.
  

On 2019-01-13 11:49, Eli Zaretskii wrote:
>> > Which is the third place?  I thought we have it only in two places.
>> > The detailed table could come instead of that second place we have
>> > now, not in addition to it.  But let's wait with that decision until
>> > we actually see the proposed change for that table.
>> 
>> 1. In "@node Mode Options"
>> 2. In "@node Interpreters"
>> 3. In the hypothetical table of MI versions.
> 
> We don't need to consider hypothetical issues.  When it becomes real,
> we can discuss whether it will replace what's currently in
> "Interpreters" or be an addition.

I have sent a patch for this:

https://sourceware.org/ml/gdb-patches/2019-01/msg00335.html

Simon