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: "Metzger, Markus T" <markus dot t dot metzger at intel dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Sun, 29 Sep 2013 19:24:16 +0200
- Subject: Re: [patch v4 23/24] record-btrace: add (reverse-)stepping support
- Authentication-results: sourceware.org; auth=none
- 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> <20130818190935 dot GR24153 at host2 dot jankratochvil dot net> <A78C989F6D9628469189715575E55B230A9CF4EA at IRSMSX104 dot ger dot corp dot intel dot com>
On Tue, 17 Sep 2013 11:43:28 +0200, Metzger, Markus T wrote:
> But this code compares a NORMAL_FRAME from before the step with a
> BTRACE_FRAME from after the wait. They will always be unequal hence
> we will never recognize that we just reverse-stepped into a function.
>
> When I reset the frame cache, GDB re-computes the stored frame and now
> compares two BTRACE_FRAMEs, which works OK.
[...]
> See above. Alternatively, I might add a special case to frame comparison,
> but this would be quite ugly, as well. Do you have a better idea?
+record_btrace_start_replaying (struct thread_info *tp)
[...]
+ /* 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);
The problem comes from the new commands like "record goto" which change
inferior content without resuming+stopping it.
Former "record full" could only change history position by "step/reverse-step"
(or similar commands) which did resume+stop the inferior.
To make the "record goto" command friendly to the GDB infrastructure
expectations I think you should put a temporary breakpoint to the target
instruction, resume the inferior and simulate stop at the temporary
breakpoint.
I think all the registers_changed_ptid() calls could be removed afterwards.
> > 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.
>
> I don't think performance is an issue, here. I tried that and it didn't seem
> to stop correctly resulting in lots of test fails. I have not investigated it.
My idea was wrong, handle_inferior_event checks for
breakpoint_inserted_here_p() only if it sees GDB_SIGNAL_TRAP. With
TARGET_WAITKIND_SPURIOUS it does not notice any breakpoint.
(One could return TARGET_WAITKIND_SPURIOUS instead of looping in
BTHR_CONT+BTHR_RCONT but that has no advantage, it is just slower.)
And sure reporting GDB_SIGNAL_TRAP without breakpoint_inserted_here_p() also
does not work, that ends up with:
Program received signal SIGTRAP, Trace/breakpoint trap.
So I agree with your implementation, record-full.c also does it that way.
> > > 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?
>
> Record-full is not supporting non-stop, either. I'm wondering what other
> issues I might run into with non-stop mode that I am currently not aware of.
I do not know an answer without trying it.
Thanks,
Jan