[RFC] Fix the MI result of -break-insert with multiple locations

[RFC] Fix the MI 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>

	* 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 216ac73..7050f96 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6216,7 +6216,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
@@ -6235,8 +6234,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 =
@@ -6244,8 +6246,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

RE: [RFC] Fix the MI result of -break-insert with multiple locations

[Marc Khouzam]

> -----Original Message-----
> From: Mircea Gherzan [mailto:mircea.gherzan@intel.com] 
> Sent: Tuesday, January 29, 2013 9:36 AM
> To: tromey@redhat.com; vladimir@codesourcery.com; Marc Khouzam
> Cc: gdb-patches@sourceware.org; mircea.gherzan@intel.com
> Subject: [RFC] Fix the MI result of -break-insert with 
> multiple locations
> 
> 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", ...}

The above is an excerpt of the slightly more complete excerpt:
body=[bkpt={number="1", ...},{number="1.1", ...},{number="1.2", ...}]

and I think this fits with MI grammar:
"body" => variable
=
[bkpt=*] => value which is a list
and a list can be composed of many values which can be tuples:
{number="1", ...},{number="1.1", ...},{number="1.2", ...}

So, I believe the current syntax is valid, no?

BTW, Eclipse does not parse it properly currently because we made
the assumption there would be only one tuple.  We'll need to fix 
that in Eclipse.

Marc


> 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>
> 
> 	* 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 216ac73..7050f96 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -6216,7 +6216,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
> @@ -6235,8 +6234,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 =
> @@ -6244,8 +6246,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
> 
> 

Re: [RFC] Fix the MI result of -break-insert with multiple locations

[Mircea Gherzan]

On 29.01.2013 16:39, Marc Khouzam wrote:
 > The above is an excerpt of the slightly more complete excerpt:
 > body=[bkpt={number="1", ...},{number="1.1", ...},{number="1.2", ...}]
 >
 > and I think this fits with MI grammar:
 > "body" => variable
 > =
 > [bkpt=*] => value which is a list
 > and a list can be composed of many values which can be tuples:
 > {number="1", ...},{number="1.1", ...},{number="1.2", ...}

With a recent snapshot of the master branch [1], here's the full output 
without the patch:

(gdb)
-break-insert foobar
^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="<MULTIPLE>",times="0",original-location="foobar"},{number="1.1",enabled="y",addr="0x00000000004008d7",func="foobar()",file="overload.cpp",fullname=".../src/test-bktp-mi-overload/overload.cpp",line="22",thread-groups=["i1"]},{number="1.2",enabled="y",addr="0x00000000004008e0",func="foobar(int)",file="overload.cpp",fullname=".../src/test-bktp-mi-overload/overload.cpp",line="26",thread-groups=["i1"]}
(gdb)

So no square brackets. Therefore not a list and IMHO not a valid MI output.

Thanks,
Mircea

[1] Commit f04df06ec3ee7785676ce4e5e2ee49cf73b286ab


-- 

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: [RFC] Fix the MI result of -break-insert with multiple locations

[Marc Khouzam]

> -----Original Message-----
> From: Mircea Gherzan [mailto:mircea.gherzan@intel.com] 
> Sent: Tuesday, January 29, 2013 10:52 AM
> To: Marc Khouzam
> Cc: 'tromey@redhat.com'; 'vladimir@codesourcery.com'; 
> 'gdb-patches@sourceware.org'
> Subject: Re: [RFC] Fix the MI result of -break-insert with 
> multiple locations
> 
> On 29.01.2013 16:39, Marc Khouzam wrote:
>  > The above is an excerpt of the slightly more complete excerpt:
>  > body=[bkpt={number="1", ...},{number="1.1", 
> ...},{number="1.2", ...}]
>  >
>  > and I think this fits with MI grammar:
>  > "body" => variable
>  > =
>  > [bkpt=*] => value which is a list
>  > and a list can be composed of many values which can be tuples:
>  > {number="1", ...},{number="1.1", ...},{number="1.2", ...}
> 
> With a recent snapshot of the master branch [1], here's the 
> full output 
> without the patch:
> 
> (gdb)
> -break-insert foobar
> ^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="
> y",addr="<MULTIPLE>",times="0",original-location="foobar"},{nu
mber="1.1",enabled="y",addr="0x00000000004008d7",func="foobar()",file="overload.cpp",fullname=".../src/test-bktp-mi-> overload/overload.cpp",line="22",thread-groups=["i1"]},{number
="1.2",enabled="y",addr="0x00000000004008e0",func="foobar(int)",file="overload.cpp",fullname=".../src/test-bktp-mi-> overload/overload.cpp",line="26",thread-groups=["i1"]}
> (gdb)
> 
> So no square brackets. Therefore not a list and IMHO not a 
> valid MI output.

