[PATCH 1/3] MI: fix the result of -break-insert with multiple locations

[PATCH 1/3] MI: fix the result of -break-insert with multiple locations

[Mircea Gherzan]

The current MI output when printing a breakpoint with multiple locations
is not conformant to the MI specification:

  bkpt={number="1", ...},{number="1.1", ...},{number="1.2", ...}

This patch fixes this issue by moving the locations to a list inside the
first tuple:

  bkpt={number="1", ... , locations=[{number="1.1", ...}, ...]}

2013-01-28  Mircea Gherzan  <mircea.gherzan@intel.com>

gdb:
	* breakpoint.c (print_one_breakpoint): Use a list of breakpoint
	locations that adheres to the MI specification.

Signed-off-by: Mircea Gherzan <mircea.gherzan@intel.com>
---
 gdb/breakpoint.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index fb57a57..67da346 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6213,7 +6213,6 @@ print_one_breakpoint (struct breakpoint *b,
   bkpt_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "bkpt");
 
   print_one_breakpoint_location (b, NULL, 0, last_loc, allflag);
-  do_cleanups (bkpt_chain);
 
   /* If this breakpoint has custom print function,
      it's already printed.  Otherwise, print individual
@@ -6232,8 +6231,11 @@ print_one_breakpoint (struct breakpoint *b,
 	  && (b->loc->next || !b->loc->enabled))
 	{
 	  struct bp_location *loc;
+	  struct cleanup *loc_list;
 	  int n = 1;
 
+	  loc_list = make_cleanup_ui_out_list_begin_end (uiout, "locations");
+
 	  for (loc = b->loc; loc; loc = loc->next, ++n)
 	    {
 	      struct cleanup *inner2 =
@@ -6241,8 +6243,12 @@ print_one_breakpoint (struct breakpoint *b,
 	      print_one_breakpoint_location (b, loc, n, last_loc, allflag);
 	      do_cleanups (inner2);
 	    }
+
+	  do_cleanups (loc_list);
 	}
     }
+
+  do_cleanups (bkpt_chain);
 }
 
 static int
-- 
1.7.1

[PATCH 2/3] MI: add tests for breakpoints with multiple locations

[Mircea Gherzan]

2013-02-27  Mircea Gherzan  <mircea.gherzan@intel.com>

gdb/testsuite:
	* gdb.mi/overloaded.cc: New file.
	* gdb.mi/mi-breakpoint-multiple.exp: New file.

Signed-off-by: Mircea Gherzan <mircea.gherzan@intel.com>
---
 gdb/testsuite/gdb.mi/mi-breakpoint-multiple.exp |   46 +++++++++++++++++++++++
 gdb/testsuite/gdb.mi/overloaded.cc              |   40 ++++++++++++++++++++
 2 files changed, 86 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi-breakpoint-multiple.exp
 create mode 100644 gdb/testsuite/gdb.mi/overloaded.cc

diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-multiple.exp b/gdb/testsuite/gdb.mi/mi-breakpoint-multiple.exp
new file mode 100644
index 0000000..bc0419f
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-breakpoint-multiple.exp
@@ -0,0 +1,46 @@
+# Copyright 2012-2013 Free Software Foundation, Inc.
+#
+# Contributed by Intel Corp. <mircea.gherzan@intel.com>
+#
+# 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/>.
+
+load_lib mi-support.exp
+
+if {[skip_cplus_tests]} {
+    return -1
+}
+
+gdb_exit
+if [mi_gdb_start] {
+    continue
+}
+
+standard_testfile overloaded.cc
+
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug c++}] != "" } {
+    untested mi-breakpoint-multiple.exp
+    return -1
+}
+
+set line_foo_void_body  [gdb_get_line_number "foo_void_body"]
+set line_foo_int_body   [gdb_get_line_number "foo_int_body"]
+
+mi_run_to_main
+
+mi_gdb_test "11-break-insert foo" \
+    "11\\^done,bkpt=\{number=\"2\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"<MULTIPLE>\",times=\"0\",original-location=\"foo\",locations=\\\[\{number=\"2\\.1\",enabled=\"y\",addr=\"$hex\",func=\"foo\\(\\)\",file=\".*overloaded.cc\",fullname=\".*overloaded.cc\",line=\"$line_foo_void_body\",thread-groups=\\\[\"i1\"\\\]\},\{number=\"2\\.2\",enabled=\"y\",addr=\"$hex\",func=\"foo\\(int\\)\",file=\".*overloaded.cc\",fullname=\".*overloaded.cc\",line=\"$line_foo_int_body\",thread-groups=\\\[\"i1\"\\\]\}\\\]\}"  \
+    "insert breakpoint on breakpoint with multiple locations"
+
+mi_gdb_exit
+
diff --git a/gdb/testsuite/gdb.mi/overloaded.cc b/gdb/testsuite/gdb.mi/overloaded.cc
new file mode 100644
index 0000000..52430d0
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/overloaded.cc
@@ -0,0 +1,40 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2012-2013 Free Software Foundation, Inc.
+
+   Contributed by Intel Corp. <mircea.gherzan@intel.com>
+
+   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/>.  */
+
+int
+foo (void)
+{
+  return 0; /* foo_void_body */
+}
+
+int
+foo (int bar)
+{
+  return 0; /* foo_int_body */
+}
+
+int
+main (void)
+{
+  int x = foo ();
+
+  foo (x);
+
+  return 0;
+}
-- 
1.7.1

