This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Thanks Eli.

On Sat, Nov 8, 2008 at 00:00, Eli Zaretskii <eliz@gnu.org> wrote:
>> 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.

OK. I will let it 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.
>

OK. I will fix 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?

What about change it to "Do you want to auto delete first execute log
entry when record/replay buffer becomes full(record-stop-at-limit)?"

>
>> +               error (_("Process record: record stop the program."));
>
> Do you mean
>
>    Process record: program recording stopped.
>
> ?

What about "Process record stop inferior."


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

Yes, I will fix it.

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

What about "Process record execute log error"?

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

OK. I will.

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

I will fix it.

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

OK. I will try to fix all of them.

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

Could you tell me how to output this message clear?

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

Change it to "record execute log"?

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

What about change it to "destory the execute log"?

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

I will fix it.

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

I will remove them.

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

I think maybe we need let user know when delete some log entries.


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

I will fix it.

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

What about this:
_("When process record target running in replay mode,\n\
delete the next running messages and begin to record\n\
the execute log at current address.")




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

I will fix them.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]