Re: Testing of reverse debug commands

Re: Testing of reverse debug commands

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 3877 bytes --]

On Mon, Jul 13, 2009 at 03:25, Marc Khouzam<marc.khouzam@ericsson.com> wrote:
>> -----Original Message-----
>> From: Pedro Alves [mailto:pedro@codesourcery.com]
>> Sent: July-12-09 3:08 PM
>> To: gdb@sourceware.org
>> Cc: Marc Khouzam; Michael Snyder; Jan Kratochvil; Joel
>> Brobecker; Hui Zhu
>> Subject: Re: Testing of reverse debug commands
>>
>> On Sunday 12 July 2009 19:49:58, Marc Khouzam wrote:
>>
>> > From a frontend perspective, the current 'record' command
>> is not very
>> > good.
>>
>> > First, there is no MI equivalent, although that is not a big deal.
>> > But since it does not report error, the frontend must always assumes
>> > that
>> > the command worked.
>>
>> Are you sure that is the case with "record"?  I see `error' calls
>> in record.c:record_open.  If there are some missing, let's add them.
>
> As usual you are right. :-)
>
> (gdb) record
> &"record\n"
> &"Process record target can't debug inferior in asynchronous mode
> (target-async).\n"
> ^error,msg="Process record target can't debug inferior in asynchronous
> mode (target-async)."
> (gdb)
>
> I had made the assumption that 'record stop' was the same as 'record'.
>
>> > Below you can see that using 'record stop' directly will
>> give a ^done
>> > instead of an ^error when it fails (although there is an
>> error message
>> > but we don't parse those in Eclipse).  Also, using -interpreter-exec
>> > is even worse as even the error message is gone.
>>
>> Note that "record stop" is really a different command, although
>> it shares a common "record" prefix.
>>
>> >
>> > (gdb)
>> > record stop
>> > &"record stop\n"
>> > ~"Process record is not started.\n"
>> > ^done
>> > (gdb)
>>
>> > So, I think some improvement would be nice for frontends.
>>
>> So, is this really an error?  Hui seems to have thought
>> it wasn't.  Hui?  If it is, then it's just a matter of
>> changing the corresponding printf_unfiltered calls in
>> record.c to `error' calls (look for the "Process record
>> is..." string).
>> Then MI will get an ^error,msg="foo", instead of a ~"foo" + ^done.
>
> That would be more consistent for a frontend.  The frontend
> can then decide if this should be reported as an error or simply
> accepted.  But that is not such a big deal anymore, now that
> you pointed out 'record' itself does report an error.
>

I think the record's query and something is make a lot of troubles.
I make a patch for it.  Please help me with it.

Thanks,
Hui

2009-07-13  Hui Zhu  <teawater@gmail.com>
	* record.c (record_open): Change "query" to "error".
	(cmd_record_stop): Change "query" to "printf_unfiltered".
---
 record.c |   14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

--- a/record.c
+++ b/record.c
@@ -436,12 +436,8 @@ record_open (char *name, int from_tty)

   /* Check if record target is already running.  */
   if (current_target.to_stratum == record_stratum)
-    {
-      if (!nquery
-	  (_("Process record target already running, do you want to delete "
-	     "the old record log?")))
-	return;
-    }
+    error (_("Process record target already running.  Use \"record stop\" to "
+	     "stop record target first."));

   /*Reset the beneath function pointers.  */
   record_beneath_to_resume = NULL;
