[PATCH v2] Fix MI output for multi-location breakpoints

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] Fix MI output for multi-location breakpoints

Simon Marchi-2
New in v2:

- Addressed comments about doc, updated the MI version table
- New doc for the Breakpoint information format
- New -fix-multi-location-breakpoint-output command, with associated
  doc, test and NEWS updated accordingly
- Fixed the output, the locations list is now actually in the tuple
  representing the breakpoint.

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.

The patch also adds the command -fix-multi-location-breakpoint-output,
which front ends can use to enable this behavior with MI <= 2.

[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-main.h (mi_cmd_fix_multi_location_breakpoint_output):
        New.
        (mi_multi_location_breakpoint_output_fixed): New.
        * mi/mi-main.c (fix_multi_location_breakpoint_output): New.
        (mi_cmd_fix_multi_location_breakpoint_output): New.
        (mi_multi_location_breakpoint_output_fixed): New.
        * mi/mi-interp.c (mi_interp::init): Change default MI version to
        3.
        * mi/mi-cmds.c (mi_cmds): Register command
        -fix-multi-location-breakpoint-output.

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 (Mode Options): Mention mi3.
        (Interpreters): Likewise.
        (GDB/MI Development and Front Ends): Add entry for MI 3 in
        version table.  Document -fix-multi-location-breakpoint-output.
        (GDB/MI Breakpoint Information): Document format of breakpoint
        location output.
---
 gdb/NEWS                                      |  14 ++
 gdb/breakpoint.c                              |  24 +++-
 gdb/doc/gdb.texinfo                           | 100 +++++++++++--
 gdb/mi/mi-cmds.c                              |   2 +
 gdb/mi/mi-interp.c                            |   5 +-
 gdb/mi/mi-main.c                              |  27 ++++
 gdb/mi/mi-main.h                              |  13 ++
 .../gdb.mi/mi-breakpoint-location-ena-dis.exp |  56 --------
 ...cc => mi-breakpoint-multiple-locations.cc} |   0
 .../mi-breakpoint-multiple-locations.exp      | 131 ++++++++++++++++++
 10 files changed, 294 insertions(+), 78 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..531296f481e 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,18 @@ 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
+
+     The -fix-multi-location-breakpoint-output command can be used to enable
+     this behavior with previous MI versions.
+
 * New native configurations
 
 GNU/Linux/RISC-V riscv*-*-linux*
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 3166fa01296..7385166a37f 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-main.h"
 
 /* readline include files */
 #include "readline/readline.h"
