Re: [RFA] Submit process record and replay fourth time, 0/8

Pedro Alves
2009-04-02T14:26:50Z

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