[PATCH 1/3] gdbstub: Refactor fork() handling

Ilya Leoshkevich posted 3 patches 9 months, 4 weeks ago
There is a newer version of this series
[PATCH 1/3] gdbstub: Refactor fork() handling
Posted by Ilya Leoshkevich 9 months, 4 weeks ago
Prepare for implementing follow-fork-mode child:
* Introduce gdbserver_fork_start(), which for now is a no-op.
* Rename gdbserver_fork() to gdbserver_fork_end(), call it in both
  parent and child processes, and pass the fork()'s return value to it.
* Factor out disable_gdbstub().
* Update ts_tid in the forked child.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 bsd-user/freebsd/os-proc.h  |  6 +++---
 bsd-user/main.c             |  8 ++++++--
 bsd-user/qemu.h             |  2 +-
 gdbstub/user.c              | 25 +++++++++++++++++++------
 include/gdbstub/user.h      | 11 ++++++++---
 linux-user/main.c           |  8 ++++++--
 linux-user/syscall.c        |  4 ++--
 linux-user/user-internals.h |  2 +-
 8 files changed, 46 insertions(+), 20 deletions(-)

diff --git a/bsd-user/freebsd/os-proc.h b/bsd-user/freebsd/os-proc.h
index d6418780344..3003c8cb637 100644
--- a/bsd-user/freebsd/os-proc.h
+++ b/bsd-user/freebsd/os-proc.h
@@ -208,7 +208,7 @@ static inline abi_long do_freebsd_fork(void *cpu_env)
      */
     set_second_rval(cpu_env, child_flag);
 
-    fork_end(child_flag);
+    fork_end(ret);
 
     return ret;
 }
@@ -252,7 +252,7 @@ static inline abi_long do_freebsd_rfork(void *cpu_env, abi_long flags)
      * value: 0 for parent process, 1 for child process.
      */
     set_second_rval(cpu_env, child_flag);
-    fork_end(child_flag);
+    fork_end(ret);
 
     return ret;
 
@@ -285,7 +285,7 @@ static inline abi_long do_freebsd_pdfork(void *cpu_env, abi_ulong target_fdp,
      * value: 0 for parent process, 1 for child process.
      */
     set_second_rval(cpu_env, child_flag);
-    fork_end(child_flag);
+    fork_end(ret);
 
     return ret;
 }
diff --git a/bsd-user/main.c b/bsd-user/main.c
index e5efb7b8458..8ecfa395cc5 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -106,10 +106,13 @@ void fork_start(void)
     start_exclusive();
     cpu_list_lock();
     mmap_fork_start();
+    gdbserver_fork_start();
 }
 
-void fork_end(int child)
+void fork_end(abi_long pid)
 {
+    int child = pid == 0;
+
     if (child) {
         CPUState *cpu, *next_cpu;
         /*
@@ -127,10 +130,11 @@ void fork_end(int child)
          * state, so we don't need to end_exclusive() here.
          */
         qemu_init_cpu_list();
-        gdbserver_fork(thread_cpu);
+        gdbserver_fork_end(pid);
     } else {
         mmap_fork_end(child);
         cpu_list_unlock();
+        gdbserver_fork_end(pid);
         end_exclusive();
     }
 }
diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index dc842fffa7d..2414a87559b 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -180,7 +180,7 @@ void cpu_loop(CPUArchState *env);
 char *target_strerror(int err);
 int get_osversion(void);
 void fork_start(void);
-void fork_end(int child);
+void fork_end(abi_long pid);
 
 #include "qemu/log.h"
 
diff --git a/gdbstub/user.c b/gdbstub/user.c
index 766f7c08848..120eb7fc117 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -356,16 +356,29 @@ int gdbserver_start(const char *port_or_path)
     return -1;
 }
 
+void gdbserver_fork_start(void)
+{
+}
+
+static void disable_gdbstub(void)
+{
+    CPUState *cpu;
+
+    close(gdbserver_user_state.fd);
+    gdbserver_user_state.fd = -1;
+    CPU_FOREACH(cpu) {
+        cpu_breakpoint_remove_all(cpu, BP_GDB);
+        /* no cpu_watchpoint_remove_all for user-mode */
+    }
+}
+
 /* Disable gdb stub for child processes.  */
