Re: [RFA] Resubmit process record and replay, 6/10

[teawater <teawater@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2751 bytes --]

Hi,

I make a tmp patch to make this idea clear.  It test with testsuite is OK.

And I still didn't change "record_not_record_set" cause I didn't have
idea on it.

Thanks,
Hui




On Wed, Nov 26, 2008 at 10:33, teawater <teawater@gmail.com> wrote:
> Sorry I forget a big part that need it.
> When GDB work in replay mode, P record will set regs and memory in
> record_wait. All of them can't be record.
>
> So what about set not_record flag to record_wait in replay mode,
> record_insert_breakpoint and record_remove_breakpoint.
>
> And about the name of this flag, do you have some idea on it?
>
>
> Thanks,
> Hui
>
>
> On Wed, Nov 26, 2008 at 02:22, Michael Snyder <msnyder@vmware.com> wrote:
>> teawater wrote:
>>>
>>> On Tue, Nov 25, 2008 at 03:16, Michael Snyder <msnyder@vmware.com> wrote:
>>>>
>>>> teawater wrote:
>>>>>
>>>>> Hi Michael,
>>>>>
>>>>> About "record_not_record_set", It set record_not_record to let P
>>>>> record doesn't record the memory and registers control behaviors of
>>>>> GDB in function record_store_registers and record_xfer_partial.
>>>>>
>>>>> So I think the name "record_not_record_set" and
>>>>> "record_skip_recording" are not very clear.
>>>>> Could you please give me some advices on it?
>>>>
>>>> Yeah, that's pretty much the way I understood it.
>>>>
>>>> It sets a one-time flag that says "omit (skip) recording
>>>> registers and memory that would otherwise be recorded".
>>>>
>>>> And if I understand correctly, this is to avoid adding
>>>> changes to the record log that are made by gdb when it
>>>> resumes the target.  It's only called from "proceed()".
>>>>
>>>> I'm not completely clear on what those changes are.
>>>> Is gdb modifying the PC?  Or are you perhaps trying to
>>>> avoid recording breakpoints?
>>>
>>> I think avoid recording breakpoints is the main affect.
>>> Another function is help deal with displaced step. Of course, P record
>>> and displaced step will not work together now.
>>>
>>> I think I add "record_not_record" function is because I want
>>> record_store_registers and record_xfer_partial just record the user
>>> level change, not for others.
>>> What do you think about it?
>>
>> OK, so if we ignore displaced stepping for now, then can we
>> limit the issue to breakpoints?
>>
>> Breakpoint writes will all pass through functions called
>> memory_insert_breakpoint and memory_remove_breakpoint (mem-break.c).
>>
>> So what we want to do is get the information from there into
>> record.c.  I guess you could do pretty much what you are doing
>> now, only call the access function from mem-break.c instead of
>> from infrun.  It would help to localize it and make its meaning
>> clear.
>>
>> Maybe call it "dont_record_memory_breakpoint" or something like that.
>>
>>
>

[-- Attachment #2: tmp-skip.txt --]
[-- Type: text/plain, Size: 4976 bytes --]

Index: gdb/infrun.c
===================================================================
--- gdb.orig/infrun.c	2008-12-11 10:57:22.000000000 +0800
+++ gdb/infrun.c	2008-12-11 11:03:14.000000000 +0800
@@ -607,7 +607,7 @@
 	    && non_stop)
 	   || can_use_displaced_stepping == can_use_displaced_stepping_on)
 	  && gdbarch_displaced_step_copy_insn_p (gdbarch)
-	  && !RECORD_IS_USED);
+	  && !TARGET_IS_PROCESS_RECORD);
 }
 
 /* Clean out any stray displaced stepping state.  */
@@ -1312,12 +1312,6 @@
   if (step < 0)
     stop_after_trap = 1;
 