[PATCH 3/3] MI: document the format for breakpoints with multiple locations

[Mircea Gherzan]

2013-02-28  Mircea Gherzan  <mircea.gherzan@intel.com>

gdb/doc:
	* gdb.texinfo (GDB/MI Breakpoint Information): Add a description
	and an example for the locations field.

Signed-off-by: Mircea Gherzan <mircea.gherzan@intel.com>
---
 gdb/doc/gdb.texinfo |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 5f39d2e..d75b1af 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -28140,6 +28140,11 @@ This field is only given for tracepoints.  This is either @samp{y},
 meaning that the tracepoint is installed, or @samp{n}, meaning that it
 is not.
 
+@item locations
+This field is only given for breakpoints with multiple locations.  It is
+a list of breakpoint tuples with actual addresses and line information,
+one tuple for every location of the breakpoint.
+
 @item what
 Some extra data, the exact contents of which are type-dependent.
 
@@ -28157,6 +28162,20 @@ For example, here is what the output of @code{-break-insert}
 <- (gdb)
 @end smallexample
 
+For a breakpoint with multiple locations:
+
+@smallexample
+-> -break-insert foo
+<- ^done,bkpt=@{number="2",type="breakpoint",disp="keep",enabled="y",
+    addr="<MULTIPLE>",times="0",original-location="foo",
+    locations=[@{number="2.1",enabled="y",addr="0x00000000004005a8",
+    func="foo()",file="overloaded.cc",fullname="/home/mircea/overloaded.cc",
+    line="25",thread-groups=["i1"]@},@{number="2.2",enabled="y",
+    addr="0x00000000004005ba",func="foo(int)",file="overloaded.cc",
+    fullname="/home/mircea/overloaded.cc",line="31",thread-groups=["i1"]@}]@}
+<-  (gdb)
+@end smallexample
+
 @node GDB/MI Frame Information
 @subsection @sc{gdb/mi} Frame Information
 
-- 
1.7.1

Re: [PATCH 3/3] MI: document the format for breakpoints with multiple locations

[Eli Zaretskii]

> From: Mircea Gherzan <mircea.gherzan@intel.com>
> Cc: gdb-patches@sourceware.org, mgherzan@gmail.com,        Mircea Gherzan <mircea.gherzan@intel.com>
> Date: Thu, 28 Feb 2013 19:29:57 +0100
> 
> 2013-02-28  Mircea Gherzan  <mircea.gherzan@intel.com>
> 
> gdb/doc:
> 	* gdb.texinfo (GDB/MI Breakpoint Information): Add a description
> 	and an example for the locations field.

OK.

Thanks.

Re: [PATCH 1/3] MI: fix the result of -break-insert with multiple locations

[André Pönitz]

On Thu, Feb 28, 2013 at 07:29:55PM +0100, Mircea Gherzan wrote:
> The current MI output when printing a breakpoint with multiple locations
> is not conformant to the MI specification:
> 
>   bkpt={number="1", ...},{number="1.1", ...},{number="1.2", ...}
> 
> This patch fixes this issue by moving the locations to a list inside the
> first tuple:
> 
>   bkpt={number="1", ... , locations=[{number="1.1", ...}, ...]}

This breaks GDB frontends that parse the original output.

Andre'

Re: [PATCH 1/3] MI: fix the result of -break-insert with multiple locations

[Mircea Gherzan]

On 02.03.2013 17:53, André Pönitz wrote:
> On Thu, Feb 28, 2013 at 07:29:55PM +0100, Mircea Gherzan wrote:
>> The current MI output when printing a breakpoint with multiple locations
>> is not conformant to the MI specification:
>>
>>    bkpt={number="1", ...},{number="1.1", ...},{number="1.2", ...}
>>
>> This patch fixes this issue by moving the locations to a list inside the
>> first tuple:
>>
>>    bkpt={number="1", ... , locations=[{number="1.1", ...}, ...]}
>
> This breaks GDB frontends that parse the original output.

This has been discussed before. Marc has confirmed that Eclipse does not 
parse the original output either. With this patch series, the printing 
of breakpoints with multiple locations is:
   A) conformant to the MI syntax
   B) documented
   C) tested

With some review this should make it into 7.6.

Thanks,
Mircea

-- 

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [PATCH 1/3] MI: fix the result of -break-insert with multiple locations

[André Pönitz]

