Re: [+rfc] Re: [patch v6 00/21] record-btrace: reverse

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Thu, 28 Nov 2013 09:42:16 +0100, Metzger, Markus T wrote:
> We store the frame_id of the current frame and do a single-step.  
> Then we try to detect stepping into a subroutine by unwinding
> the stack frames and comparing the frame_id's with our stored
> frame_id.

OK, understood now.

In fact the time you change btinfo->replay you also change register contents.
Therefore the registers_changed_ptid() call is there right.

For the frame_id change one could also provide mapping of old frame_id to new
frame_id in frame_id_eq() but that is worse than just re-setting it.


> Do you have a better idea?

I agree in general.  Just:

Rather than get_current_frame_nocheck I find safer to just temporarily switch
off the executing flag.  There are many other checks which make sense which
were omitted.

Calling set_step_info seems needlessly intrusive to me, there is no need to
re-set tp->current_symtab + tp->current_line.  Despite in the moment of
record_btrace_start_replaying() I see step_frame_id should be the current
frame id it does not have to be.  Such as when we reverse-continue, it will be
null_frame_id.

The attached patch(es) is on top of yours.

I still believe inferior should resume + wait if it changes its PC.

I am again not sure if the patch passes without FAILs due to my BTS hardware.


Thanks,
Jan

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index c50e11b..4a57b51 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -1281,9 +1281,8 @@ record_btrace_start_replaying (struct thread_info *tp)
 {
   struct btrace_thread_info *btinfo;
   struct btrace_insn_iterator *replay;
-  const struct btrace_insn *insn;
-  struct symtab_and_line sal;
-  struct frame_info *frame;
+  volatile struct gdb_exception except;
+  int executing;
 
   btinfo = &tp->btrace;
 
@@ -1291,24 +1290,50 @@ record_btrace_start_replaying (struct thread_info *tp)
   if (btinfo->begin == NULL)
     return NULL;
 
-  /* We start replaying at the end of the branch trace.  This corresponds to the
-     current instruction.  */
-  replay = xmalloc (sizeof (*replay));
-  btrace_insn_end (replay, btinfo);
+  executing = is_executing (tp->ptid);
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      int update_step_frame_id, update_step_stack_frame_id;
+      struct frame_id frame_id;
 
-  /* We're not replaying, yet.  */
-  gdb_assert (btinfo->replay == NULL);
-  btinfo->replay = replay;
+      /* We start replaying at the end of the branch trace.  This corresponds to the
+	 current instruction.  */
+      replay = xmalloc (sizeof (*replay));
+      btrace_insn_end (replay, btinfo);
 
-  /* Make sure we're not using any stale registers.  */
-  registers_changed_ptid (tp->ptid);
+      if (executing)
+	{
+	  /* get_current_frame would error out otherwise.  */
+	  set_executing (tp->ptid, 0);
+	}
+
+      frame_id = get_frame_id (get_current_frame ());
 
-  /* We just started replaying.  The frame id cached for stepping is based
-     on unwinding, not on branch tracing.  Recompute it.  */
-  frame = get_current_frame_nocheck ();
-  insn = btrace_insn_get (replay);
-  sal = find_pc_line (insn->pc, 0);
-  set_step_info (frame, sal);
+      update_step_frame_id = frame_id_eq (frame_id, tp->control.step_frame_id);
+      update_step_stack_frame_id = frame_id_eq (frame_id,
+					       tp->control.step_stack_frame_id);
+
+      /* We're not replaying, yet.  */
+      gdb_assert (btinfo->replay == NULL);
+      btinfo->replay = replay;
+
+      /* Make sure we're not using any stale registers.  */
+      registers_changed_ptid (tp->ptid);
+
+      /* We just started replaying.  The frame id cached for stepping is based
+	 on unwinding, not on branch tracing.  Recompute it.  */
+
+      frame_id = get_frame_id (get_current_frame ());
+
+      if (update_step_frame_id)
+	tp->control.step_frame_id = frame_id;
+      if (update_step_stack_frame_id)
+	tp->control.step_stack_frame_id = frame_id;
+    }
+  if (executing)
+    set_executing (tp->ptid, 1);
+  if (except.reason < 0)
+    throw_exception (except);
 
   return replay;
 }

