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

[Pedro Alves <pedro@codesourcery.com>]

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

A Monday 20 October 2008 01:39:30, Michael Snyder escreveu:
> Pedro Alves wrote:
> > On Sunday 19 October 2008 23:39:20, Michael Snyder wrote:
> >> After codgitating for a bit (that's "thinking" when you're over 50),
> >> I've decided that you're right.
> >>
> >> However, I have a new concern -- I'm worried about what it will do
> >> when it's replaying but going forward.
> >>
> >> Could you possibly revisit your test and see what it does
> >> if you record all the way to line 9 or 10, then back up
> >> to line 6, then continue with breakpoints at 6 and 7?
> > 
> > Eh, you're right.  It's broken.
> 
> Thought so.
> 
> See, the problem is that "adjust_pc_after_break" is assuming
> memory-breakpoint semantics, but Process Record/Replay actually
> implements hardware-breakpoint semantics.  It watches the
> instruction-address "bus" and stops when the PC matches the
> address of a breakpoint.
> 
> I suspect this is probably a problem with other record/replay
> back-ends too, but I haven't confirmed it yet.
> 
> Still, I think that the patch you committed was correct
> for the reverse case.  

> This is a corner case that reveals 
> that "reverse" and "replay" are not synonymous.

They certainly aren't.  When replaying, I believe it's just best to
behave as close as possible to when it the inferior is really running.
From the inferior control side, GDB be mostly as agnostic about
"replay" vs normal run as possibly.

IIUC from reading the code, I see two issues.

 1) When going forward and in reply mode, breakpoint hits are being checked
    *after* a record item is replays.  IIUC, we should check *before*,
    and report an adjusted PC.

 2) Un-inserted breakpoints weren't accounted for AFAICT (GDB will
    un-inserted breakpoints temporarily when stepping over them).
    Maybe they are, I got lost.  :-)  There's a loop going through the
    bp_location_chain.  Can you get rid of that and use
    regular_breakpoint_inserted_here_p or similars?

Below is a 10 minutes hack at it, as a starting point.  Replay stil
isn't perfect, mainly because I got lost in the record_wait maze --- that,
needs a bit of clean up.  :-)

> 
> > (gdb) record
> > (gdb) b 6
> > Breakpoint 2 at 0x8048352: file nop.c, line 6.
> > (gdb) b 7
> > Breakpoint 3 at 0x8048353: file nop.c, line 7.
> > (gdb) n
> > 
> > Breakpoint 3, main () at nop.c:7
> > 7               asm ("nop");
> > (gdb) n
> > 8               asm ("nop");
> > (gdb)
> > 9               asm ("nop");
> > (gdb) n
> > 10      }
> > (gdb) rc
> > Continuing.
> > 
> > Breakpoint 3, main () at nop.c:7
> > 7               asm ("nop");
> > (gdb) rn
> > 
> > No more reverse-execution history.
> > main () at nop.c:6
> > 6               asm ("nop");
> > (gdb) n
> > 
> > Breakpoint 2, main () at nop.c:6
> > 6               asm ("nop");
> > (gdb)
> > 8               asm ("nop");
> > (gdb)
> > 9               asm ("nop");
> > (gdb)
> > 
> > 
> > 
> > --
> > Pedro Alves
> 
> 



-- 
Pedro Alves