-void gdbserver_fork(CPUState *cpu)
+void gdbserver_fork_end(pid_t pid)
 {
-    if (!gdbserver_state.init || gdbserver_user_state.fd < 0) {
+    if (pid != 0 || !gdbserver_state.init || gdbserver_user_state.fd < 0) {
         return;
     }
-    close(gdbserver_user_state.fd);
-    gdbserver_user_state.fd = -1;
-    cpu_breakpoint_remove_all(cpu, BP_GDB);
-    /* no cpu_watchpoint_remove_all for user-mode */
+    disable_gdbstub();
 }
 
 /*
diff --git a/include/gdbstub/user.h b/include/gdbstub/user.h
index 68b6534130c..1694d4fd330 100644
--- a/include/gdbstub/user.h
+++ b/include/gdbstub/user.h
@@ -46,10 +46,15 @@ static inline int gdb_handlesig(CPUState *cpu, int sig)
 void gdb_signalled(CPUArchState *as, int sig);
 
 /**
- * gdbserver_fork() - disable gdb stub for child processes.
- * @cs: CPU
+ * gdbserver_fork_start() - inform gdb of the upcoming fork()
+ */
+void gdbserver_fork_start(void);
+
+/**
+ * gdbserver_fork_end() - disable gdb stub for child processes.
+ * @pid: 0 if in child process, -1 if fork failed, child process pid otherwise
  */
-void gdbserver_fork(CPUState *cs);
+void gdbserver_fork_end(pid_t pid);
 
 /**
  * gdb_syscall_entry() - inform gdb of syscall entry and yield control to it
diff --git a/linux-user/main.c b/linux-user/main.c
index c9470eeccfc..b42c8f36a1d 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -144,10 +144,13 @@ void fork_start(void)
     mmap_fork_start();
     cpu_list_lock();
     qemu_plugin_user_prefork_lock();
+    gdbserver_fork_start();
 }
 
-void fork_end(int child)
+void fork_end(abi_long pid)
 {
+    int child = pid == 0;
+
     qemu_plugin_user_postfork(child);
     mmap_fork_end(child);
     if (child) {
@@ -160,10 +163,11 @@ void fork_end(int child)
             }
         }
         qemu_init_cpu_list();
-        gdbserver_fork(thread_cpu);
+        ((TaskState *)thread_cpu->opaque)->ts_tid = (pid_t)syscall(SYS_gettid);
     } else {
         cpu_list_unlock();
     }
+    gdbserver_fork_end(pid);
     /*
      * qemu_init_cpu_list() reinitialized the child exclusive state, but we
      * also need to keep current_cpu consistent, so call end_exclusive() for
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index e384e142489..8be0bb57778 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6669,7 +6669,7 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
         if (ret == 0) {
             /* Child Process.  */
             cpu_clone_regs_child(env, newsp, flags);
-            fork_end(1);
+            fork_end(ret);
             /* 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
@@ -6701,7 +6701,7 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
 #endif
                 put_user_u32(pid_fd, parent_tidptr);
                 }
-            fork_end(0);
+            fork_end(ret);
         }
         g_assert(!cpu_in_exclusive_context(cpu));
     }
diff --git a/linux-user/user-internals.h b/linux-user/user-internals.h
index c63ef45fc78..9014014d920 100644
--- a/linux-user/user-internals.h
+++ b/linux-user/user-internals.h
@@ -71,7 +71,7 @@ const char *target_strerror(int err);
 int get_osversion(void);
 void init_qemu_uname_release(void);
 void fork_start(void);
-void fork_end(int child);
+void fork_end(abi_long pid);
 
 /**
  * probe_guest_base:
-- 
2.43.0
Re: [PATCH 1/3] gdbstub: Refactor fork() handling
Posted by Alex Bennée 9 months, 4 weeks ago
Ilya Leoshkevich <iii@linux.ibm.com> writes:

> Prepare for implementing follow-fork-mode child:
> * Introduce gdbserver_fork_start(), which for now is a no-op.
> * Rename gdbserver_fork() to gdbserver_fork_end(), call it in both
>   parent and child processes, and pass the fork()'s return value to it.
> * Factor out disable_gdbstub().
> * Update ts_tid in the forked child.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  bsd-user/freebsd/os-proc.h  |  6 +++---
>  bsd-user/main.c             |  8 ++++++--
>  bsd-user/qemu.h             |  2 +-
>  gdbstub/user.c              | 25 +++++++++++++++++++------
>  include/gdbstub/user.h      | 11 ++++++++---
>  linux-user/main.c           |  8 ++++++--
>  linux-user/syscall.c        |  4 ++--
>  linux-user/user-internals.h |  2 +-
>  8 files changed, 46 insertions(+), 20 deletions(-)
>
<snip>
>  
>  /*
> diff --git a/include/gdbstub/user.h b/include/gdbstub/user.h
> index 68b6534130c..1694d4fd330 100644
> --- a/include/gdbstub/user.h
> +++ b/include/gdbstub/user.h
> @@ -46,10 +46,15 @@ static inline int gdb_handlesig(CPUState *cpu, int sig)
>  void gdb_signalled(CPUArchState *as, int sig);
>  
>  /**
> - * gdbserver_fork() - disable gdb stub for child processes.
> - * @cs: CPU
> + * gdbserver_fork_start() - inform gdb of the upcoming fork()
> + */
> +void gdbserver_fork_start(void);
> +
> +/**
> + * gdbserver_fork_end() - disable gdb stub for child processes.
> + * @pid: 0 if in child process, -1 if fork failed, child process pid otherwise
>   */
> -void gdbserver_fork(CPUState *cs);
> +void gdbserver_fork_end(pid_t pid);
>

This had merge conflicts when I tried to apply to my tree.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro