From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15955 invoked by alias); 11 Aug 2007 02:15:06 -0000 Received: (qmail 15622 invoked by uid 22791); 11 Aug 2007 02:15:03 -0000 X-Spam-Check-By: sourceware.org Received: from rv-out-0910.google.com (HELO rv-out-0910.google.com) (209.85.198.187) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sat, 11 Aug 2007 02:14:58 +0000 Received: by rv-out-0910.google.com with SMTP id l15so738467rvb for ; Fri, 10 Aug 2007 19:14:57 -0700 (PDT) Received: by 10.143.11.13 with SMTP id o13mr157222wfi.1186798497055; Fri, 10 Aug 2007 19:14:57 -0700 (PDT) Received: by 10.142.72.10 with HTTP; Fri, 10 Aug 2007 19:14:57 -0700 (PDT) Message-ID: Date: Sat, 11 Aug 2007 02:15:00 -0000 From: teawater To: gdb@sources.redhat.com Subject: Re: GDB record target 0.0.1 for GDB-6.6 release (It make GDB support Reversible Debugging) In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline 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/msg00103.txt.bz2 Thanks a lot to your comments. I will deal with it. On 8/10/07, Eli Zaretskii wrote: > > 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. >