gdbserver: don't pick a random thread if the current thread dies
authorPedro Alves <palves@redhat.com>
Fri, 21 Aug 2015 18:20:31 +0000 (19:20 +0100)
committerPedro Alves <palves@redhat.com>
Fri, 21 Aug 2015 18:20:31 +0000 (19:20 +0100)
In all-stop mode, if the current thread disappears while stopping all
threads, gdbserver calls set_desired_thread(0) ['0' means "I want the
continue thread"] which just picks the first thread in the list.

This looks like a dangerous thing to do.  GDBserver continues
processing whatever it was doing, but to the wrong thread.  If
debugging more than one process, we may even pick the wrong process.
Instead, GDBserver should detect the situation and bail out of
whatever is was doing.

The backends used to pay attention to the set 'cont_thread' (the Hc
thread, used in the old way to resume threads, before vCont), but all
such 'cont_thread' checks have been eliminated meanwhile.  The
remaining implicit dependencies that I found on there being a selected
thread in the backends are in the Ctrl-C handling, which some backends
use as thread to send a signal to.  Even that seems to me to be better
handled by always using the first thread in the list or by using the
signal_pid PID.

In order to make this a systematic approach, I'm making
set_desired_thread never fallback to a random thread, and instead end
up with current_thread == NULL, like already done in non-stop mode.
Then I updated all callers to handle the situation.

I stumbled on this while fixing other bugs exposed by
gdb.threads/fork-plus-threads.exp test.  The problems I saw were fixed
in a different way, but in any case, I think the potential for
problems is more or less obvious, and the resulting code looks a bit
less magical to me.

Tested on x86-64 Fedora 20, w/ native-extended-gdbserver board.

gdb/gdbserver/ChangeLog:
2015-08-21  Pedro Alves  <palves@redhat.com>

* linux-low.c (wait_for_sigstop): Always switch to no thread
selected if the previously current thread dies.
* lynx-low.c (lynx_request_interrupt): Use the first thread's
process instead of the current thread's.
* remote-utils.c (input_interrupt): Don't check if there's no
current thread.
* server.c (gdb_read_memory, gdb_write_memory): If setting the
current thread to the general thread fails, error out.
(handle_qxfer_auxv, handle_qxfer_libraries)
(handle_qxfer_libraries_svr4, handle_qxfer_siginfo)
(handle_qxfer_spu, handle_qxfer_statictrace, handle_qxfer_fdpic)
(handle_query): Check if there's a thread selected instead of
checking whether there's any thread in the thread list.
(handle_qxfer_threads, handle_qxfer_btrace)
(handle_qxfer_btrace_conf): Don't error out early if there's no
thread in the thread list.
(handle_v_cont, myresume): Don't set the current thread to the
continue thread.
(process_serial_event) <Hg handling>: Also set thread_id if the
previous general thread is still alive.
(process_serial_event) <g/G handling>: If setting the current
thread to the general thread fails, error out.
* spu-low.c (spu_resume, spu_request_interrupt): Use the first
thread's lwp instead of the current thread's.
* target.c (set_desired_thread): If the desired thread was not
found, leave the current thread pointing to NULL.  Return an int
(boolean) indicating success.
* target.h (set_desired_thread): Change return type to int.

gdb/gdbserver/ChangeLog
gdb/gdbserver/linux-low.c
gdb/gdbserver/lynx-low.c
gdb/gdbserver/remote-utils.c
gdb/gdbserver/server.c
gdb/gdbserver/spu-low.c
gdb/gdbserver/target.c
gdb/gdbserver/target.h

index 10961b0cb80c303b1a409f0b873480c6be3bb5b8..20b3e1c1c3904be98dad3425bad5273ab52d2f8f 100644 (file)
@@ -1,3 +1,34 @@
+2015-08-21  Pedro Alves  <palves@redhat.com>
+
+       * linux-low.c (wait_for_sigstop): Always switch to no thread
+       selected if the previously current thread dies.
+       * lynx-low.c (lynx_request_interrupt): Use the first thread's
+       process instead of the current thread's.
+       * remote-utils.c (input_interrupt): Don't check if there's no
+       current thread.
+       * server.c (gdb_read_memory, gdb_write_memory): If setting the
+       current thread to the general thread fails, error out.
+       (handle_qxfer_auxv, handle_qxfer_libraries)
+       (handle_qxfer_libraries_svr4, handle_qxfer_siginfo)
+       (handle_qxfer_spu, handle_qxfer_statictrace, handle_qxfer_fdpic)
+       (handle_query): Check if there's a thread selected instead of
+       checking whether there's any thread in the thread list.
+       (handle_qxfer_threads, handle_qxfer_btrace)
+       (handle_qxfer_btrace_conf): Don't error out early if there's no
+       thread in the thread list.
+       (handle_v_cont, myresume): Don't set the current thread to the
+       continue thread.
+       (process_serial_event) <Hg handling>: Also set thread_id if the
+       previous general thread is still alive.
+       (process_serial_event) <g/G handling>: If setting the current
+       thread to the general thread fails, error out.
+       * spu-low.c (spu_resume, spu_request_interrupt): Use the first
+       thread's lwp instead of the current thread's.
+       * target.c (set_desired_thread): If the desired thread was not
+       found, leave the current thread pointing to NULL.  Return an int
+       (boolean) indicating success.
+       * target.h (set_desired_thread): Change return type to int.
+
 2015-08-20  Max Filippov  <jcmvbkbc@gmail.com>
 
        * configure.srv (xtensa*-*-linux*): Add srv_linux_thread_db=yes.
index c37c6d2f3d533cdf27dffcb7f550650d292bc289..2bc91c276d6c4fd4ba7b4c60847357f000a0e79f 100644 (file)
@@ -3617,18 +3617,10 @@ wait_for_sigstop (void)
       if (debug_threads)
        debug_printf ("Previously current thread died.\n");
 
-      if (non_stop)
-       {
-         /* We can't change the current inferior behind GDB's back,
-            otherwise, a subsequent command may apply to the wrong
-            process.  */
-         current_thread = NULL;
-       }
-      else
-       {
-         /* Set a valid thread as current.  */
-         set_desired_thread (0);
-       }
+      /* We can't change the current inferior behind GDB's back,
+        otherwise, a subsequent command may apply to the wrong
+        process.  */
+      current_thread = NULL;
     }
 }
 