@@ -6361,12 +6362,15 @@ print_one_breakpoint (struct breakpoint *b,
       int allflag)
 {
   struct ui_out *uiout = current_uiout;
+  bool use_fixed_output = mi_multi_location_breakpoint_output_fixed (uiout);
 
-  {
-    ui_out_emit_tuple tuple_emitter (uiout, "bkpt");
+  gdb::optional<ui_out_emit_tuple> tuple_emitter (gdb::in_place, uiout, "bkpt");
+  print_one_breakpoint_location (b, NULL, 0, last_loc, allflag);
 
-    print_one_breakpoint_location (b, NULL, 0, last_loc, allflag);
-  }
+  /* The mi2 broken format: the main breakpoint tuple ends here, the locations
+     are outside.  */
+  if (!use_fixed_output)
+    tuple_emitter.reset ();
 
   /* If this breakpoint has custom print function,
      it's already printed.  Otherwise, print individual
@@ -6384,10 +6388,16 @@ 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 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 () && use_fixed_output)
+    locations_list.emplace (uiout, "locations");
 
-  for (loc = b->loc; loc; loc = loc->next, ++n)
+  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 173d18be6f7..c268a423b20 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -1268,12 +1268,12 @@ 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
-@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.
+@samp{--interpreter=mi} (or @samp{--interpreter=mi3}) causes
+@value{GDBN} to use the @dfn{@sc{gdb/mi} interface} version 3 (@pxref{GDB/MI, ,
+The @sc{gdb/mi} Interface}) included since @value{GDBN} version 9.1.  @sc{gdb/mi}
+version 2 (@code{mi2}), included in @value{GDBN} 6.0 and version 1 (@code{mi1}),
+included in @value{GDBN} 5.3, are also available.  Earlier @sc{gdb/mi}
+interfaces are no longer supported.
 
 @item -write
 @cindex @code{--write}
@@ -26501,18 +26501,22 @@ 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.1.
+
 @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
 
@@ -27834,8 +27838,36 @@ than a tuple.
 a tuple.
 @end itemize
 
+@item
+@center 3
+@tab
+@center 9.1
+@tab
+
+@itemize
+@item
+The output of information about multi-location breakpoints has changed in the
+responses to the @code{-break-insert} and @code{-break-info} commands, as well
+as in the @code{=breakpoint-created} and @code{=breakpoint-modified} events.
+The multiple locations are now placed in a @code{locations} field, whose value
+is a list.
+@end itemize
+
 @end multitable
 
+The following commands can be used to selectively enable behaviors from a
+newer MI version.  They can be useful if you want to take advantage of feature
+or bug fix only available in a more recent MI version than what you are using,
+but can't yet migrate to that version for some reason.
+
+@table @code
+
+@item -fix-multi-location-breakpoint-output
+Use the output for multi-location breakpoints which was introduced by MI 3.
+This has no effect when using MI version 3 or later.
+
+@end table
+
 The best way to avoid unexpected changes in MI that might break your front
 end is to make your project known to @value{GDBN} developers and
 follow development on @email{gdb@@sourceware.org} and
@@ -28164,9 +28196,7 @@ following fields:
 
 @table @code
 @item number
-The breakpoint number.  For a breakpoint that represents one location
-of a multi-location breakpoint, this will be a dotted pair, like
-@samp{1.2}.
+The breakpoint number.
 
 @item type
 The type of the breakpoint.  For ordinary breakpoints this will be
@@ -28266,6 +28296,52 @@ is not.
 @item what
 Some extra data, the exact contents of which are type-dependent.
 
+@item locations
+This field is present if the breakpoint has multiple locations.  It is also
+exceptionally present if the breakpoint is enabled and has a single, disabled
+location.
+
+The value is a list of locations.  The format of a location is decribed below.
+
+@end table
+
+A location in a multi-location breakpoint is represented as a tuple with the
+following fields:
+
+@table @code
+
+@item number
+The location number as a dotted pair, like @samp{1.2}.  The first digit is the
+number of the parent breakpoint.  The second digit is the number of the
+location within that breakpoint.
+
+@item enabled
+This indicates whether the location is enabled, in which case the
+value is @samp{y}, or disabled, in which case the value is @samp{n}.
+Note that this is not the same as the field @code{enable}.
+
+@item addr
+The address of this location as an hexidecimal number.
+
+@item func
+If known, the function in which the location appears.
+If not known, this field is not present.
+
+@item file
+The name of the source file which contains this location, if known.
+If not known, this field is not present.
+
+@item fullname
+The full file name of the source file which contains this location, if
+known.  If not known, this field is not present.
+
+@item line
+The line number at which this location appears, if known.
+If not known, this field is not present.
+
+@item thread-groups
+The thread groups this location is in.
+
 @end table
 
 For example, here is what the output of @code{-break-insert}
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index bb7c20c777b..fe30ac2e822 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -118,6 +118,8 @@ static struct mi_cmd mi_cmds[] =
   DEF_MI_CMD_MI ("file-list-shared-libraries",
  mi_cmd_file_list_shared_libraries),
   DEF_MI_CMD_CLI ("file-symbol-file", "symbol-file", 1),
+  DEF_MI_CMD_MI ("fix-multi-location-breakpoint-output",
+ mi_cmd_fix_multi_location_breakpoint_output),
   DEF_MI_CMD_MI ("gdb-exit", mi_cmd_gdb_exit),
   DEF_MI_CMD_CLI_1 ("gdb-set", "set", 1,
     &mi_suppress_notification.cmd_param_changed),
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index e45ddfe31ad..ead3ff3bd10 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/mi/mi-main.c b/gdb/mi/mi-main.c
index dc96032b0d4..4c9340302a7 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -2710,6 +2710,33 @@ mi_cmd_trace_frame_collected (const char *command, char **argv, int argc)
   }
 }
 
+/* Whether to use the fixed output when printing information about a
+   multi-location breakpoint (see PR 9659).  */
+
+static bool fix_multi_location_breakpoint_output = false;
+
+/* See mi/mi-main.h.  */
+
+void
+mi_cmd_fix_multi_location_breakpoint_output (const char *command, char **argv,
+     int argc)
+{
+  fix_multi_location_breakpoint_output = true;
+}
+
+/* See mi/mi-main.h.  */
+
+bool
+mi_multi_location_breakpoint_output_fixed (ui_out *uiout)
+{
+  mi_ui_out *mi_uiout = dynamic_cast<mi_ui_out *> (uiout);
+
+  if (mi_uiout == nullptr)
+    return false;
+
+  return mi_uiout->version () >= 3 || fix_multi_location_breakpoint_output;
+}
+
 void
 _initialize_mi_main (void)
 {
diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h
index a81aa9c2de1..cec9e2dc68a 100644
--- a/gdb/mi/mi-main.h
+++ b/gdb/mi/mi-main.h
@@ -54,5 +54,18 @@ struct mi_suppress_notification
 };
 extern struct mi_suppress_notification mi_suppress_notification;
 
+/* Implementation of -fix-multi-location-breakpoint-output.  */
+
+extern void mi_cmd_fix_multi_location_breakpoint_output (const char *command,
+ char **argv, int argc);
+
+/* Return whether -break-list, -break-insert, =breakpoint-created and
+   =breakpoint-modified should use the "fixed" output format (see PR
+   9659).
+
+   Return false if UIOUT is not an MI UI.  */
+
+extern bool mi_multi_location_breakpoint_output_fixed (ui_out *uiout);
+
 #endif
 
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..5d0452ad212
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-breakpoint-multiple-locations.exp
@@ -0,0 +1,131 @@
+# 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.
+#
+# - EXPECT_FIXED_OUTPUT: If true (non-zero), we expect GDB to output the fixed
+#   output for multi-locations breakpoint, else we expect it to output the
+#   broken pre-mi2 format.
+# - 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 { expect_fixed_output bp_num loc1_en loc2_en } {
+    if $expect_fixed_output {
+ 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 with the following parameters:
+#
+# - MI_VERSION: the version of the MI interpreter to use (e.g. "2")
+# - USE_FIX_FLAG: Whether to issue the -fix-multi-location-breakpoint-output
+#   command after starting GDB
+# - EXPECT_FIXED_OUTPUT: If true (non-zero), we expect GDB to output the fixed
+#   output for multi-locations breakpoint, else we expect it to output the
+#   broken pre-mi2 format.
+
+proc do_test { mi_version use_fix_flag expect_fixed_output } {
+    with_test_prefix "mi_version=${mi_version}" {
+ with_test_prefix "use_fix_flag=${use_fix_flag}" {
+    global MIFLAGS decimal
+    set MIFLAGS "-i=mi${mi_version}"
+
+    gdb_exit
+    if {[mi_gdb_start]} {
+ return
+    }
+
+    mi_run_to_main
+
+    if $use_fix_flag {
+ mi_gdb_test "-fix-multi-location-breakpoint-output" "\\^done" \
+    "send -fix-multi-location-breakpoint-output"
+    }
+
+    # Check the breakpoint-created event.
+    set pattern [make_breakpoints_pattern $expect_fixed_output 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 $expect_fixed_output 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 $expect_fixed_output 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 $expect_fixed_output 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
+ }
+    }
+}
+
+# Vanilla mi2
+do_test 2 0 0
+
+# mi2 with -fix-multi-location-breakpoint-output
+do_test 2 1 1
+
+# Vanilla mi3
+do_test 3 0 1
+
+# mi3 with -fix-multi-location-breakpoint-output
+do_test 3 1 1
+
+# Whatever MI version is currently the default one, vanilla
+do_test "" 0 1
+
+# Whatever MI version is currently the default one, with
+# -fix-multi-location-breakpoint-output
+do_test "" 1 1
--
2.20.1

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Simon Marchi <[hidden email]>
> CC: Simon Marchi <[hidden email]>
> Date: Fri, 18 Jan 2019 19:57:11 +0000
>
> +The following commands can be used to selectively enable behaviors from a
> +newer MI version.  They can be useful if you want to take advantage of feature
> +or bug fix only available in a more recent MI version than what you are using,
> +but can't yet migrate to that version for some reason.
> +
> +@table @code
> +
> +@item -fix-multi-location-breakpoint-output
> +Use the output for multi-location breakpoints which was introduced by MI 3.
> +This has no effect when using MI version 3 or later.

I needed to read this text and the one in NEWS several times before I
figured out what is meant here.  How about this alternative instead:

  If your front end cannot yet migrate to a more recent version of the
  MI protocol, you can nevertheless selectively enable specific features
  available in those recent MI versions, using the following commands:

  @table @code

  @item -fix-multi-location-breakpoint-output
  Use the output for multi-location breakpoints which was introduced by
  MI 3, even when using MI versions 2 or 1.  This command has no
  effect when using MI version 3 or later.

  @end @table

Otherwise, the documentation parts are OK.

Thanks.
Reply | Threaded
Open this post in threaded view
|

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

Simon Marchi-2
On 2019-01-18 4:46 p.m., Eli Zaretskii wrote:

>> From: Simon Marchi <[hidden email]>
>> CC: Simon Marchi <[hidden email]>
>> Date: Fri, 18 Jan 2019 19:57:11 +0000
>>
>> +The following commands can be used to selectively enable behaviors from a
>> +newer MI version.  They can be useful if you want to take advantage of feature
>> +or bug fix only available in a more recent MI version than what you are using,
>> +but can't yet migrate to that version for some reason.
>> +
>> +@table @code
>> +
>> +@item -fix-multi-location-breakpoint-output
>> +Use the output for multi-location breakpoints which was introduced by MI 3.
>> +This has no effect when using MI version 3 or later.
>
> I needed to read this text and the one in NEWS several times before I
> figured out what is meant here.  How about this alternative instead:
>
>   If your front end cannot yet migrate to a more recent version of the
>   MI protocol, you can nevertheless selectively enable specific features
>   available in those recent MI versions, using the following commands:
>
>   @table @code
>
>   @item -fix-multi-location-breakpoint-output
>   Use the output for multi-location breakpoints which was introduced by
>   MI 3, even when using MI versions 2 or 1.  This command has no
>   effect when using MI version 3 or later.
>
>   @end @table

Thanks, I adopted this wording.

> Otherwise, the documentation parts are OK.

Thanks for the reviews.

I am still open to comments about the code parts.  In any case, the patch will
remain up for review at least until the GDB 8.3 branch is created.

Simon


Reply | Threaded
Open this post in threaded view
|

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

Simon Marchi-4
On 2019-01-21 12:09 p.m., Simon Marchi wrote:

> On 2019-01-18 4:46 p.m., Eli Zaretskii wrote:
>>> From: Simon Marchi <[hidden email]>
>>> CC: Simon Marchi <[hidden email]>
>>> Date: Fri, 18 Jan 2019 19:57:11 +0000
>>>
>>> +The following commands can be used to selectively enable behaviors from a
>>> +newer MI version.  They can be useful if you want to take advantage of feature
>>> +or bug fix only available in a more recent MI version than what you are using,
>>> +but can't yet migrate to that version for some reason.
>>> +
>>> +@table @code
>>> +
>>> +@item -fix-multi-location-breakpoint-output
>>> +Use the output for multi-location breakpoints which was introduced by MI 3.
>>> +This has no effect when using MI version 3 or later.
>>
>> I needed to read this text and the one in NEWS several times before I
>> figured out what is meant here.  How about this alternative instead:
>>
>>    If your front end cannot yet migrate to a more recent version of the
>>    MI protocol, you can nevertheless selectively enable specific features
>>    available in those recent MI versions, using the following commands:
>>
>>    @table @code
>>
>>    @item -fix-multi-location-breakpoint-output
>>    Use the output for multi-location breakpoints which was introduced by
>>    MI 3, even when using MI versions 2 or 1.  This command has no
>>    effect when using MI version 3 or later.
>>
>>    @end @table
>
> Thanks, I adopted this wording.
>
>> Otherwise, the documentation parts are OK.
>
> Thanks for the reviews.
>
> I am still open to comments about the code parts.  In any case, the patch will
> remain up for review at least until the GDB 8.3 branch is created.
>
> Simon
>
>

This is now pushed.

Simon
Reply | Threaded
Open this post in threaded view
|

--disable-gdbmi build broken

Tom de Vries
[ was: Re: [PATCH v2] Fix MI output for multi-location breakpoints ]

On 13-03-19 20:16, Simon Marchi wrote:

> On 2019-01-21 12:09 p.m., Simon Marchi wrote:
>> On 2019-01-18 4:46 p.m., Eli Zaretskii wrote:
>>>> From: Simon Marchi <[hidden email]>
>>>> CC: Simon Marchi <[hidden email]>
>>>> Date: Fri, 18 Jan 2019 19:57:11 +0000
>>>>
>>>> +The following commands can be used to selectively enable behaviors
>>>> from a
>>>> +newer MI version.  They can be useful if you want to take advantage
>>>> of feature
>>>> +or bug fix only available in a more recent MI version than what you
>>>> are using,
>>>> +but can't yet migrate to that version for some reason.
>>>> +
>>>> +@table @code
>>>> +
>>>> +@item -fix-multi-location-breakpoint-output
>>>> +Use the output for multi-location breakpoints which was introduced
>>>> by MI 3.
>>>> +This has no effect when using MI version 3 or later.
>>>
>>> I needed to read this text and the one in NEWS several times before I
>>> figured out what is meant here.  How about this alternative instead:
>>>
>>>    If your front end cannot yet migrate to a more recent version of the
>>>    MI protocol, you can nevertheless selectively enable specific
>>> features
>>>    available in those recent MI versions, using the following commands:
>>>
>>>    @table @code
>>>
>>>    @item -fix-multi-location-breakpoint-output
>>>    Use the output for multi-location breakpoints which was introduced by
>>>    MI 3, even when using MI versions 2 or 1.  This command has no
>>>    effect when using MI version 3 or later.
>>>
>>>    @end @table
>>
>> Thanks, I adopted this wording.
>>
>>> Otherwise, the documentation parts are OK.
>>
>> Thanks for the reviews.
>>
>> I am still open to comments about the code parts.  In any case, the
>> patch will
>> remain up for review at least until the GDB 8.3 branch is created.
>>
>> Simon
>>
>>
>
> This is now pushed.
>

Hi,

this change breaks the --disable-gdbmi build:
...
ld: breakpoint.o: in function `print_one_breakpoint(breakpoint*,
bp_location**, int)':
src/gdb/breakpoint.c:6365: undefined reference to
`mi_multi_location_breakpoint_output_fixed(ui_out*)'
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:1893: gdb] Fout 1
...

Thanks,
- Tom
Reply | Threaded
Open this post in threaded view
|

Re: --disable-gdbmi build broken

Simon Marchi-4
On 2019-05-10 6:41 a.m., Tom de Vries wrote:
> this change breaks the --disable-gdbmi build:
> ...
> ld: breakpoint.o: in function `print_one_breakpoint(breakpoint*,
> bp_location**, int)':
> src/gdb/breakpoint.c:6365: undefined reference to
> `mi_multi_location_breakpoint_output_fixed(ui_out*)'
> collect2: error: ld returned 1 exit status
> make[1]: *** [Makefile:1893: gdb] Fout 1

Thanks for the report, I will look into fixing this.

Just wondering, do you actually use that configure option or find it useful?
How did you stumble on this? Last year, Tom Tromey suggested to remove it, but
we ended up keeping it just in case somebody actually used it, but there wasn't
convincing evidence that it was actually used:

  https://sourceware.org/ml/gdb-patches/2018-07/msg00507.html

So if you actually use that option, it would give us a data point and a reason
for us to keep it.

Simon
Reply | Threaded
Open this post in threaded view
|

Re: --disable-gdbmi build broken

Tom de Vries
On 10-05-19 18:21, Simon Marchi wrote:

> On 2019-05-10 6:41 a.m., Tom de Vries wrote:
>> this change breaks the --disable-gdbmi build:
>> ...
>> ld: breakpoint.o: in function `print_one_breakpoint(breakpoint*,
>> bp_location**, int)':
>> src/gdb/breakpoint.c:6365: undefined reference to
>> `mi_multi_location_breakpoint_output_fixed(ui_out*)'
>> collect2: error: ld returned 1 exit status
>> make[1]: *** [Makefile:1893: gdb] Fout 1
>
> Thanks for the report, I will look into fixing this.
>
> Just wondering, do you actually use that configure option or find it useful?
> How did you stumble on this? Last year, Tom Tromey suggested to remove it, but
> we ended up keeping it just in case somebody actually used it, but there wasn't
> convincing evidence that it was actually used:
>
>   https://sourceware.org/ml/gdb-patches/2018-07/msg00507.html
>
> So if you actually use that option, it would give us a data point and a reason
> for us to keep it.

I did a bisect today for PR24545 - "Symbol loading performance
regression with cc1" (
https://sourceware.org/bugzilla/show_bug.cgi?id=24545 ), and I wanted a
minimal build time for a fast bisect, so I went to look what
gdb/configure listed as --disable-something options, which is how I
noticed this.

I don't know whether --disable-gdbmi actually makes the build much
shorter, I just used it because it was advertised.

Thanks,
- Tom
Reply | Threaded
Open this post in threaded view
|

Re: --disable-gdbmi build broken

Simon Marchi-4
On 2019-05-10 1:01 p.m., Tom de Vries wrote:
> I did a bisect today for PR24545 - "Symbol loading performance
> regression with cc1" (
> https://sourceware.org/bugzilla/show_bug.cgi?id=24545 ), and I wanted a
> minimal build time for a fast bisect, so I went to look what
> gdb/configure listed as --disable-something options, which is how I
> noticed this.
>
> I don't know whether --disable-gdbmi actually makes the build much
> shorter, I just used it because it was advertised.

Ok I see.  Well, as long as the option exist, you should expect it to work.

Patch sent here: https://sourceware.org/ml/gdb-patches/2019-05/msg00258.html

Simon