[RFA] Patch to fix reverse return from subroutine error

[RFA] Patch to fix reverse return from subroutine error

[Hui Zhu]

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

Hi,

This patch is to fix bug in http://sourceware.org/ml/gdb/2009-06/msg00089.html

2009-06-11  Hui Zhu  <teawater@gmail.com>

	* infrun.c (handle_inferior_event): Reverse return from
	subroutine doesn't use code "infrun: stepped into subroutine".

There still are 3 reverse patches like it need be review:
http://sourceware.org/ml/gdb-patches/2009-05/msg00103.html
http://sourceware.org/ml/gdb-patches/2009-05/msg00104.html
http://sourceware.org/ml/gdb-patches/2009-05/msg00105.html

And other one need to be comment:
http://sourceware.org/ml/gdb-patches/2009-05/msg00078.html

Maybe some nice people can spend sometime on these poor babies.  :)

Thanks,
Hui

[-- Attachment #2: fix-reverse-return-from-subroutine.txt --]
[-- Type: text/plain, Size: 740 bytes --]

---
 infrun.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

--- a/infrun.c
+++ b/infrun.c
@@ -3708,7 +3708,12 @@ infrun: not switching back to stepped th
 		    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))
+	  || (execution_direction == EXEC_REVERSE
+	      && ecs->event_thread->step_frame_id.stack_addr_p
+	      && get_frame_id (get_current_frame ()).stack_addr_p
+              && !gdbarch_inner_than (current_gdbarch,
+				      ecs->event_thread->step_frame_id.stack_addr,
+				      get_frame_id (get_current_frame()).stack_addr))))
     {
       CORE_ADDR real_stop_pc;
 

Re: [RFA] Patch to fix reverse return from subroutine error

[Michael Snyder]

Hui Zhu wrote:
> Hi,
> 
> This patch is to fix bug in http://sourceware.org/ml/gdb/2009-06/msg00089.html
> 
> 2009-06-11  Hui Zhu  <teawater@gmail.com>
> 
> 	* infrun.c (handle_inferior_event): Reverse return from
> 	subroutine doesn't use code "infrun: stepped into subroutine".

Hi Hui,

This patch is approved, if you'll make the changes shown below.


> ------------------------------------------------------------------------
> 
> ---
>  infrun.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> --- a/infrun.c
> +++ b/infrun.c
> @@ -3708,7 +3708,12 @@ infrun: not switching back to stepped th
>  		    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))
> +	  || (execution_direction == EXEC_REVERSE
> +	      && ecs->event_thread->step_frame_id.stack_addr_p
> +	      && get_frame_id (get_current_frame ()).stack_addr_p

Replace "get_current_frame ()" with "frame".

> +              && !gdbarch_inner_than (current_gdbarch,

Replace "current_gdbarch" with "gdbarch".

> +				      ecs->event_thread->step_frame_id.stack_addr,
> +				      get_frame_id (get_current_frame()).stack_addr))))

Replace "get_current_frame()" with "frame".

