From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14354 invoked by alias); 19 May 2008 21:20:25 -0000 Received: (qmail 14149 invoked by uid 22791); 19 May 2008 21:20:23 -0000 X-Spam-Check-By: sourceware.org Received: from igw3.br.ibm.com (HELO igw3.br.ibm.com) (32.104.18.26) by sourceware.org (qpsmtpd/0.31) with ESMTP; Mon, 19 May 2008 21:19:56 +0000 Received: from mailhub3.br.ibm.com (unknown [9.18.232.110]) by igw3.br.ibm.com (Postfix) with ESMTP id 8ACDD39021D for ; Mon, 19 May 2008 18:04:55 -0300 (BRST) Received: from d24av02.br.ibm.com (d24av02.br.ibm.com [9.18.232.47]) by mailhub3.br.ibm.com (8.13.8/8.13.8/NCO v8.7) with ESMTP id m4JLJrSr1491120 for ; Mon, 19 May 2008 18:19:53 -0300 Received: from d24av02.br.ibm.com (loopback [127.0.0.1]) by d24av02.br.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id m4JLJnoI008982 for ; Mon, 19 May 2008 18:19:49 -0300 Received: from [9.8.3.110] ([9.8.3.110]) by d24av02.br.ibm.com (8.12.11.20060308/8.12.11) with ESMTP id m4JLJnkG008956; Mon, 19 May 2008 18:19:49 -0300 Subject: Re: GDB record patch 0.1.3.1 for GDB-6.8 release From: Thiago Jung Bauermann To: Tea Cc: Michael Snyder , gdb-patches@sourceware.org In-Reply-To: References: Content-Type: text/plain Date: Tue, 20 May 2008 04:33:00 -0000 Message-Id: <1211231955.32587.23.camel@localhost.localdomain> Mime-Version: 1.0 X-Mailer: Evolution 2.22.1.1 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-05/txt/msg00581.txt.bz2 Hi teawater, On Wed, 2008-04-23 at 14:16 +0800, Tea wrote: > most part of GDB record patch 0.1.3.1 is same with GDB record patch > 0.1.3 (http://sourceware.org/ml/gdb-patches/2008-04/msg00300.html). I > just did some change according to your mail. I must express the > particularly grateful to you. Thanks for your changes. They made the patch easier to review, and put it a bit closer to being merged, IMHO. Here are my comments on the patch. I'm sorry I took so long to get to it, but it is a big patch, and it takes some time to understand it. :-) General comments: - I think it would be useful to put a limit on the amount of entries that are kept in the record list, to avoid having GDB use all of the machine's memory on a big program. A way to say "record the last 50k instructions" would be nice, IMHO. This would make it practical to use the functionality when debugging larger programs. But perhaps this should be left as an improvement for the future? - A question: if you record changes, then reverse GDB through them but stop it half-way, and then set the direction (gear? :-) ) back to forward, GDB will just play back the changes it has recorded, and not continue the inferior until it reaches the end of the record list, right? If so this means that if the user goes back a few insns, modifies a variable which is used in an if condition, the code will still appear to take the branch it took before. This is unintuitive and may lead the inferior to an undefined state, impossible to achieve by regular runs of the program. So I think GDB either needs to be put in a "read-only" mode where it refuses to change inferior memory and registers, or it needs to discard the records made after the point where the change in inferior state was made. What do you think? - Before the patch is committed, the user manual needs to be updated to explain how to use this feature, and also talk about its limitations. It would be better to extend the GDB Internals document as well, to provide an outline on how reversible debugging is implemented, and what needs to be done to make it support a new platform. IMHO this can wait until more discussion goes into the feature, to avoid having to change the documentation later. - Also, before the patch is committed it needs to be refreshed to apply against CVS HEAD and not 6.8. Also, you can choose to do this closer to when the patch is committed to avoid having to re-refresh it in case CVS HEAD changes too much. - Disclaimer: my knowledge of infrun.c and infcmd.c is very limited. I took only a cursory look at the changes there. I'll read the patch again another day, to see if I have anything else to add. Now, focusing on the patch: > i386-linux-tdep.c | 2533 +++++++++++++++++++++++++++++++++++++++++++++++++ > i386-tdep.c | 2769 +++++++++++++++++++++++++++++++++++++++++++++++++++--- This feature increases the size of these files a great deal. They are currently 459 lines and 2561 lines respectively (as of GDB 6.8). Perhaps creating separate files for platform-specific record code makes things more manageable (e.g., i386-linux-rec.c and i386-rec.c)? I guess that's a question to the GDB maintainers. > mips-tdep.c | 734 ++++++++++++++ OTOH, mips-tdep.c has originally 6124 lines and this patch adds relatively little to that. > --- a/gdb/gdbarch.c > +++ b/gdb/gdbarch.c > @@ -230,6 +230,8 @@ struct gdbarch > gdbarch_core_read_description_ftype *core_read_description; > gdbarch_static_transform_name_ftype *static_transform_name; > int sofun_address_maybe_missing; > + gdbarch_record_ftype *record; > + gdbarch_record_dasm_ftype *record_dasm; > }; > > > @@ -352,6 +354,8 @@ struct gdbarch startup_gdbarch = > 0, /* core_read_description */ > 0, /* static_transform_name */ > 0, /* sofun_address_maybe_missing */ > + NULL, > + NULL, > /* startup_gdbarch() */ > }; There should be comments in the lines above mentioning what those NULLs are for (record and record_dasm), like in the other elements of the structure. > --- a/gdb/i386-linux-tdep.c > +++ b/gdb/i386-linux-tdep.c > @@ -35,6 +35,9 @@ > #include "solib-svr4.h" > #include "symtab.h" > > +#include "record.h" > +#include Can we just include and not worry, now? I think so, but not sure. Also, record.h needs to be added to i386-linux-tdep.c's dependencies list in Makefile.in. Same applies to other files which include record.h. > @@ -335,6 +338,2533 @@ i386_linux_write_pc (struct regcache *re > restarted. */ > regcache_cooked_write_unsigned (regcache, I386_LINUX_ORIG_EAX_REGNUM, -1); > } > + > + > +/* These macros are the size of the type that will be use in system call. The values of > + these macros are gotten from Linux Kernel source. */ > +#define I386_RECORD_SIZE__old_kernel_stat 32 > +#define I386_RECORD_SIZE_tms 16 I wonder if there is a way to get these sizes by including the Linux kernel header files? The way it is done here looks very fragile and tied to a specific Linux kernel version to me... Granted, using kernel includes will still be fragile and version-specific, but at least to update GDB only a recompile is needed, as oposed to manually figuring out and editing these #defines. Since glibc has to have access to these structures anyway, I think it is possible. And this also means that at least some binary compatibility stability is to be expected, right? Same concerns regarding the other #defines following these. > + if (!need_dasm) > + { > + if (record_arch_list_add_reg (I386_EIP_REGNUM)) > + { > + return (-1); > + } > + } > + if (record_arch_list_add_end (need_dasm)) > + { > + return (-1); > + } Looking at i386_record, need_dasm is always zero so the variable can be removed, and the call to record_arch_list_add_reg above doens't need to be inside the if (!need_dasm). > + if (record_list && (record_list->next || gdb_is_reverse)) > + { > + discard_cleanups (old_cleanups); > + record_wait_step = step; > + return; > + } I think it's better to have a comment above the if condition explaining what it means. If I understood things correctly, if the condition is true then GDB is not really executing the inferior but merely playing back the recorded states stored by the record functionality, right? > @@ -1026,10 +1047,18 @@ wait_for_inferior (int treat_exec_as_sig > > while (1) > { > - if (deprecated_target_wait_hook) > - ecs->ptid = deprecated_target_wait_hook (ecs->waiton_ptid, ecs->wp); > + if (record_list && (record_list->next || gdb_is_reverse)) > + { > + ecs->ptid = record_wait (current_gdbarch, ecs->waiton_ptid, ecs->wp); > + } Same remark here, about having a comment explaining the if condition. > @@ -2004,10 +2033,19 @@ handle_inferior_event (struct execution_ > SPARC. */ > > if (stop_signal == TARGET_SIGNAL_TRAP) > - ecs->random_signal > - = !(bpstat_explains_signal (stop_bpstat) > - || stepping_over_breakpoint > - || (step_range_end && step_resume_breakpoint == NULL)); > + { > + if (gdb_is_reverse || gdb_is_recording) > + { > + ecs->random_signal = 0; > + } > + else > + { > + ecs->random_signal > + = !(bpstat_explains_signal (stop_bpstat) > + || stepping_over_breakpoint > + || (step_range_end && step_resume_breakpoint == NULL)); > + } > + } IMHO, it is clearer to just extend the expression for random_signal, rather than introduce a special case for reversible debugging. What about the following? - || (step_range_end && step_resume_breakpoint == NULL)); + || (step_range_end && step_resume_breakpoint == NULL) + || gdb_is_reverse + || gdb_is_recording); > --- a/gdb/Makefile.in > +++ b/gdb/Makefile.in In addition to the changes you made to Makefile.in, you need to add a record.o target listing the source and header files it depends on. See the other .o targets there for an example. > +static void > +record_list_release (record_t * rec) > +{ > + record_t *tmp; > + > + while (rec) > + { > + tmp = rec; > + rec = rec->prev; > + xfree (tmp); > + } > +} This function will leak memory. You also need to free rec->u.reg.val or rec->u.mem.val, depending on the type of the record. > + rec = (record_t *) xmalloc (sizeof (record_t)); > + if (!rec) > + { > + fprintf_unfiltered (gdb_stdlog, "record: alloc memory error.\n"); > + return (-1); > + } xmalloc will make GDB exit if memory is exhausted, so there's no need to check the return value. > + else if (record_list->type == record_mem) > + { > + /* mem */ > + gdb_byte mem[record_list->u.mem.len]; Variable length array is a C99 construct. Currently GDB code needs to restrict itself to at most C90 features, so you will need to explicitly allocate memory here. If there are other places where this syntax is used, they need to be changed as well. > + //registers_changed (); The commented out call should be removed. > +void > +record_close (void) > +{ > + gdb_is_recording = 0; > + gdb_is_reverse = 0; > + record_list_release (record_list); This function will leak memory if record_list doesn't point to the very end of the linked list. From what I understood of the code, record_close may be called when GDB is playing back the records, so record_list can be pointing at any element of the list. -- []'s Thiago Jung Bauermann Software Engineer IBM Linux Technology Center