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

Jan Kratochvil
2013-11-27T18:58:55Z

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

On Thu, 07 Nov 2013 16:41:40 +0100, Metzger, Markus T wrote:
> 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.

In such case it is not finished. These hacks should not be needed.

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

I have patched what you describe as the problem. But as I do not have a box
with reliably working BTS so it is difficult for me to say whether it works or
not. I can look at other problems if you describe them from a reliable box.

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

It can be cleaned up as I did there, thanks for catching it.

Thanks,
Jan

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

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index c50e11b..9fbff15 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -1300,16 +1300,6 @@ record_btrace_start_replaying (struct thread_info *tp)
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 = get_current_frame_nocheck ();
- insn = btrace_insn_get (replay);
- sal = find_pc_line (insn->pc, 0);
- set_step_info (frame, sal);
-
return replay;
}

@@ -1324,9 +1314,6 @@ record_btrace_stop_replaying (struct thread_info *tp)

xfree (btinfo->replay);
btinfo->replay = NULL;
-
- /* Make sure we're not leaving any stale registers. */
- registers_changed_ptid (tp->ptid);
}

/* The to_resume method of target record-btrace. */
@@ -1619,10 +1606,6 @@ record_btrace_wait (struct target_ops *ops, ptid_t ptid,
/* Start record histories anew from the current position. */
record_btrace_clear_histories (&tp->btrace);

- /* GDB seems to need this. Without, a stale PC seems to be used resulting in
- the current location to be displayed incorrectly. */
- registers_changed_ptid (tp->ptid);
-
return tp->ptid;
}

@@ -1662,6 +1645,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 +1671,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 +1692,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. */