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: "Metzger, Markus T" <markus dot t dot metzger at intel dot com>
- To: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Tue, 17 Sep 2013 09:43:28 +0000
- 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>
> -----Original Message-----
> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com]
> Sent: Sunday, August 18, 2013 9:10 PM
> To: Metzger, Markus T
Thanks for your review.
> > 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.
That's the place. I don't have any code that detects this.
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.
> > 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.
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?
> 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.
> > 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.
> > + 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.
I need the address space which is stored in the inferior struct.
Regards,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052