Testing of reverse debug commands

Testing of reverse debug commands

[Michael Snyder]

Hello all,

If you were to take a look at the expect scripts for any of the
tests I've submitted for testing reverse debugging, you would
notice a couple of FIXME comments in there, eg.:

     gdb_test "record" "" "Turn on process record"
     # FIXME: command ought to acknowledge, so we can test if it succeeded.

     gdb_test "set exec-dir reverse" "" "set reverse execution"
     # FIXME: command needs to acknowledge, so we can test if it succeeded.

This is my concern.  These commands don't produce any output,
so we can't directly test whether they succeeded or not.

So I'm asking for input.  Do you think we should make these
commands generate a little output, so we know they worked?

Or do we just depend on the following tests to fail if they didn't?

Cheers,
Michael

Re: Testing of reverse debug commands

[Joel Brobecker]

>     gdb_test "record" "" "Turn on process record"
>     # FIXME: command ought to acknowledge, so we can test if it succeeded.

This is just a shot in the dark since I really don't have much time
to double-check this, but does gdb_test_multiple allow you to verify
that no output was generated? For some reason, I thought it did.

-- 
Joel

Re: Testing of reverse debug commands

[Pedro Alves]

On Sunday 12 July 2009 09:01:45, Joel Brobecker wrote:
> >     gdb_test "record" "" "Turn on process record"
> >     # FIXME: command ought to acknowledge, so we can test if it succeeded.
> 
> This is just a shot in the dark since I really don't have much time
> to double-check this, but does gdb_test_multiple allow you to verify
> that no output was generated? For some reason, I thought it did.

You should be able to see the precord target in the target stack
with "maint print target-stack".  You could also use it to plug
that FIXME, but, it's a bit ugly since it exposes precord's
implementation details.

Can't the user ask GDB if process record is in effect (at any time)?  This
seems like something a user would want to do --- "hmmm, I forget, am
I recording now?".  I can think of other interesting things that would be
nice to be able to query GDB, e.g.: "what is the status of the
recording buffers?" --- maybe there should be a "record status" command
or something like that.  Is there one already?

Another hack would be to send GDB another "record" and see if this query
shows up:

