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

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

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

Mircea Gherzan-6
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  <[hidden email]>

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

Signed-off-by: Mircea Gherzan <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|

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

Marc Khouzam

> -----Original Message-----
> From: Mircea Gherzan [mailto:[hidden email]]
> Sent: Tuesday, January 29, 2013 9:36 AM
> To: [hidden email]; [hidden email]; Marc Khouzam
> Cc: [hidden email]; [hidden email]
> 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  <[hidden email]>
>
> * breakpoint.c (print_one_breakpoint): Use a list of breakpoint
> locations that adheres to the MI specification.
>
> Signed-off-by: Mircea Gherzan <[hidden email]>
> ---
>  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
>
>
Reply | Threaded
Open this post in threaded view
|

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

Mircea Gherzan-6
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

Reply | Threaded
Open this post in threaded view
|

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

Marc Khouzam

> -----Original Message-----
> From: Mircea Gherzan [mailto:[hidden email]]
> Sent: Tuesday, January 29, 2013 10:52 AM
> To: Marc Khouzam
> Cc: '[hidden email]'; '[hidden email]';
> '[hidden email]'
> 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
>
>
Reply | Threaded
Open this post in threaded view
|

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

Vladimir Prus-3
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/
Reply | Threaded
Open this post in threaded view
|

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
Reply | Threaded
Open this post in threaded view
|

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

André Pönitz
In reply to this post by Mircea Gherzan-6
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'
Reply | Threaded
Open this post in threaded view
|

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

Vladimir Prus-3
In reply to this post by Mircea Gherzan
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/
Reply | Threaded
Open this post in threaded view
|

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

Marc Khouzam
In reply to this post by André Pönitz
> -----Original Message-----
> From: [hidden email]
> [mailto:[hidden email]] On Behalf Of André Pönitz
> Sent: Tuesday, January 29, 2013 2:45 PM
> To: Mircea Gherzan
> Cc: [hidden email]; [hidden email]; Marc
> Khouzam; [hidden email]
> 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'
Reply | Threaded
Open this post in threaded view
|

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

Mircea Gherzan
In reply to this post by Vladimir Prus-3
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
Reply | Threaded
Open this post in threaded view
|

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

André Pönitz
In reply to this post by Marc Khouzam
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.

Reply | Threaded
Open this post in threaded view
|

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

Marc Khouzam

> -----Original Message-----
> From: [hidden email]
> [mailto:[hidden email]] On Behalf Of André Pönitz
> Sent: Tuesday, January 29, 2013 5:14 PM
> To: Marc Khouzam
> Cc: 'Mircea Gherzan'; '[hidden email]';
> '[hidden email]'; '[hidden email]'
> 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.
>
>