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

Jan Kratochvil
2013-11-28T21:16:38Z

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

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

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

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;
}

[-- Attachment #3: 2 --]
[-- Type: text/plain, Size: 2047 bytes --]

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? */

[-- Attachment #4: 3 --]
[-- Type: text/plain, Size: 1619 bytes --]

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. */