RFC: fix race in multiexec case

RFC: fix race in multiexec case

[Vladimir Prus]

While testing my MI multiexec support patches, I've got GDB to crash. What happened
is that:

- inferior 1 is run
- MI switches to inferior 2, which is never run. inferior_ptid gets set to
  null_ptid
- MI tries to run inferior 2
- GDB noticed gets an even in inferior 1
- handle_inferior_event calls get_current_regcache()
- get_current_regcache() calls get_thread_regcache (inferior_ptid),
  and inferior_ptid is still null_ptid
- get_thread_regcache indirectly calls linux_nat_thread_address_space,
and it has a code like this:

  if (GET_LWP (ptid) == 0)
    {
      ...
      lwp = find_lwp_pid (ptid);
      pid = GET_PID (lwp->ptid);
    }

However, find_lwp_pid returns NULL for null_ptid, and this code segfaults.
I attach a minimal patch that appears to fix this, but I feel uneasy about it.
Maybe, inferior_ptid should be reset much earlier?

Thanks,
Volodya


diff --git a/gdb/infrun.c b/gdb/infrun.c
index d8ca40d..300af62 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3232,7 +3232,8 @@ targets should add new threads to the thread list themselves in non-stop mode.")
   if (ecs->event_thread->stop_signal == TARGET_SIGNAL_TRAP)
     {
       int thread_hop_needed = 0;
-      struct address_space *aspace = get_regcache_aspace (get_current_regcache ());
+      struct address_space *aspace =
+       get_regcache_aspace (get_thread_regcache (ecs->ptid));

       /* Check if a regular breakpoint has been hit before checking
          for a potential single step breakpoint. Otherwise, GDB will


- Volodya

Re: RFC: fix race in multiexec case

[Pedro Alves]

On Tuesday 05 January 2010 08:26:58, Vladimir Prus wrote:

> However, find_lwp_pid returns NULL for null_ptid, and this code segfaults.
> I attach a minimal patch that appears to fix this, but I feel uneasy about it.

Ah, this bit runs before the almighty context_switch line.  Your fix is correct.

> Maybe, inferior_ptid should be reset much earlier?

Yes, at some point, we should clean this all up and context switch
as soon as we see an event, like in non-stop (fetch_inferior_event).
Not much point in not doing so, now that the globals that formed
the supposed "context" are gone.

> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index d8ca40d..300af62 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -3232,7 +3232,8 @@ targets should add new threads to the thread list themselves in non-stop mode.")
>    if (ecs->event_thread->stop_signal == TARGET_SIGNAL_TRAP)
>      {
>        int thread_hop_needed = 0;
> -      struct address_space *aspace = get_regcache_aspace (get_current_regcache ());
> +      struct address_space *aspace =
> +       get_regcache_aspace (get_thread_regcache (ecs->ptid));
> 
>        /* Check if a regular breakpoint has been hit before checking
>           for a potential single step breakpoint. Otherwise, GDB will
> 

This is OK.

-- 
Pedro Alves

Re: RFC: fix race in multiexec case

[Vladimir Prus]

On Wednesday 06 January 2010 19:31:59 Pedro Alves wrote:

> On Tuesday 05 January 2010 08:26:58, Vladimir Prus wrote:
> 
> > However, find_lwp_pid returns NULL for null_ptid, and this code segfaults.
> > I attach a minimal patch that appears to fix this, but I feel uneasy about it.
> 
> Ah, this bit runs before the almighty context_switch line.  Your fix is correct.
...
> > diff --git a/gdb/infrun.c b/gdb/infrun.c
> > index d8ca40d..300af62 100644
> > --- a/gdb/infrun.c
> > +++ b/gdb/infrun.c
> > @@ -3232,7 +3232,8 @@ targets should add new threads to the thread list themselves in non-stop mode.")
> >    if (ecs->event_thread->stop_signal == TARGET_SIGNAL_TRAP)
> >      {
> >        int thread_hop_needed = 0;
> > -      struct address_space *aspace = get_regcache_aspace (get_current_regcache ());
> > +      struct address_space *aspace =
> > +       get_regcache_aspace (get_thread_regcache (ecs->ptid));
> > 
> >        /* Check if a regular breakpoint has been hit before checking
> >           for a potential single step breakpoint. Otherwise, GDB will
> > 
> 
> This is OK.

What is the right way to regression test this? Do you think running testsuite with no special
arguments is sufficient, or I need to try async/non-stop?

- Volodya

Re: RFC: fix race in multiexec case

[Pedro Alves]

On Friday 08 January 2010 14:09:55, Vladimir Prus wrote:

> What is the right way to regression test this? Do you think running testsuite with no special
> arguments is sufficient, or I need to try async/non-stop?

Just the regular testing is fine.

-- 
Pedro Alves

Re: RFC: fix race in multiexec case

[Vladimir Prus]

[-- Attachment #1: Type: Text/Plain, Size: 363 bytes --]

On Friday 08 January 2010 17:23:35 Pedro Alves wrote:

> On Friday 08 January 2010 14:09:55, Vladimir Prus wrote:
> 
> > What is the right way to regression test this? Do you think running testsuite with no special
> > arguments is sufficient, or I need to try async/non-stop?
> 
> Just the regular testing is fine.

Thanks. Tested and checked in now.

- Volodya

[-- Attachment #2: final.diff --]
[-- Type: text/x-patch, Size: 1327 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.11212
diff -u -p -r1.11212 ChangeLog
--- ChangeLog	8 Jan 2010 13:54:40 -0000	1.11212
+++ ChangeLog	8 Jan 2010 16:53:31 -0000
@@ -1,3 +1,9 @@
+2010-01-08  Vladimir Prus  <vladimir@codesourcery.com>
+
+	Fix multiexec race.
+	* infrun.c (handle_inferior_event): Use get_thread_regcache
+	with events ptid, not get_current_regcache.
+
 2009-01-08  Joel Brobecker  <brobecker@adacore.com>
 
 	GDB crash with empty executable name (MinGW).
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.423
diff -u -p -r1.423 infrun.c
--- infrun.c	5 Jan 2010 20:55:18 -0000	1.423
+++ infrun.c	8 Jan 2010 16:53:31 -0000
@@ -3232,7 +3232,8 @@ targets should add new threads to the th
   if (ecs->event_thread->stop_signal == TARGET_SIGNAL_TRAP)
     {
       int thread_hop_needed = 0;
-      struct address_space *aspace = get_regcache_aspace (get_current_regcache ());
+      struct address_space *aspace = 
+	get_regcache_aspace (get_thread_regcache (ecs->ptid));
 
       /* Check if a regular breakpoint has been hit before checking
          for a potential single step breakpoint. Otherwise, GDB will