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

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

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 2521 bytes --]

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

[-- Attachment #2: fix-function-tail-stack-same.txt --]
[-- Type: text/plain, Size: 695 bytes --]

---
 infrun.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--- a/infrun.c
+++ b/infrun.c
@@ -3397,7 +3397,10 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
      beyond the step range, and NOT the address of the last instruction
      within it! */
   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
+      && (frame_id_eq (get_frame_id (get_current_frame ()),
+		      ecs->event_thread->step_frame_id)
+          || execution_direction != EXEC_REVERSE))
     {
       if (debug_infrun)
 	fprintf_unfiltered (gdb_stdlog, "infrun: stepping inside range [0x%s-0x%s]\n",

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

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 3252 bytes --]

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
>

[-- Attachment #2: fix-function-tail-stack-same.txt --]
[-- Type: text/plain, Size: 695 bytes --]

---
 infrun.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--- a/infrun.c
+++ b/infrun.c
@@ -3421,7 +3421,10 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
      beyond the step range, and NOT the address of the last instruction
      within it! */
   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
+      && (frame_id_eq (get_frame_id (get_current_frame ()),
+		      ecs->event_thread->step_frame_id)
+          || execution_direction != EXEC_REVERSE))
     {
       if (debug_infrun)
 	fprintf_unfiltered (gdb_stdlog, "infrun: stepping inside range [0x%s-0x%s]\n",

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

[Hui Zhu]

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

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

[Hui Zhu]

PING

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

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

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 3958 bytes --]

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?

Thx again,
Michael


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


[-- Attachment #2: tail.txt --]
[-- Type: text/plain, Size: 1303 bytes --]

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

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

[Hui Zhu]

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?

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

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

[Marc Khouzam]

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

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

[Michael Snyder]

Marc Khouzam wrote:
>> -----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.

Great, so sounds like this one can go in.

Michael

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

[Michael Snyder]

Michael Snyder wrote:
> Marc Khouzam wrote:
>>> -----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.
> 
> Great, so sounds like this one can go in.

... and committed.