From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32611 invoked by alias); 10 Aug 2007 14:41:04 -0000 Received: (qmail 32532 invoked by uid 22791); 10 Aug 2007 14:41:02 -0000 X-Spam-Check-By: sourceware.org Received: from heller.inter.net.il (HELO heller.inter.net.il) (213.8.233.23) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 10 Aug 2007 14:40:51 +0000 Received: from HOME-C4E4A596F7 (IGLD-80-230-204-18.inter.net.il [80.230.204.18]) by heller.inter.net.il (MOS 3.7.3a-GA) with ESMTP id DIO83652 (AUTH halo1); Fri, 10 Aug 2007 17:40:47 +0300 (IDT) Date: Fri, 10 Aug 2007 14:41:00 -0000 Message-Id: From: Eli Zaretskii To: teawater CC: gdb@sources.redhat.com In-reply-to: (message from teawater on Fri, 10 Aug 2007 17:31:51 +0800) Subject: Re: GDB record target 0.0.1 for GDB-6.6 release (It make GDB support Reversible Debugging) Reply-to: Eli Zaretskii References: X-IsSubscribed: yes Mailing-List: contact gdb-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-owner@sourceware.org X-SW-Source: 2007-08/txt/msg00096.txt.bz2 > Date: Fri, 10 Aug 2007 17:31:51 +0800 > From: teawater > > 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 Can we portably depend on stdint.h being available? I think we cannot. Can we do without it? > +#include Why do you need termios.h here? > +#include 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.