Eli Zaretskii wrote: >> Date: Sat, 17 Oct 2009 11:36:35 -0700 >> From: Michael Snyder >> CC: "gdb-patches@sourceware.org" , >> "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. OK, see revised patch. >>>> + 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. OK. > >> 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? That's what version 1 of the file format did. I found it wasteful, hence this change. > >>>> + 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? OK. > >> 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. Well, save/restore depends on core files, so I guess it won't work in go32. >> 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)"? Ah. Yes -- you're looking at the version 1 header, but I do need to change the comment for the version 2 header. See revised patch below.