[Qemu-devel] [6095] User-mode GDB stub improvements - handle fork

[Qemu-devel] [6095] User-mode GDB stub improvements - handle fork

[Aurelien Jarno]

Revision: 6095
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6095
Author:   aurel32
Date:     2008-12-18 22:44:04 +0000 (Thu, 18 Dec 2008)

Log Message:
-----------
User-mode GDB stub improvements - handle fork

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>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

Modified Paths:
--------------
    trunk/gdbstub.c
    trunk/gdbstub.h
    trunk/linux-user/main.c
    trunk/linux-user/syscall.c

Modified: trunk/gdbstub.c
===================================================================
--- trunk/gdbstub.c	2008-12-18 22:43:56 UTC (rev 6094)
+++ trunk/gdbstub.c	2008-12-18 22:44:04 UTC (rev 6095)
@@ -1996,6 +1996,18 @@
     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)
 {

Modified: trunk/gdbstub.h
===================================================================
--- trunk/gdbstub.h	2008-12-18 22:43:56 UTC (rev 6094)
+++ trunk/gdbstub.h	2008-12-18 22:44:04 UTC (rev 6095)
@@ -13,6 +13,7 @@
 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

Modified: trunk/linux-user/main.c
===================================================================
--- trunk/linux-user/main.c	2008-12-18 22:43:56 UTC (rev 6094)
+++ trunk/linux-user/main.c	2008-12-18 22:44:04 UTC (rev 6095)
@@ -162,6 +162,7 @@
         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_end(int child)
 {
+    if (child) {
+        gdbserver_fork(thread_env);
+    }
 }
 #endif
 

Modified: trunk/linux-user/syscall.c
===================================================================
--- trunk/linux-user/syscall.c	2008-12-18 22:43:56 UTC (rev 6094)
+++ trunk/linux-user/syscall.c	2008-12-18 22:44:04 UTC (rev 6095)
@@ -2960,17 +2960,17 @@
             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)
@@ -2979,14 +2979,10 @@
             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;
 }

Re: [Qemu-devel] [6095] User-mode GDB stub improvements - handle fork

[Martin Mohring]

I seem to have found the cause, why ARM user mode with nptl does not
work as before. see below.

Martin

Aurelien Jarno wrote:
> Revision: 6095
>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6095
> Author:   aurel32
> Date:     2008-12-18 22:44:04 +0000 (Thu, 18 Dec 2008)
>
> Log Message:
> -----------
> User-mode GDB stub improvements - handle fork
>
> 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>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>
> Modified Paths:
> --------------
>     trunk/gdbstub.c
>     trunk/gdbstub.h
>     trunk/linux-user/main.c
>     trunk/linux-user/syscall.c
>
> Modified: trunk/gdbstub.c
> ===================================================================
> --- trunk/gdbstub.c	2008-12-18 22:43:56 UTC (rev 6094)
> +++ trunk/gdbstub.c	2008-12-18 22:44:04 UTC (rev 6095)
> @@ -1996,6 +1996,18 @@
>      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);
> +}
>   
In case of USE_NPTL is defined, it seems that either s or env can be
NULL, so I get a exception (yes, nptl works on arm in user mode).
My understanding is that this might be a race condition of the first
fork() call coming even before env or gdbserver_state is initialised
towards !NULL.

