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)
- From: Eli Zaretskii <eliz at gnu dot org>
- To: teawater <teawater at gmail dot com>
- Cc: gdb at sources dot redhat dot com
- Date: Fri, 10 Aug 2007 17:40:47 +0300
- Subject: Re: GDB record target 0.0.1 for GDB-6.6 release (It make GDB support Reversible Debugging)
- References: <daef60380708100231n2a926967q2f55b9a2194b982d@mail.gmail.com>
- Reply-to: Eli Zaretskii <eliz at gnu dot org>
> 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.