You're right.  The output I took was from -break-list, but
for -break-insert, and probably some others, this does not
look to be valid syntax.

I like your proposal of 
  bkpt={number="1", ... , locations=[{number="1.1", ...}, ...]}

which would allow to easily differentiate between the "main bp
entry" and the "sub entries".

Marc


> 
> Thanks,
> Mircea
> 
> [1] Commit f04df06ec3ee7785676ce4e5e2ee49cf73b286ab
> 
> 
> -- 
> 
> 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: [RFC] Fix the MI result of -break-insert with multiple locations

[Vladimir Prus]

On 29.01.2013 20:04, Marc Khouzam wrote:

>> (gdb)
>> -break-insert foobar
>> ^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="
>> y",addr="<MULTIPLE>",times="0",original-location="foobar"},{nu
> mber="1.1",enabled="y",addr="0x00000000004008d7",func="foobar()",file="overload.cpp",fullname=".../src/test-bktp-mi-> overload/overload.cpp",line="22",thread-groups=["i1"]},{number
> ="1.2",enabled="y",addr="0x00000000004008e0",func="foobar(int)",file="overload.cpp",fullname=".../src/test-bktp-mi-> overload/overload.cpp",line="26",thread-groups=["i1"]}
>> (gdb)
>>
>> So no square brackets. Therefore not a list and IMHO not a
>> valid MI output.
>
> You're right.  The output I took was from -break-list, but
> for -break-insert, and probably some others, this does not
> look to be valid syntax.
>
> I like your proposal of
>    bkpt={number="1", ... , locations=[{number="1.1", ...}, ...]}
>
> which would allow to easily differentiate between the "main bp
> entry" and the "sub entries".

I think it would help to see complete output, without ellipsis.

(And when we agree that the proposed output is good, the patch would need testsuite changes to be accepted)

Thanks,

-- 
Vladimir Prus
CodeSourcery / Mentor Graphics
http://www.mentor.com/embedded-software/

Re: [RFC] Fix the MI result of -break-insert with multiple locations

[Mircea Gherzan]

Am 29.01.2013 17:26, schrieb Vladimir Prus:
> I think it would help to see complete output, without ellipsis.

I provided the full output of -break-insert with an overloaded function
in a previous e-mail [1].

> (And when we agree that the proposed output is good, the patch would
> need testsuite changes to be accepted)

AFAICT the MI test suite does not currently contain any tests for the
multiple location case. So I guess you want a new test in mi-break.exp,
right?

Thanks,
Mircea

[1] http://www.sourceware.org/ml/gdb-patches/2013-01/msg00684.html

Re: [RFC] Fix the MI result of -break-insert with multiple locations

[Vladimir Prus]

On 29.01.2013 23:28, Mircea Gherzan wrote:
> Am 29.01.2013 17:26, schrieb Vladimir Prus:
>> I think it would help to see complete output, without ellipsis.
>
> I provided the full output of -break-insert with an overloaded function
> in a previous e-mail [1].

I did read that, but don't see the output after your patch.

>
>> (And when we agree that the proposed output is good, the patch would
>> need testsuite changes to be accepted)
>
> AFAICT the MI test suite does not currently contain any tests for the
> multiple location case. So I guess you want a new test in mi-break.exp,
> right?

Yes, sorry for being unclear.

-- 
Vladimir Prus
CodeSourcery / Mentor Graphics
http://www.mentor.com/embedded-software/

Re: [RFC] Fix the MI result of -break-insert with multiple locations

[Mircea Gherzan]

Am 29.01.2013 21:35, schrieb Vladimir Prus:
> On 29.01.2013 23:28, Mircea Gherzan wrote:
>> Am 29.01.2013 17:26, schrieb Vladimir Prus:
>>> I think it would help to see complete output, without ellipsis.
>>
>> I provided the full output of -break-insert with an overloaded function
>> in a previous e-mail [1].
> 
> I did read that, but don't see the output after your patch.

Here it is:

^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="<MULTIPLE>",times="0",original-location="foobar",locations=[{number="1.1",enabled="y",addr="0x00000000004008d7",func="foobar()",file="overload.cpp",fullname=".../src/test-bktp-mi-overload/overload.cpp",line="22",thread-groups=["i1"]},{number="1.2",enabled="y",addr="0x00000000004008e0",func="foobar(int)",file="overload.cpp",fullname=".../src/test-bktp-mi-overload/overload.cpp",line="26",thread-groups=["i1"]}]}

Thanks,
-- 
Mircea

Re: [RFC] Fix the MI result of -break-insert with multiple locations

[André Pönitz]

On Tue, Jan 29, 2013 at 03:36:04PM +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 seems more like an additional burden for frontends which cannot
rely on a specific gdb version being installed as they have to keep
code to parse both results, for years.

