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

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