This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: GDB record patch 0.1.3.1 for GDB-6.8 release
- From: Tea <teawater at gmail dot com>
- To: "Thiago Jung Bauermann" <bauerman at br dot ibm dot com>
- Cc: "Michael Snyder" <msnyder at specifix dot com>, gdb-patches at sourceware dot org
- Date: Tue, 20 May 2008 12:32:39 +0800
- Subject: Re: GDB record patch 0.1.3.1 for GDB-6.8 release
- References: <daef60380804222316j1b2f10afm77e01ac2618a86ba@mail.gmail.com> <1211231955.32587.23.camel@localhost.localdomain>
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
<bauerman@br.ibm.com> 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 <stdint.h>
>
> Can we just include <stdint.h> 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
>
>