Date: Thu, 18 Dec 2008 23:55:54 +0100
From: Aurelien Jarno <aurelien@aurel32.net>
Subject: Re: [Qemu-devel] [1/2] User-mode GDB stub improvements - handle fork
Message-ID: <20081218225554.GK12048@volta.aurel32.net>
References: <20081212190756.GA31647@caradoc.them.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-15
Content-Disposition: inline
In-Reply-To: <20081212190756.GA31647@caradoc.them.org>
Reply-To: qemu-devel@nongnu.org
To: qemu-devel@nongnu.org

On Fri, Dec 12, 2008 at 02:07:56PM -0500, Daniel Jacobowitz wrote:
> From: Daniel Jacobowitz <dan@codesourcery.com>
> 
> Close gdbserver in child processes, so that only one stub tries to talk
> to GDB at a time.  Updated from an earlier patch by Paul Brook.
> 
> Signed-off-by: Daniel Jacobowitz <dan@codesourcery.com>

Thanks, applied.

> ---
> 
> This patch lets GDB debug a parent process, even if it calls fork.
> Otherwise you end up with two QEMU processes sharing a single fd
> for the GDB socket, and both writing replies to it.  This will more or
> less stutter along for a while, with GDB discarding lots of errors,
> but eventually the two processes will diverge and things will blow up
> depending on which response GDB sees first.
> 
> Index: linux-user/syscall.c
> ===================================================================
> --- linux-user/syscall.c	(revision 5995)
> +++ linux-user/syscall.c	(working copy)
> @@ -2958,17 +2958,17 @@ static int do_fork(CPUState *env, unsign
>              return -EINVAL;
>          fork_start();
>          ret = fork();
> -#if defined(USE_NPTL)
> -        /* There is a race condition here.  The parent process could
> -           theoretically read the TID in the child process before the child
> -           tid is set.  This would require using either ptrace
> -           (not implemented) or having *_tidptr to point at a shared memory
> -           mapping.  We can't repeat the spinlock hack used above because
> -           the child process gets its own copy of the lock.  */
>          if (ret == 0) {
> +            /* Child Process.  */
>              cpu_clone_regs(env, newsp);
>              fork_end(1);
> -            /* Child Process.  */
> +#if defined(USE_NPTL)
> +            /* There is a race condition here.  The parent process could
> +               theoretically read the TID in the child process before the child
> +               tid is set.  This would require using either ptrace
> +               (not implemented) or having *_tidptr to point at a shared memory
> +               mapping.  We can't repeat the spinlock hack used above because
> +               the child process gets its own copy of the lock.  */
>              if (flags & CLONE_CHILD_SETTID)
>                  put_user_u32(gettid(), child_tidptr);
>              if (flags & CLONE_PARENT_SETTID)
> @@ -2977,14 +2977,10 @@ static int do_fork(CPUState *env, unsign
>              if (flags & CLONE_SETTLS)
>                  cpu_set_tls (env, newtls);
>              /* TODO: Implement CLONE_CHILD_CLEARTID.  */
> +#endif
>          } else {
>              fork_end(0);
>          }
> -#else
> -        if (ret == 0) {
> -            cpu_clone_regs(env, newsp);
> -        }
> -#endif
>      }
>      return ret;
>  }
> Index: linux-user/main.c
> ===================================================================
> --- linux-user/main.c	(revision 5995)
> +++ linux-user/main.c	(working copy)
> @@ -162,6 +162,7 @@ void fork_end(int child)
>          pthread_cond_init(&exclusive_cond, NULL);
>          pthread_cond_init(&exclusive_resume, NULL);
>          pthread_mutex_init(&tb_lock, NULL);
> +        gdbserver_fork(thread_env);
>      } else {
>          pthread_mutex_unlock(&exclusive_lock);
>          pthread_mutex_unlock(&tb_lock);
> @@ -254,6 +255,9 @@ void fork_start(void)
>  
>  void fork_end(int child)
>  {
> +    if (child) {
> +        gdbserver_fork(thread_env);
> +    }
>  }
>  #endif
>  
> Index: gdbstub.c
> ===================================================================
> --- gdbstub.c	(revision 5995)
> +++ gdbstub.c	(working copy)
> @@ -1996,6 +1996,18 @@ int gdbserver_start(int port)
>      gdb_accept();
>      return 0;
>  }
> +
> +/* Disable gdb stub for child processes.  */
> +void gdbserver_fork(CPUState *env)
> +{
> +    GDBState *s = gdbserver_state;
> +    if (s->fd < 0)
> +      return;
> +    close(s->fd);
> +    s->fd = -1;
> +    cpu_breakpoint_remove_all(env, BP_GDB);
> +    cpu_watchpoint_remove_all(env, BP_GDB);
> +}
>  #else
>  static int gdb_chr_can_receive(void *opaque)
>  {
> Index: gdbstub.h
> ===================================================================
> --- gdbstub.h	(revision 5995)
> +++ gdbstub.h	(working copy)
> @@ -13,6 +13,7 @@ void gdb_set_stop_cpu(CPUState *env);
>  int gdb_handlesig (CPUState *, int);
>  void gdb_exit(CPUState *, int);
>  int gdbserver_start(int);
> +void gdbserver_fork(CPUState *);
>  #else
>  int gdbserver_start(const char *port);
>  #endif
> 
> -- 
> Daniel Jacobowitz
> CodeSourcery
> 
> 
> 

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net