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

["Metzger, Markus T" <markus.t.metzger@intel.com>]

> -----Original Message-----
> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com]
> Sent: Sunday, October 06, 2013 9:59 PM

Hello Jan,

> Besides some nitpicks there remains the larger request
> 	Re: [patch v4 23/24] record-btrace: add (reverse-)stepping support
> 	https://sourceware.org/ml/gdb-patches/2013-09/msg01004.html
> 	Message-ID: <20130930102533.GA29665@host2.jankratochvil.net>
> 
> 
> to always use to_resume + to_wait for any change of history position.  This
> also means the "hacks" like
> 	+record_btrace_goto_end (void)
> 	+  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC,
> 1);
> can be removed as normal GDB inferior stop will print the frame.
> 
> Other comments are welcome if it is not a too strict requirement from me
> but
> I think we could face some forgotten resetting of future caches avoiding the
> normal resume+wait path this way.  Or ... we would not face them but it
> would
> violate localized of cache resets in to_resume+to_wait code path.

I hacked a first prototype of this (see below).  It passes most tests but
results in three fails in the record_goto suite.

One thing that it shows, though, is that it only removes the 'mostly harmless'
hack in the various goto functions shown above.

The more serious hacks in record_btrace_start_replaying

	  /* 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 = get_current_frame_nocheck ();
	  insn = btrace_insn_get (replay);
	  sal = find_pc_line (insn->pc, 0);
	  set_step_info (frame, sal);

and record_btrace_stop_replaying

	  /* Make sure we're not leaving any stale registers.  */
	  registers_changed_ptid (tp->ptid);

however, are not removed by this.

They are required when reverse-stepping the first time or when
stepping past the end of the execution trace.

I don't think that there's a big benefit in pursuing this.  Plus the patch has the
potential of messing things up pretty badly if somehow (maybe due to some
unexpected error () somewhere in proceed ()) the record goto command does
not complete and BTHR_GOTO remains set.


Regards,
Markus.


diff --git a/gdb/btrace.h b/gdb/btrace.h
index 5b52ec6..c1a5d14 100644
--- a/gdb/btrace.h
+++ b/gdb/btrace.h
@@ -169,7 +169,10 @@ enum btrace_thread_flag
   BTHR_RCONT = (1 << 3),
 
   /* The thread is to be moved.  */
-  BTHR_MOVE = (BTHR_STEP | BTHR_RSTEP | BTHR_CONT | BTHR_RCONT)
+  BTHR_MOVE = (BTHR_STEP | BTHR_RSTEP | BTHR_CONT | BTHR_RCONT),
+
+  /* The thread is to go to GOTO_TARGET in its BTINFO.  */
+  BTHR_GOTO = (1 << 4)
 };
 
 /* Branch trace information per thread.
@@ -212,6 +215,9 @@ struct btrace_thread_info
 
   /* The current replay position.  NULL if not replaying.  */
   struct btrace_insn_iterator *replay;
+
+  /* The goto target replay position.  NULL if not active.  */
+  struct btrace_insn_iterator *goto_target;
 };
 
 /* Enable branch tracing for a thread.  */
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index f32f1fd..c559327 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -1459,6 +1459,17 @@ record_btrace_step_thread (struct thread_info *tp)
 
   DEBUG ("stepping %d (%s): %u", tp->num, target_pid_to_str (tp->ptid), flags);
 
+  if ((btinfo->flags & BTHR_GOTO) != 0)
+    {
+      btinfo->replay = btinfo->goto_target;
+      btinfo->goto_target = NULL;
+      btinfo->flags &= ~BTHR_GOTO;
+
+      xfree (replay);
+
+      return btrace_step_stopped ();
+    }
+
   switch (flags)
     {
     default:
@@ -1635,32 +1646,57 @@ record_btrace_find_new_threads (struct target_ops *ops)
       }
 }
 
-/* Set the replay branch trace instruction iterator.  If IT is NULL, replay
+/* Move to the IT in the branch trace of TP.  If IT is NULL, replay
    is stopped.  */
 
 static void
-record_btrace_set_replay (struct thread_info *tp,
-			  const struct btrace_insn_iterator *it)
+record_btrace_goto_target (struct thread_info *tp,
+			   const struct btrace_insn_iterator *it)
 {
+  struct btrace_insn_iterator *goto_target;
   struct btrace_thread_info *btinfo;
+  struct target_waitstatus ws;
 
   btinfo = &tp->btrace;
 
-  if (it == NULL || it->function == NULL)
-    record_btrace_stop_replaying (tp);
+  if (btinfo->goto_target != NULL || (btinfo->flags & BTHR_GOTO) != 0)
+    error (_("Record goto already in progress."));
+
+  if ((btinfo->flags & BTHR_MOVE) != 0)
+    error (_("Thread is already moving."));
+
+  /* If we're not replaying, to_resume might misinterpret our request. */
+  if (btinfo->replay == NULL)
+    record_btrace_start_replaying (tp);
+
+  if (it == NULL)
+    goto_target = NULL;
   else
     {
-      if (btinfo->replay == NULL)
-	record_btrace_start_replaying (tp);
-      else if (btrace_insn_cmp (btinfo->replay, it) == 0)
-	return;
-
-      *btinfo->replay = *it;
-      registers_changed_ptid (tp->ptid);
+      goto_target = xmalloc (sizeof(*goto_target));
+      *goto_target = *it;
     }
 
-  /* Start anew from the new replay position.  */
-  record_btrace_clear_histories (btinfo);
+  btinfo->flags |= BTHR_GOTO;
+  btinfo->goto_target = goto_target;
+
+#if 0
+  if (goto_target != NULL)
+    tp->control.exception_resume_breakpoint =
+      set_momentary_breakpoint_at_pc (target_gdbarch (),
+				      btrace_insn_get (goto_target)->pc,
+				      bp_until);
+#endif
+#if 0
+  target_resume (tp->ptid, 0, GDB_SIGNAL_0);
+  target_wait (tp->ptid, &ws, 0);
+#else
+  clear_proceed_status ();
+  proceed ((CORE_ADDR) -1, GDB_SIGNAL_0, 0);
+#endif
+
+  if (btinfo->goto_target != NULL || (btinfo->flags & BTHR_GOTO) != 0)
+    error (_("Record goto failed."));
 }
 
 /* The to_goto_record_begin method of target record-btrace.  */
@@ -1674,9 +1710,8 @@ record_btrace_goto_begin (void)
   tp = require_btrace_thread ();
 
   btrace_insn_begin (&begin, &tp->btrace);
-  record_btrace_set_replay (tp, &begin);
 
-  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1);
+  record_btrace_goto_target (tp, &begin);
 }
 
 /* The to_goto_record_end method of target record-btrace.  */
@@ -1688,9 +1723,7 @@ record_btrace_goto_end (void)
 
   tp = require_btrace_thread ();
 
-  record_btrace_set_replay (tp, NULL);
-
-  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1);
+  record_btrace_goto_target (tp, NULL);
 }
 
 /* The to_goto_record method of target record-btrace.  */
@@ -1715,9 +1748,7 @@ record_btrace_goto (ULONGEST insn)
   if (found == 0)
     error (_("No such instruction."));
 
-  record_btrace_set_replay (tp, &it);
-
-  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1);
+  record_btrace_goto_target (tp, &it);
 }
 
 /* Initialize the record-btrace target ops.  */

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052