Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater <teawater@gmail.com>]

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

The old patch make my_waitpid_record set pc even if this is not a breakpoint.
So I make a new patch that my_waitpid_record just set pc when this is
a breakpoint.


2008-10-24  Hui Zhu  <teawater@gmail.com>

	* record.c (record_wait): Check breakpint before forward
	execute in replay mode.
	Check breakpoint use function "breakpoint_inserted_here_p"
	in replay mode.
	Set pc if forward execute, gdbarch_decr_pc_after_break is not
	0 and this is not single step in replay mode.

	* linux-nat.c (my_waitpid_record): Add
	gdbarch_decr_pc_after_break to pc if need.



On Fri, Oct 24, 2008 at 17:57, teawater <teawater@gmail.com> wrote:
> Hi buddies,
>
> This is the new patch that fix the break bug.
>
> But I think I still need to add some code to deal with signal.
>
> 2008-10-24  Hui Zhu  <teawater@gmail.com>
>
>        * record.c (record_wait): Check breakpint before forward
>        execute in replay mode.
>        Check breakpoint use function "breakpoint_inserted_here_p"
>        in replay mode.
>        Set pc if forward execute, gdbarch_decr_pc_after_break is not
>        0 and this is not single step in replay mode.
>
>        * linux-nat.c (my_waitpid_record): Add
>        gdbarch_decr_pc_after_break to pc if need.
>
> Thanks,
> Hui
>
> On Fri, Oct 24, 2008 at 16:10, teawater <teawater@gmail.com> wrote:
>> Thanks Pedro and Michael,
>>
>> I think the reason is P record let inferior step recycle in the
>> linux-nat target.
>> So when it break by breakpint, it will not let
>> (pc+gdbarch_decr_pc_after_break (gdbarch)). Then after
>> adjust_pc_after_break, The PC is error.
>>
>> I will try to deal with it.
>>
>> Hui
>>
>> On Fri, Oct 24, 2008 at 09:50, Pedro Alves <pedro@codesourcery.com> wrote:
>>> On Friday 24 October 2008 01:37:31, Michael Snyder wrote:
>>>> > In sum, it appears that decr_pc_after_break doesn't matter when you have
>>>> > continguous breakpoints, as long as you get from from B1's address to B2's
>>>> > address by single-stepping.  All is good then, it appears!
>>>>
>>>> I agree, at least that is the conclusion I am leaning toward.
>>>>
>>>
>>> Not so fast!  I knew I had to spend a little extra thinking about
>>> it, 'cause I knew something was broken, just couldn't find what.  :-)
>>> *as long as you get from from B1's address to B2's address
>>> by single-stepping* was a restriction that doesn't always apply.
>>>
>>> Here's a test that will fail in forward record/replay mode, but not
>>> in normal "play" mode.
>>>
>>> volatile int global_foo = 0;
>>>
>>> int
>>> main (int argc, char **argv)
>>> {
>>>  asm ("nop"); /* 1st insn */
>>>  asm ("nop"); /* 2nd insn */
>>>  asm ("nop"); /* 3rd insn */
>>>  asm ("nop"); /* 4th insn */
>>>  if (!global_foo)
>>>    goto ahead;
>>>  asm ("nop"); /* 5th insn */
>>>  asm ("nop"); /* 6th insn */
>>>  asm ("nop"); /* 7th insn */
>>>  asm ("nop"); /* 8th insn */  <<< break 1 here
>>>  ahead:
>>>  asm ("nop"); /* 9th insn */  <<< break 2 here
>>>  end:
>>>  return 0;
>>> }
>>>
>>> If you let the program reply until break 2 is hit, and assuming insn
>>> 8th and 9th are assembled as contiguous (they do on x86 -O0 for me), you'll
>>> see that adjust_pc_after_break will indeed make it appear that breakpoint
>>> 1 was hit.  Now, nops are nops, but real code could have something
>>> else there...
>>>
>>> /me goes back to bed.
>>>
>>> --
>>> Pedro Alves
>>>
>>
>

