From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28495 invoked by alias); 21 May 2008 08:54:42 -0000 Received: (qmail 28483 invoked by uid 22791); 21 May 2008 08:54:41 -0000 X-Spam-Check-By: sourceware.org Received: from ti-out-0910.google.com (HELO ti-out-0910.google.com) (209.85.142.187) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 21 May 2008 08:54:04 +0000 Received: by ti-out-0910.google.com with SMTP id d10so1362366tib.12 for ; Wed, 21 May 2008 01:54:02 -0700 (PDT) Received: by 10.110.26.20 with SMTP id 20mr1231820tiz.23.1211360041911; Wed, 21 May 2008 01:54:01 -0700 (PDT) Received: by 10.110.105.20 with HTTP; Wed, 21 May 2008 01:54:01 -0700 (PDT) Message-ID: Date: Wed, 21 May 2008 17:14: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/msg00631.txt.bz2 Hi Thiago, > > 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? > This idea is cool. I like it. I will try to add it to record. Thanks. :-) > - 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? > Yes, it's 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? > I think this is a bug of record. I will fix it. > - 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. > Doc is always a big hard job for programer. :-) > - 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. > OK. > >> 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. "a question to the GDB maintainers." I like this idea. :-D > >> mips-tdep.c | 734 ++++++++++++++ > > OTOH, mips-tdep.c has originally 6124 lines and this patch adds > relatively little to that. This is because MIPS instructions is more simple than I386 instructions. Deal with i386 instructions is not very easy. Sometime I hate CISC arch. :-) > >> --- 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. OK. > >> --- 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. OK. > >> + 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). > OK. I will remove it. >> + 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? > Yes, you are right. And I will add comment there. >> @@ -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. OK. > >> @@ -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); > It is cool. I will use it. Thanks. :-) >> --- 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. > OK. I will deal with it. >> +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. > I will fix it. >> + 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. > OK. I will remove it. >> + 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. > I will change it to alloc. >> + //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. > I will fix it. > -- > []'s > Thiago Jung Bauermann > Software Engineer > IBM Linux Technology Center > > Thanks for your mail. It really help me a lot. I will deal with most of them in record 0.1.4 that will release in this or next week. For some big job such as read-only mode and limit instruction number, I will deal with them in record 0.1.5. How do you think about it. Thanks, teawater