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


Hi Pedro,

The attachment is the compress for all the patches of precord.
And I will post each patch to maillist too.

Please help me review it.


The following is what I updated according to your comment:
>
>> +record_list_release (record_t * rec)
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?^
>
> drop the space between '*' and rec, here and elsewhere.

It was fixed in 3.

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

record_t is changed to record_entry in 3.

>
> ?Add a blank line between comment and function.
>
I fixed it in all the patches of p record.

>> +  record_regcache = get_current_regcache ();
>

>
>  - 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))
record_regcache was removed.  Now, regcache is the argument of a lot
of functions of precord.


> 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.  */
It was deleted.

> * 4-linux-record.txt
>
>> +   Copyright (C) 2008 Free Software Foundation, Inc.
>
>     2009.
Done.

>> +#include <stdint.h>
> Remove this.  defs.h includes stdint.h.
Done.

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

> * 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.
OK.  I make it to a macro.





The following is my explain for your comment:
>
>> + ? ? ? ? ? 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?
>

Because if user said yes, it will auto delete each time.  It will not
ask user again.  So I say when.

>  - When I read the first time, my question is: 'What is a "record message"'?
GDB record the running message.

>> +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.
>
		{
		  /* In EXEC_REVERSE mode, this is the record_end of prev
		     instruction.
		     In EXEC_FORWARD mode, this is the record_end of current
		     instruction.  */
		  /* step */
		  if (record_resume_step)
		    {
		      if (record_debug > 1)
			fprintf_unfiltered (gdb_stdlog,
					    "Process record: step.\n");
		      continue_flag = 0;
		    }

		  /* check breakpoint */
		  tmp_pc = regcache_read_pc (regcache);
		  if (breakpoint_inserted_here_p (tmp_pc))
		    {
		      if (record_debug)
			fprintf_unfiltered (gdb_stdlog,
					    "Process record: break "
					    "at 0x%s.\n",
					    paddr_nz (tmp_pc));
		      if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache))
			  && execution_direction == EXEC_FORWARD
			  && !record_resume_step)
			regcache_write_pc (regcache,
					   tmp_pc +
					   gdbarch_decr_pc_after_break
					   (get_regcache_arch (regcache)));
		      continue_flag = 0;
		    }
		}
When p record get a sig, I need it stop like single step.  So set
record_resume_step to 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.
>

  if (set_cleanups)
    do_cleanups (set_cleanups);
void
do_cleanups (struct cleanup *old_chain)
{
  do_my_cleanups (&cleanup_chain, old_chain);
}
static void
do_my_cleanups (struct cleanup **pmy_chain,
		struct cleanup *old_chain)
{
  struct cleanup *ptr;
  while ((ptr = *pmy_chain) != old_chain)
    {
      *pmy_chain = ptr->next;	/* Do this first incase recursion */
      (*ptr->function) (ptr->arg);
      if (ptr->free_arg)
	(*ptr->free_arg) (ptr->arg);
      xfree (ptr);
    }
}
NULL will make all the chain clean.


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

This cleanup will make record_list point to the record entry that
execution before this record entry because set this record entry get
fail.
This part is not very easy to make clear.  I make clear it use a lot
of time.  :P
About goto, most of them are deleted.  I just keep one:
      /* Check breakpoint when forward execute.  */
      if (execution_direction == EXEC_FORWARD)
	{
            xxx
            xxx
	      goto replay_out;
	    }
	}

> + ? ? ? 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.
For now, record just support i386 arch.  Please let us keep it to the future.


Thanks,
Hui

Attachment: prec20090416.tar.gz
Description: GNU Zip compressed data


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