This is the mail archive of the
gdb@sourceware.org
mailing list for the GDB project.
Re: GDB record target 0.0.1 for GDB-6.6 release (It make GDB support Reversible Debugging)
Thanks a lot to your comments. I will deal with it.
On 8/10/07, Eli Zaretskii <eliz@gnu.org> wrote:
> > Date: Fri, 10 Aug 2007 17:31:51 +0800
> > From: teawater <teawater@gmail.com>
> >
> > The attachment is a patch for the GDB-6.6 that will add two commands
> > ("record" and "reverse") and a new target "record" to the GDB-6.6.
> >
> > The command "record" can record running message such as the program pc
> > register value and some frame message to a record file that default
> > name is "now.rec".
> >
> > The target "record" can open this record file and debug the program.
> > And if the current target is the "record", you can use command
> > "reverse" set debug to the reverse debug mode. If you set GDB to the
> > reverse debug mode. The program will reverse run. Most of GDB command
> > such as "step", "next" and "breakpoint" can be use in this mode.
>
> I think this is a very good feature. Thanks!
>
> > Please give me your thought about the "record". Thanks a lot.
>
> I have some comments on the patch. (Btw, in the future, please send
> the patch in the body of the message as text, do not compress and
> attach it as a binary attachment, as that makes reviewing the patch
> less convenient.)
>
> > +/*teawater rec begin----------------------------------------------------------*/
> > [...]
> > +/*teawater rec end------------------------------------------------------------*/
>
> These markings have to go.
>
> > - error (_("Cannot find bounds of current function"));
> > + error (_("Cannot find bounds of current function1"));
>
> > - error (_("Cannot find bounds of current function"));
> > + error (_("Cannot find bounds of current function2"));
>
> These changes are superfluous and should not be part of your patch.
>
> > + if (stop_signal == TARGET_SIGNAL_TRAP) {
> > + if (gdb_is_reverse) {
> > + ecs->random_signal = 0;
> > + }
> > + else {
> > + ecs->random_signal
> > + = !(bpstat_explains_signal (stop_bpstat)
> > + || trap_expected
> > + || (step_range_end && step_resume_breakpoint == NULL));
> > + }
> > + }
>
> This is not the GNU style of formatting C blocks. Please use the
> style the rest of GDB uses. The indentation is also incorrect (GNU
> style uses 2 columns for each level, not 4).
>
> > +static __inline__ void __list_add(struct list_head * new,
>
> I don't think we can use __inline__ without catering to compilers that
> don't support it.
>
> > +#include <stdint.h>
>
> Can we portably depend on stdint.h being available? I think we
> cannot. Can we do without it?
>
> > +#include <termios.h>
>
> Why do you need termios.h here?
>
> > +#include <netinet/in.h>
>
> Is this really needed?
>
> > + //open file
> > + if (name) {
> > + printf_unfiltered ("Record the paogram running message to the file \"%s\".\n", name);
> > + record_fd = open (name, O_RDONLY);
> > + }
> > + else {
> > + printf_unfiltered ("Record the paogram running message to the file \"%s\".\n", record_DEF_FILE);
> > + record_fd = open (record_DEF_FILE, O_RDONLY);
> > + }
>
> You need to open the file with O_BINARY, because otherwise file I/O to
> the recording file will not work on MS-Windows.
>
> > + printf_unfiltered ("Record the paogram running message to the file \"%s\".\n", name);
> > [...]
> > + error ("Get size of file error.");
>
> Please make all your messages translatable, by enclosing them in
> `_()', like we do elsewhere in GDB.
>
> > + //mmap record_mem
> > + record_mem = mmap (0, record_mem_size, PROT_READ, MAP_FILE | MAP_SHARED, record_fd, 0);
> > + if (record_mem == (caddr_t)-1) {
> > + record_close (0);
> > + error ("Mmap file is error.");
> > + }
>
> Why do we need mmap here? it's not universally supported. Can't we
> just read() the file into memory?
>
> > + struct sigaction act, old_act;
> > +
> > + record_get_sig = 0;
> > + act.sa_handler = record_sig_handler;
> > + act.sa_mask = record_maskall;
> > + act.sa_flags = SA_RESTART;
> > + if (sigaction (SIGINT, &act, &old_act)) {
> > + perror_with_name ("sigaction");
> > + }
>
> I don't think we can portably use sigaction here.
>
> > + printf_unfiltered ("Record the paogram running message to the file \"%s\".\n", args);
> ^^^^^^^
> Please spell-check your comments and strings, there are quite a few
> misspellings and incorrect usage of English.
>
> > + fd = open (args, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>
> I think this needs O_BINARY.
>
> > + add_com ("record", class_obscure, record_to_file, _("Record registers valut to file."));
> > + add_com ("rec", class_obscure, record_to_file, _("Record registers valut to file."));
> > + add_com ("reverse", class_obscure, set_gdb_is_reverse, _("Set GDB to the reverse debug mode or the normal debug mode."));
> > + add_com ("rev", class_obscure, set_gdb_is_reverse, _("Set GDB to the reverse debug mode or the normal debug mode."));
> > +}
>
> These commands need to be described in the user manual. So, if your
> patch is accepted, please send a patch for the manual to accompany it.
>
> Thanks for working on this.
>