GDB record patch 0.1.3.1 for GDB-6.8 release
Thiago Jung Bauermann
bauerman@br.ibm.com
Tue May 20 04:33:00 GMT 2008
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