Re: [patch v4 23/24] record-btrace: add (reverse-)stepping support

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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

On Wed, 03 Jul 2013 11:14:33 +0200, Markus Metzger wrote:
> There's an open regarding frame unwinding.  When I start stepping, the frame
> cache will still be based on normal unwinding as will the frame cached in the
> thread's stepping context.  This will prevent me from detecting that i stepped
> into a subroutine.

Where do you detect you have stepped into a subroutine? That is up to GDB
after your to_wait returns, in handle_inferior_event.


> To overcome that, I'm resetting the frame cache and setting the thread's
> stepping cache based on the current frame - which is now computed using branch
> tracing unwind.  I had to split get_current_frame to avoid checks that would
> prevent me from doing this.

This is not correct, till to_wait finishes the inferior is still executing and
you cannot query its current state (such as its frame/pc/register).

I probably still miss why you do so.


Proposing some hacked draft patch but for some testcases it FAILs for me; but
they FAIL even without this patch as I run it on Nehalem.  I understand I may
miss some problem there, though.


> It looks like I don't need any special support for breakpoints.  Is there a
> scenario where normal breakpoints won't work?

You already handle it specially in BTHR_CONT and in BTHR_RCONT by
breakpoint_here_p.  As btrace does not record any data changes that may be
enough.  "record full" has different situation as it records data changes.
I think it is fine as you wrote it.

You could handle BTHR_CONT and BTHR_RCONT equally to BTHR_STEP and BTHR_RSTEP,
just returning TARGET_WAITKIND_SPURIOUS instead of TARGET_WAITKIND_STOPPED.
This way you would not need to handle specially breakpoint_here_p.
But it would be sure slower.


> Non-stop mode is not working.  Do not allow record-btrace in non-stop mode.

While that seems OK for the initial check-in I do not think it is convenient.
Some users use for example Eclipse in non-stop mode, they would not be able to
use btrace then as one cannot change non-stop state when the inferior is
running.  You can just disable the ALL_THREADS cases in record-btrace.c, can't
you?


This mail is not really reviewed yet as the design should be settled down
first.


> --- a/gdb/btrace.h
> +++ b/gdb/btrace.h
> @@ -149,6 +149,25 @@ struct btrace_call_history
>    struct btrace_call_iterator end;
>  };
>  
> +/* Branch trace thread flags.  */
> +enum btrace_thread_flag
> +  {

enum btrace_thread_flag
{


> +    /* The thread is to be stepped forwards.  */
> +    BTHR_STEP = (1 << 0),
> +
> +    /* The thread is to be stepped backwards.  */
> +    BTHR_RSTEP = (1 << 1),
> +
> +    /* The thread is to be continued forwards.  */
> +    BTHR_CONT = (1 << 2),
> +
> +    /* The thread is to be continued backwards.  */
> +    BTHR_RCONT = (1 << 3),
> +
> +    /* The thread is to be moved.  */
> +    BTHR_MOVE = (BTHR_STEP | BTHR_RSTEP | BTHR_CONT | BTHR_RCONT)
> +  };
> +
>  /* Branch trace information per thread.
>  
>     This represents the branch trace configuration as well as the entry point
> @@ -176,6 +195,9 @@ struct btrace_thread_info
>       becomes zero.  */
>    int level;
>  
> +  /* A bit-vector of btrace_thread_flag.  */
> +  unsigned int flags;

enum btrace_thread_flag
The values are then also properly displayed by GDB.


> +
>    /* The instruction history iterator.  */
>    struct btrace_insn_history *insn_history;
>  
[...]
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -1367,6 +1367,29 @@ 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)
>  {


> @@ -1381,6 +1404,7 @@ get_current_frame (void)
>      error (_("No stack."));
>    if (!target_has_memory)
>      error (_("No memory."));
> +
>    /* Traceframes are effectively a substitute for the live inferior.  */
>    if (get_traceframe_number () < 0)
>      {

Unrelated patch chunk.  But the get_current_frame() part of the patch should
be dropped anyway.


> @@ -1392,19 +1416,7 @@ get_current_frame (void)
>  	error (_("Target is executing."));
>      }
>  
> -  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;
> +  return get_current_frame_nocheck ();
>  }
>  
>  /* The "selected" stack frame is used by default for local and arg
[...]
> +    case BTHR_CONT:
> +      /* We're done if we're not replaying.  */
> +      if (replay == NULL)
> +	return btrace_step_no_history ();
> +
> +      /* I'd much rather go from TP to its inferior, but how?  */

find_inferior_pid (ptid_get_pid (tp->ptid))
Although I do not see why you prefer the inferior here.


> +      aspace = current_inferior ()->aspace;
> +
> +      /* Determine the end of the instruction trace.  */
> +      btrace_insn_end (&end, btinfo);
> +
> +      for (;;)
> +	{
> +	  const struct btrace_insn *insn;
> +
> +	  /* We are always able to step at least once.  */
> +	  steps = btrace_insn_next (replay, 1);
> +	  gdb_assert (steps == 1);
> +
> +	  /* We stop replaying if we reached the end of the trace.  */
> +	  if (btrace_insn_cmp (replay, &end) == 0)
> +	    {
> +	      record_btrace_stop_replaying (btinfo);
> +	      return btrace_step_no_history ();
> +	    }
> +
> +	  insn = btrace_insn_get (replay);
> +	  gdb_assert (insn);
> +
> +	  DEBUG ("stepping %d (%s) ... %s", tp->num,
> +		 target_pid_to_str (tp->ptid),
> +		 core_addr_to_string_nz (insn->pc));
> +
> +	  if (breakpoint_here_p (aspace, insn->pc))
> +	    return btrace_step_stopped ();
> +	}
> +