diff --git a/gdb/frame.c b/gdb/frame.c
index 5a6f107..0fd98ff 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1367,29 +1367,6 @@ unwind_to_current_frame (struct ui_out *ui_out, void *args)
   return 0;
 }
 
-/* See frame.h.  */
-
-struct frame_info *get_current_frame_nocheck (void)
-{
-  if (current_frame == NULL)
-    {
-      struct frame_info *sentinel_frame =
-	create_sentinel_frame (current_program_space, get_current_regcache ());
-
-      if (catch_exceptions (current_uiout, unwind_to_current_frame,
-			    sentinel_frame, RETURN_MASK_ERROR) != 0)
-	{
-	  /* Oops! Fake a current frame?  Is this useful?  It has a PC
-             of zero, for instance.  */
-	  current_frame = sentinel_frame;
-	}
-    }
-
-  return current_frame;
-}
-
-/* See frame.h.  */
-
 struct frame_info *
 get_current_frame (void)
 {
@@ -1415,7 +1392,19 @@ get_current_frame (void)
 	error (_("Target is executing."));
     }
 
-  return get_current_frame_nocheck ();
+  if (current_frame == NULL)
+    {
+      struct frame_info *sentinel_frame =
+	create_sentinel_frame (current_program_space, get_current_regcache ());
+      if (catch_exceptions (current_uiout, unwind_to_current_frame,
+			    sentinel_frame, RETURN_MASK_ERROR) != 0)
+	{
+	  /* Oops! Fake a current frame?  Is this useful?  It has a PC
+             of zero, for instance.  */
+	  current_frame = sentinel_frame;
+	}
+    }
+  return current_frame;
 }
 
 /* The "selected" stack frame is used by default for local and arg
diff --git a/gdb/frame.h b/gdb/frame.h
index cd2033d..f0da19e 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -242,10 +242,6 @@ enum frame_type
    error.  */
 extern struct frame_info *get_current_frame (void);
 
-/* Similar to get_current_frame except that we omit all checks.  May
-   return NULL if unwinding fails.  */
-extern struct frame_info *get_current_frame_nocheck (void);
-
 /* Does the current target interface have enough state to be able to
    query the current inferior for frame info, and is the inferior in a
    state where that is possible?  */

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index c50e11b..4a57b51 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -1662,6 +1687,8 @@ record_btrace_goto_target (struct thread_info *tp,
   struct btrace_insn_iterator *goto_target;
   struct btrace_thread_info *btinfo;
   struct target_waitstatus ws;
+  struct btrace_insn_iterator target_it;
+  volatile struct gdb_exception exception;
 
   btinfo = &tp->btrace;
 
@@ -1686,11 +1713,17 @@ record_btrace_goto_target (struct thread_info *tp,
   btinfo->flags |= BTHR_GOTO;
   btinfo->goto_target = goto_target;
 
-#if 0
+  TRY_CATCH (exception, RETURN_MASK_ALL)
+    {
+
+#if 1
   if (goto_target != NULL)
+    target_it = *goto_target;
+  else
+    btrace_insn_end (&target_it, btinfo);
     tp->control.exception_resume_breakpoint =
       set_momentary_breakpoint_at_pc (target_gdbarch (),
-				      btrace_insn_get (goto_target)->pc,
+				      btrace_insn_get (&target_it)->pc,
 				      bp_until);
 #endif
 #if 0
@@ -1701,8 +1734,18 @@ record_btrace_goto_target (struct thread_info *tp,
   proceed ((CORE_ADDR) -1, GDB_SIGNAL_0, 0);
 #endif
 
-  if (btinfo->goto_target != NULL || (btinfo->flags & BTHR_GOTO) != 0)
+  // It will need a fix if reverse mode supports target-async mode.
+  if ((btinfo->flags & BTHR_GOTO) != 0)
     error (_("Record goto failed."));
+  gdb_assert (btinfo->goto_target == NULL);
+
+    }
+  if (exception.reason < 0)
+    {
+      xfree (btinfo->goto_target);
+      btinfo->goto_target = NULL;
+      btinfo->flags &= ~BTHR_GOTO;
+    }
 }
 
 /* The to_goto_record_begin method of target record-btrace.  */