[RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Michael Snyder]

Eli Zaretskii wrote:
>> Date: Fri, 16 Oct 2009 18:27:14 -0700
>> From: Michael Snyder <msnyder@vmware.com>
>>
>> If I need to resubmit the docs, I'll do that separately.
> 
> Yes, please.  It should document "record save" and "record restore".
> An entry in NEWS is also in order.

Sure.  I'll follow up with a fourth patch.

> What follows are mainly usability and UI related comments.
> 
>> +  if (record_debug)
>> +    printf_filtered (_("Restoring recording from core file.\n"));
> 
> Debug messages are not supposed to be marked for translation.  (You
> have several more of those in the patch.)

They're not?  OK -- I'll change them.   ;-)

>> +  printf_filtered ("Find precord section %s.\n",
>> +		   osec ? "succeeded" : "failed");
> 
> Is this also a debug printout?  If so, why isn't it conditioned by
> record_debug?  If it isn't, then why not marked for translation?  (I
> think this kind of message should definitely be printed only under
> record_debug.)

Right.

>> +  bfdcore_read (core_bfd, osec, &magic, sizeof (magic), &bfd_offset);
>> +  if (magic != RECORD_FILE_MAGIC)
>> +    error (_("Version mis-match or file format error."));
> 
> It would be nice to say what file this refers to.

OK.  Also adding file name to several other error messages.

>> +        default:
>> +          error (_("Format of core file is not right."));
> 
> Suggest something like "Incorrect or unsupported format of core file."
> "Not right" is too vague, IMO.

OK.

>> +  printf_filtered ("Restored records from core file.\n");
> 
> This should be marked for translation.

Check.

>> +  /* FIXME why doesn't this go in record_core_open?  */
>> +  if (strcmp (current_target.to_shortname, "record_core") == 0)
>> +    record_restore ();
> 
> Yes, why indeed?  Can this be resolved before the patch goes in?

Ah, thanks for poking me on that.  It's fixed now.

>> +     4 bytes: magic number htonl(0x20090829).
>                               ^^^^^
> Elsewhere in this documentation you use a more human-readable "network
> byte order".

Hmmm.  OK, yeah, that'll work.
Can get rid of an include file now, too.


>> +       NOTE: be sure to change whenever this file format changes!
>> +
>> +   Records:
>> +     record_end:
>> +       1 byte:  record type (record_end).
>                                 ^^^^^^^^^^
> This probably has some integer value, right?  Or does this indicate
> something other than an integer type?

It's an enum.  Why?  You think I should give its numeric
value in the comment, instead of its symbolic value?


>> +     record_reg:
>> +       1 byte:  record type (record_reg).
>> +       4 bytes: register id (network byte order).
>> +       n bytes: register value (n == register size).
>                                         ^^^^^^^^^^^^^
> How does one know what is the correct register size?

We get it from the current regcache and regnum.  What can I say about
it here, that wouldn't be arch-specific?  I mean, I could say:

    This will generally be 4 bytes for x86, 8 bytes for x86_64.

but that doesn't seem very useful or scalable.  Plus it will
be different for floating point regs, once we handle them too.



>> +    error (_("Failed to write %d bytes to core file ('%s').\n"),
>> +	   len, bfd_errmsg (bfd_get_error ()));
> 
> How about stating the name of the file?

Yep.

>> +  if (strcmp (current_target.to_shortname, "record") != 0)
>> +    error (_("Process record is not started.\n"));
> 
> Suggest to add to the message text something that tells the user how
> to remedy this situation.  E.g., "Invoke FOO command first."

OK.  It's a target-specific command, that can only be used
with target 'record'.  How about this?

   error (_("This command can only be used with target 'record'.\n"));

In general, I think we need a better idea about how to handle
commands that can only be used with a specific target or backend.

>> +      snprintf (recfilename_buffer, sizeof (recfilename_buffer),
>> +                "gdb_record.%d", PIDGET (inferior_ptid));
> 
> What will this do in go32, where the "PID" is always 42?  Does a
> target or an architecture have a way of overriding this default?

Dunno, currently the only OSABI that we work with is linux.
But you're right, if and when we add more, we'll need to
handle this better.

This is the same approach that is used by the "gcore" command.
How does "gcore" work with go32, if at all?

> 
>> +  if (record_debug)
>> +    printf_filtered (_("Saving recording to file '%s'\n"),
>> +		     recfilename);
> 
> Suggest to say "Saving execution log to core file '%s'\n".  I added
> "core" because you use that freely elsewhere, and the user should
> therefore know that the recorded data goes to a file formatted as a
> core file.

OK.

>> +  /* Need a cleanup that will close the file (FIXME: delete it?).  */

Hmmm, yeah.  Done.

>> +  if (osec == NULL)
>> +    error (_("Failed to create 'precord' section for corefile: %s"),
>> +           bfd_errmsg (bfd_get_error ()));
> 
> Again, adding the name of the file will go a long way towards making
> the intent clear to a user who has no clue in how precord works.

Done.

>> +  printf_filtered (_("Saved recfile %s.\n"), recfilename);
> 
> "recfile"?  What's that? ;-)

Right.

>> +Argument is filename.  File must be created with 'record dump'."),
>                                                      ^^^^^^^^^^^
> You mean, "record save", right?

Yes.  Thanks for all your suggestions, Eli.
Please see revised patch attached.


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: tmp9b.txt
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20091017/e936516e/attachment.txt>