The fact that the current output might not conform to the documented
grammar has no impact to existing frontends as they have/had to cope
with this kind of slight deviations anyway.

An alternative approach would be to just make the documentation match
the actual output. This is not unprecedented.

Andre'

RE: [RFC] Fix the MI result of -break-insert with multiple locations

[Marc Khouzam]

> -----Original Message-----
> From: apoe@hrz.tu-chemnitz.de 
> [mailto:apoe@hrz.tu-chemnitz.de] On Behalf Of André Pönitz
> Sent: Tuesday, January 29, 2013 2:45 PM
> To: Mircea Gherzan
> Cc: tromey@redhat.com; vladimir@codesourcery.com; Marc 
> Khouzam; gdb-patches@sourceware.org
> Subject: Re: [RFC] Fix the MI result of -break-insert with 
> multiple locations
> 
> On Tue, Jan 29, 2013 at 03:36:04PM +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 seems more like an additional burden for frontends which cannot
> rely on a specific gdb version being installed as they have to keep
> code to parse both results, for years.

Although that is a good point, keeping it forces frontends to deal
with that case.  I believe this invalid output was introduced with 
GDB 7.5.  If it was fixed, an existing or future frontend may choose 
not to support the invalid output and rely on GDB 7.6.  I don't know
yet what we will do in Eclipse but I know that we use GDB 7.5 now
even though we don't parse the special broken output for breakpoints.
So things are not broken enough to force us to support the invalid
output (or at least I haven't realized that they are broken :)).


> The fact that the current output might not conform to the documented
> grammar has no impact to existing frontends as they have/had to cope
> with this kind of slight deviations anyway.
> 
> An alternative approach would be to just make the documentation match
> the actual output. This is not unprecedented.
> 
> Andre'

Re: [RFC] Fix the MI result of -break-insert with multiple locations

[André Pönitz]

On Tue, Jan 29, 2013 at 08:42:39PM +0000, Marc Khouzam wrote:
> > This seems more like an additional burden for frontends which cannot
> > rely on a specific gdb version being installed as they have to keep
> > code to parse both results, for years.
> 
> Although that is a good point, keeping it forces frontends to deal
> with that case.

I don't think it needs special "dealing" as I assume that existing 
frontend parsers are just robust enough to accept such cases.

Changing the nesting of structures on the other hand usually
needs adjustment to the interpretation of the parsed structure.

I am not scared of adding yet another ten lines to find out which
kind of output the user's choice of GDB produces and to handle
both versions, I just find it not particular convenient. 

Andre'

PS: This point remains:

> > An alternative approach would be to just make the
> > documentation match the actual output. This is not
> > unprecedented.

PPS: In theory "-list-features" could be used to announce output
changes to avoid the "guessing" phase.

RE: [RFC] Fix the MI result of -break-insert with multiple locations

[Marc Khouzam]

> -----Original Message-----
> From: apoe@hrz.tu-chemnitz.de 
> [mailto:apoe@hrz.tu-chemnitz.de] On Behalf Of André Pönitz
> Sent: Tuesday, January 29, 2013 5:14 PM
> To: Marc Khouzam
> Cc: 'Mircea Gherzan'; 'tromey@redhat.com'; 
> 'vladimir@codesourcery.com'; 'gdb-patches@sourceware.org'
> Subject: Re: [RFC] Fix the MI result of -break-insert with 
> multiple locations
> 
> On Tue, Jan 29, 2013 at 08:42:39PM +0000, Marc Khouzam wrote:
> > > This seems more like an additional burden for frontends 
> which cannot
> > > rely on a specific gdb version being installed as they 
> have to keep
> > > code to parse both results, for years.
> > 
> > Although that is a good point, keeping it forces frontends to deal
> > with that case.
> 
> I don't think it needs special "dealing" as I assume that existing 
> frontend parsers are just robust enough to accept such cases.

I don't believe eclipse can handle the current output as it stands,
not in the reply to -break-insert at least.  The code expects
a single breakpoint tuple, so we'll need to change things either way.

I'm not saying that is a good enough reason to change the output :)
I'm just mentioning that some frontends can't handle this already.


> Changing the nesting of structures on the other hand usually
> needs adjustment to the interpretation of the parsed structure.
> 
> I am not scared of adding yet another ten lines to find out which
> kind of output the user's choice of GDB produces and to handle
> both versions, I just find it not particular convenient. 
> 
> Andre'
> 
> PS: This point remains:
> 
> > > An alternative approach would be to just make the
> > > documentation match the actual output. This is not
> > > unprecedented.
> 
> PPS: In theory "-list-features" could be used to announce output
> changes to avoid the "guessing" phase.
> 
>