@@ -1157,9 +1153,9 @@ cmd_record_stop (char *args, int from_tt
 {
   if (current_target.to_stratum == record_stratum)
     {
-      if (!record_list || !from_tty || query (_("Delete recorded log and "
-	                                        "stop recording?")))
-	unpush_target (&record_ops);
+      unpush_target (&record_ops);
+      printf_unfiltered (_("Process record is stoped and all execution "
+                           "log is deleted.\n"));
     }
   else
     printf_unfiltered (_("Process record is not started.\n"));

[-- Attachment #2: prec_remove_query.txt --]
[-- Type: text/plain, Size: 1166 bytes --]

---
 record.c |   14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

--- a/record.c
+++ b/record.c
@@ -436,12 +436,8 @@ record_open (char *name, int from_tty)
 
   /* Check if record target is already running.  */
   if (current_target.to_stratum == record_stratum)
-    {
-      if (!nquery
-	  (_("Process record target already running, do you want to delete "
-	     "the old record log?")))
-	return;
-    }
+    error (_("Process record target already running.  Use \"record stop\" to "
+	     "stop record target first."));
 
   /*Reset the beneath function pointers.  */
   record_beneath_to_resume = NULL;
@@ -1157,9 +1153,9 @@ cmd_record_stop (char *args, int from_tt
 {
   if (current_target.to_stratum == record_stratum)
     {
-      if (!record_list || !from_tty || query (_("Delete recorded log and "
-	                                        "stop recording?")))
-	unpush_target (&record_ops);
+      unpush_target (&record_ops);
+      printf_unfiltered (_("Process record is stoped and all execution "
+                           "log is deleted.\n"));
     }
   else
     printf_unfiltered (_("Process record is not started.\n"));

Re: Testing of reverse debug commands

[Pedro Alves]

On Monday 13 July 2009 04:31:37, Hui Zhu wrote:
> On Mon, Jul 13, 2009 at 03:25, Marc Khouzam<marc.khouzam@ericsson.com> wrote:
> >> Pedro Alves wrote:
> >> >
> >> > (gdb)
> >> > record stop
> >> > &"record stop\n"
> >> > ~"Process record is not started.\n"
> >> > ^done
> >> > (gdb)
> >>
> >> > So, I think some improvement would be nice for frontends.
> >>
> >> So, is this really an error?  Hui seems to have thought
> >> it wasn't.  Hui?  If it is, then it's just a matter of
> >> changing the corresponding printf_unfiltered calls in
> >> record.c to `error' calls (look for the "Process record
> >> is..." string).
> >> Then MI will get an ^error,msg="foo", instead of a ~"foo" + ^done.
> >
> > That would be more consistent for a frontend.  The frontend
> > can then decide if this should be reported as an error or simply
> > accepted.  But that is not such a big deal anymore, now that
> > you pointed out 'record' itself does report an error.
> >
> 
> I think the record's query and something is make a lot of troubles.
> I make a patch for it.  Please help me with it.

I got confused, since this isn't answering the question I asked.

This particular issue will be resolved when the query/nquery/yquery/MI
discussion reaches a conclusion, yes?  Or is this an independent change?
Do note that we have other CLI commands that query and default to
a "destructive" 'yes', like "run -> attach (kill?)", for example.
Maybe you should post this in its own new thread.

I think it would be nice if we crafted a GDB HIG.  Do we have
something of the sorts already?

-- 
Pedro Alves

Re: Testing of reverse debug commands

[Hui Zhu]

On Mon, Jul 20, 2009 at 21:16, Pedro Alves<pedro@codesourcery.com> wrote:
> On Monday 13 July 2009 04:31:37, Hui Zhu wrote:
>> On Mon, Jul 13, 2009 at 03:25, Marc Khouzam<marc.khouzam@ericsson.com> wrote:
>> >> Pedro Alves wrote:
>> >> >
>> >> > (gdb)
>> >> > record stop
>> >> > &"record stop\n"
>> >> > ~"Process record is not started.\n"
>> >> > ^done
>> >> > (gdb)
>> >>
>> >> > So, I think some improvement would be nice for frontends.
>> >>
>> >> So, is this really an error?  Hui seems to have thought
>> >> it wasn't.  Hui?  If it is, then it's just a matter of
>> >> changing the corresponding printf_unfiltered calls in
>> >> record.c to `error' calls (look for the "Process record
>> >> is..." string).
>> >> Then MI will get an ^error,msg="foo", instead of a ~"foo" + ^done.
>> >
>> > That would be more consistent for a frontend.  The frontend
>> > can then decide if this should be reported as an error or simply
>> > accepted.  But that is not such a big deal anymore, now that
>> > you pointed out 'record' itself does report an error.
>> >
>>
>> I think the record's query and something is make a lot of troubles.
>> I make a patch for it.  Please help me with it.
>
> I got confused, since this isn't answering the question I asked.
>
> This particular issue will be resolved when the query/nquery/yquery/MI
> discussion reaches a conclusion, yes?  Or is this an independent change?
> Do note that we have other CLI commands that query and default to
> a "destructive" 'yes', like "run -> attach (kill?)", for example.
> Maybe you should post this in its own new thread.
>
> I think it would be nice if we crafted a GDB HIG.  Do we have
> something of the sorts already?
>

Actually,  I got confused too.  Sorry for that.  This is a long ... thread.
Somebody told about query,  somebody told about error.

I send a patch to show my idea with it.

Maybe some people can help me with it.  :)

Thanks,
Hui

Re: Testing of reverse debug commands

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> I think it would be nice if we crafted a GDB HIG.  Do we have
Pedro> something of the sorts already?

Not that I know of.

Tom