[-- Attachment #2: record_decr_pc.diff --]
[-- Type: text/x-diff, Size: 8073 bytes --]

---
 gdb/record.c |  159 +++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 101 insertions(+), 58 deletions(-)

Index: src/gdb/record.c
===================================================================
--- src.orig/gdb/record.c	2008-10-20 00:48:50.000000000 +0100
+++ src/gdb/record.c	2008-10-20 13:02:38.000000000 +0100
@@ -497,6 +497,9 @@ 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 pc;
+      record_t *curr_record;
+      int first = 1;
 
       record_get_sig = 0;
       act.sa_handler = record_sig_handler;
@@ -512,20 +515,13 @@ record_wait (ptid_t ptid, struct target_
          Then set it to terminal_ours to make GDB get the signal.  */
       target_terminal_ours ();
 
-      /* In EXEC_FORWARD mode, record_list point to the tail of prev
-         instruction.  */
-      if (execution_direction == EXEC_FORWARD && record_list->next)
-        {
-	  record_list = record_list->next;
-	}
-
       /* 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.  */
-	  if (execution_direction == EXEC_REVERSE 
+	  if (execution_direction == EXEC_REVERSE
 	      && record_list == &record_first)
 	    {
 	      /* Hit beginning of record log in reverse.  */
@@ -539,8 +535,51 @@ record_wait (ptid_t ptid, struct target_
 	      break;
 	    }
 
+	  /* Check for breakpoint hits in forward execution.  */
+	  pc = read_pc ();
+	  if (execution_direction == EXEC_FORWARD
+	      && regular_breakpoint_inserted_here_p (pc)
+	      /* && !single-stepping */)
+	    {
+	      status->kind = TARGET_WAITKIND_STOPPED;
+	      status->value.sig = TARGET_SIGNAL_TRAP;
+	      if (software_breakpoint_inserted_here_p (pc))
+		pc += gdbarch_decr_pc_after_break (current_gdbarch);
+	      write_pc (pc);
+
+	      if (sigaction (SIGALRM, &old_act, NULL))
+		perror_with_name (_("Process record: sigaction"));
+
+	      discard_cleanups (old_cleanups);
+	      return inferior_ptid;
+	    }
+
+	  if (first)
+	    {
+	      first = 0;
+	      /* In EXEC_FORWARD mode, record_list point to the tail of prev
+		 instruction.  */
+	      if (execution_direction == EXEC_FORWARD && record_list->next)
+		{
+		  record_list = record_list->next;
+		}
+	    }
+
+	  curr_record = record_list;
+
+	  if (execution_direction == EXEC_REVERSE)
+	    {
+	      if (record_list->prev)
+		record_list = record_list->prev;
+	    }
+	  else
+	    {
+	      if (record_list->next)
+		record_list = record_list->next;
+	    }
+
 	  /* set ptid, register and memory according to record_list */
-	  if (record_list->type == record_reg)
+	  if (curr_record->type == record_reg)
 	    {
 	      /* reg */
 	      gdb_byte reg[MAX_REGISTER_SIZE];
@@ -548,43 +587,43 @@ record_wait (ptid_t ptid, struct target_
 		{
 		  fprintf_unfiltered (gdb_stdlog,
 				      "Process record: record_reg 0x%s to inferior num = %d.\n",
-				      paddr_nz ((CORE_ADDR)record_list),
-				      record_list->u.reg.num);
+				      paddr_nz ((CORE_ADDR)curr_record),
+				      curr_record->u.reg.num);
 		}
-	      regcache_cooked_read (regcache, record_list->u.reg.num, reg);
-	      regcache_cooked_write (regcache, record_list->u.reg.num,
-				     record_list->u.reg.val);
-	      memcpy (record_list->u.reg.val, reg, MAX_REGISTER_SIZE);
+	      regcache_cooked_read (regcache, curr_record->u.reg.num, reg);
+	      regcache_cooked_write (regcache, curr_record->u.reg.num,
+				     curr_record->u.reg.val);
+	      memcpy (curr_record->u.reg.val, reg, MAX_REGISTER_SIZE);
 	    }
-	  else if (record_list->type == record_mem)
+	  else if (curr_record->type == record_mem)
 	    {
 	      /* mem */
-	      gdb_byte *mem = alloca (record_list->u.mem.len);
+	      gdb_byte *mem = alloca (curr_record->u.mem.len);
 	      if (record_debug > 1)
 		{
 		  fprintf_unfiltered (gdb_stdlog,
 				      "Process record: record_mem 0x%s to inferior addr = 0x%s len = %d.\n",
-				      paddr_nz ((CORE_ADDR)record_list),
-				      paddr_nz (record_list->u.mem.addr),
-				      record_list->u.mem.len);
+				      paddr_nz ((CORE_ADDR)curr_record),
+				      paddr_nz (curr_record->u.mem.addr),
+				      curr_record->u.mem.len);
 		}
 	      if (target_read_memory
-		  (record_list->u.mem.addr, mem, record_list->u.mem.len))
+		  (curr_record->u.mem.addr, mem, curr_record->u.mem.len))
 		{
 		  error (_("Process record: read memory addr = 0x%s len = %d error."),
-			 paddr_nz (record_list->u.mem.addr),
-			 record_list->u.mem.len);
+			 paddr_nz (curr_record->u.mem.addr),
+			 curr_record->u.mem.len);
 		}
 	      if (target_write_memory
-		  (record_list->u.mem.addr, record_list->u.mem.val,
-		   record_list->u.mem.len))
+		  (curr_record->u.mem.addr, curr_record->u.mem.val,
+		   curr_record->u.mem.len))
 		{
 		  error (_
 			 ("Process record: write memory addr = 0x%s len = %d error."),
-			 paddr_nz (record_list->u.mem.addr),
-			 record_list->u.mem.len);
+			 paddr_nz (curr_record->u.mem.addr),
+			 curr_record->u.mem.len);
 		}