>  #else
>  static int gdb_chr_can_receive(void *opaque)
>  {
>
> Modified: trunk/gdbstub.h
> ===================================================================
> --- trunk/gdbstub.h	2008-12-18 22:43:56 UTC (rev 6094)
> +++ trunk/gdbstub.h	2008-12-18 22:44:04 UTC (rev 6095)
> @@ -13,6 +13,7 @@
>  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
>
> Modified: trunk/linux-user/main.c
> ===================================================================
> --- trunk/linux-user/main.c	2008-12-18 22:43:56 UTC (rev 6094)
> +++ trunk/linux-user/main.c	2008-12-18 22:44:04 UTC (rev 6095)
> @@ -162,6 +162,7 @@
>          pthread_cond_init(&exclusive_cond, NULL);
>          pthread_cond_init(&exclusive_resume, NULL);
>          pthread_mutex_init(&tb_lock, NULL);
> +        gdbserver_fork(thread_env);
>   
This gdbserver_fork(thread_env) call then causes the exception.

Re: [Qemu-devel] [6095] User-mode GDB stub improvements - handle fork

[Laurent Desnogues]

On Tue, Jan 6, 2009 at 5:19 PM, Martin Mohring
<martin.mohring@opensuse.org> wrote:
[...]
> Aurelien Jarno wrote:
>> Revision: 6095
>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6095
>> Author:   aurel32
>> Date:     2008-12-18 22:44:04 +0000 (Thu, 18 Dec 2008)
>>
>> Log Message:
>> -----------
>> User-mode GDB stub improvements - handle fork
>>
>> 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>
>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>>
>> Modified Paths:
>> --------------
>>     trunk/gdbstub.c
>>     trunk/gdbstub.h
>>     trunk/linux-user/main.c
>>     trunk/linux-user/syscall.c
>>
>> Modified: trunk/gdbstub.c
>> ===================================================================
>> --- trunk/gdbstub.c   2008-12-18 22:43:56 UTC (rev 6094)
>> +++ trunk/gdbstub.c   2008-12-18 22:44:04 UTC (rev 6095)
>> @@ -1996,6 +1996,18 @@
>>      gdb_accept();
>>      return 0;
>>  }
>> +
>> +/* Disable gdb stub for child processes.  */
>> +void gdbserver_fork(CPUState *env)
>> +{
>> +    GDBState *s = gdbserver_state;
>> +    if (s->fd < 0)

Other qemu gdb functions do this:

    if (gdbserver_fd < 0 || s->fd < 0)

>> +      return;
>> +    close(s->fd);
>> +    s->fd = -1;
>> +    cpu_breakpoint_remove_all(env, BP_GDB);
>> +    cpu_watchpoint_remove_all(env, BP_GDB);
>> +}
>>
> In case of USE_NPTL is defined, it seems that either s or env can be
> NULL, so I get a exception (yes, nptl works on arm in user mode).
> My understanding is that this might be a race condition of the first
> fork() call coming even before env or gdbserver_state is initialised
> towards !NULL.

Could you check if my proposed change above fixes your issue?


Laurent

Re: [Qemu-devel] [6095] User-mode GDB stub improvements - handle fork

[Martin Mohring]

Martin Mohring wrote:
> I seem to have found the cause, why ARM user mode with nptl does not
> work as before. see below.
>
> Martin
>
> Aurelien Jarno wrote:
>   
>> Revision: 6095
>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6095
>> Author:   aurel32
>> Date:     2008-12-18 22:44:04 +0000 (Thu, 18 Dec 2008)
>>
>> Log Message:
>> -----------
>> User-mode GDB stub improvements - handle fork
>>
>> 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>
>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>>
>> Modified Paths:
>> --------------
>>     trunk/gdbstub.c
>>     trunk/gdbstub.h
>>     trunk/linux-user/main.c
>>     trunk/linux-user/syscall.c
>>
>> Modified: trunk/gdbstub.c
>> ===================================================================
>> --- trunk/gdbstub.c	2008-12-18 22:43:56 UTC (rev 6094)
>> +++ trunk/gdbstub.c	2008-12-18 22:44:04 UTC (rev 6095)
>> @@ -1996,6 +1996,18 @@
>>      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);
>> +}
>>   
>>     
> In case of USE_NPTL is defined, it seems that either s or env can be
> NULL, so I get a exception (yes, nptl works on arm in user mode).
> My understanding is that this might be a race condition of the first
> fork() call coming even before env or gdbserver_state is initialised
> towards !NULL.
>   
The reason why I wrote this and did not propose a fix like Laurent did
now was the fact that:

- gdbserver_fork() is called unconditionally before gdbserver_start()
has ever been called
- gdbserver_fork() as result accesses gdbserver data structures before
they are inited, so we have NULL pointer access

Another issue is that gdbserver_fork() is only defined when
CONFIG_USER_ONLY is not defined but gdbserver_fork() is called when
always when USE_NPTL is defined.
So switching on CONFIG_USER_ONLY and USE_NPTL will result in
gdbserver_fork() beeing undefined. That was why my impression was that
the code is somewhat incomplete.

Also, does debugging work in user mode at all if USE_NPTL is on? If not
I would remove the call to gdbserver_fork() in this case completely.

Martin