Re: [RFA] Process record and replay, 3/10

[Eli Zaretskii <eliz@gnu.org>]

> Date: Thu, 6 Nov 2008 15:46:41 +0800
> From: teawater <teawater@gmail.com>
> 
> This patch add the process record and replay target.  This is the core
> part of process record and replay.

Thanks!

> +  if (target_read_memory (addr, rec->u.mem.val, len))
> +    {
> +      fprintf_unfiltered (gdb_stdlog,
> +			  "Process record: read memory addr = 0x%s len = %d error.\n",

This looks like a debugging printout, but it is not conditioned on
record_debug.

> +	  /* Ask user how to do */
                      ^^^^^^^^^
"what to do".  And please put a period at the end of the sentence, and
add one more space after it.

> +	      q = yquery (_("The record instruction number (record-insn-number) is equal to record-insn-number-max.  Do you want to close record/replay stop when record/replay buffer becomes full(record-stop-at-limit) then auto delete first record_t?"));

There's something wrong with this query.  First, why both "close" and
"stop"?  Also, what is "record_t"? a typo?

> +		  error (_("Process record: record stop the program."));

Do you mean

    Process record: program recording stopped.

?

> +  if (ret > 0)
> +    error (_("Process record pause the program."));
                                ^^^^^
Do you mean "paused"?

> +  if (ret < 0)
> +    error (_("Process record record message error."));

Do you mean something like "Process record error."?  That is, does
this happen when some error is encountered inside
gdbarch_process_record?

> +  if (non_stop)
> +    {
> +      error (_("Process record target can't debug inferior in non-stop mode (non-stop)."));
> +    }
> +  if (target_async_permitted)
> +    {
> +      error (_("Process record target can't debug the GNU/Linux inferior in asynchronous mode (linux-async)."));
> +    }

I think these limitations should also be mentioned in the manual.

> +  if (RECORD_IS_USED)
> +    {
> +      if (!nquery
> +	  (_("Process record target already running, do you want delete the old record log?")))
                                                            ^^^^^^^^^^^
"want to delete"

> +      if (sigaction (SIGINT, &act, &old_act))
> +	{
> +	  perror_with_name (_("Process record: sigaction"));
> +	}

Can we have a better error message here?  Something like

  Process record: sigaction failed

(There are more instances of this in the patch.)

> +	      if (target_read_memory
> +		  (record_list->u.mem.addr, mem, record_list->u.mem.len))
> +		{
> +		  error (_("Process record: read memory addr = 0x%s len = %d error."),

Here also, the error message should be more clearly phrased.

> +	  if (record_arch_list_add_reg (i))
> +	    {
> +	      record_list_release (record_arch_list_tail);
> +	      error (_("Process record: record message error."));

Same here.  (There are more like this one.)

> +		nquery (_
> +			("Becuse GDB is in replay mode, changing the value of a register will destroy the record from this point forward. Change all register?"));

"all registers", in plural.

Also, I'm not sure I would understand what you mean by ``destroy the
record''?  Are you saying that process recording will effectively stop
working from this point onward?

> +	      error (_("Process record cancel the operation."));
                                       ^^^^^^
"canceled", I think.

> +static void
> +cmd_record_delete (char *args, int from_tty)
> +{
> +  if (RECORD_IS_USED)
> +    {
> +      if (RECORD_IS_REPLAY)
> +	{
> +	  if (!from_tty || query (_("Process record: delete the log from this point forward and begin to record the running message at current PC?")))

I think I'd prefer if we lose the "Process record:" prefix in this
query.  After all, the user knows very well that she invoked a command
related to process recording and replay, so there's no need to remind
her that.

> +	  printf_unfiltered (_("Process record: already at end of record list.\n"));

Same here.

> +static void
> +cmd_record_stop (char *args, int from_tty)
> +{
> +  if (RECORD_IS_USED)
> +    {
> +      if (!record_list || !from_tty || query (_("Process record: delete recorded log and stop recording?")))

And here.

> +static void
> +set_record_insn_max_num (char *args, int from_tty, struct cmd_list_element *c)
> +{
> +  if (record_insn_num > record_insn_max_num && record_insn_max_num)
> +    {
> +      printf_unfiltered (_("Process record: record instructions number is bigger than record instructions max number.  Auto delete the first ones.\n"));

And here.  Also, we need a question mark at the end of the second
sentence.

> +  if (sigfillset (&record_maskall) == -1)
> +    {
> +      perror_with_name (_("Process record: sigfillset"));

Unclear error message.

> +  add_com ("delrecord", class_obscure, cmd_record_delete,
> +	   _("When process record target running in replay mode, delete the next running messages and begin to record the running message at current address."));

This doc string should be made shorter, to fit a single terminal line,
and the first line should not include comma characters, because
otherwise some help commands will not display anything beyond the
first comma.

> +  add_setshow_boolean_cmd ("record-stop-at-limit", no_class,
> +			    &record_stop_at_limit,
> +			    _("Set record/replay stop when record/replay buffer becomes full."),

"Set whether record/replay stops when ..."

> +When enable, if the record/replay buffer becomes full,\n\
        ^^^^^^
"enabled"

> +ask user how to do.\n\
            ^^^^^^^^^
"what to do"

> +When disable, if the record/replay buffer becomes full,\n\
        ^^^^^^^
"disabled"