GDB record patch 0.1.3.1 for GDB-6.8 release

Tea teawater@gmail.com
Tue May 20 15:33:00 GMT 2008


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
>
>



More information about the Gdb-patches mailing list