[RFA/tracepoint] Make GDB can work with some old GDB server

[RFA/tracepoint] Make GDB can work with some old GDB server

[Hui Zhu]

Hi,

I got some bug report about ppa in https://lkml.org/lkml/2011/6/4/65
It is:
kgdb_breakpoint () at kernel/kgdb.c:1721
1721            wmb(); /* Sync point after breakpoint */
Sending packet: $qSymbol::#5b...Ack
Packet received:
Packet qSymbol (symbol-lookup) is NOT supported
Sending packet: $qTStatus#49...Ack
Packet received: E22
trace API error 0x2.
(gdb) bt
No stack.
(gdb)

This is because this kgdb(I think trunk have fixed this bug) don't
support qtstatus and reply -0x22.
So gdb throw a error.  Then all connect process stop.

This is a bug of kgdb, but I think make gdb just output a warning is
not affect anything else.  So I make this patch.

Please help me review it.

Thanks,
Hui


2011-06-24  Hui Zhu  <teawater@gmail.com>

	* remote.c (remote_start_remote): Add TRY_CATCH for
	remote_get_trace_status.
	* tracepoint.c (disconnect_tracing): Ditto.
---
 remote.c     |   13 ++++++++++++-
 tracepoint.c |   14 +++++++++++++-
 2 files changed, 25 insertions(+), 2 deletions(-)

