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

["Metzger, Markus T" <markus.t.metzger@intel.com>]

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