-	      memcpy (record_list->u.mem.val, mem, record_list->u.mem.len);
+	      memcpy (curr_record->u.mem.val, mem, curr_record->u.mem.len);
 	    }
 	  else
 	    {
@@ -596,13 +635,13 @@ record_wait (ptid_t ptid, struct target_
 		{
 		  fprintf_unfiltered (gdb_stdlog,
 				      "Process record: record_end 0x%s to inferior need_dasm = %d.\n",
-				      paddr_nz ((CORE_ADDR)record_list),
-				      record_list->u.need_dasm);
+				      paddr_nz ((CORE_ADDR)curr_record),
+				      curr_record->u.need_dasm);
 		}
 
 	      if (execution_direction == EXEC_FORWARD)
 		{
-		  need_dasm = record_list->u.need_dasm;
+		  need_dasm = curr_record->u.need_dasm;
 		}
 	      if (need_dasm)
 		{
@@ -631,45 +670,48 @@ record_wait (ptid_t ptid, struct target_
 		      continue_flag = 0;
 		    }
 
-		  /* check breakpoint */
-		  tmp_pc = read_pc ();
-		  for (bl = bp_location_chain; bl; bl = bl->global_next)
+		  if (execution_direction == EXEC_REVERSE)
 		    {
-		      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)
-			{
-			  continue;
-			}
-		      if (bl->address == tmp_pc)
+		      /* check breakpoint */
+		      tmp_pc = read_pc ();
+		      for (bl = bp_location_chain; bl; bl = bl->global_next)
 			{
-			  if (record_debug)
+			  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)
+			    {
+			      continue;
+			    }
+			  if (bl->address == tmp_pc)
 			    {
-			      fprintf_unfiltered (gdb_stdlog,
-						  "Process record: break at 0x%s.\n",
-						  paddr_nz (tmp_pc));
+			      if (record_debug)
+				{
+				  fprintf_unfiltered (gdb_stdlog,
+						      "Process record: break at 0x%s.\n",
+						      paddr_nz (tmp_pc));
+				}
+			      continue_flag = 0;
+			      break;
 			    }
-			  continue_flag = 0;
-			  break;
 			}
 		    }
 		}
 	      if (execution_direction == EXEC_REVERSE)
 		{
-		  need_dasm = record_list->u.need_dasm;
+		  need_dasm = curr_record->u.need_dasm;
 		}
 	    }
 
-next:
+#if 0
 	  if (continue_flag)
 	    {
 	      if (execution_direction == EXEC_REVERSE)
@@ -683,6 +725,7 @@ next:
 		    record_list = record_list->next;
 		}
 	    }
+#endif
 	}
       while (continue_flag);