RE: [RFA] Patch to fix reverse-debug recursion function tail bug

["Marc Khouzam" <marc dot khouzam at ericsson dot com>]

> -----Original Message-----
> From: Hui Zhu [mailto:teawater@gmail.com] 
> Sent: June-14-09 11:37 PM
> To: Michael Snyder; Marc Khouzam
> Cc: gdb-patches@sourceware.org
> Subject: Re: [RFA] Patch to fix reverse-debug recursion 
> function tail bug
> 
> On Mon, Jun 15, 2009 at 08:55, Michael 
> Snyder<msnyder@vmware.com> wrote:
> > Hui Zhu wrote:
> >>
> >> PING
> >
> > Thanks for the reminder.
> >
> > I added some comment and changed the order of evaluation a bit,
> > hoping to reduce the performance impact on normal debugging.
> > And I ran the testsuites, before and after.
> >
> > Modified patch is attached -- is this OK with you guys?
> > Mark, can you confirm that it fixes your original bug?
> >
> 
> This patch is OK with me.
> Marc, what do you think about it?

I tested before and after the patch and it does fix
the problem for me.

Thanks!

> 
> Thanks,
> Hui
> 
> 
> >
> >
> >> On Mon, May 11, 2009 at 15:07, Hui Zhu<teawater@gmail.com> wrote:
> >>>
> >>> PING
> >>>
> >>> On Wed, May 6, 2009 at 15:23, Hui Zhu <teawater@gmail.com> wrote:
> >>>>
> >>>> Hi Michael,
> >>>>
> >>>> Like the prev patch I send to you, this issue still 
> affect cvs-head
> >>>> and the patch can fix it.
> >>>> Please help me review it.
> >>>>
> >>>> The attachment is the new patch follow cvs-head.
> >>>>
> >>>> 2009-05-06 ?Hui Zhu ?<teawater@gmail.com>
> >>>>
> >>>> ? ? ?* infrun.c (handle_inferior_event): Check frame_id when
> >>>> ? ? ?check range in reverse debug mode.
> >>>>
> >>>> Thanks,
> >>>> Hui
> >>>>
> >>>> On Sat, Mar 21, 2009 at 16:52, Hui Zhu 
> <teawater@gmail.com> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> This patch is for bug report by Marc in
> >>>>> http://sourceware.org/ml/gdb/2009-03/msg00127.html.
> >>>>>
> >>>>> This bug in "handle_inferior_event" deal with recursion 
> function tail
> >>>>> in reverse debug.
> >>>>> infrun: infwait_normal_state
> >>>>> infrun: TARGET_WAITKIND_STOPPED
> >>>>> infrun: stop_pc = 0x8048457
> >>>>> infrun: stepping inside range [0x8048457-0x804845a]
> >>>>> infrun: stop_stepping
> >>>>> factorial (x=4) at b.cc:5
> >>>>>
> >>>>> Inferior already step into another frame. But because this is a
> >>>>> recursion function call, And 0x8048457 is in
> >>>>> ecs->event_thread->step_range_start and
> >>>>> ecs->event_thread->step_range_start.
> >>>>>
> >>>>> So gdb run in:
> >>>>>
> >>>>> if (stop_pc >= ecs->event_thread->step_range_start
> >>>>> ? ? && stop_pc < ecs->event_thread->step_range_end)
> >>>>> ? {
> >>>>>
> >>>>> This code is in front of:
> >>>>> ?if (!frame_id_eq (get_frame_id (get_current_frame ()),
> >>>>> ? ? ? ? ? ? ? ? ? ecs->event_thread->step_frame_id)
> >>>>> ? ? && (frame_id_eq (frame_unwind_id (get_current_frame ()),
> >>>>> ? ? ? ? ? ? ? ? ? ? ?ecs->event_thread->step_frame_id)
> >>>>> ? ? ? ? || execution_direction == EXEC_REVERSE))
> >>>>>
> >>>>> So gdb check range without check frame_id.
> >>>>>
> >>>>> So I make a patch to check frame_id when check range in 
> reverse debug
> >>>>> mode.
> >>>>>
> >>>>> 2008-03-21 ?Hui Zhu ?<teawater@gmail.com>
> >>>>>
> >>>>> ? ? ? * infrun.c (handle_inferior_event): Check frame_id when
> >>>>> ? ? ? check range in reverse debug mode.
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> Actually, there is another thing, when gdb begin 
> reverse-debug, it's
> >>>>> range is:
> >>>>> ?8048439: ? ? ? 8b 45 08 ? ? ? ? ? ? ? ?mov ? ?0x8(%ebp),%eax
> >>>>> ?804843c: ? ? ? 83 e8 01 ? ? ? ? ? ? ? ?sub ? ?$0x1,%eax
> >>>>> ?804843f: ? ? ? 89 04 24 ? ? ? ? ? ? ? ?mov ? ?%eax,(%esp)
> >>>>> ?8048442: ? ? ? e8 dd ff ff ff ? ? ? ? ?call ? 8048424 
> <_Z9factoriali>
> >>>>> ?8048447: ? ? ? 0f af 45 08 ? ? ? ? ? ? imul ? 0x8(%ebp),%eax
> >>>>> ?804844b: ? ? ? 89 45 fc ? ? ? ? ? ? ? ?mov ? ?%eax,-0x4(%ebp)
> >>>>> Why is changed to infrun: stepping inside range 
> [0x8048457-0x804845a]?
> >>>>> That is because when inferior step at:
> >>>>> ?8048458: ? ? ? c3 ? ? ? ? ? ? ? ? ? ? ?ret
> >>>>> In this address, $ebp is same with high level function and this
> >>>>> function is factorial too.
> >>>>> So the gdb can't found inferior step into another 
> frame. ?It will run
> >>>>> to:
> >>>>> ?ecs->event_thread->step_range_start = stop_pc_sal.pc;
> >>>>> ?ecs->event_thread->step_range_end = stop_pc_sal.end;
> >>>>> ?ecs->event_thread->step_frame_id = get_frame_id 
> (get_current_frame
> >>>>> ());
> >>>>> ?ecs->event_thread->current_line = stop_pc_sal.line;
> >>>>> ?ecs->event_thread->current_symtab = stop_pc_sal.symtab;
> >>>>>
> >>>>> ?if (debug_infrun)
> >>>>> ? ?fprintf_unfiltered (gdb_stdlog, "infrun: keep going\n");
> >>>>> ?keep_going (ecs);
> >>>>> }
> >>>>> So ecs->event_thread->step_range_start and
> >>>>> ecs->event_thread->step_range_end.
> >>>>>
> >>>>> I don't find that it affect the reverse debug or 
> something. ?So I
> >>>>> didn't fix it.
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> Thanks,
> >>>>> Hui
> >>>>>
> >>
> >>
> >
> >
> > 2009-06-14 ?Hui Zhu ?<teawater@gmail.com>
> > ? ? ? ? ? ?Michael Snyder ?<msnyder@vmware.com>
> >
> > ? ? ? ?* infrun.c (handle_inferior_event): Improve reverse stepping
> > ? ? ? ?through function epilogue.
> >
> > Index: infrun.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/infrun.c,v
> > retrieving revision 1.387
> > diff -u -p -r1.387 infrun.c
> > --- infrun.c ? ?11 Jun 2009 11:57:46 -0000 ? ? ?1.387
> > +++ infrun.c ? ?15 Jun 2009 00:45:17 -0000
> > @@ -3623,9 +3623,17 @@ infrun: not switching back to stepped th
> >
> > ? ? ?Note that step_range_end is the address of the first 
> instruction
> > ? ? ?beyond the step range, and NOT the address of the last 
> instruction
> > - ? ? within it! */
> > + ? ? within it!
> > +
> > + ? ? Note also that during reverse execution, we may be stepping
> > + ? ? through a function epilogue and therefore must detect when
> > + ? ? the current-frame changes in the middle of a line. ?*/
> > +
> > ? if (stop_pc >= ecs->event_thread->step_range_start
> > - ? ? ?&& stop_pc < ecs->event_thread->step_range_end)
> > + ? ? ?&& stop_pc < ecs->event_thread->step_range_end
> > + ? ? ?&& (execution_direction != EXEC_REVERSE
> > + ? ? ? ? || frame_id_eq (get_frame_id (get_current_frame ()),
> > + ? ? ? ? ? ? ? ? ? ? ? ? ecs->event_thread->step_frame_id)))
> > ? ? {
> > ? ? ? if (debug_infrun)
> > ? ? ? ?fprintf_unfiltered (gdb_stdlog, "infrun: stepping 
> inside range
> > [0x%s-0x%s]\n",
> >
> >
>