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

[teawater <teawater@gmail.com>]

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

Thanks Pedro.

I make a patch too.


2008-10-20  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".

Thanks,
Hui

On Mon, Oct 20, 2008 at 20:09, Pedro Alves <pedro@codesourcery.com> wrote:
> 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_wait_breakpoint.txt --]
[-- Type: text/plain, Size: 2708 bytes --]

--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2008-10-20  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".
+
 2008-10-19  Hui Zhu  <teawater@gmail.com>
 
 	* infrun.c (handle_inferior_event): Set "stop_pc" when
--- a/record.c
+++ b/record.c
@@ -498,6 +498,23 @@ record_wait (ptid_t ptid, struct target_
       int first_record_end = 1;
       struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
 
+      /* Check breakpoint when forward execute.  */
+      if (execution_direction == EXEC_FORWARD)
+	{
+	  if (breakpoint_inserted_here_p (read_pc ()))
+	    {
+	      if (record_debug)
+		{
+		  fprintf_unfiltered (gdb_stdlog,
+				      "Process record: break at 0x%s.\n",
+				      paddr_nz (regcache_read_pc
+						(get_thread_regcache
+						 (ptid))));
+		}
+	      goto replay_out;
+	    }
+	}
+
       record_get_sig = 0;
       act.sa_handler = record_sig_handler;
       act.sa_mask = record_maskall;
@@ -588,10 +605,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 +645,17 @@ record_wait (ptid_t ptid, struct target_
 		    }
 
 		  /* check breakpoint */
-		  tmp_pc = read_pc ();
-		  for (bl = bp_location_chain; bl; bl = bl->global_next)
+		  if (breakpoint_inserted_here_p (read_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;
-			}
-		      if (bl->address == tmp_pc)
-			{
-			  if (record_debug)
-			    {
-			      fprintf_unfiltered (gdb_stdlog,
-						  "Process record: break at 0x%s.\n",
-						  paddr_nz (tmp_pc));
-			    }
-			  continue_flag = 0;
-			  break;
+			  fprintf_unfiltered (gdb_stdlog,
+					      "Process record: break at 0x%s.\n",
+					      paddr_nz (regcache_read_pc
+							(get_thread_regcache
+							 (ptid))));
 			}
+		      continue_flag = 0;
 		    }
 		}
 	      if (execution_direction == EXEC_REVERSE)
@@ -691,6 +686,7 @@ next:
 	  perror_with_name (_("Process record: sigaction"));
 	}
 
+replay_out:
       if (record_get_sig)
 	{
 	  status->value.sig = TARGET_SIGNAL_INT;