Re: GDB record target 0.0.1 for GDB-6.6 release (It make GDB support Reversible Debugging)

[teawater <teawater at gmail dot com>]

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