From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3934 invoked by alias); 20 May 2008 04:33:11 -0000 Received: (qmail 3925 invoked by uid 22791); 20 May 2008 04:33:09 -0000 X-Spam-Check-By: sourceware.org Received: from ti-out-0910.google.com (HELO ti-out-0910.google.com) (209.85.142.184) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 20 May 2008 04:32:43 +0000 Received: by ti-out-0910.google.com with SMTP id d10so1077484tib.12 for ; Mon, 19 May 2008 21:32:40 -0700 (PDT) Received: by 10.110.7.18 with SMTP id 18mr1018082tig.39.1211257960021; Mon, 19 May 2008 21:32:40 -0700 (PDT) Received: by 10.110.105.20 with HTTP; Mon, 19 May 2008 21:32:39 -0700 (PDT) Message-ID: Date: Tue, 20 May 2008 15:33:00 -0000 From: Tea To: "Thiago Jung Bauermann" Subject: Re: GDB record patch 0.1.3.1 for GDB-6.8 release Cc: "Michael Snyder" , gdb-patches@sourceware.org In-Reply-To: <1211231955.32587.23.camel@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <1211231955.32587.23.camel@localhost.localdomain> 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/msg00590.txt.bz2 Hi Thiago, Thank your help. It is so great. I will modify record according to your mail. Thanks, teawater On Tue, May 20, 2008 at 5:19 AM, Thiago Jung Bauermann wrote: > 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 > >