This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] Submit process record and replay fourth time, 0/8
- From: Pedro Alves <pedro at codesourcery dot com>
- To: gdb-patches at sourceware dot org
- Cc: Hui Zhu <teawater at gmail dot com>, Marc Khouzam <marc dot khouzam at ericsson dot com>, Michael Snyder <msnyder at vmware dot com>, Thiago Jung Bauermann <bauerman at br dot ibm dot com>, Eli Zaretskii <eliz at gnu dot org>, paawan1982 at yahoo dot com
- Date: Thu, 2 Apr 2009 15:26:52 +0100
- Subject: Re: [RFA] Submit process record and replay fourth time, 0/8
- References: <daef60380903210858l5e7868d6g7ce240459c00bae3@mail.gmail.com> <daef60380903250023h3bfd7068gef837333443c09a4@mail.gmail.com>
On Wednesday 25 March 2009 07:23:45, Hui Zhu wrote:
> Some update.
Thanks. I'm not sure I'm looking at the most up-to-date version.
Here's a few review comments.
* 1-gdbarch.txt
This one's OK.
* 2-target_record_stratum.txt
This one's OK.
* 3-record_target.txt
> +record_list_release (record_t * rec)
^
drop the space between '*' and rec, here and elsewhere.
Is record_t supposed to be an opaque type? Throughout
gdb, we tend to only "_t" struct typedef types if they're
going to be value-like, passed by type, and opaque.
Otherwise, we just use something like:
record_list_release (struct record_entry *rec)
that is, no typedef.
> +/* Add a record_end type record_t to record_arch_list. */
> +int
> +record_arch_list_add_end (void)
Add a blank line between comment and function.
> + q = yquery (_("Do you want to auto delete previous execution "
> + "log entries when record/replay buffer becomes "
> + "full (record-stop-at-limit)?"));
What do you mean here by "when"? Didn't the user just hit
the limit *now*, and that is why you're asking what to do?
> +/* Before inferior step (when GDB record the running message, inferior
> + only can step), GDB will call this function to record the values to
> + record_list. This function will call gdbarch_process_record to
> + record the running message of inferior and set them to
> + record_arch_list, and add it to record_list. */
> +
(...)
> +
> +static int
> +record_message (void *args)
> +{
> + int ret;
> + struct gdbarch *gdbarch = args;
> + struct cleanup *old_cleanups = make_cleanup (record_message_cleanups, 0);
> +
> + record_arch_list_head = NULL;
> + record_arch_list_tail = NULL;
> +
> + /* Check record_insn_num. */
> + record_check_insn_num (1);
> +
> + record_regcache = get_current_regcache ();
- When I read the first time, my question is: 'What is a "record message"'?
- ARGS is a `struct gdbarch', yet you access the current regcache.
I see you pass current_gdbarch to do_record_message. We're trying
to eliminate the current_gdbarch global.
> + if (do_record_message (current_gdbarch))
You can get at the correct gdbarch with get_regcache_arch.
While we're there, what is the point of the record_regcache global?
We *are* trying to move away from global state, but I don't see how
this new global is any better than accessing get_current_regcache
directly. The alternative would be to pass the regcache as parameter
instead.
> +static void
> +record_sig_handler (int signo)
> +{
> + if (record_debug)
> + fprintf_unfiltered (gdb_stdlog, "Process record: get a signal\n");
> +
> + /* It will break the running inferior in replay mode. */
> + record_resume_step = 1;
I still don't understand this comment. Please explain *why* you
need to set this here.
> +
> + /* It will let record_wait set inferior status to get the signal
> + SIGINT. */
> + record_get_sig = 1;
> +}
> +struct cleanup *
> +record_gdb_operation_disable_set (void)
> +{
> + struct cleanup *old_cleanups = NULL;
> +
> + if (!record_gdb_operation_disable)
> + {
> + old_cleanups =
> + make_cleanup_restore_integer (&record_gdb_operation_disable);
> + record_gdb_operation_disable = 1;
> + }
> +
> + return old_cleanups;
> +}
This returns a NULL cleanup if record_gdb_operation_disable is
already set, but make_cleanup also returns a legitimate
NULL, and it is not an error. It returns NULL when for the
first cleanup put in the chain. See make_my_cleanup2.
You could make use of "make_cleanup (null_cleanup, NULL)", but,
simply:
struct cleanup *
record_gdb_operation_disable_set (void)
{
struct cleanup *old_cleanups = NULL;
old_cleanups =
make_cleanup_restore_integer (&record_gdb_operation_disable);
record_gdb_operation_disable = 1;
return old_cleanups;
}
... should do. make_cleanup_restore_integer restores the
previous value, so nested calls are safe.
> +static int
> +record_insert_breakpoint (struct bp_target_info *bp_tgt)
> +{
> + if (!RECORD_IS_REPLAY)
> + {
> + struct cleanup *old_cleanups = record_gdb_operation_disable_set ();
> + int ret = record_beneath_to_insert_breakpoint (bp_tgt);
> +
> + if (old_cleanups)
> + do_cleanups (old_cleanups);
So here, this cleanup handling is wrong. Don't check for NULL
old_cleanups. Call do_cleanups unconditionally. Here and
and elsewhere you used this idiom.
> +static void
> +record_wait_cleanups (void *ignore)
> +{
> + if (execution_direction == EXEC_REVERSE)
> + {
> + if (record_list->next)
> + record_list = record_list->next;
> + }
> + else
> + record_list = record_list->prev;
> +}
> +static ptid_t
> +record_wait (struct target_ops *ops,
> + ptid_t ptid, struct target_waitstatus *status)
(...)
> +struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
This cleanup looks suspiciously broken to me. It appears that
is will do weird things depending on when an exception is thrown.
But then again, record_wait's nesting and use of goto's makes
my eyes bleed. :P
Please delete this comment:
> + /* XXX: I try to use some simple commands such as "disconnect" and
> + "detach" to support this functions. But these commands all have
> + other affect to GDB such as call function "no_shared_libraries".
> + So I add special commands to GDB. */
I was looking at the commands record, delrecord, stoprecord,
record-stop-at-limit, record-insn-number-max, record-insn-number,
and wondering what would you think if we made all record commands
under a "record" prefix?
record
record stop
record delete
set record stop-at-limit
set record insn-number-max
info record insn-number
* 4-linux-record.txt
> + Copyright (C) 2008 Free Software Foundation, Inc.
2009.
> +#include <stdint.h>
Remove this. defs.h includes stdint.h.
+ uint32_t addr, count;
+ regcache_raw_read (record_regcache, tdep->arg2, (gdb_byte *) & addr);
This (and many more similar instances) is assuming
host-endianness == target-endianess, that the registers are 32-bit, and
probably that signed<->unsigned bit representation is equal. Is
this what you want? When you get to 64-bit, most of this will break,
it seems.
* 5-infrun.txt
infun.c:
> + #include "record.h"
> + && current_target.to_stratum != record_stratum);
Sigh, I've spent to much time trying to explain why this was
wrong. Let's at least leave this behind the macro you used to
have.
--
Pedro Alves