[PATCH] Fix for 15072: stopped response missing for reverse-finish

[PATCH] Fix for 15072: stopped response missing for reverse-finish

[Andreas Kaufmann]

Hi!

The following patch fixes the issue with "*stopped" response missing (for MI interface) when 
"reverse-finish" command hits a breakpoint. Please see http://sourceware.org/bugzilla/show_bug.cgi?id=15072
This is quite nasty bug when using reverse debugging with Eclipse. Basically, without "*stopped" 
response Eclipse will show state as "running" and the user has to restart the debug session.

Here is explanation why this change is needed.
GDB code has special handling for "forward finish", since returned value is included in the response:
static void
finish_forward (struct symbol *function, struct frame_info *frame)
{
. . .
  add_continuation (tp, finish_command_continuation, cargs,
                    finish_command_continuation_free_arg);
. . . 
  
static void
finish_command_continuation (void *arg, int err)
{
. . .
observer_notify_normal_stop (bs, 1 /* print frame */);
 
The above is not done (and nor needed) for "reverse-finish". So I added additional
check  "execution_direction != EXEC_REVERSE" to ensure that observer_notify_normal_stop()
is called for "reverse-finish" from normal_stop() in gdb/infrun.c.

Please let me know if you have any comments! 

Thanks,
Andreas Kaufmann
Synopsys Inc



2013-04-18 Andreas Kaufmann <Andreas.Kaufmann@synopsys.com>

    PR gdb/15072
    * infrun.c (normal_stop): Ensure "observer_notify_normal_stop" is called
      when "reverse-finish" command hits a breakpoint.


Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.577
diff -u -p -r1.577 infrun.c
--- infrun.c          13 Mar 2013 18:34:53 -0000          1.577
+++ infrun.c       18 Apr 2013 13:45:09 -0000
@@ -6149,7 +6149,8 @@ done:
       || (!(inferior_thread ()->step_multi
                   && inferior_thread ()->control.stop_step)
                 && !(inferior_thread ()->control.stop_bpstat
-                     && inferior_thread ()->control.proceed_to_finish)
+                    && inferior_thread ()->control.proceed_to_finish
+                    && execution_direction != EXEC_REVERSE)
                 && !inferior_thread ()->control.in_infcall))
     {
       if (!ptid_equal (inferior_ptid, null_ptid))

Re: [PATCH] Fix for 15072: stopped response missing for reverse-finish

[Abid, Hafiz]

On 22/04/13 08:33:51, Andreas Kaufmann wrote:
> Hi!
> 
> The following patch fixes the issue with "*stopped" response missing  
> (for MI interface) when
> "reverse-finish" command hits a breakpoint. Please see  
> http://sourceware.org/bugzilla/show_bug.cgi?id=15072
> This is quite nasty bug when using reverse debugging with Eclipse.  
> Basically, without "*stopped"
> response Eclipse will show state as "running" and the user has to  
> restart the debug session.
> 
> Here is explanation why this change is needed.
> GDB code has special handling for "forward finish", since returned  
> value is included in the response:
> static void
> finish_forward (struct symbol *function, struct frame_info *frame)
> {
> . . .
>   add_continuation (tp, finish_command_continuation, cargs,
>                     finish_command_continuation_free_arg);
> . . .
> 
> static void
> finish_command_continuation (void *arg, int err)
> {
> . . .
> observer_notify_normal_stop (bs, 1 /* print frame */);
> 
> The above is not done (and nor needed) for "reverse-finish". So I  
> added additional
> check  "execution_direction != EXEC_REVERSE" to ensure that  
> observer_notify_normal_stop()
> is called for "reverse-finish" from normal_stop() in gdb/infrun.c.
> 
> Please let me know if you have any comments!
> 
> Thanks,
> Andreas Kaufmann
> Synopsys Inc
> 
> 
> 
> 2013-04-18 Andreas Kaufmann <Andreas.Kaufmann@synopsys.com>
nit: Please add one more space before and after the name.

> 
>     PR gdb/15072
>     * infrun.c (normal_stop): Ensure "observer_notify_normal_stop" is  
> called
>       when "reverse-finish" command hits a breakpoint.
> 
> 
> Index: infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.577
> diff -u -p -r1.577 infrun.c
> --- infrun.c          13 Mar 2013 18:34:53 -0000          1.577
> +++ infrun.c       18 Apr 2013 13:45:09 -0000
> @@ -6149,7 +6149,8 @@ done:
>        || (!(inferior_thread ()->step_multi
>                    && inferior_thread ()->control.stop_step)
>                  && !(inferior_thread ()->control.stop_bpstat
> -                     && inferior_thread  
> ()->control.proceed_to_finish)
> +                    && inferior_thread ()->control.proceed_to_finish
> +                    && execution_direction != EXEC_REVERSE)
>                  && !inferior_thread ()->control.in_infcall))
>      {
>        if (!ptid_equal (inferior_ptid, null_ptid))
> 
> 
> 

Re: [PATCH] Fix for 15072: stopped response missing for reverse-finish

[Tom Tromey]

>>>>> "Andreas" == Andreas Kaufmann <Andreas.Kaufmann@synopsys.com> writes:

Andreas> The above is not done (and nor needed) for "reverse-finish". So
Andreas> I added additional check "execution_direction != EXEC_REVERSE"
Andreas> to ensure that observer_notify_normal_stop() is called for
Andreas> "reverse-finish" from normal_stop() in gdb/infrun.c.

It seems reasonable to me (though this isn't really my area and so Pedro
should probably weigh in).

Andreas> 2013-04-18 Andreas Kaufmann <Andreas.Kaufmann@synopsys.com>

Andreas>     PR gdb/15072
Andreas>     * infrun.c (normal_stop): Ensure "observer_notify_normal_stop" is called
Andreas>       when "reverse-finish" command hits a breakpoint.

I think a test case is needed.

But that will probably push the patch over the limit where we need
copyright papers (unless maybe there is some way to see the bug with the
CLI and the test case can be added to an existing file).  Contact me
off-list to get started on that; or if you already have them in place
and I forgot, just mention it.  Thanks.

Tom

Re: [PATCH] Fix for 15072: stopped response missing for reverse-finish

[Pedro Alves]

On 04/24/2013 09:48 PM, Tom Tromey wrote:
>>>>>> "Andreas" == Andreas Kaufmann <Andreas.Kaufmann@synopsys.com> writes:
> 
> Andreas> The above is not done (and nor needed) for "reverse-finish". So
> Andreas> I added additional check "execution_direction != EXEC_REVERSE"
> Andreas> to ensure that observer_notify_normal_stop() is called for
> Andreas> "reverse-finish" from normal_stop() in gdb/infrun.c.
> 
> It seems reasonable to me (though this isn't really my area and so Pedro
> should probably weigh in).

Looks good to me too.

> I think a test case is needed.

Agreed.


> 
> But that will probably push the patch over the limit where we need
> copyright papers (unless maybe there is some way to see the bug with the
> CLI and the test case can be added to an existing file).

Yes, there's an MI one even.  gdb.mi/mi-reverse.exp.

> Contact me
> off-list to get started on that; or if you already have them in place
> and I forgot, just mention it.  Thanks.

Thanks,
-- 
Pedro Alves