On Thu, Mar 07, 2013 at 05:31:51PM +0100, Mircea Gherzan wrote:
> On 02.03.2013 17:53, André Pönitz wrote:
> >On Thu, Feb 28, 2013 at 07:29:55PM +0100, Mircea Gherzan wrote:
> >>The current MI output when printing a breakpoint with multiple locations
> >>is not conformant to the MI specification:
> >>
> >>   bkpt={number="1", ...},{number="1.1", ...},{number="1.2", ...}
> >>
> >>This patch fixes this issue by moving the locations to a list inside the
> >>first tuple:
> >>
> >>   bkpt={number="1", ... , locations=[{number="1.1", ...}, ...]}
> >
> >This breaks GDB frontends that parse the original output.
> 
> This has been discussed before.

As in "we ignored your input".

> Marc has confirmed that Eclipse does not parse the original output either.

How does the non-affectedness of a specific frontend matter
in that context? Are there first and second class citizens 
when it comes to gdb frontends?

This breaks frontends that parses the original output and do
(obviously...) not understand your changed version.

Have fun.
Andre'

RE: [PATCH 1/3] MI: fix the result of -break-insert with multiple locations

[Marc Khouzam]

> On Thu, Mar 07, 2013 at 05:31:51PM +0100, Mircea Gherzan wrote:
> > On 02.03.2013 17:53, André Pönitz wrote:
> > >On Thu, Feb 28, 2013 at 07:29:55PM +0100, Mircea Gherzan wrote:
> > >>The current MI output when printing a breakpoint with multiple locations
> > >>is not conformant to the MI specification:
> > >>
> > >>   bkpt={number="1", ...},{number="1.1", ...},{number="1.2", ...}
> > >>
> > >>This patch fixes this issue by moving the locations to a list inside the
> > >>first tuple:
> > >>
> > >>   bkpt={number="1", ... , locations=[{number="1.1", ...}, ...]}
> > >
> > >This breaks GDB frontends that parse the original output.
> >
> > This has been discussed before.
> 
> As in "we ignored your input".
> 
> > Marc has confirmed that Eclipse does not parse the original output either.
> 
> How does the non-affectedness of a specific frontend matter
> in that context? Are there first and second class citizens
> when it comes to gdb frontends?

I think it may simply be that Mircea is not familiar with the frontend
landscape.  Eclipse is not the only frontend that uses MI to control
GDB.  Andre takes care of another such frontend (I apologize but
its names escapes me right at this moment).

Normally, MI changes should be backwards compatible (even
if they are fixing something broken).  I have seen that for difficult
changes, exceptions were made as long as no one mentioned 
the change broke a frontend.  This is not the case here.

> This breaks frontends that parses the original output and do
> (obviously...) not understand your changed version.

Re: [PATCH 1/3] MI: fix the result of -break-insert with multiple locations

[André Pönitz]

On Thu, Mar 07, 2013 at 09:43:41PM +0000, Marc Khouzam wrote:
> > How does the non-affectedness of a specific frontend matter
> > in that context? Are there first and second class citizens
> > when it comes to gdb frontends?
> 
> I think it may simply be that Mircea is not familiar with the frontend
> landscape.  Eclipse is not the only frontend that uses MI to control
> GDB.  Andre takes care of another such frontend (I apologize but
> its names escapes me right at this moment).

Nevermind. Happens to me, too...
 
> Normally, MI changes should be backwards compatible (even
> if they are fixing something broken).  I have seen that for difficult
> changes, exceptions were made as long as no one mentioned 
> the change broke a frontend.  This is not the case here.

I have changed my side to transparently parse either result now.

Nevertheless, it would be nice if the application of the patch could be
delayed by a couple of weeks to keep the time window small where people
might use a newer gdb with an older version of said frontend.
That'd not exactly be vital, just - nice.

Andre'

Re: [PATCH 1/3] MI: fix the result of -break-insert with multiple locations

[Mircea Gherzan]

On 08.03.2013 01:15, André Pönitz wrote:
> On Thu, Mar 07, 2013 at 09:43:41PM +0000, Marc Khouzam wrote:
>>> How does the non-affectedness of a specific frontend matter
>>> in that context? Are there first and second class citizens
>>> when it comes to gdb frontends?
>>
>> I think it may simply be that Mircea is not familiar with the frontend
>> landscape.  Eclipse is not the only frontend that uses MI to control
>> GDB.  Andre takes care of another such frontend (I apologize but
>> its names escapes me right at this moment).
>
> Nevermind. Happens to me, too...
>
>> Normally, MI changes should be backwards compatible (even
>> if they are fixing something broken).  I have seen that for difficult
>> changes, exceptions were made as long as no one mentioned
>> the change broke a frontend.  This is not the case here.
>
> I have changed my side to transparently parse either result now.

Thanks!

> Nevertheless, it would be nice if the application of the patch could be
> delayed by a couple of weeks to keep the time window small where people
> might use a newer gdb with an older version of said frontend.
> That'd not exactly be vital, just - nice.

The patch as RFC was posted in January. The current version is ten days 
old. Eclipse doesn't care, your fronted can now parse this and I have 
not seen any comments from other frontend maintainers. Given this, would 
it OK to apply this _today_? Tommorow the 7.6 branch will be cut off.

Thanks,
Mircea


-- 

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052