This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]