--- a/remote.c
+++ b/remote.c
@@ -3146,6 +3146,8 @@ remote_start_remote (int from_tty, struc
   struct remote_state *rs = get_remote_state ();
   struct packet_config *noack_config;
   char *wait_status = NULL;
+  int ret = 0;
+  volatile struct gdb_exception ex;

   immediate_quit++;		/* Allow user to interrupt it.  */

@@ -3389,7 +3391,16 @@ remote_start_remote (int from_tty, struc

   /* Possibly the target has been engaged in a trace run started
      previously; find out where things are at.  */
-  if (remote_get_trace_status (current_trace_status ()) != -1)
+  TRY_CATCH (ex, RETURN_MASK_ERROR)
+    {
+      ret = remote_get_trace_status (current_trace_status ());
+    }
+  if (ex.reason < 0)
+    {
+      warning(_("%s"), ex.message);
+      ret = -1;
+    }
+  if (ret != -1)
     {
       struct uploaded_tp *uploaded_tps = NULL;
       struct uploaded_tsv *uploaded_tsvs = NULL;
--- a/tracepoint.c
+++ b/tracepoint.c
@@ -1939,11 +1939,23 @@ trace_status_mi (int on_stop)
 void
 disconnect_tracing (int from_tty)
 {
+  int ret = 0;
+  volatile struct gdb_exception ex;
+
   /* It can happen that the target that was tracing went away on its
      own, and we didn't notice.  Get a status update, and if the
      current target doesn't even do tracing, then assume it's not
      running anymore.  */
-  if (target_get_trace_status (current_trace_status ()) < 0)
+  TRY_CATCH (ex, RETURN_MASK_ERROR)
+    {
+      ret = target_get_trace_status (current_trace_status ());
+    }
+  if (ex.reason < 0)
+    {
+      warning(_("%s"), ex.message);
+      ret = -1;
+    }
+  if (ret < 0)
     current_trace_status ()->running = 0;

   /* If running interactively, give the user the option to cancel and

Re: [RFA/tracepoint] Make GDB can work with some old GDB server

[Hui Zhu]

Ping.

On Fri, Jun 24, 2011 at 16:46, Hui Zhu <teawater@gmail.com> wrote:
> Hi,
>
> I got some bug report about ppa in https://lkml.org/lkml/2011/6/4/65
> It is:
> kgdb_breakpoint () at kernel/kgdb.c:1721
> 1721            wmb(); /* Sync point after breakpoint */
> Sending packet: $qSymbol::#5b...Ack
> Packet received:
> Packet qSymbol (symbol-lookup) is NOT supported
> Sending packet: $qTStatus#49...Ack
> Packet received: E22
> trace API error 0x2.
> (gdb) bt
> No stack.
> (gdb)
>
> This is because this kgdb(I think trunk have fixed this bug) don't
> support qtstatus and reply -0x22.
> So gdb throw a error.  Then all connect process stop.
>
> This is a bug of kgdb, but I think make gdb just output a warning is
> not affect anything else.  So I make this patch.
>
> Please help me review it.
>
> Thanks,
> Hui
>
>
> 2011-06-24  Hui Zhu  <teawater@gmail.com>
>
>        * remote.c (remote_start_remote): Add TRY_CATCH for
>        remote_get_trace_status.
>        * tracepoint.c (disconnect_tracing): Ditto.
> ---
>  remote.c     |   13 ++++++++++++-
>  tracepoint.c |   14 +++++++++++++-
>  2 files changed, 25 insertions(+), 2 deletions(-)
>
> --- a/remote.c
> +++ b/remote.c
> @@ -3146,6 +3146,8 @@ remote_start_remote (int from_tty, struc
>   struct remote_state *rs = get_remote_state ();
>   struct packet_config *noack_config;
>   char *wait_status = NULL;
> +  int ret = 0;
> +  volatile struct gdb_exception ex;
>
>   immediate_quit++;            /* Allow user to interrupt it.  */
>
> @@ -3389,7 +3391,16 @@ remote_start_remote (int from_tty, struc
>
>   /* Possibly the target has been engaged in a trace run started
>      previously; find out where things are at.  */
> -  if (remote_get_trace_status (current_trace_status ()) != -1)
> +  TRY_CATCH (ex, RETURN_MASK_ERROR)
> +    {
> +      ret = remote_get_trace_status (current_trace_status ());
> +    }
> +  if (ex.reason < 0)
> +    {
> +      warning(_("%s"), ex.message);
> +      ret = -1;
> +    }
> +  if (ret != -1)
>     {
>       struct uploaded_tp *uploaded_tps = NULL;
>       struct uploaded_tsv *uploaded_tsvs = NULL;
> --- a/tracepoint.c
> +++ b/tracepoint.c
> @@ -1939,11 +1939,23 @@ trace_status_mi (int on_stop)
>  void
>  disconnect_tracing (int from_tty)
>  {
> +  int ret = 0;
> +  volatile struct gdb_exception ex;
> +
>   /* It can happen that the target that was tracing went away on its
>      own, and we didn't notice.  Get a status update, and if the
>      current target doesn't even do tracing, then assume it's not
>      running anymore.  */
> -  if (target_get_trace_status (current_trace_status ()) < 0)
> +  TRY_CATCH (ex, RETURN_MASK_ERROR)
> +    {
> +      ret = target_get_trace_status (current_trace_status ());
> +    }
> +  if (ex.reason < 0)
> +    {
> +      warning(_("%s"), ex.message);
> +      ret = -1;
> +    }
> +  if (ret < 0)
>     current_trace_status ()->running = 0;
>
>   /* If running interactively, give the user the option to cancel and
>

Re: [RFA/tracepoint] Make GDB can work with some old GDB server

[Tom Tromey]

>>>>> ">" == Hui Zhu <teawater@gmail.com> writes:

>> 2011-06-24  Hui Zhu  <teawater@gmail.com>
>> 	* remote.c (remote_start_remote): Add TRY_CATCH for
>> 	remote_get_trace_status.
>> 	* tracepoint.c (disconnect_tracing): Ditto.

It seems reasonable enough to me, but I would prefer that somebody more
knowledgeable about remote.c review this.

>> +      warning(_("%s"), ex.message);

Space before the first open paren.

>> +      warning(_("%s"), ex.message);

Likewise.

Tom

Re: [RFA/tracepoint] Make GDB can work with some old GDB server

[Hui Zhu]

Hi Tom,

Thanks for your review.
Fixed and checked in.

Do you think this patch is OK for 7.3 branch?

Best,
Hui

On Wed, Jul 6, 2011 at 02:33, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> ">" == Hui Zhu <teawater@gmail.com> writes:
>
>>> 2011-06-24  Hui Zhu  <teawater@gmail.com>
>>>      * remote.c (remote_start_remote): Add TRY_CATCH for
>>>      remote_get_trace_status.
>>>      * tracepoint.c (disconnect_tracing): Ditto.
>
> It seems reasonable enough to me, but I would prefer that somebody more
> knowledgeable about remote.c review this.
>
>>> +      warning(_("%s"), ex.message);
>
> Space before the first open paren.
>
>>> +      warning(_("%s"), ex.message);
>
> Likewise.
>
> Tom
>

Re: [RFA/tracepoint] Make GDB can work with some old GDB server

[Tom Tromey]

>>>>> ">" == Hui Zhu <teawater@gmail.com> writes:

>> Thanks for your review.
>> Fixed and checked in.

I didn't intend to approve this.  I am sorry this was not clear.  I do
not know remote.c well enough to approve a subtle patch like this.

>> Do you think this patch is OK for 7.3 branch?

No.

Tom

Re: [RFA/tracepoint] Make GDB can work with some old GDB server

[Hui Zhu]

Oops, sorry for misunderstand your mail.  If this patch doesn't get
review after 3 days, I will revert it.

Thanks,
hui

On Wed, Jul 6, 2011 at 22:36, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> ">" == Hui Zhu <teawater@gmail.com> writes:
>
>>> Thanks for your review.
>>> Fixed and checked in.
>
> I didn't intend to approve this.  I am sorry this was not clear.  I do
> not know remote.c well enough to approve a subtle patch like this.
>
>>> Do you think this patch is OK for 7.3 branch?
>
> No.
>
> Tom
>

Re: [RFA/tracepoint] Make GDB can work with some old GDB server

[Jan Kratochvil]

On Fri, 24 Jun 2011 10:46:58 +0200, Hui Zhu wrote:
> Sending packet: $qTStatus#49...Ack
> Packet received: E22
> trace API error 0x2.
[...]
> This is because this kgdb(I think trunk have fixed this bug) don't
> support qtstatus and reply -0x22.
> So gdb throw a error.  Then all connect process stop.

Why not to just put it more close to the error invocation?  Still some calls
to target_get_trace_status would remain unprotected otherwise.


Thanks,
Jan


gdb/
2011-07-07  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Work around kgdb.
	* remote.c (remote_get_trace_status): New variable ex.  Put
	remote_get_noisy_reply into TRY_CATCH.  Call exception_fprintf for it.

--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -10045,11 +10045,21 @@ remote_get_trace_status (struct trace_status *ts)
   char *p;
   /* FIXME we need to get register block size some other way.  */
   extern int trace_regblock_size;
+  volatile struct gdb_exception ex;
 
   trace_regblock_size = get_remote_arch_state ()->sizeof_g_packet;
 
   putpkt ("qTStatus");
-  p = remote_get_noisy_reply (&target_buf, &target_buf_size);
+
+  TRY_CATCH (ex, RETURN_MASK_ERROR)
+    {
+      p = remote_get_noisy_reply (&target_buf, &target_buf_size);
+    }
+  if (ex.reason < 0)
+    {
+      exception_fprintf (gdb_stderr, ex, "qTStatus: ");
+      return -1;
+    }
 
   /* If the remote target doesn't do tracing, flag it.  */
   if (*p == '\0')

Re: [RFA/tracepoint] Make GDB can work with some old GDB server

[Hui Zhu]

On Thu, Jul 7, 2011 at 17:19, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> On Fri, 24 Jun 2011 10:46:58 +0200, Hui Zhu wrote:
>> Sending packet: $qTStatus#49...Ack
>> Packet received: E22
>> trace API error 0x2.
> [...]
>> This is because this kgdb(I think trunk have fixed this bug) don't
>> support qtstatus and reply -0x22.
>> So gdb throw a error.  Then all connect process stop.
>
> Why not to just put it more close to the error invocation?  Still some calls
> to target_get_trace_status would remain unprotected otherwise.
>
>
> Thanks,
> Jan
>
>
> gdb/
> 2011-07-07  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>        Work around kgdb.
>        * remote.c (remote_get_trace_status): New variable ex.  Put
>        remote_get_noisy_reply into TRY_CATCH.  Call exception_fprintf for it.
>
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -10045,11 +10045,21 @@ remote_get_trace_status (struct trace_status *ts)
>   char *p;
>   /* FIXME we need to get register block size some other way.  */
>   extern int trace_regblock_size;
> +  volatile struct gdb_exception ex;
>
>   trace_regblock_size = get_remote_arch_state ()->sizeof_g_packet;
>
>   putpkt ("qTStatus");
> -  p = remote_get_noisy_reply (&target_buf, &target_buf_size);
> +
> +  TRY_CATCH (ex, RETURN_MASK_ERROR)
> +    {
> +      p = remote_get_noisy_reply (&target_buf, &target_buf_size);
> +    }
> +  if (ex.reason < 0)
> +    {
> +      exception_fprintf (gdb_stderr, ex, "qTStatus: ");
> +      return -1;
> +    }
>
>   /* If the remote target doesn't do tracing, flag it.  */
>   if (*p == '\0')
>

Cool.  This one is more better.  I have reverted my patch.

Suggest this patch can check in to both 7.3 and trunk.

Thanks,
Hui

Re: [RFA/tracepoint] Make GDB can work with some old GDB server

[Jan Kratochvil]

On Thu, 07 Jul 2011 15:39:42 +0200, Hui Zhu wrote:
> On Thu, Jul 7, 2011 at 17:19, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> > gdb/
> > 2011-07-07  Jan Kratochvil  <jan.kratochvil@redhat.com>
> >
> >        Work around kgdb.
> >        * remote.c (remote_get_trace_status): New variable ex.  Put
> >        remote_get_noisy_reply into TRY_CATCH.  Call exception_fprintf for it.
> Cool.  This one is more better.  I have reverted my patch.
> 
> Suggest this patch can check in to both 7.3 and trunk.

As Pedro is probably not going to review it till 7.3 and the patch looks safe
enough to me and having no impact on FSF gdbservers checked it in:
	http://sourceware.org/ml/gdb-cvs/2011-07/msg00131.html
7.3:
	http://sourceware.org/ml/gdb-cvs/2011-07/msg00132.html


Thanks,
Jan

Re: [RFA/tracepoint] Make GDB can work with some old GDB server

[Hui Zhu]

Thanks Jan.

Best,
Hui

On Thu, Jul 14, 2011 at 01:12, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> On Thu, 07 Jul 2011 15:39:42 +0200, Hui Zhu wrote:
>> On Thu, Jul 7, 2011 at 17:19, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
>> > gdb/
>> > 2011-07-07  Jan Kratochvil  <jan.kratochvil@redhat.com>
>> >
>> >        Work around kgdb.
>> >        * remote.c (remote_get_trace_status): New variable ex.  Put
>> >        remote_get_noisy_reply into TRY_CATCH.  Call exception_fprintf for it.
>> Cool.  This one is more better.  I have reverted my patch.
>>
>> Suggest this patch can check in to both 7.3 and trunk.
>
> As Pedro is probably not going to review it till 7.3 and the patch looks safe
> enough to me and having no impact on FSF gdbservers checked it in:
>        http://sourceware.org/ml/gdb-cvs/2011-07/msg00131.html
> 7.3:
>        http://sourceware.org/ml/gdb-cvs/2011-07/msg00132.html
>
>
> Thanks,
> Jan
>