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

[Eli Zaretskii <eliz@gnu.org>]

> Date: Sat, 17 Oct 2009 11:36:35 -0700
> From: Michael Snyder <msnyder@vmware.com>
> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>, 
>  "teawater@gmail.com" <teawater@gmail.com>
> 
> >> +       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?

Either the numeric value or a more explicit reference to the enum
(e.g., say that it's an enum defined on such-and-such file).

IOW, imagine that someone reads these comments because they want to
write a utility that reads these files, and ask yourself what would
they be missing in this text.

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

Well, saying it's the exact size of the register being recorded works
for me.

> I mean, I could say:
> 
>     This will generally be 4 bytes for x86, 8 bytes for x86_64.

That's good as an example.

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

Would it make sense to use here some size that can accommodate all the
known registers, to make the file format truly architecture
independent?  Or would it be more trouble than it's worth?

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

That's good, but maybe adding "Use 'record start' first." will be
better?

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

It doesn't.  DJGPP cannot generate core files.

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

It's fine, except for this:

> +   Header:
> +     4 bytes: magic number htonl(0x20090829).

I thought you agreed to replace this with "0x20090829 (network byte
order)"?