[-- Attachment #2: record_wait_breakpoint.txt --]
[-- Type: text/plain, Size: 4456 bytes --]

--- a/linux-nat.c
+++ b/linux-nat.c
@@ -514,7 +514,9 @@ my_waitpid_record (int pid, int *status,
   struct bp_location *bl;
   struct breakpoint *b;
   CORE_ADDR pc;
+  CORE_ADDR decr_pc_after_break;
   struct lwp_info *lp;
+  int is_breakpoint = 1;
 
 wait_begin:
   ret = my_waitpid (pid, status, flags);
@@ -530,7 +532,7 @@ wait_begin:
 
   if (WIFSTOPPED (*status) && WSTOPSIG (*status) == SIGTRAP)
     {
-      /* Check if there is a breakpoint */
+      /* Check if there is a breakpoint.  */
       pc = 0;
       registers_changed ();
       for (bl = bp_location_chain; bl; bl = bl->global_next)
@@ -602,7 +604,26 @@ wait_begin:
       goto wait_begin;
     }
 
+  is_breakpoint = 0;
+
 out:
+  /* Add gdbarch_decr_pc_after_break to pc because pc will be break at address
+     add gdbarch_decr_pc_after_break when inferior non-step execute.  */
+  if (is_breakpoint)
+    {
+      decr_pc_after_break = gdbarch_decr_pc_after_break
+	(get_regcache_arch (get_thread_regcache (pid_to_ptid (ret))));
+      if (decr_pc_after_break)
+	{
+	  if (!pc)
+	    {
+	      pc = regcache_read_pc (get_thread_regcache (pid_to_ptid (ret)));
+	    }
+	  regcache_write_pc (get_thread_regcache (pid_to_ptid (ret)),
+			     pc + decr_pc_after_break);
+	}
+    }
+
   return ret;
 }
 
--- a/record.c
+++ b/record.c
@@ -497,6 +497,33 @@ record_wait (ptid_t ptid, struct target_
       int continue_flag = 1;
       int first_record_end = 1;
       struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
+      CORE_ADDR tmp_pc;
+
+      status->kind = TARGET_WAITKIND_STOPPED;
+
+      /* Check breakpoint when forward execute.  */
+      if (execution_direction == EXEC_FORWARD)
+	{
+	  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))
+		  && !record_resume_step)
+		{
+		  regcache_write_pc (regcache,
+				     tmp_pc +
+				     gdbarch_decr_pc_after_break
+				     (get_regcache_arch (regcache)));
+		}
+	      goto replay_out;
+	    }
+	}
 
       record_get_sig = 0;
       act.sa_handler = record_sig_handler;
@@ -521,7 +548,6 @@ record_wait (ptid_t ptid, struct target_
 
       /* Loop over the record_list, looking for the next place to
 	 stop.  */
-      status->kind = TARGET_WAITKIND_STOPPED;
       do
 	{
 	  /* Check for beginning and end of log.  */
@@ -588,10 +614,6 @@ record_wait (ptid_t ptid, struct target_
 	    }
 	  else
 	    {
-	      CORE_ADDR tmp_pc;
-	      struct bp_location *bl;
-	      struct breakpoint *b;
-
 	      if (record_debug > 1)
 		{
 		  fprintf_unfiltered (gdb_stdlog,
@@ -632,35 +654,25 @@ record_wait (ptid_t ptid, struct target_
 		    }
 
 		  /* check breakpoint */
-		  tmp_pc = read_pc ();
-		  for (bl = bp_location_chain; bl; bl = bl->global_next)
+		  tmp_pc = regcache_read_pc (regcache);
+		  if (breakpoint_inserted_here_p (tmp_pc))
 		    {
-		      b = bl->owner;
-		      gdb_assert (b);
-		      if (b->enable_state != bp_enabled
-			  && b->enable_state != bp_permanent)
-			continue;
-
-		      if (b->type == bp_watchpoint || b->type == bp_catch_fork
-			  || b->type == bp_catch_vfork
-			  || b->type == bp_catch_exec
-			  || b->type == bp_hardware_watchpoint
-			  || b->type == bp_read_watchpoint
-			  || b->type == bp_access_watchpoint)
+		      if (record_debug)
 			{
-			  continue;
+			  fprintf_unfiltered (gdb_stdlog,
+					      "Process record: break at 0x%s.\n",
+					      paddr_nz (tmp_pc));
 			}
-		      if (bl->address == tmp_pc)
+		      if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache))
+			  && execution_direction == EXEC_FORWARD
+			  && !record_resume_step)
 			{
-			  if (record_debug)
-			    {
-			      fprintf_unfiltered (gdb_stdlog,
-						  "Process record: break at 0x%s.\n",
-						  paddr_nz (tmp_pc));
-			    }
-			  continue_flag = 0;
-			  break;
+			  regcache_write_pc (regcache,
+					     tmp_pc +
+					     gdbarch_decr_pc_after_break
+					     (get_regcache_arch (regcache)));
 			}
+		      continue_flag = 0;
 		    }
 		}
 	      if (execution_direction == EXEC_REVERSE)
@@ -691,6 +703,7 @@ next:
 	  perror_with_name (_("Process record: sigaction"));
 	}
 
+replay_out:
       if (record_get_sig)
 	{
 	  status->value.sig = TARGET_SIGNAL_INT;