index 5cf03be3d26877ab0f2a64b1420352afe5d74674..ee4d0e8395853b7e902adee0e7ed60cf7697cf5d 100644 (file)
@@ -713,7 +713,7 @@ lynx_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
 static void
 lynx_request_interrupt (void)
 {
-  ptid_t inferior_ptid = thread_to_gdb_id (current_thread);
+  ptid_t inferior_ptid = thread_to_gdb_id (get_first_thread ());
 
   kill (lynx_ptid_get_pid (inferior_ptid), SIGINT);
 }
index bb3145683bbea5fb0c461a9d1ed90b25dbdc8b34..05563bea7e6e4f4139d3f5b35596482d9f90e5e5 100644 (file)
@@ -747,7 +747,7 @@ input_interrupt (int unused)
          fprintf (stderr, "client connection closed\n");
          return;
        }
-      else if (cc != 1 || c != '\003' || current_thread == NULL)
+      else if (cc != 1 || c != '\003')
        {
          fprintf (stderr, "input_interrupt, count = %d c = %d ", cc, c);
          if (isprint (c))
index d3cc0f8487062610d7b3026ce7dea9a5ad2b317e..58907ec4a9ffa3bfbf6845b4c95e5081d2e81fa2 100644 (file)
@@ -816,7 +816,10 @@ gdb_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
   res = prepare_to_access_memory ();
   if (res == 0)
     {
-      res = read_inferior_memory (memaddr, myaddr, len);
+      if (set_desired_thread (1))
+       res = read_inferior_memory (memaddr, myaddr, len);
+      else
+       res = 1;
       done_accessing_memory ();
 
       return res == 0 ? len : -1;
@@ -840,7 +843,10 @@ gdb_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
       ret = prepare_to_access_memory ();
       if (ret == 0)
        {
-         ret = write_inferior_memory (memaddr, myaddr, len);
+         if (set_desired_thread (1))
+           ret = write_inferior_memory (memaddr, myaddr, len);
+         else
+           ret = EIO;
          done_accessing_memory ();
        }
       return ret;
@@ -1171,7 +1177,7 @@ handle_qxfer_auxv (const char *annex,
   if (the_target->read_auxv == NULL || writebuf != NULL)
     return -2;
 
-  if (annex[0] != '\0' || !target_running ())
+  if (annex[0] != '\0' || current_thread == NULL)
     return -1;
 
   return (*the_target->read_auxv) (offset, readbuf, len);
@@ -1316,7 +1322,7 @@ handle_qxfer_libraries (const char *annex,
   if (writebuf != NULL)
     return -2;
 
-  if (annex[0] != '\0' || !target_running ())
+  if (annex[0] != '\0' || current_thread == NULL)
     return -1;
 
   total_len = 64;
@@ -1360,7 +1366,7 @@ handle_qxfer_libraries_svr4 (const char *annex,
   if (writebuf != NULL)
     return -2;
 
-  if (!target_running () || the_target->qxfer_libraries_svr4 == NULL)
+  if (current_thread == NULL || the_target->qxfer_libraries_svr4 == NULL)
     return -1;
 
   return the_target->qxfer_libraries_svr4 (annex, readbuf, writebuf, offset, len);
@@ -1389,7 +1395,7 @@ handle_qxfer_siginfo (const char *annex,
   if (the_target->qxfer_siginfo == NULL)
     return -2;
 
-  if (annex[0] != '\0' || !target_running ())
+  if (annex[0] != '\0' || current_thread == NULL)
     return -1;
 
   return (*the_target->qxfer_siginfo) (annex, readbuf, writebuf, offset, len);
@@ -1405,7 +1411,7 @@ handle_qxfer_spu (const char *annex,
   if (the_target->qxfer_spu == NULL)
     return -2;
 
-  if (!target_running ())
+  if (current_thread == NULL)
     return -1;
 
   return (*the_target->qxfer_spu) (annex, readbuf, writebuf, offset, len);
@@ -1423,7 +1429,7 @@ handle_qxfer_statictrace (const char *annex,
   if (writebuf != NULL)
     return -2;
 
-  if (annex[0] != '\0' || !target_running () || current_traceframe == -1)
+  if (annex[0] != '\0' || current_thread == NULL || current_traceframe == -1)
     return -1;
 
   if (traceframe_read_sdata (current_traceframe, offset,
@@ -1486,7 +1492,7 @@ handle_qxfer_threads (const char *annex,
   if (writebuf != NULL)
     return -2;
 
-  if (!target_running () || annex[0] != '\0')
+  if (annex[0] != '\0')
     return -1;
 
   if (offset == 0)
@@ -1582,7 +1588,7 @@ handle_qxfer_fdpic (const char *annex, gdb_byte *readbuf,
   if (the_target->read_loadmap == NULL)
     return -2;
 
-  if (!target_running ())
+  if (current_thread == NULL)
     return -1;
 
   return (*the_target->read_loadmap) (annex, offset, readbuf, len);
@@ -1602,9 +1608,6 @@ handle_qxfer_btrace (const char *annex,
   if (the_target->read_btrace == NULL || writebuf != NULL)
     return -2;
 
-  if (!target_running ())
-    return -1;
-
   if (ptid_equal (general_thread, null_ptid)
       || ptid_equal (general_thread, minus_one_ptid))
     {
@@ -1676,7 +1679,7 @@ handle_qxfer_btrace_conf (const char *annex,
   if (the_target->read_btrace_conf == NULL || writebuf != NULL)
     return -2;
 
-  if (annex[0] != '\0' || !target_running ())
+  if (annex[0] != '\0')
     return -1;
 
   if (ptid_equal (general_thread, null_ptid)
@@ -1978,7 +1981,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
       if (target_supports_tracepoints ())
        tracepoint_look_up_symbols ();
 
-      if (target_running () && the_target->look_up_symbols != NULL)
+      if (current_thread != NULL && the_target->look_up_symbols != NULL)
        (*the_target->look_up_symbols) ();
 
       strcpy (own_buf, "OK");
@@ -2575,8 +2578,6 @@ handle_v_cont (char *own_buf)
   if (i < n)
     resume_info[i] = default_action;
 
-  set_desired_thread (0);
-
   resume (resume_info, n);
   free (resume_info);
   return;
@@ -2878,8 +2879,6 @@ myresume (char *own_buf, int step, int sig)
   int n = 0;
   int valid_cont_thread;
 
-  set_desired_thread (0);
-
   valid_cont_thread = (!ptid_equal (cont_thread, null_ptid)
                         && !ptid_equal (cont_thread, minus_one_ptid));
 
@@ -3908,14 +3907,13 @@ process_serial_event (void)
                    (struct thread_info *) find_inferior_id (&all_threads,
                                                             general_thread);
                  if (thread == NULL)
-                   {
-                     thread = get_first_thread ();
-                     thread_id = thread->entry.id;
-                   }
+                   thread = get_first_thread ();
+                 thread_id = thread->entry.id;
                }
 
              general_thread = thread_id;
              set_desired_thread (1);
+             gdb_assert (current_thread != NULL);
            }
          else if (own_buf[1] == 'c')
            cont_thread = thread_id;
@@ -3947,9 +3945,13 @@ process_serial_event (void)
        {
          struct regcache *regcache;
 
-         set_desired_thread (1);
-         regcache = get_thread_regcache (current_thread, 1);
-         registers_to_string (regcache, own_buf);
+         if (!set_desired_thread (1))
+           write_enn (own_buf);
+         else
+           {
+             regcache = get_thread_regcache (current_thread, 1);
+             registers_to_string (regcache, own_buf);
+           }
        }
       break;
     case 'G':
@@ -3960,10 +3962,14 @@ process_serial_event (void)
        {
          struct regcache *regcache;
 
-         set_desired_thread (1);
-         regcache = get_thread_regcache (current_thread, 1);
-         registers_from_string (regcache, &own_buf[1]);
-         write_ok (own_buf);
+         if (!set_desired_thread (1))
+           write_enn (own_buf);
+         else
+           {
+             regcache = get_thread_regcache (current_thread, 1);
+             registers_from_string (regcache, &own_buf[1]);
+             write_ok (own_buf);
+           }
        }
       break;
     case 'm':
index cbee960d8a0495b0296f3aacf2531e22d885775d..2ca91597924f984b5fe9c9528936e4c081bf7f79 100644 (file)
@@ -383,11 +383,12 @@ spu_thread_alive (ptid_t ptid)
 static void
 spu_resume (struct thread_resume *resume_info, size_t n)
 {
+  struct thread_info *thr = get_first_thread ();
   size_t i;
 
   for (i = 0; i < n; i++)
     if (ptid_equal (resume_info[i].thread, minus_one_ptid)
-       || ptid_equal (resume_info[i].thread, current_ptid))
+       || ptid_equal (resume_info[i].thread, ptid_of (thr)))
       break;
 
   if (i == n)
@@ -401,7 +402,7 @@ spu_resume (struct thread_resume *resume_info, size_t n)
   regcache_invalidate ();
 
   errno = 0;
-  ptrace (PTRACE_CONT, ptid_get_lwp (current_ptid), 0, resume_info[i].sig);
+  ptrace (PTRACE_CONT, ptid_get_lwp (ptid_of (thr)), 0, resume_info[i].sig);
   if (errno)
     perror_with_name ("ptrace");
 }
@@ -633,7 +634,9 @@ spu_look_up_symbols (void)
 static void
 spu_request_interrupt (void)
 {
-  syscall (SYS_tkill, ptid_get_lwp (current_ptid), SIGINT);
+  struct thread_info *thr = get_first_thread ();
+
+  syscall (SYS_tkill, ptid_get_lwp (thr), SIGINT);
 }
 
 static struct target_ops spu_target_ops = {
index 14999e6eb6f444a88167354cae9598bde4cc0b0f..8fcfe9b0ab658a937a60c0549b8234dc91692425 100644 (file)
@@ -23,7 +23,7 @@
 
 struct target_ops *the_target;
 
-void
+int
 set_desired_thread (int use_general)
 {
   struct thread_info *found;
@@ -33,10 +33,8 @@ set_desired_thread (int use_general)
   else
     found = find_thread_ptid (cont_thread);
 
-  if (found == NULL)
-    current_thread = get_first_thread ();
-  else
-    current_thread = found;
+  current_thread = found;
+  return (current_thread != NULL);
 }
 
 int
index fefd8d180ecadc448244c042e93955423846ebe6..3e3b80fe2c5d44f51377ef6878de2de59f0b0311 100644 (file)
@@ -645,7 +645,7 @@ int read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len);
 int write_inferior_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
                           int len);
 
-void set_desired_thread (int id);
+int set_desired_thread (int id);
 
 const char *target_pid_to_str (ptid_t);