record_open ()

  /* 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;
    }

That's also hackish, but the good thing is that you end up adding a test
for a code path that isn't tested currently.  :-)

-- 
Pedro Alves

Re: Testing of reverse debug commands

[Jan Kratochvil]

On Sun, 12 Jul 2009 10:01:45 +0200, Joel Brobecker wrote:
> >     gdb_test "record" "" "Turn on process record"
> >     # FIXME: command ought to acknowledge, so we can test if it succeeded.
> 
> This is just a shot in the dark since I really don't have much time
> to double-check this, but does gdb_test_multiple allow you to verify
> that no output was generated? For some reason, I thought it did.

This one works but not sure if it cannot have some problems:

set cmd "set verbose 0"
gdb_test $cmd "[string_to_regexp $cmd]"


Regards,
Jan

Re: Testing of reverse debug commands

[Michael Snyder]

Jan Kratochvil wrote:
> On Sun, 12 Jul 2009 10:01:45 +0200, Joel Brobecker wrote:
>>>     gdb_test "record" "" "Turn on process record"
>>>     # FIXME: command ought to acknowledge, so we can test if it succeeded.
>> This is just a shot in the dark since I really don't have much time
>> to double-check this, but does gdb_test_multiple allow you to verify
>> that no output was generated? For some reason, I thought it did.
> 
> This one works but not sure if it cannot have some problems:
> 
> set cmd "set verbose 0"
> gdb_test $cmd "[string_to_regexp $cmd]"

Hmm, ok, three people so far have responded with
work-arounds (thanks).

Does that mean y'all lean toward NOT making the
commands generate some output of their own?

Now would be the time to decide, before the first official release...

Re: Testing of reverse debug commands

[Joel Brobecker]

> Does that mean y'all lean toward NOT making the
> commands generate some output of their own?

Generally speaking, I don't think that we should produce some output
only if it is useful, not just for the sake of testing. I have no
issue if you think a confirmation message would be useful.

-- 
Joel

Re: Testing of reverse debug commands

[Pedro Alves]

On Sunday 12 July 2009 18:14:36, Michael Snyder wrote:
> Jan Kratochvil wrote:
> > On Sun, 12 Jul 2009 10:01:45 +0200, Joel Brobecker wrote:
> >>>     gdb_test "record" "" "Turn on process record"
> >>>     # FIXME: command ought to acknowledge, so we can test if it succeeded.
> >> This is just a shot in the dark since I really don't have much time
> >> to double-check this, but does gdb_test_multiple allow you to verify
> >> that no output was generated? For some reason, I thought it did.
> > 
> > This one works but not sure if it cannot have some problems:
> > 
> > set cmd "set verbose 0"
> > gdb_test $cmd "[string_to_regexp $cmd]"
> 
> Hmm, ok, three people so far have responded with
> work-arounds (thanks).
> 
> Does that mean y'all lean toward NOT making the
> commands generate some output of their own?

We have many other commands that are silent on success, and we
still test them.  The important question is: would *users* find
some output useful, not if the testsuite would.  In this case, I've
no real opinion.

Assuming no extra verbosity, if you expect that *only* the gdb
prompt is output, then you know that you had success.  The issue
with that hunk you pasted above, is that gdb_test "" eats any and
all output before the prompt, so you can't use it, because
you'd eat errors as well.  Well, as they say: then don't do it.

Do you agree with the point I raised about not being able to
query the status of recording at any time?  If we had such a
way, I'd suggest checking if recording was in fact enabled
with it, just like:

 http://sourceware.org/ml/gdb-patches/2009-06/msg00012.html

-- 
Pedro Alves

RE: Testing of reverse debug commands

[Marc Khouzam]

> -----Original Message-----
> From: gdb-owner@sourceware.org 
> [mailto:gdb-owner@sourceware.org] On Behalf Of Michael Snyder
> Sent: July-12-09 1:15 PM
> To: Jan Kratochvil
> Cc: Joel Brobecker; gdb@sourceware.org; Hui Zhu
> Subject: Re: Testing of reverse debug commands
> 
> Jan Kratochvil wrote:
> > On Sun, 12 Jul 2009 10:01:45 +0200, Joel Brobecker wrote:
> >>>     gdb_test "record" "" "Turn on process record"
> >>>     # FIXME: command ought to acknowledge, so we can test 
> if it succeeded.
> >> This is just a shot in the dark since I really don't have much time
> >> to double-check this, but does gdb_test_multiple allow you 
> to verify
> >> that no output was generated? For some reason, I thought it did.
> > 
> > This one works but not sure if it cannot have some problems:
> > 
> > set cmd "set verbose 0"
> > gdb_test $cmd "[string_to_regexp $cmd]"
> 
> Hmm, ok, three people so far have responded with
> work-arounds (thanks).
> 
> Does that mean y'all lean toward NOT making the
> commands generate some output of their own?
> 
> Now would be the time to decide, before the first official release...

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.
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.

(gdb) 
record stop
&"record stop\n"
~"Process record is not started.\n"
^done
(gdb) 
-interpreter-exec console record stop
^done
(gdb) 

So, I think some improvement would be nice for frontends.

There is also a need to handle the default answers of queries
better for a frontend.  They are almost entirely used by PRecord.
I've been meaning to write a patch about that.  I'll try to 
get to it soon unless someone else gets to it first :-)

Marc

Re: Testing of reverse debug commands

[Pedro Alves]

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.

> 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.

-- 
Pedro Alves

RE: Testing of reverse debug commands

[Marc Khouzam]

> -----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.

Thanks

Marc