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 ();
> +	}
> +