>      {
>        CORE_ADDR real_stop_pc;
>  

Thanks,
Michael

Re: [RFA] Patch to fix reverse return from subroutine error

[Pedro Alves]

> -         || execution_direction == EXEC_REVERSE))
> +         || (execution_direction == EXEC_REVERSE
> +             && ecs->event_thread->step_frame_id.stack_addr_p
> +             && get_frame_id (get_current_frame ()).stack_addr_p
> +              && !gdbarch_inner_than (current_gdbarch,
> +                                     ecs->event_thread->step_frame_id.stack_addr,
> +                                     get_frame_id

Sorry to pitch in so late, but this doesn't look right to me.
Common code shouldn't be accessing frame id members directly, frame ids
are supposed to be opaque.  What is this trying to do?

-- 
Pedro Alves

Re: [RFA] Patch to fix reverse return from subroutine error

[Daniel Jacobowitz]

On Sat, Jun 27, 2009 at 08:49:35PM +0100, Pedro Alves wrote:
> > -         || execution_direction == EXEC_REVERSE))
> > +         || (execution_direction == EXEC_REVERSE
> > +             && ecs->event_thread->step_frame_id.stack_addr_p
> > +             && get_frame_id (get_current_frame ()).stack_addr_p
> > +              && !gdbarch_inner_than (current_gdbarch,
> > +                                     ecs->event_thread->step_frame_id.stack_addr,
> > +                                     get_frame_id
> 
> Sorry to pitch in so late, but this doesn't look right to me.
> Common code shouldn't be accessing frame id members directly, frame ids
> are supposed to be opaque.  What is this trying to do?

Nor should it be using gdbarch_inner_than this way; Ulrich just got
through removing most of those.  Stack frames do not necessarily need
to be inner than the previous frame.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Patch to fix reverse return from subroutine error

[Michael Snyder]

Pedro Alves wrote:
>> -         || execution_direction == EXEC_REVERSE))
>> +         || (execution_direction == EXEC_REVERSE
>> +             && ecs->event_thread->step_frame_id.stack_addr_p
>> +             && get_frame_id (get_current_frame ()).stack_addr_p
>> +              && !gdbarch_inner_than (current_gdbarch,
>> +                                     ecs->event_thread->step_frame_id.stack_addr,
>> +                                     get_frame_id
> 
> Sorry to pitch in so late, but this doesn't look right to me.
> Common code shouldn't be accessing frame id members directly, frame ids
> are supposed to be opaque.  What is this trying to do?

It's trying to answer the question "have we stepped into a
subroutine call?", in reverse.  This unfortunately involves
corner cases that we don't see when we're going forward.

Originally the code just looked (approximately) like this:

   /* Check for subroutine calls.  The check for the current frame
      equalling the step ID is not necessary - the check of the
      previous frame's ID is sufficient - but it is a common case and
      cheaper than checking the previous frame's ID.  */

   if (!frame_id_eq (get_frame_id (frame), step_frame_id)
       && frame_id_eq (frame_unwind_id (frame), step_frame_id))

The problem is that the second "frame_id_eq" test fails in
the case where we've just stepped backward to the RET instruction
of a function which, in forward-time, had just returned.

It's possible that what we're trying to do here is work around a
bug in the i386 implementation of frame_unwind_id.  When I look at
the frame_id that it returns at this point, it does not match either
the caller or the callee, and its code_addr is particularly wrong.

We don't encounter this situation in forward execution, because
it is caught earler by the stepping-within-line-range code, and
we never reach this test on the RET instruction.

Re: [RFA] Patch to fix reverse return from subroutine error

[Daniel Jacobowitz]

On Sat, Jun 27, 2009 at 01:57:22PM -0700, Michael Snyder wrote:
> The problem is that the second "frame_id_eq" test fails in
> the case where we've just stepped backward to the RET instruction
> of a function which, in forward-time, had just returned.
> 
> It's possible that what we're trying to do here is work around a
> bug in the i386 implementation of frame_unwind_id.  When I look at
> the frame_id that it returns at this point, it does not match either
> the caller or the callee, and its code_addr is particularly wrong.

I wrote about this problem in my GCC summit paper.  All released
versions of GCC generate unwind info that is wrong in epilogues.
I believe it's fixed in trunk GCC, although maybe for specific
platforms only.

In order to get this to work for my demo I had an epilogue-specific
unwinder for ARM.  It detected common epilogue sequences and analyzed
them to find the frame ID, and was installed at higher priority than
the DWARF unwinder.

I think you're going to need the same thing here, or else use the
existing gdbarch epilogue hook somehow.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Patch to fix reverse return from subroutine error

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Sat, Jun 27, 2009 at 01:57:22PM -0700, Michael Snyder wrote:
>> The problem is that the second "frame_id_eq" test fails in
>> the case where we've just stepped backward to the RET instruction
>> of a function which, in forward-time, had just returned.
>>
>> It's possible that what we're trying to do here is work around a
>> bug in the i386 implementation of frame_unwind_id.  When I look at
>> the frame_id that it returns at this point, it does not match either
>> the caller or the callee, and its code_addr is particularly wrong.
> 
> I wrote about this problem in my GCC summit paper.  All released
> versions of GCC generate unwind info that is wrong in epilogues.
> I believe it's fixed in trunk GCC, although maybe for specific
> platforms only.
> 
> In order to get this to work for my demo I had an epilogue-specific
> unwinder for ARM.  It detected common epilogue sequences and analyzed
> them to find the frame ID, and was installed at higher priority than
> the DWARF unwinder.
> 
> I think you're going to need the same thing here, or else use the
> existing gdbarch epilogue hook somehow.

Great, I looked up your paper, and found it helpful.
Are your gdb patches available online?  Any head start
would be a boon...

Michael

Re: [RFA] Patch to fix reverse return from subroutine error

[Daniel Jacobowitz]

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

On Sun, Jun 28, 2009 at 11:44:37AM -0700, Michael Snyder wrote:
> Great, I looked up your paper, and found it helpful.
> Are your gdb patches available online?  Any head start
> would be a boon...

I think I've posted them before, but I can't remember where.  A copy
is attached.  The epilogue bits are in patch-4.

-- 
Daniel Jacobowitz
CodeSourcery

[-- Attachment #2: reverse-patches.tar --]
[-- Type: application/x-tar, Size: 13127 bytes --]

Re: [RFA] Patch to fix reverse return from subroutine error

[Hui Zhu]

Sorry for late everybody.
Looks like we still have trouble with it,right?

We have plan B  for this issue.  This is a old implement when p record
still have reverse command inside it.

This issue is because we don't know the frame_info of step_frame_id.
Without it, we cannot get the frame_info of high level function with
function get_prev_frame.
Without frame_info, we cannot get the frame_id.
Then we cannot make sure if this inferior return from subroutine.

So we can save this prev_frame_id when we save step_frame_id in reverse mode.
in step_once,
      frame = get_current_frame ();
      tp->step_frame_id = get_frame_id (frame);
Update it to inferior_status.
And update it in handle_inferior_event:
  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 (frame);
  ecs->event_thread->current_line = stop_pc_sal.line;
  ecs->event_thread->current_symtab = stop_pc_sal.symtab;

Then, we can use it to make sure if inferior return from subroutine or not.

What do you think about it?


Thanks,
Hui



On Mon, Jun 29, 2009 at 05:08, Daniel Jacobowitz<drow@false.org> wrote:
> On Sun, Jun 28, 2009 at 11:44:37AM -0700, Michael Snyder wrote:
>> Great, I looked up your paper, and found it helpful.
>> Are your gdb patches available online?  Any head start
>> would be a boon...
>
> I think I've posted them before, but I can't remember where.  A copy
> is attached.  The epilogue bits are in patch-4.
>
> --
> Daniel Jacobowitz
> CodeSourcery
>