-   /* When GDB resume the inferior, process record target doesn't need to
-      record the memory and register store operation of GDB. So set
-      record_not_record to 1. */
-  if (RECORD_IS_USED)
-    record_not_record_set ();
-
   if (addr == (CORE_ADDR) -1)
     {
       if (pc == stop_pc && breakpoint_here_p (pc) 
@@ -2050,6 +2044,10 @@
   if (software_breakpoint_inserted_here_p (breakpoint_pc)
       || (non_stop && moribund_breakpoint_here_p (breakpoint_pc)))
     {
+      struct cleanup *old_cleanups;
+      if (TARGET_IS_PROCESS_RECORD)
+	old_cleanups = record_not_record_set ();
+
       /* When using hardware single-step, a SIGTRAP is reported for both
 	 a completed single-step and a software breakpoint.  Need to
 	 differentiate between the two, as the latter needs adjusting
@@ -2073,6 +2071,9 @@
 	  || !currently_stepping (ecs->event_thread)
 	  || ecs->event_thread->prev_pc == breakpoint_pc)
 	regcache_write_pc (regcache, breakpoint_pc);
+
+      if (TARGET_IS_PROCESS_RECORD)
+	do_cleanups (old_cleanups);
     }
 }
 
Index: gdb/record.c
===================================================================
--- gdb.orig/record.c	2008-12-11 10:57:22.000000000 +0800
+++ gdb/record.c	2008-12-11 11:03:56.000000000 +0800
@@ -173,6 +173,13 @@
 static void
 record_arch_list_add (record_t * rec)
 {
+  if (record_debug > 1)
+    {
+      fprintf_unfiltered (gdb_stdlog,
+			  "Process record: record_arch_list_add 0x%s.\n",
+			  paddr_nz ((CORE_ADDR)rec));
+    }
+
   if (record_arch_list_tail)
     {
       record_arch_list_tail->next = rec;
@@ -372,11 +379,13 @@
   record_not_record = 0;
 }
 
-void
+struct cleanup *
 record_not_record_set (void)
 {
   struct cleanup *old_cleanups = make_cleanup (record_not_record_cleanups, 0);
   record_not_record = 1;
+
+  return old_cleanups;
 }
 
 static void
@@ -407,7 +416,7 @@
     }
 
   /* Check if record target is already running */
-  if (RECORD_IS_USED)
+  if (TARGET_IS_PROCESS_RECORD)
     {
       if (!nquery
 	  (_("Process record target already running, do you want to delete the old record log?")))
@@ -483,6 +492,8 @@
 static ptid_t
 record_wait (ptid_t ptid, struct target_waitstatus *status)
 {
+  struct cleanup *set_cleanups = record_not_record_set ();
+
   if (record_debug)
     {
       fprintf_unfiltered (gdb_stdlog,
@@ -835,6 +846,7 @@
       discard_cleanups (old_cleanups);
     }
 
+  do_cleanups (set_cleanups);
   return inferior_ptid;
 }
 
@@ -1073,7 +1085,12 @@
 {
   if (!RECORD_IS_REPLAY)
     {
-      return record_beneath_to_insert_breakpoint (bp_tgt);
+      struct cleanup *old_cleanups = record_not_record_set ();
+      int ret = record_beneath_to_insert_breakpoint (bp_tgt);
+
+      do_cleanups (old_cleanups);
+
+      return ret;
     }
 
   return 0;
@@ -1084,7 +1101,12 @@
 {
   if (!RECORD_IS_REPLAY)
     {
-      return record_beneath_to_remove_breakpoint (bp_tgt);
+      struct cleanup *old_cleanups = record_not_record_set ();
+      int ret = record_beneath_to_remove_breakpoint (bp_tgt);
+
+      do_cleanups (old_cleanups);
+
+      return ret;
     }
 
   return 0;
@@ -1142,7 +1164,7 @@
 static void
 cmd_record_delete (char *args, int from_tty)
 {
-  if (RECORD_IS_USED)
+  if (TARGET_IS_PROCESS_RECORD)
     {
       if (RECORD_IS_REPLAY)
 	{
@@ -1168,7 +1190,7 @@
 static void
 cmd_record_stop (char *args, int from_tty)
 {
-  if (RECORD_IS_USED)
+  if (TARGET_IS_PROCESS_RECORD)
     {
       if (!record_list || !from_tty || query (_("Delete recorded log and stop recording?")))
 	{
Index: gdb/record.h
===================================================================
--- gdb.orig/record.h	2008-12-11 10:57:22.000000000 +0800
+++ gdb/record.h	2008-12-11 11:03:33.000000000 +0800
@@ -20,7 +20,7 @@
 #ifndef _RECORD_H_
 #define _RECORD_H_
 
-#define RECORD_IS_USED   \
+#define TARGET_IS_PROCESS_RECORD   \
      (current_target.beneath == &record_ops)
 #define RECORD_IS_REPLAY \
      (record_list->next || execution_direction == EXEC_REVERSE)
@@ -80,7 +80,7 @@
 extern int record_arch_list_add_mem (CORE_ADDR addr, int len);
 extern int record_arch_list_add_end (int need_dasm);
 extern void record_message (struct gdbarch *gdbarch);
-extern void record_not_record_set (void);
+extern struct cleanup * record_not_record_set (void);
 
 extern void (*record_beneath_to_resume) (ptid_t, int, enum target_signal);
 extern ptid_t (*record_beneath_to_wait) (ptid_t, struct target_waitstatus *);