[RFA] (Ada/ravenscar) error during "continue" after task/thread switch

  Hello,

When debugging a program using the Ada ravenscar profile, resuming
a program's execution after having switched to a different task
sometimes yields the following error:

     (gdb) cont
     Continuing.
     Cannot execute this command while the target is running.
     Use the "interrupt" command to stop the target
     and then try again.

In short, the Ravenscar profile is a standardized subset of Ada which
allows tasking (often mapped to threads). We often use it on baremetal
targets where there is no OS support. Thread support is implemented
as a thread target_ops layer. It sits on top of the "remote" layer,
so we can do thread debugging against baremetal targets to which GDB
is connected via "target remote".

What happens, when the user request the program to resume execution,
is the following:

  - the ravenscar-thread target_ops layer gets the order to resume
    the program's execution. The current thread is not the active
    thread in the inferior, and the "remote" layer doesn't know
    about that thread anyway. So what we do is (see ravenscar_resume):

       + swtich inferior_ptid to the ptid of the actually active thread;
       + ask the layer beneath us to actually do the resume.

  - Once that's done, the resuming itself is done. But execute_command
    (in top.c) actually does a bit more. More precisely, it unconditionally
    checks to see if the language may no longer be maching the current
    frame:

        check_frame_language_change ();

The problem, here, is that we haven't received the "stop" event
from the inferior, yet. This part will be handled by the event loop,
which is done later. So, checking for the language-change here
doesn't make sense, since we don't really have a frame. In our
case, the error comes from the fact that we end up trying to read
the registers, which causes the error while the remote protocol
is waiting for the event showing the inferior stopped.

This apparently used to work, but it is believed that this was only
accidental. In other words, we had enough information already cached
within GDB that we were able to perform the entire call to
check_frame_language_change without actually querying the target.
On PowerPC targets, this started to fail as a side-effect of a minor
change in the way we get to the regcache during the handling of
software-single-step (which seems fine).

This patch fixes the issue by only calling check_frame_language_change
in cases the inferior isn't running. Otherwise, it skips it, knowning
that the event loop should eventually get to it.

gdb/ChangeLog:

        * top.c (execute_command): Do not call check_frame_language_change
        if the inferior is running.

Tested on x86_64-linux, no regression. Also tested on aarch64-elf,
arm-elf, leon3-elf, and ppc-elf, but using AdaCore's testsuite.

OK to push?

Thank you!
  

Hi Joel,

The change to top.c looks right regardless of having been noticed
on ravenscar only so far.  I can imagine other scenarios where this
would be necessary even on GNU/Linux.

So LGTM, with minor nits below fixed.

On 04/23/2018 10:51 PM, Joel Brobecker wrote:
>        + swtich inferior_ptid to the ptid of the actually active thread;

>     checks to see if the language may no longer be maching the current

> in cases the inferior isn't running. Otherwise, it skips it, knowning
My Thunderbird noticed these typos above:

 swtich -> switch
 maching -> matching
 knowning -> knowing

> diff --git a/gdb/top.c b/gdb/top.c
> index 8903a92..0480726 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -642,7 +642,12 @@ execute_command (const char *p, int from_tty)
>  	}
>      }
>  
> -  check_frame_language_change ();
> +  /* Only perform the frame-language-change check unless the command
> +     we just finished executing did not resume the inferior's execution.
> +     If it did resume the inferior, we will do that check after
> +     the inferior stopped.  */

"only ... unless ... did not" sounds odd.  Did you mean:

 -  /* Only perform the frame-language-change check unless the command
 +  /* Only perform the frame-language-change check if the command
       we just finished executing did not resume the inferior's execution.
       If it did resume the inferior, we will do that check after
       the inferior stopped.  */

?

> +  if (has_stack_frames () && !is_running (inferior_ptid))
> +     check_frame_language_change ();


Please double-check indentation of the
check_frame_language_change call line.

Thanks,
Pedro Alves
  

Hi Pedro,

> The change to top.c looks right regardless of having been noticed
> on ravenscar only so far.  I can imagine other scenarios where this
> would be necessary even on GNU/Linux.

My thinking also.

> So LGTM, with minor nits below fixed.
> 
> On 04/23/2018 10:51 PM, Joel Brobecker wrote:
> >        + swtich inferior_ptid to the ptid of the actually active thread;
> 
> >     checks to see if the language may no longer be maching the current
> 
> > in cases the inferior isn't running. Otherwise, it skips it, knowning
> My Thunderbird noticed these typos above:
> 
>  swtich -> switch
>  maching -> matching
>  knowning -> knowing

Thanks! Fixed.

> > diff --git a/gdb/top.c b/gdb/top.c
> > index 8903a92..0480726 100644
> > --- a/gdb/top.c
> > +++ b/gdb/top.c
> > @@ -642,7 +642,12 @@ execute_command (const char *p, int from_tty)
> >  	}
> >      }
> >  
> > -  check_frame_language_change ();
> > +  /* Only perform the frame-language-change check unless the command
> > +     we just finished executing did not resume the inferior's execution.
> > +     If it did resume the inferior, we will do that check after
> > +     the inferior stopped.  */
> 
> "only ... unless ... did not" sounds odd.  Did you mean:
> 
>  -  /* Only perform the frame-language-change check unless the command
>  +  /* Only perform the frame-language-change check if the command
>        we just finished executing did not resume the inferior's execution.
>        If it did resume the inferior, we will do that check after
>        the inferior stopped.  */

Indeed. I hesitated between two forms a little too much and ended up
choosing both at the same time... I took your suggestion.

> > +  if (has_stack_frames () && !is_running (inferior_ptid))
> > +     check_frame_language_change ();
[...]
> Please double-check indentation of the
> check_frame_language_change call line.

Hmmm, sorry, good catch. Fixed also.

I retested the patch on x86_64-linux and pushed it to master.

Thanks again,