Re: [patch v4 23/24] record-btrace: add (reverse-)stepping support

[Jan Kratochvil <jan.kratochvil@redhat.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