[-- Attachment #2: btrace-towait.patch --]
[-- Type: text/plain, Size: 4297 bytes --]

diff --git a/gdb/btrace.h b/gdb/btrace.h
index 22fabb5..8eceec4 100644
--- a/gdb/btrace.h
+++ b/gdb/btrace.h
@@ -27,6 +27,7 @@
    list of sequential control-flow blocks, one such list per thread.  */
 
 #include "btrace-common.h"
+#include "target.h"
 
 struct thread_info;
 struct btrace_function;
@@ -198,6 +199,8 @@ struct btrace_thread_info
   /* A bit-vector of btrace_thread_flag.  */
   unsigned int flags;
 
+struct target_waitstatus status;
+
   /* The instruction history iterator.  */
   struct btrace_insn_history *insn_history;
 
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 9feda30..633990a 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -1190,6 +1190,8 @@ static const struct frame_unwind record_btrace_frame_unwind =
   record_btrace_frame_dealloc_cache
 };
 
+static struct target_waitstatus record_btrace_step_thread (struct thread_info *tp);
+
 /* Indicate that TP should be resumed according to FLAG.  */
 
 static void
@@ -1209,6 +1211,10 @@ record_btrace_resume_thread (struct thread_info *tp,
   btrace_fetch (tp);
 
   btinfo->flags |= flag;
+
+
+/* We only move a single thread.  We're not able to correlate threads.  */
+btinfo->status = record_btrace_step_thread (tp);
 }
 
 /* Find the thread to resume given a PTID.  */
@@ -1248,6 +1254,7 @@ record_btrace_start_replaying (struct btrace_thread_info *btinfo)
   gdb_assert (btinfo->replay == NULL);
   btinfo->replay = replay;
 
+#if 0
   /* Make sure we're not using any stale registers or frames.  */
   registers_changed ();
   reinit_frame_cache ();
@@ -1258,6 +1265,7 @@ record_btrace_start_replaying (struct btrace_thread_info *btinfo)
   insn = btrace_insn_get (replay);
   sal = find_pc_line (insn->pc, 0);
   set_step_info (frame, sal);
+#endif
 
   return replay;
 }
@@ -1271,6 +1279,8 @@ record_btrace_stop_replaying (struct btrace_thread_info *btinfo)
   btinfo->replay = NULL;
 }
 
+static int forward_to_beneath;
+
 /* The to_resume method of target record-btrace.  */
 
 static void
@@ -1290,7 +1300,9 @@ record_btrace_resume (struct target_ops *ops, ptid_t ptid, int step,
       record_btrace_stop_replaying (&other->btrace);
 
   /* As long as we're not replaying, just forward the request.  */
-  if (!record_btrace_is_replaying () && execution_direction != EXEC_REVERSE)
+  forward_to_beneath = (!record_btrace_is_replaying ()
+                        && execution_direction != EXEC_REVERSE);
+  if (forward_to_beneath)
     {
       for (ops = ops->beneath; ops != NULL; ops = ops->beneath)
 	if (ops->to_resume != NULL)
@@ -1400,7 +1412,7 @@ record_btrace_step_thread (struct thread_info *tp)
   replay = btinfo->replay;
 
   flag = btinfo->flags & BTHR_MOVE;
-  btinfo->flags &= ~BTHR_MOVE;
+//  btinfo->flags &= ~BTHR_MOVE;
 
   DEBUG ("stepping %d (%s): %u", tp->num, target_pid_to_str (tp->ptid), flag);
 
@@ -1517,7 +1529,7 @@ record_btrace_wait (struct target_ops *ops, ptid_t ptid,
   DEBUG ("wait %s (0x%x)", target_pid_to_str (ptid), options);
 
   /* As long as we're not replaying, just forward the request.  */
-  if (!record_btrace_is_replaying () && execution_direction != EXEC_REVERSE)
+  if (forward_to_beneath)
     {
       for (ops = ops->beneath; ops != NULL; ops = ops->beneath)
 	if (ops->to_wait != NULL)
@@ -1536,8 +1548,11 @@ record_btrace_wait (struct target_ops *ops, ptid_t ptid,
       return minus_one_ptid;
     }
 
+#if 0
   /* We only move a single thread.  We're not able to correlate threads.  */
   *status = record_btrace_step_thread (tp);
+#endif
+*status=tp->btrace.status;
 
   /* Stop all other threads. */
   if (!non_stop)
@@ -1547,9 +1562,11 @@ record_btrace_wait (struct target_ops *ops, ptid_t ptid,
   /* Start record histories anew from the current position.  */
   record_btrace_clear_histories (&tp->btrace);
 
+#if 0
   /* GDB seems to need this.  Without, a stale PC seems to be used resulting in
      the current location to be displayed incorrectly.  */
   registers_changed ();
+#endif
 
   return tp->ptid;
 }
diff --git a/gdb/target.h b/gdb/target.h
index 4a20533..e85b063 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -62,7 +62,7 @@ struct expression;
 #include "memattr.h"
 #include "vec.h"
 #include "gdb_signals.h"
-#include "btrace.h"
+#include "btrace-common.h"
 #include "command.h"
 
 enum strata