Re: [RFA] Reverse Debugging, 5/5

[Michael Snyder <msnyder at vmware dot com>]

Eli Zaretskii wrote:
> > Date: Wed, 01 Oct 2008 12:20:06 -0700
> > From: Michael Snyder <msnyder@vmware.com>
> > CC: Daniel Jacobowitz <drow@false.org>,   Pedro Alves <pedro@codesourcery.com>,  teawater <teawater@gmail.com>
> >
> > +static void
> > +set_exec_direction_func (char *args, int from_tty,
> > +                      struct cmd_list_element *cmd)
> > +{
> > +  if (target_get_execution_direction () != EXEC_ERROR)
> > +    {
> > +      enum exec_direction_kind dir = EXEC_ERROR;
> > +
> > +      if (!strcmp (exec_direction, exec_forward))
> > +     dir = EXEC_FORWARD;
> > +      else if (!strcmp (exec_direction, exec_reverse))
> > +     dir = EXEC_REVERSE;
> > +
> > +      if (target_set_execution_direction (dir) != EXEC_ERROR)
> > +     return;
> > +    }
> > +}
>
> I'm not sure I get your intent with the last two lines: if
> target_set_execution_direction returns anything but EXEC_ERROR, you
> return, but if not, you ... return?  What am I missing?


Well, this is the implementation of a "set" command.
If target_get_execution_direction returns EXEC_ERROR,
it means that the target doesn't support reverse, in
which case there's nothing to set, so I just return.

Otherwise, I pass the request to the target.

Possibly I should output something, like "target does
not support this comand", but since it's a "set" command,
it isn't expected to output anything and it doesn't
receive a struct ui_file among its arguments.

Originally I had it call error() in that case, but I
found that caused a problem in the testsuite, default.exp.
Apparently doing an "info set" causes every registered
"set" function to be called with no arguments?


> > +  case EXEC_ERROR:
> > +  default:
> > +    fprintf_filtered,  (out,
> > +                     _("Target `%s' does not support execution-direction."),
> > +                     target_shortname);
> > +    break;
>
> Why print an error message? isn't it better to say the direction is
> "forward" (which is documented as the default in your patch for the
> manual)?

Maybe.  I'm open to discussion.

The context is, the user says "show exec-direction"
with a target that doesn't support reverse.

Is it better to just say "Forward", with no comment,
or is it better to let the user know that the question
is not applicable?  Or both?

> > +  if (dir == EXEC_REVERSE)
> > +    error (_("Already in reverse mode.  Use '%s' or 'set exec-dir forward'."),
> > +        cmd);
>
> Isn't it better to silently do the equivalent of "cmd"?

Possibly.  Again, I'm open to discussion.  But I think
we've discussed this one before, and I hope we don't
get bogged down again...

Here the context is that the user has set a sticky
mode bit ("reverse"), but then has used one of the
non-sticky commands such as "reverse-step".

What should we do?  A reverse-reverse-step, ie. forward?
That's logical, but is it what the user wanted?

The problem is compounded by the fact that it won't
always be easy for the user to know whether we in fact
went forward or back.  If s/he realizes that we have
gone the 'wrong' way, s/he can correct it; but if not...

> > +  add_com ("reverse-step", class_run, reverse_step, _("\
> > +Step program backward until it reaches the beginning of another source line.\n\
> > +Argument N means do this N times (or till program stops for another reason).")
>
> This sounds as if you are single-stepping the program until it reaches
> the previous line (as opposed to running uninterrupted until you hit
> previous line).  Are you?

Of course I am single-stepping until it reaches the
previous line.  It's the same way we do forward stepping.

We don't and can't implement step by using a breakpoint,
because we don't know whether the source line contains
jumps or calls.  We only use the stepping-breakpoint
to get back after we have single-stepped thru a call
(or something similar).

I'm using the same step-range-start / step-range-end
logic that we use to step forward (with minor but
essential modifications).

> > +  add_com ("reverse-next", class_run, reverse_next, _("\
> > +Step program backward, proceeding through subroutine calls.\n\
>                         ^
> Won't commands like "apropos" stop at the first comma when they
> display the short descriptions of commands?

Yep, you're right!

I should omit the comma, huh?

> > +@node Reverse Execution
> > +@chapter Running programs backward
>
> Please add a @cindex entry here.

OK.

> > +program was executing normally.  Variables, registers etc. should
>
> Please put a @: after "etc.", to prevent the period being interpreted
> by TeX as an end of a sentence.

OK.

> >                                                    After executing
> > +a piece of source code in reverse, all side effects of that code
> > +should be ``undone''
>
> ALL side effects?  I thought some of them cannot be undone,
> un-outputting to the various I/O devices etc.

That depends on the implementation on the target side.
For instance, VMware's implementation can undo device I/O,
so long as the device is virtual, but gdb-freeplay cannot.

GDB has no way of knowing this, but perhaps I should say
something more explicit about it in the doc?

> > +Assuming you are debugging in a target environment that supports
>
> "IF you are debugging in a target ..." sounds better, I think.

Agreed.

> >                                                         Starting from
> > +the first line of a function, @code{reverse-next} will take you back
> > +to the caller of that function, @emph{before} the function was called.
>
> Shouldn't we have some kind of caveat here regarding function prologue
> and epilogue?

Like what?

If I've done my job right, prologues and epilogues
should be handled transparently, just like they are
when stepping forward.

And that seems to be reflected by the step-reverse.exp test.

> > +@item set exec-direction
>
> There should be a @kindex entry here for this command.

OK.

Thanks a lot for your comments, Eli.