This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch v4 23/24] record-btrace: add (reverse-)stepping support
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: Markus Metzger <markus dot t dot metzger at intel dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Sun, 18 Aug 2013 21:09:35 +0200
- Subject: Re: [patch v4 23/24] record-btrace: add (reverse-)stepping support
- References: <1372842874-28951-1-git-send-email-markus dot t dot metzger at intel dot com> <1372842874-28951-24-git-send-email-markus dot t dot metzger at intel dot com>
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 ();
> + }
> +
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