Re: [RFA] Reverse Debugging, 5/5

[Eli Zaretskii <eliz@gnu.org>]

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

> +  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)?

> +  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"?

> +  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?

> +  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?

> +@node Reverse Execution
> +@chapter Running programs backward

Please add a @cindex entry here.

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

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

> +Assuming you are debugging in a target environment that supports

"IF you are debugging in a target ..." sounds better, I think.

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

> +@item set exec-direction

There should be a @kindex entry here for this command.

Thanks!