[PATCH 1/2] Add constructor and destructor to thread_info

[PATCH 1/2] Add constructor and destructor to thread_info

[Yao Qi]

This patch adds constructor and destructor to thread_info.

gdb:

2017-03-28  Yao Qi  <yao.qi@linaro.org>

	* gdbthread.h (struct thread_info): Declare constructor and
	destructor.  Add some member initializers.
	* thread.c (free_thread): Remove.
	(init_thread_list): Call delete instead of free_thread.
	(new_thread): Call thread_info constructor.
	(thread_info::thread_info): New function.
	(thread_info::~thread_info): New function.
	(delete_thread_1): Call delete instead of free_thread.
	(make_cleanup_restore_current_thread): Move tp and frame to
	inner block.
---
 gdb/gdbthread.h | 48 ++++++++++++++++++++-----------------
 gdb/thread.c    | 73 ++++++++++++++++++++++++++++++---------------------------
 2 files changed, 65 insertions(+), 56 deletions(-)

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 06ed78f..8ada8f7 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -179,7 +179,11 @@ typedef VEC (value_ptr) value_vec;
 
 struct thread_info
 {
-  struct thread_info *next;
+public:
+  explicit thread_info (struct inferior *inf, ptid_t ptid);
+  ~thread_info ();
+
+  struct thread_info *next = NULL;
   ptid_t ptid;			/* "Actual process id";
 				    In fact, this may be overloaded with 
 				    kernel thread id, etc.  */
@@ -226,13 +230,13 @@ struct thread_info
 
   /* The name of the thread, as specified by the user.  This is NULL
      if the thread does not have a user-given name.  */
-  char *name;
+  char *name = NULL;
 
   /* Non-zero means the thread is executing.  Note: this is different
      from saying that there is an active target and we are stopped at
      a breakpoint, for instance.  This is a real indicator whether the
      thread is off and running.  */
-  int executing;
+  int executing = 0;
 
   /* Non-zero if this thread is resumed from infrun's perspective.
      Note that a thread can be marked both as not-executing and
@@ -241,19 +245,19 @@ struct thread_info
      thread really run until that wait status has been processed, but
      we should not process that wait status if we didn't try to let
      the thread run.  */
-  int resumed;
+  int resumed = 0;
 
   /* Frontend view of the thread state.  Note that the THREAD_RUNNING/
      THREAD_STOPPED states are different from EXECUTING.  When the
      thread is stopped internally while handling an internal event,
      like a software single-step breakpoint, EXECUTING will be false,
      but STATE will still be THREAD_RUNNING.  */
-  enum thread_state state;
+  enum thread_state state = THREAD_STOPPED;
 
   /* If this is > 0, then it means there's code out there that relies
      on this thread being listed.  Don't delete it from the lists even
      if we detect it exiting.  */
-  int refcount;
+  int refcount = 0;
 
   /* State of GDB control of inferior thread execution.
      See `struct thread_control_state'.  */
@@ -263,8 +267,8 @@ struct thread_info
      call.  See `struct thread_suspend_state'.  */
   struct thread_suspend_state suspend;
 
-  int current_line;
-  struct symtab *current_symtab;
+  int current_line = 0;
+  struct symtab *current_symtab = 0;
 
   /* Internal stepping state.  */
 
@@ -274,20 +278,20 @@ struct thread_info
      by proceed and keep_going, and among other things, it's used in
      adjust_pc_after_break to distinguish a hardware single-step
      SIGTRAP from a breakpoint SIGTRAP.  */
-  CORE_ADDR prev_pc;
+  CORE_ADDR prev_pc = 0;
 
   /* Did we set the thread stepping a breakpoint instruction?  This is
      used in conjunction with PREV_PC to decide whether to adjust the
      PC.  */
-  int stepped_breakpoint;
+  int stepped_breakpoint = 0;
 
   /* Should we step over breakpoint next time keep_going is called?  */
-  int stepping_over_breakpoint;
+  int stepping_over_breakpoint = 0;
 
   /* Should we step over a watchpoint next time keep_going is called?
      This is needed on targets with non-continuable, non-steppable
      watchpoints.  */
-  int stepping_over_watchpoint;
+  int stepping_over_watchpoint = 0;
 
   /* Set to TRUE if we should finish single-stepping over a breakpoint
      after hitting the current step-resume breakpoint.  The context here
@@ -298,12 +302,12 @@ struct thread_info
      step_after_step_resume_breakpoint is set to TRUE at this moment in
      order to keep GDB in mind that there is still a breakpoint to step over
      when GDB gets back SIGTRAP from step_resume_breakpoint.  */
-  int step_after_step_resume_breakpoint;
+  int step_after_step_resume_breakpoint = 0;
 
   /* Pointer to the state machine manager object that handles what is
      left to do for the thread's execution command after the target
      stops.  Several execution commands use it.  */
-  struct thread_fsm *thread_fsm;
+  struct thread_fsm *thread_fsm = NULL;
 
   /* This is used to remember when a fork or vfork event was caught by
      a catchpoint, and thus the event is to be followed at the next
@@ -311,37 +315,37 @@ struct thread_info
   struct target_waitstatus pending_follow;
 
   /* True if this thread has been explicitly requested to stop.  */
-  int stop_requested;
+  int stop_requested = 0;
 
   /* The initiating frame of a nexting operation, used for deciding
      which exceptions to intercept.  If it is null_frame_id no
      bp_longjmp or bp_exception but longjmp has been caught just for
      bp_longjmp_call_dummy.  */
-  struct frame_id initiating_frame;
+  struct frame_id initiating_frame = null_frame_id;
 
   /* Private data used by the target vector implementation.  */
-  struct private_thread_info *priv;
+  struct private_thread_info *priv = NULL;
 
   /* Function that is called to free PRIVATE.  If this is NULL, then
      xfree will be called on PRIVATE.  */
-  void (*private_dtor) (struct private_thread_info *);
+  void (*private_dtor) (struct private_thread_info *) = NULL;
 
   /* Branch trace information for this thread.  */
   struct btrace_thread_info btrace;
 
   /* Flag which indicates that the stack temporaries should be stored while
      evaluating expressions.  */
-  int stack_temporaries_enabled;
+  int stack_temporaries_enabled = 0;
 
   /* Values that are stored as temporaries on stack while evaluating
      expressions.  */
-  value_vec *stack_temporaries;
+  value_vec *stack_temporaries = NULL;
 
   /* Step-over chain.  A thread is in the step-over queue if these are
      non-NULL.  If only a single thread is in the chain, then these
      fields point to self.  */
-  struct thread_info *step_over_prev;
-  struct thread_info *step_over_next;
+  struct thread_info *step_over_prev = NULL;
+  struct thread_info *step_over_next = NULL;
 };
 
 /* Create an empty thread list, or empty the existing one.  */
diff --git a/gdb/thread.c b/gdb/thread.c
index 99fe424..28907c5 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -192,21 +192,6 @@ clear_thread_inferior_resources (struct thread_info *tp)
   thread_cancel_execution_command (tp);
 }
 
-static void
-free_thread (struct thread_info *tp)
-{
-  if (tp->priv)
-    {
-      if (tp->private_dtor)
-	tp->private_dtor (tp->priv);
-      else
-	xfree (tp->priv);
-    }
-
-  xfree (tp->name);
-  xfree (tp);
-}
-
 void
 init_thread_list (void)
 {
@@ -220,7 +205,8 @@ init_thread_list (void)
   for (tp = thread_list; tp; tp = tpnext)
     {
       tpnext = tp->next;
-      free_thread (tp);
+      delete tp;
+
     }
 
   thread_list = NULL;
@@ -233,16 +219,7 @@ init_thread_list (void)
 static struct thread_info *
 new_thread (struct inferior *inf, ptid_t ptid)
 {
-  struct thread_info *tp;
-
-  gdb_assert (inf != NULL);
-
-  tp = XCNEW (struct thread_info);
-
-  tp->ptid = ptid;
-  tp->global_num = ++highest_thread_num;
-  tp->inf = inf;
-  tp->per_inf_num = ++inf->highest_thread_num;
+  struct thread_info *tp = new thread_info (inf, ptid);
 
   if (thread_list == NULL)
     thread_list = tp;
@@ -255,11 +232,6 @@ new_thread (struct inferior *inf, ptid_t ptid)
       last->next = tp;
     }
 
-  /* Nothing to follow yet.  */
-  tp->pending_follow.kind = TARGET_WAITKIND_SPURIOUS;
-  tp->state = THREAD_STOPPED;
-  tp->suspend.waitstatus.kind = TARGET_WAITKIND_IGNORE;
-
   return tp;
 }
 
@@ -336,6 +308,38 @@ add_thread (ptid_t ptid)
   return add_thread_with_info (ptid, NULL);
 }
 
+thread_info::thread_info (struct inferior *inf, ptid_t ptid)
+{
+  gdb_assert (inf != NULL);
+
+  this->ptid = ptid;
+  this->global_num = ++highest_thread_num;
+  this->inf = inf;
+  this->per_inf_num = ++inf->highest_thread_num;
+
+  /* Nothing to follow yet.  */
+  memset (&this->pending_follow, 0, sizeof (this->pending_follow));
+  memset (&this->control, 0, sizeof (this->control));
+  memset (&this->suspend, 0, sizeof (this->suspend));
+  memset (&this->btrace, 0, sizeof (this->btrace));
+
+  this->pending_follow.kind = TARGET_WAITKIND_SPURIOUS;
+  this->suspend.waitstatus.kind = TARGET_WAITKIND_IGNORE;
+}
+
+thread_info::~thread_info ()
+{
+  if (this->priv)
+    {
+      if (this->private_dtor)
+	this->private_dtor (this->priv);
+      else
+	xfree (this->priv);
+    }
+
+  xfree (this->name);
+}
+
 /* Add TP to the end of the step-over chain LIST_P.  */
 
 static void
@@ -470,7 +474,7 @@ delete_thread_1 (ptid_t ptid, int silent)
   else
     thread_list = tp->next;
 
-  free_thread (tp);
+  delete tp;
 }
 
 /* Delete thread PTID and notify of thread exit.  If this is
@@ -1655,8 +1659,6 @@ set_thread_refcount (void *data)
 struct cleanup *
 make_cleanup_restore_current_thread (void)
 {
-  struct thread_info *tp;
-  struct frame_info *frame;
   struct current_thread_cleanup *old = XNEW (struct current_thread_cleanup);
 
   old->inferior_ptid = inferior_ptid;
@@ -1668,6 +1670,9 @@ make_cleanup_restore_current_thread (void)
 
   if (!ptid_equal (inferior_ptid, null_ptid))
     {
+      struct thread_info *tp;
+      struct frame_info *frame;
+
       old->was_stopped = is_stopped (inferior_ptid);
       if (old->was_stopped
 	  && target_has_registers
-- 
1.9.1

[PATCH 2/2] Delete thread_info is refcount is zero

[Yao Qi]

I build GDB with asan, and run test case hook-stop.exp, and threadapply.exp,
I got the following asan error,

=================================================================^M
^[[1m^[[31m==2291==ERROR: AddressSanitizer: heap-use-after-free on address 0x6160000999c4 at pc 0x000000826022 bp 0x7ffd28a8ff70 sp 0x7ffd28a8ff60^M
^[[1m^[[0m^[[1m^[[34mREAD of size 4 at 0x6160000999c4 thread T0^[[1m^[[0m^M
    #0 0x826021 in release_stop_context_cleanup ../../binutils-gdb/gdb/infrun.c:8203^M
    #1 0x72798a in do_my_cleanups ../../binutils-gdb/gdb/common/cleanups.c:154^M
    #2 0x727a32 in do_cleanups(cleanup*) ../../binutils-gdb/gdb/common/cleanups.c:176^M
    #3 0x826895 in normal_stop() ../../binutils-gdb/gdb/infrun.c:8381^M
    #4 0x815208 in fetch_inferior_event(void*) ../../binutils-gdb/gdb/infrun.c:4011^M
    #5 0x868aca in inferior_event_handler(inferior_event_type, void*) ../../binutils-gdb/gdb/inf-loop.c:44^M
....
^[[1m^[[32m0x6160000999c4 is located 68 bytes inside of 568-byte region [0x616000099980,0x616000099bb8)^M
^[[1m^[[0m^[[1m^[[35mfreed by thread T0 here:^[[1m^[[0m^M
    #0 0x7fb0bc1312ca in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x982ca)^M
    #1 0xb8c62f in xfree(void*) ../../binutils-gdb/gdb/common/common-utils.c:100^M
    #2 0x83df67 in free_thread ../../binutils-gdb/gdb/thread.c:207^M
    #3 0x83dfd2 in init_thread_list() ../../binutils-gdb/gdb/thread.c:223^M
    #4 0x805494 in kill_command ../../binutils-gdb/gdb/infcmd.c:2595^M
....

Detaching from program: /home/yao.qi/SourceCode/gnu/build-with-asan/gdb/testsuite/outputs/gdb.threads/threadapply/threadapply, process 2399^M
=================================================================^M
^[[1m^[[31m==2387==ERROR: AddressSanitizer: heap-use-after-free on address 0x6160000a98c0 at pc 0x00000083fd28 bp 0x7ffd401c3110 sp 0x7ffd401c3100^M
^[[1m^[[0m^[[1m^[[34mREAD of size 4 at 0x6160000a98c0 thread T0^[[1m^[[0m^M
    #0 0x83fd27 in thread_alive ../../binutils-gdb/gdb/thread.c:741^M
    #1 0x844277 in thread_apply_all_command ../../binutils-gdb/gdb/thread.c:1804^M
....
^M
^[[1m^[[32m0x6160000a98c0 is located 64 bytes inside of 568-byte region [0x6160000a9880,0x6160000a9ab8)^M
^[[1m^[[0m^[[1m^[[35mfreed by thread T0 here:^[[1m^[[0m^M
    #0 0x7f59a7e322ca in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x982ca)^M
    #1 0xb8c62f in xfree(void*) ../../binutils-gdb/gdb/common/common-utils.c:100^M
    #2 0x83df67 in free_thread ../../binutils-gdb/gdb/thread.c:207^M
    #3 0x83dfd2 in init_thread_list() ../../binutils-gdb/gdb/thread.c:223^M

This patch fixes the issue by always checking refcount before decreasing it.
If it is zero already, delete the thread_info.

gdb:

2017-03-28  Yao Qi  <yao.qi@linaro.org>

	PR gdb/19942
	* infrun.c (release_stop_context_cleanup): If refcount is zero
	delete thread, otherwise decrease the refcount.
	* thread.c (init_thread_list): Likewise.
	(restore_current_thread_cleanup_dtor): Likewise.
	(set_thread_refcount): Likewise.
---
 gdb/infrun.c |  7 ++++++-
 gdb/thread.c | 19 ++++++++++++++++---
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index c8c2d6e..f8eeddb 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -8182,7 +8182,12 @@ release_stop_context_cleanup (void *arg)
   struct stop_context *sc = (struct stop_context *) arg;
 
   if (sc->thread != NULL)
-    sc->thread->refcount--;
+    {
+      if (sc->thread->refcount == 0)
+	delete sc->thread;
+      else
+	sc->thread->refcount--;
+    }
   xfree (sc);
 }
 
diff --git a/gdb/thread.c b/gdb/thread.c
index 28907c5..b38c5bd 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -205,8 +205,11 @@ init_thread_list (void)
   for (tp = thread_list; tp; tp = tpnext)
     {
       tpnext = tp->next;
-      delete tp;
 
+      if (tp->refcount == 0)
+	delete tp;
+      else
+	tp->refcount--;
     }
 
   thread_list = NULL;
@@ -1636,7 +1639,12 @@ restore_current_thread_cleanup_dtor (void *arg)
 
   tp = find_thread_ptid (old->inferior_ptid);
   if (tp)
-    tp->refcount--;
+    {
+      if (tp->refcount == 0)
+	delete tp;
+      else
+	tp->refcount--;
+    }
   inf = find_inferior_id (old->inf_id);
   if (inf != NULL)
     inf->removable = old->was_removable;
@@ -1653,7 +1661,12 @@ set_thread_refcount (void *data)
     = (struct thread_array_cleanup *) data;
 
   for (k = 0; k != ta_cleanup->count; k++)
-    ta_cleanup->tp_array[k]->refcount--;
+    {
+      if (ta_cleanup->tp_array[k]->refcount == 0)
+	delete ta_cleanup->tp_array[k];
+      else
+	ta_cleanup->tp_array[k]->refcount--;
+    }
 }
 
 struct cleanup *
-- 
1.9.1

[PATCH 0/2] Fix thread_info refcount

[Yao Qi]

This patch set fixes PR gdb/19942 and hopefully fixes a GDB internal
error reported https://sourceware.org/ml/gdb/2017-03/msg00054.html
The fix in general is that we should free/delete thread_info when
its refcount is zero.  free_thread is a static function, which is
not visible out side of thread.c, so have to make it external.
That is what I did in the patch fixing this problem for 7.12 branch
(https://sourceware.org/ml/gdb/2017-03/msg00062.html)

However, we can add constructor and destructor to thread_info, and
use delete instead of free_thread.  That is why patch 1 add constructor
and destructor to thread_info.  Also, this change paves the way of
using std::shared_ptr, so that "refcount" can be moved.

They are tested on x86_64-linux.

*** BLURB HERE ***

Yao Qi (2):
  Add constructor and destructor to thread_info
  Delete thread_info is refcount is zero

 gdb/gdbthread.h | 48 ++++++++++++++++--------------
 gdb/infrun.c    |  7 ++++-
 gdb/thread.c    | 90 ++++++++++++++++++++++++++++++++++-----------------------
 3 files changed, 86 insertions(+), 59 deletions(-)

-- 
1.9.1

Re: [PATCH 2/2] Delete thread_info is refcount is zero

[Pedro Alves]

On 03/28/2017 09:24 AM, Yao Qi wrote:
> I build GDB with asan, and run test case hook-stop.exp, and threadapply.exp,
> I got the following asan error,
> 
> =================================================================^M
> ^[[1m^[[31m==2291==ERROR: AddressSanitizer: heap-use-after-free on address 0x6160000999c4 at pc 0x000000826022 bp 0x7ffd28a8ff70 sp 0x7ffd28a8ff60^M
> ^[[1m^[[0m^[[1m^[[34mREAD of size 4 at 0x6160000999c4 thread T0^[[1m^[[0m^M
>     #0 0x826021 in release_stop_context_cleanup ../../binutils-gdb/gdb/infrun.c:8203^M
>     #1 0x72798a in do_my_cleanups ../../binutils-gdb/gdb/common/cleanups.c:154^M
>     #2 0x727a32 in do_cleanups(cleanup*) ../../binutils-gdb/gdb/common/cleanups.c:176^M
>     #3 0x826895 in normal_stop() ../../binutils-gdb/gdb/infrun.c:8381^M
>     #4 0x815208 in fetch_inferior_event(void*) ../../binutils-gdb/gdb/infrun.c:4011^M
>     #5 0x868aca in inferior_event_handler(inferior_event_type, void*) ../../binutils-gdb/gdb/inf-loop.c:44^M
> ....
> ^[[1m^[[32m0x6160000999c4 is located 68 bytes inside of 568-byte region [0x616000099980,0x616000099bb8)^M
> ^[[1m^[[0m^[[1m^[[35mfreed by thread T0 here:^[[1m^[[0m^M
>     #0 0x7fb0bc1312ca in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x982ca)^M
>     #1 0xb8c62f in xfree(void*) ../../binutils-gdb/gdb/common/common-utils.c:100^M
>     #2 0x83df67 in free_thread ../../binutils-gdb/gdb/thread.c:207^M
>     #3 0x83dfd2 in init_thread_list() ../../binutils-gdb/gdb/thread.c:223^M
>     #4 0x805494 in kill_command ../../binutils-gdb/gdb/infcmd.c:2595^M
> ....
> 
> Detaching from program: /home/yao.qi/SourceCode/gnu/build-with-asan/gdb/testsuite/outputs/gdb.threads/threadapply/threadapply, process 2399^M
> =================================================================^M
> ^[[1m^[[31m==2387==ERROR: AddressSanitizer: heap-use-after-free on address 0x6160000a98c0 at pc 0x00000083fd28 bp 0x7ffd401c3110 sp 0x7ffd401c3100^M
> ^[[1m^[[0m^[[1m^[[34mREAD of size 4 at 0x6160000a98c0 thread T0^[[1m^[[0m^M
>     #0 0x83fd27 in thread_alive ../../binutils-gdb/gdb/thread.c:741^M
>     #1 0x844277 in thread_apply_all_command ../../binutils-gdb/gdb/thread.c:1804^M
> ....
> ^M
> ^[[1m^[[32m0x6160000a98c0 is located 64 bytes inside of 568-byte region [0x6160000a9880,0x6160000a9ab8)^M
> ^[[1m^[[0m^[[1m^[[35mfreed by thread T0 here:^[[1m^[[0m^M
>     #0 0x7f59a7e322ca in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x982ca)^M
>     #1 0xb8c62f in xfree(void*) ../../binutils-gdb/gdb/common/common-utils.c:100^M
>     #2 0x83df67 in free_thread ../../binutils-gdb/gdb/thread.c:207^M
>     #3 0x83dfd2 in init_thread_list() ../../binutils-gdb/gdb/thread.c:223^M
> 
> This patch fixes the issue by always checking refcount before decreasing it.
> If it is zero already, delete the thread_info.

So this init_thread_list is just blasting away all threads in the
the thread list, even if something is holding a strong reference
to some thread.  That does look wrong.

> 
> gdb:
> 
> 2017-03-28  Yao Qi  <yao.qi@linaro.org>
> 
> 	PR gdb/19942
> 	* infrun.c (release_stop_context_cleanup): If refcount is zero
> 	delete thread, otherwise decrease the refcount.
> 	* thread.c (init_thread_list): Likewise.
> 	(restore_current_thread_cleanup_dtor): Likewise.
> 	(set_thread_refcount): Likewise.
> ---
>  gdb/infrun.c |  7 ++++++-
>  gdb/thread.c | 19 ++++++++++++++++---
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index c8c2d6e..f8eeddb 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -8182,7 +8182,12 @@ release_stop_context_cleanup (void *arg)
>    struct stop_context *sc = (struct stop_context *) arg;
>  
>    if (sc->thread != NULL)
> -    sc->thread->refcount--;
> +    {
> +      if (sc->thread->refcount == 0)
> +	delete sc->thread;
> +      else
> +	sc->thread->refcount--;

This pattern now appears in multiple places.  How about
moving it into a function?  Or a method, and use the
"delete this;" idiom.  But, see below first.

> +    }
>    xfree (sc);
>  }
>  
> diff --git a/gdb/thread.c b/gdb/thread.c
> index 28907c5..b38c5bd 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -205,8 +205,11 @@ init_thread_list (void)
>    for (tp = thread_list; tp; tp = tpnext)
>      {
>        tpnext = tp->next;
> -      delete tp;
>  
> +      if (tp->refcount == 0)
> +	delete tp;
> +      else
> +	tp->refcount--;

I'm not convinced this is correct.  Only something that incremented
a refcount should decrement it back.  This looks like stealing a
refcount from something else, which is a recipe for bugs.

For example, say a thread has refcount == 2 when you get here.
init_thread_list decrements the count, and then clear the 
thread_list head pointer.  I.e., thread list is now empty, even if
there are still thread objects around.  Later on, if one of
the refcounts was owned by a make_cleanup_restore_current_thread call,
then essentially, the thread leaks, because
restore_current_thread_cleanup_dtor won't be able to find it anymore
in the thread list.

Also, if we're disposing of all threads (via init_thread_list),
but end up leaving some thread behind, I'd think we should mark
the thread as THREAD_EXITED, and maybe call clear_thread_inferior_resources
on it.

Essentially, it looks to me like we need to factor out, or copy
parts of what delete_thread_1 does and use those bits in
init_thread_list.

On thread refcounts and lifetime:

Thread start with refcount 0.  Putting a thread on the
thread list does not increment a thread's ref count.  It's an implicit
strong reference instead.  So removing a thread from the thread list should
not decrement the thread's refcount either.

Selecting a thread by making inferior_ptid point at it is another
way to create a "strong reference" to a thread.  That does not increment
the thread's refcount either, as it seems impossible to do while we
have code that switches inferior_ptid directly (and at places
inferior_ptid may not even point to a valid thread).

>      }
>  
>    thread_list = NULL;
> @@ -1636,7 +1639,12 @@ restore_current_thread_cleanup_dtor (void *arg)
>  
>    tp = find_thread_ptid (old->inferior_ptid);
>    if (tp)
> -    tp->refcount--;
> +    {
> +      if (tp->refcount == 0)

This looks really incorrect to me.  If we found the thread on the list,
then it should still have the refcount that the cleanup originally
added.  Anything else just leads to potential refcounting
mismatching bugs.

For example, say a new thread with the same ptid was added
to the thread list, between make_cleanup_restore_current_thread and the
cleanup running (the code above).  E.g., because of a command like
"thread apply all my_reattach", where "my_reattach" detaches from a process,
and reattaches to the same process (or kill+run, and thread reuse,
or disconnect/reconnect.  etc.).

The code above will incorrectly delete the _new_ thread, which has
refcount == 0, because it happened to have the same ptid as the
old one, and so find_thread_ptid finds it.

So all in all, it seems to me that init_thread_list should skip deleting
a thread if the thread still has a reference (like delete_thread_1),
and, make_cleanup_restore_current_thread should be changed to
hold a pointer to a thread instead of a ptid.  And then
do_restore_current_thread_cleanup / restore_current_thread_cleanup_dtor
should use the pointer directly instead of doing ptid look ups.
And with that, the refcounting should sort itself out.

Thanks,
Pedro Alves

Re: [PATCH 1/2] Add constructor and destructor to thread_info

[Pedro Alves]

On 03/28/2017 09:24 AM, Yao Qi wrote:
> This patch adds constructor and destructor to thread_info.

Great, thanks.

LGTM.  Some nits below.

> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> index 06ed78f..8ada8f7 100644
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -179,7 +179,11 @@ typedef VEC (value_ptr) value_vec;
>  
>  struct thread_info
>  {
> -  struct thread_info *next;
> +public:
> +  explicit thread_info (struct inferior *inf, ptid_t ptid);

I'd remove the unnecessary "struct" while at it.

> +  ~thread_info ();
> +
> +  struct thread_info *next = NULL;
>    ptid_t ptid;			/* "Actual process id";
>  				    In fact, this may be overloaded with 
>  				    kernel thread id, etc.  */
> @@ -226,13 +230,13 @@ struct thread_info
>  
>    /* The name of the thread, as specified by the user.  This is NULL
>       if the thread does not have a user-given name.  */
> -  char *name;
> +  char *name = NULL;
>  
>    /* Non-zero means the thread is executing.  Note: this is different
>       from saying that there is an active target and we are stopped at
>       a breakpoint, for instance.  This is a real indicator whether the
>       thread is off and running.  */
> -  int executing;
> +  int executing = 0;

and do:

  bool executing = false;

etc.  (and tweak "Non-zero" references to say "True" instead.)

(the patch's subject just becomes "C++-fy thread_info a bit" instead.)

>  
>    /* Non-zero if this thread is resumed from infrun's perspective.
>       Note that a thread can be marked both as not-executing and
> @@ -241,19 +245,19 @@ struct thread_info
>       thread really run until that wait status has been processed, but
>       we should not process that wait status if we didn't try to let
>       the thread run.  */
> -  int resumed;
> +  int resumed = 0;
>  
>    /* Frontend view of the thread state.  Note that the THREAD_RUNNING/
>       THREAD_STOPPED states are different from EXECUTING.  When the
>       thread is stopped internally while handling an internal event,
>       like a software single-step breakpoint, EXECUTING will be false,
>       but STATE will still be THREAD_RUNNING.  */
> -  enum thread_state state;
> +  enum thread_state state = THREAD_STOPPED;
>  
>    /* If this is > 0, then it means there's code out there that relies
>       on this thread being listed.  Don't delete it from the lists even
>       if we detect it exiting.  */
> -  int refcount;
> +  int refcount = 0;
>  
>    /* State of GDB control of inferior thread execution.
>       See `struct thread_control_state'.  */
> @@ -263,8 +267,8 @@ struct thread_info
>       call.  See `struct thread_suspend_state'.  */
>    struct thread_suspend_state suspend;
>  
> -  int current_line;
> -  struct symtab *current_symtab;
> +  int current_line = 0;
> +  struct symtab *current_symtab = 0;

NULL ?

>  
>    /* Internal stepping state.  */
>  
> @@ -274,20 +278,20 @@ struct thread_info
>       by proceed and keep_going, and among other things, it's used in
>       adjust_pc_after_break to distinguish a hardware single-step
>       SIGTRAP from a breakpoint SIGTRAP.  */
> -  CORE_ADDR prev_pc;
> +  CORE_ADDR prev_pc = 0;
>  
>    /* Did we set the thread stepping a breakpoint instruction?  This is
>       used in conjunction with PREV_PC to decide whether to adjust the
>       PC.  */
> -  int stepped_breakpoint;
> +  int stepped_breakpoint = 0;
>  
>    /* Should we step over breakpoint next time keep_going is called?  */
> -  int stepping_over_breakpoint;
> +  int stepping_over_breakpoint = 0;
>  
>    /* Should we step over a watchpoint next time keep_going is called?
>       This is needed on targets with non-continuable, non-steppable
>       watchpoints.  */
> -  int stepping_over_watchpoint;
> +  int stepping_over_watchpoint = 0;
>  
>    /* Set to TRUE if we should finish single-stepping over a breakpoint
>       after hitting the current step-resume breakpoint.  The context here
> @@ -298,12 +302,12 @@ struct thread_info
>       step_after_step_resume_breakpoint is set to TRUE at this moment in
>       order to keep GDB in mind that there is still a breakpoint to step over
>       when GDB gets back SIGTRAP from step_resume_breakpoint.  */
> -  int step_after_step_resume_breakpoint;
> +  int step_after_step_resume_breakpoint = 0;
>  
>    /* Pointer to the state machine manager object that handles what is
>       left to do for the thread's execution command after the target
>       stops.  Several execution commands use it.  */
> -  struct thread_fsm *thread_fsm;
> +  struct thread_fsm *thread_fsm = NULL;
>  
>    /* This is used to remember when a fork or vfork event was caught by
>       a catchpoint, and thus the event is to be followed at the next
> @@ -311,37 +315,37 @@ struct thread_info
>    struct target_waitstatus pending_follow;
>  
>    /* True if this thread has been explicitly requested to stop.  */
> -  int stop_requested;
> +  int stop_requested = 0;
>  
>    /* The initiating frame of a nexting operation, used for deciding
>       which exceptions to intercept.  If it is null_frame_id no
>       bp_longjmp or bp_exception but longjmp has been caught just for
>       bp_longjmp_call_dummy.  */
> -  struct frame_id initiating_frame;
> +  struct frame_id initiating_frame = null_frame_id;
>  
>    /* Private data used by the target vector implementation.  */
> -  struct private_thread_info *priv;
> +  struct private_thread_info *priv = NULL;
>  
>    /* Function that is called to free PRIVATE.  If this is NULL, then
>       xfree will be called on PRIVATE.  */
> -  void (*private_dtor) (struct private_thread_info *);
> +  void (*private_dtor) (struct private_thread_info *) = NULL;
>  
>    /* Branch trace information for this thread.  */
>    struct btrace_thread_info btrace;
>  
>    /* Flag which indicates that the stack temporaries should be stored while
>       evaluating expressions.  */
> -  int stack_temporaries_enabled;
> +  int stack_temporaries_enabled = 0;
>  
>    /* Values that are stored as temporaries on stack while evaluating
>       expressions.  */
> -  value_vec *stack_temporaries;
> +  value_vec *stack_temporaries = NULL;
>  
>    /* Step-over chain.  A thread is in the step-over queue if these are
>       non-NULL.  If only a single thread is in the chain, then these
>       fields point to self.  */
> -  struct thread_info *step_over_prev;
> -  struct thread_info *step_over_next;
> +  struct thread_info *step_over_prev = NULL;
> +  struct thread_info *step_over_next = NULL;
>  };
>  
>  /* Create an empty thread list, or empty the existing one.  */
> diff --git a/gdb/thread.c b/gdb/thread.c
> index 99fe424..28907c5 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -192,21 +192,6 @@ clear_thread_inferior_resources (struct thread_info *tp)
>    thread_cancel_execution_command (tp);
>  }
>  
> -static void
> -free_thread (struct thread_info *tp)
> -{
> -  if (tp->priv)
> -    {
> -      if (tp->private_dtor)
> -	tp->private_dtor (tp->priv);
> -      else
> -	xfree (tp->priv);
> -    }
> -
> -  xfree (tp->name);
> -  xfree (tp);
> -}
> -
>  void
>  init_thread_list (void)
>  {
> @@ -220,7 +205,8 @@ init_thread_list (void)
>    for (tp = thread_list; tp; tp = tpnext)
>      {
>        tpnext = tp->next;
> -      free_thread (tp);
> +      delete tp;
> +

Spurious whitespace.

>      }
>  
>    thread_list = NULL;
> @@ -233,16 +219,7 @@ init_thread_list (void)
>  static struct thread_info *
>  new_thread (struct inferior *inf, ptid_t ptid)
>  {
> -  struct thread_info *tp;
> -
> -  gdb_assert (inf != NULL);
> -
> -  tp = XCNEW (struct thread_info);
> -
> -  tp->ptid = ptid;
> -  tp->global_num = ++highest_thread_num;
> -  tp->inf = inf;
> -  tp->per_inf_num = ++inf->highest_thread_num;
> +  struct thread_info *tp = new thread_info (inf, ptid);

Drop "struct" ?

>  
>    if (thread_list == NULL)
>      thread_list = tp;
> @@ -255,11 +232,6 @@ new_thread (struct inferior *inf, ptid_t ptid)
>        last->next = tp;
>      }
>  
> -  /* Nothing to follow yet.  */
> -  tp->pending_follow.kind = TARGET_WAITKIND_SPURIOUS;
> -  tp->state = THREAD_STOPPED;
> -  tp->suspend.waitstatus.kind = TARGET_WAITKIND_IGNORE;
> -
>    return tp;
>  }
>  
> @@ -336,6 +308,38 @@ add_thread (ptid_t ptid)
>    return add_thread_with_info (ptid, NULL);
>  }
>  
> +thread_info::thread_info (struct inferior *inf, ptid_t ptid)
> +{
> +  gdb_assert (inf != NULL);
> +
> +  this->ptid = ptid;
> +  this->global_num = ++highest_thread_num;
> +  this->inf = inf;
> +  this->per_inf_num = ++inf->highest_thread_num;
> +
> +  /* Nothing to follow yet.  */
> +  memset (&this->pending_follow, 0, sizeof (this->pending_follow));
> +  memset (&this->control, 0, sizeof (this->control));
> +  memset (&this->suspend, 0, sizeof (this->suspend));
> +  memset (&this->btrace, 0, sizeof (this->btrace));
> +
> +  this->pending_follow.kind = TARGET_WAITKIND_SPURIOUS;
> +  this->suspend.waitstatus.kind = TARGET_WAITKIND_IGNORE;

These could all be in-class initialized like you did to the other
fields, instead of using the explicit memset:

 - struct btrace_thread_info btrace;
 + struct btrace_thread_info btrace {};

 - struct target_waitstatus pending_follow;
 + struct target_waitstatus pending_follow {TARGET_WAITKIND_SPURIOUS};

(maybe drop the "struct" too while at it).

>  /* Delete thread PTID and notify of thread exit.  If this is
> @@ -1655,8 +1659,6 @@ set_thread_refcount (void *data)
>  struct cleanup *
>  make_cleanup_restore_current_thread (void)
>  {
> -  struct thread_info *tp;
> -  struct frame_info *frame;
>    struct current_thread_cleanup *old = XNEW (struct current_thread_cleanup);
>  
>    old->inferior_ptid = inferior_ptid;
> @@ -1668,6 +1670,9 @@ make_cleanup_restore_current_thread (void)
>  
>    if (!ptid_equal (inferior_ptid, null_ptid))
>      {
> +      struct thread_info *tp;

I'd move this one further down to where it is initialized:

      thread_info *tp = find_thread_ptid (inferior_ptid);

> +      struct frame_info *frame;
> +
>        old->was_stopped = is_stopped (inferior_ptid);
>        if (old->was_stopped
>  	  && target_has_registers
> 

Thanks,
Pedro Alves

Re: [PATCH 1/2] Add constructor and destructor to thread_info

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

>>    /* Non-zero means the thread is executing.  Note: this is different
>>       from saying that there is an active target and we are stopped at
>>       a breakpoint, for instance.  This is a real indicator whether the
>>       thread is off and running.  */
>> -  int executing;
>> +  int executing = 0;
>
> and do:
>
>   bool executing = false;
>
> etc.  (and tweak "Non-zero" references to say "True" instead.)
>
> (the patch's subject just becomes "C++-fy thread_info a bit" instead.)
>

Other thread_info fields can be changed to bool too.  Once I change
"executing" to bool, the 2nd argument of set_executing should be bool
too.  I'll change them in separate patches.


>> @@ -220,7 +205,8 @@ init_thread_list (void)
>>    for (tp = thread_list; tp; tp = tpnext)
>>      {
>>        tpnext = tp->next;
>> -      free_thread (tp);
>> +      delete tp;
>> +
>
> Spurious whitespace.
>

You mean spurious newline?

> These could all be in-class initialized like you did to the other
> fields, instead of using the explicit memset:
>
>  - struct btrace_thread_info btrace;
>  + struct btrace_thread_info btrace {};
>
>  - struct target_waitstatus pending_follow;
>  + struct target_waitstatus pending_follow {TARGET_WAITKIND_SPURIOUS};
>

This will only initialize the first member of union
target_waitstatus.value to zero, so some bits of
target_waitstatus.value.related_pid is not initialized.

> (maybe drop the "struct" too while at it).

-- 
Yao (齐尧)

Re: [PATCH 1/2] Add constructor and destructor to thread_info

[Pedro Alves]

On 03/28/2017 03:08 PM, Yao Qi wrote:

>>
>> etc.  (and tweak "Non-zero" references to say "True" instead.)
>>
>> (the patch's subject just becomes "C++-fy thread_info a bit" instead.)
>>
> 
> Other thread_info fields can be changed to bool too.  

Yes, hence the "etc.".

> Once I change
> "executing" to bool, the 2nd argument of set_executing should be bool
> too.  I'll change them in separate patches.

Thanks, TBC, I'm fine with incremental progress toward bool
and with doing it opportunistically, rather than requiring targeted
patches.  Otherwise if we require changing everything at once, the
result is that people will avoid doing it, for fear of the growing
cascading work that propagating a type change everywhere tends to require.

> 
> 
>>> @@ -220,7 +205,8 @@ init_thread_list (void)
>>>    for (tp = thread_list; tp; tp = tpnext)
>>>      {
>>>        tpnext = tp->next;
>>> -      free_thread (tp);
>>> +      delete tp;
>>> +
>>
>> Spurious whitespace.
>>
> 
> You mean spurious newline?
> 

Yes, sorry.

>> These could all be in-class initialized like you did to the other
>> fields, instead of using the explicit memset:
>>
>>  - struct btrace_thread_info btrace;
>>  + struct btrace_thread_info btrace {};
>>
>>  - struct target_waitstatus pending_follow;
>>  + struct target_waitstatus pending_follow {TARGET_WAITKIND_SPURIOUS};
>>
> 
> This will only initialize the first member of union
> target_waitstatus.value to zero, so some bits of
> target_waitstatus.value.related_pid is not initialized.

Ah, blah, unions.  Well, the result is that the remaining fields of
the struct end up list->value->zero-initialized.  Zero-initialization
for unions means the first member is zero-initialized, and padding
is initialized to zero.  So I think the end result is the
same anyway.

Thanks,
Pedro Alves

Re: [PATCH 1/2] Add constructor and destructor to thread_info

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

>>>  - struct target_waitstatus pending_follow;
>>>  + struct target_waitstatus pending_follow {TARGET_WAITKIND_SPURIOUS};
>>>
>> 
>> This will only initialize the first member of union
>> target_waitstatus.value to zero, so some bits of
>> target_waitstatus.value.related_pid is not initialized.
>
> Ah, blah, unions.  Well, the result is that the remaining fields of
> the struct end up list->value->zero-initialized.  Zero-initialization
> for unions means the first member is zero-initialized, and padding
> is initialized to zero.  So I think the end result is the
> same anyway.

It is different from
"memset (&this->pending_follow, 0, sizeof (this->pending_follow))".  The
first member "integer" is zero-initialized, but it is not the largest
member, so part of "related_pid" is not zero-initialized.

  |<------------------ union----------------->|
  |<------- related_pid ------->|<- padding ->|
  |<- integer ->|
  |      0      |<uninitialized>|     0       |

-- 
Yao (齐尧)

Re: [PATCH 1/2] Add constructor and destructor to thread_info

[Pedro Alves]

On 03/28/2017 04:40 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>>>>  - struct target_waitstatus pending_follow;
>>>>  + struct target_waitstatus pending_follow {TARGET_WAITKIND_SPURIOUS};
>>>>
>>>
>>> This will only initialize the first member of union
>>> target_waitstatus.value to zero, so some bits of
>>> target_waitstatus.value.related_pid is not initialized.
>>
>> Ah, blah, unions.  Well, the result is that the remaining fields of
>> the struct end up list->value->zero-initialized.  Zero-initialization
>> for unions means the first member is zero-initialized, and padding
>> is initialized to zero.  So I think the end result is the
>> same anyway.
> 
> It is different from
> "memset (&this->pending_follow, 0, sizeof (this->pending_follow))".  The
> first member "integer" is zero-initialized, but it is not the largest
> member, so part of "related_pid" is not zero-initialized.
> 
>   |<------------------ union----------------->|
>   |<------- related_pid ------->|<- padding ->|
>   |<- integer ->|
>   |      0      |<uninitialized>|     0       |
> 

I'm not so sure about that.  That is not my interpretation,
though the standard isn't crystal clear here.  The definition
of "padding" is left vague.  It doesn't look like that's the
interpretation of either G++ nor Clang though.  G++ always zeroes
the whole object, while Clang initializes only the first member,
leaving everything else uninitialized, which I found surprising,
since no padding _at all_ of any kind is initialized then.

I've sent an inquiry to std-discussion at isocpp.org list earlier
today, though I haven't gotten an answer yet.  See:

  https://groups.google.com/a/isocpp.org/forum/#!topic/std-discussion/ljaMNnkahHA

Though given Clang's behavior, it's probably not a good idea to assume
everything's zeroed out.

target_waitstatus would be a natural fit for something like
C++17's std::variant.  Particularly, the lifetime of value.execd_pathname is
not well defined...  Let's go with what you had, and revisit if/when we
change/fix target_waitstatus.

Thanks,
Pedro Alves

Re: [PATCH 1/2] Add constructor and destructor to thread_info

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

>  Let's go with what you had, and revisit if/when we
> change/fix target_waitstatus.

OK, patch below is pushed in.

-- 
Yao (齐尧)

From 1231656410996d2cc271486adc743a0fafe2ab4d Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Wed, 29 Mar 2017 16:56:31 +0100
Subject: [PATCH] Add constructor and destructor to thread_info

This patch adds constructor and destructor to thread_info.

gdb:

2017-03-29  Yao Qi  <yao.qi@linaro.org>

	* gdbthread.h (struct thread_info): Declare constructor and
	destructor.  Add some in-class member initializers.
	* thread.c (free_thread): Remove.
	(init_thread_list): Call delete instead of free_thread.
	(new_thread): Call thread_info constructor.
	(thread_info::thread_info): New function.
	(thread_info::~thread_info): New function.
	(delete_thread_1): Call delete instead of free_thread.
	(make_cleanup_restore_current_thread): Move tp and frame to
	inner block.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3520eec..23df636 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,16 @@
+2017-03-29  Yao Qi  <yao.qi@linaro.org>
+
+	* gdbthread.h (struct thread_info): Declare constructor and
+	destructor.  Add some in-class member initializers.
+	* thread.c (free_thread): Remove.
+	(init_thread_list): Call delete instead of free_thread.
+	(new_thread): Call thread_info constructor.
+	(thread_info::thread_info): New function.
+	(thread_info::~thread_info): New function.
+	(delete_thread_1): Call delete instead of free_thread.
+	(make_cleanup_restore_current_thread): Move tp and frame to
+	inner block.
+
 2017-03-28  Anton Kolesov  <anton.kolesov@synopsys.com>
 
 	* arc-tdep.c (arc_frame_cache): Add support for prologue analysis.
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 06ed78f..9a16fe6 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -179,7 +179,11 @@ typedef VEC (value_ptr) value_vec;
 
 struct thread_info
 {
-  struct thread_info *next;
+public:
+  explicit thread_info (inferior *inf, ptid_t ptid);
+  ~thread_info ();
+
+  struct thread_info *next = NULL;
   ptid_t ptid;			/* "Actual process id";
 				    In fact, this may be overloaded with 
 				    kernel thread id, etc.  */
@@ -226,13 +230,13 @@ struct thread_info
 
   /* The name of the thread, as specified by the user.  This is NULL
      if the thread does not have a user-given name.  */
-  char *name;
+  char *name = NULL;
 
   /* Non-zero means the thread is executing.  Note: this is different
      from saying that there is an active target and we are stopped at
      a breakpoint, for instance.  This is a real indicator whether the
      thread is off and running.  */
-  int executing;
+  int executing = 0;
 
   /* Non-zero if this thread is resumed from infrun's perspective.
      Note that a thread can be marked both as not-executing and
@@ -241,30 +245,30 @@ struct thread_info
      thread really run until that wait status has been processed, but
      we should not process that wait status if we didn't try to let
      the thread run.  */
-  int resumed;
+  int resumed = 0;
 
   /* Frontend view of the thread state.  Note that the THREAD_RUNNING/
      THREAD_STOPPED states are different from EXECUTING.  When the
      thread is stopped internally while handling an internal event,
      like a software single-step breakpoint, EXECUTING will be false,
      but STATE will still be THREAD_RUNNING.  */
-  enum thread_state state;
+  enum thread_state state = THREAD_STOPPED;
 
   /* If this is > 0, then it means there's code out there that relies
      on this thread being listed.  Don't delete it from the lists even
      if we detect it exiting.  */
-  int refcount;
+  int refcount = 0;
 
   /* State of GDB control of inferior thread execution.
      See `struct thread_control_state'.  */
-  struct thread_control_state control;
+  thread_control_state control {};
 
   /* State of inferior thread to restore after GDB is done with an inferior
      call.  See `struct thread_suspend_state'.  */
-  struct thread_suspend_state suspend;
+  thread_suspend_state suspend {};
 
-  int current_line;
-  struct symtab *current_symtab;
+  int current_line = 0;
+  struct symtab *current_symtab = NULL;
 
   /* Internal stepping state.  */
 
@@ -274,20 +278,20 @@ struct thread_info
      by proceed and keep_going, and among other things, it's used in
      adjust_pc_after_break to distinguish a hardware single-step
      SIGTRAP from a breakpoint SIGTRAP.  */
-  CORE_ADDR prev_pc;
+  CORE_ADDR prev_pc = 0;
 
   /* Did we set the thread stepping a breakpoint instruction?  This is
      used in conjunction with PREV_PC to decide whether to adjust the
      PC.  */
-  int stepped_breakpoint;
+  int stepped_breakpoint = 0;
 
   /* Should we step over breakpoint next time keep_going is called?  */
-  int stepping_over_breakpoint;
+  int stepping_over_breakpoint = 0;
 
   /* Should we step over a watchpoint next time keep_going is called?
      This is needed on targets with non-continuable, non-steppable
      watchpoints.  */
-  int stepping_over_watchpoint;
+  int stepping_over_watchpoint = 0;
 
   /* Set to TRUE if we should finish single-stepping over a breakpoint
      after hitting the current step-resume breakpoint.  The context here
@@ -298,12 +302,12 @@ struct thread_info
      step_after_step_resume_breakpoint is set to TRUE at this moment in
      order to keep GDB in mind that there is still a breakpoint to step over
      when GDB gets back SIGTRAP from step_resume_breakpoint.  */
-  int step_after_step_resume_breakpoint;
+  int step_after_step_resume_breakpoint = 0;
 
   /* Pointer to the state machine manager object that handles what is
      left to do for the thread's execution command after the target
      stops.  Several execution commands use it.  */
-  struct thread_fsm *thread_fsm;
+  struct thread_fsm *thread_fsm = NULL;
 
   /* This is used to remember when a fork or vfork event was caught by
      a catchpoint, and thus the event is to be followed at the next
@@ -311,37 +315,37 @@ struct thread_info
   struct target_waitstatus pending_follow;
 
   /* True if this thread has been explicitly requested to stop.  */
-  int stop_requested;
+  int stop_requested = 0;
 
   /* The initiating frame of a nexting operation, used for deciding
      which exceptions to intercept.  If it is null_frame_id no
      bp_longjmp or bp_exception but longjmp has been caught just for
      bp_longjmp_call_dummy.  */
-  struct frame_id initiating_frame;
+  struct frame_id initiating_frame = null_frame_id;
 
   /* Private data used by the target vector implementation.  */
-  struct private_thread_info *priv;
+  struct private_thread_info *priv = NULL;
 
   /* Function that is called to free PRIVATE.  If this is NULL, then
      xfree will be called on PRIVATE.  */
-  void (*private_dtor) (struct private_thread_info *);
+  void (*private_dtor) (struct private_thread_info *) = NULL;
 
   /* Branch trace information for this thread.  */
-  struct btrace_thread_info btrace;
+  struct btrace_thread_info btrace {};
 
   /* Flag which indicates that the stack temporaries should be stored while
      evaluating expressions.  */
-  int stack_temporaries_enabled;
+  int stack_temporaries_enabled = 0;
 
   /* Values that are stored as temporaries on stack while evaluating
      expressions.  */
-  value_vec *stack_temporaries;
+  value_vec *stack_temporaries = NULL;
 
   /* Step-over chain.  A thread is in the step-over queue if these are
      non-NULL.  If only a single thread is in the chain, then these
      fields point to self.  */
-  struct thread_info *step_over_prev;
-  struct thread_info *step_over_next;
+  struct thread_info *step_over_prev = NULL;
+  struct thread_info *step_over_next = NULL;
 };
 
 /* Create an empty thread list, or empty the existing one.  */
diff --git a/gdb/thread.c b/gdb/thread.c
index 99fe424..fd9022f 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -192,21 +192,6 @@ clear_thread_inferior_resources (struct thread_info *tp)
   thread_cancel_execution_command (tp);
 }
 
-static void
-free_thread (struct thread_info *tp)
-{
-  if (tp->priv)
-    {
-      if (tp->private_dtor)
-	tp->private_dtor (tp->priv);
-      else
-	xfree (tp->priv);
-    }
-
-  xfree (tp->name);
-  xfree (tp);
-}
-
 void
 init_thread_list (void)
 {
@@ -220,7 +205,7 @@ init_thread_list (void)
   for (tp = thread_list; tp; tp = tpnext)
     {
       tpnext = tp->next;
-      free_thread (tp);
+      delete tp;
     }
 
   thread_list = NULL;
@@ -233,16 +218,7 @@ init_thread_list (void)
 static struct thread_info *
 new_thread (struct inferior *inf, ptid_t ptid)
 {
-  struct thread_info *tp;
-
-  gdb_assert (inf != NULL);
-
-  tp = XCNEW (struct thread_info);
-
-  tp->ptid = ptid;
-  tp->global_num = ++highest_thread_num;
-  tp->inf = inf;
-  tp->per_inf_num = ++inf->highest_thread_num;
+  thread_info *tp = new thread_info (inf, ptid);
 
   if (thread_list == NULL)
     thread_list = tp;
@@ -255,11 +231,6 @@ new_thread (struct inferior *inf, ptid_t ptid)
       last->next = tp;
     }
 
-  /* Nothing to follow yet.  */
-  tp->pending_follow.kind = TARGET_WAITKIND_SPURIOUS;
-  tp->state = THREAD_STOPPED;
-  tp->suspend.waitstatus.kind = TARGET_WAITKIND_IGNORE;
-
   return tp;
 }
 
@@ -336,6 +307,33 @@ add_thread (ptid_t ptid)
   return add_thread_with_info (ptid, NULL);
 }
 
+thread_info::thread_info (struct inferior *inf_, ptid_t ptid_)
+  : ptid (ptid_), inf (inf_)
+{
+  gdb_assert (inf_ != NULL);
+
+  this->global_num = ++highest_thread_num;
+  this->per_inf_num = ++inf_->highest_thread_num;
+
+  /* Nothing to follow yet.  */
+  memset (&this->pending_follow, 0, sizeof (this->pending_follow));
+  this->pending_follow.kind = TARGET_WAITKIND_SPURIOUS;
+  this->suspend.waitstatus.kind = TARGET_WAITKIND_IGNORE;
+}
+
+thread_info::~thread_info ()
+{
+  if (this->priv)
+    {
+      if (this->private_dtor)
+	this->private_dtor (this->priv);
+      else
+	xfree (this->priv);
+    }
+
+  xfree (this->name);
+}
+
 /* Add TP to the end of the step-over chain LIST_P.  */
 
 static void
@@ -470,7 +468,7 @@ delete_thread_1 (ptid_t ptid, int silent)
   else
     thread_list = tp->next;
 
-  free_thread (tp);
+  delete tp;
 }
 
 /* Delete thread PTID and notify of thread exit.  If this is
@@ -1655,8 +1653,6 @@ set_thread_refcount (void *data)
 struct cleanup *
 make_cleanup_restore_current_thread (void)
 {
-  struct thread_info *tp;
-  struct frame_info *frame;
   struct current_thread_cleanup *old = XNEW (struct current_thread_cleanup);
 
   old->inferior_ptid = inferior_ptid;
@@ -1668,6 +1664,8 @@ make_cleanup_restore_current_thread (void)
 
   if (!ptid_equal (inferior_ptid, null_ptid))
     {
+      struct frame_info *frame;
+
       old->was_stopped = is_stopped (inferior_ptid);
       if (old->was_stopped
 	  && target_has_registers
@@ -1687,7 +1685,8 @@ make_cleanup_restore_current_thread (void)
       old->selected_frame_id = get_frame_id (frame);
       old->selected_frame_level = frame_relative_level (frame);
 
-      tp = find_thread_ptid (inferior_ptid);
+      struct thread_info *tp = find_thread_ptid (inferior_ptid);
+
       if (tp)
 	tp->refcount++;
     }

[PATCH 2/2] Don't delete thread_info if refcount isn't zero

[Yao Qi]

I build GDB with asan, and run test case hook-stop.exp, and threadapply.exp,
I got the following asan error,

=================================================================^M
^[[1m^[[31m==2291==ERROR: AddressSanitizer: heap-use-after-free on address 0x6160000999c4 at pc 0x000000826022 bp 0x7ffd28a8ff70 sp 0x7ffd28a8ff60^M
^[[1m^[[0m^[[1m^[[34mREAD of size 4 at 0x6160000999c4 thread T0^[[1m^[[0m^M
    #0 0x826021 in release_stop_context_cleanup ../../binutils-gdb/gdb/infrun.c:8203^M
    #1 0x72798a in do_my_cleanups ../../binutils-gdb/gdb/common/cleanups.c:154^M
    #2 0x727a32 in do_cleanups(cleanup*) ../../binutils-gdb/gdb/common/cleanups.c:176^M
    #3 0x826895 in normal_stop() ../../binutils-gdb/gdb/infrun.c:8381^M
    #4 0x815208 in fetch_inferior_event(void*) ../../binutils-gdb/gdb/infrun.c:4011^M
    #5 0x868aca in inferior_event_handler(inferior_event_type, void*) ../../binutils-gdb/gdb/inf-loop.c:44^M
....
^[[1m^[[32m0x6160000999c4 is located 68 bytes inside of 568-byte region [0x616000099980,0x616000099bb8)^M
^[[1m^[[0m^[[1m^[[35mfreed by thread T0 here:^[[1m^[[0m^M
    #0 0x7fb0bc1312ca in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x982ca)^M
    #1 0xb8c62f in xfree(void*) ../../binutils-gdb/gdb/common/common-utils.c:100^M
    #2 0x83df67 in free_thread ../../binutils-gdb/gdb/thread.c:207^M
    #3 0x83dfd2 in init_thread_list() ../../binutils-gdb/gdb/thread.c:223^M
    #4 0x805494 in kill_command ../../binutils-gdb/gdb/infcmd.c:2595^M
....

Detaching from program: /home/yao.qi/SourceCode/gnu/build-with-asan/gdb/testsuite/outputs/gdb.threads/threadapply/threadapply, process 2399^M
=================================================================^M
^[[1m^[[31m==2387==ERROR: AddressSanitizer: heap-use-after-free on address 0x6160000a98c0 at pc 0x00000083fd28 bp 0x7ffd401c3110 sp 0x7ffd401c3100^M
^[[1m^[[0m^[[1m^[[34mREAD of size 4 at 0x6160000a98c0 thread T0^[[1m^[[0m^M
    #0 0x83fd27 in thread_alive ../../binutils-gdb/gdb/thread.c:741^M
    #1 0x844277 in thread_apply_all_command ../../binutils-gdb/gdb/thread.c:1804^M
....
^M
^[[1m^[[32m0x6160000a98c0 is located 64 bytes inside of 568-byte region [0x6160000a9880,0x6160000a9ab8)^M
^[[1m^[[0m^[[1m^[[35mfreed by thread T0 here:^[[1m^[[0m^M
    #0 0x7f59a7e322ca in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x982ca)^M
    #1 0xb8c62f in xfree(void*) ../../binutils-gdb/gdb/common/common-utils.c:100^M
    #2 0x83df67 in free_thread ../../binutils-gdb/gdb/thread.c:207^M
    #3 0x83dfd2 in init_thread_list() ../../binutils-gdb/gdb/thread.c:223^M

This patch fixes the issue by deleting thread_info object if it is
deletable, otherwise, mark it as exited (by set_thread_exited).
Function set_thread_exited is shared from delete_thread_1.  This patch
also moves field "refcount" to private and methods inc_refcount and
dec_refcount.  Additionally, we stop using "ptid_t" in
"struct current_thread_cleanup" to reference threads, instead we use
"thread_info" directly.  Due to this change, we don't need
restore_current_thread_ptid_changed anymore.

gdb:

2017-04-05  Yao Qi  <yao.qi@linaro.org>

	PR gdb/19942
	* gdbthread.h (thread_info::deletable): New method.
	(thread_info::inc_refcount): New method.
	(thread_info::dec_refcount): New method.
	(thread_info::refcount): Move it to private.
	* infrun.c (save_stop_context): Call inc_refcount.
	(release_stop_context_cleanup): Likewise.
	* thread.c (set_thread_exited): New function.
	(init_thread_list): Delete "tp" only it is deletable, otherwise
	call set_thread_exited.
	(delete_thread_1): Call set_thread_exited.
	(current_thread_cleanup) <inferior_pid>: Remove.
	<thread>: New field.
	(restore_current_thread_ptid_changed): Removed.
	(do_restore_current_thread_cleanup): Adjust.
	(restore_current_thread_cleanup_dtor): Don't call
	find_thread_ptid.
	(set_thread_refcount): Use dec_refcount.
	(make_cleanup_restore_current_thread): Adjust.
	(thread_apply_all_command): Call inc_refcount.
	(_initialize_thread): Don't call
	observer_attach_thread_ptid_changed.
---
 gdb/gdbthread.h | 33 ++++++++++++++++---
 gdb/infrun.c    |  4 +--
 gdb/thread.c    | 98 +++++++++++++++++++++++++--------------------------------
 3 files changed, 73 insertions(+), 62 deletions(-)

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 9a16fe6..4838aea 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -183,6 +183,27 @@ public:
   explicit thread_info (inferior *inf, ptid_t ptid);
   ~thread_info ();
 
+  bool deletable ()
+  {
+    /* If this is the current thread, or there's code out there that
+       relies on it existing (refcount > 0) we can't delete yet.  */
+    return (refcount == 0 && !ptid_equal (ptid, inferior_ptid));
+  }
+
+  /* Increase the refcount.  */
+  void inc_refcount ()
+  {
+    gdb_assert (refcount >= 0);
+    refcount++;
+  }
+
+  /* Decrease the refcount.  */
+  void dec_refcount ()
+  {
+    refcount--;
+    gdb_assert (refcount >= 0);
+  }
+
   struct thread_info *next = NULL;
   ptid_t ptid;			/* "Actual process id";
 				    In fact, this may be overloaded with 
@@ -254,11 +275,6 @@ public:
      but STATE will still be THREAD_RUNNING.  */
   enum thread_state state = THREAD_STOPPED;
 
-  /* If this is > 0, then it means there's code out there that relies
-     on this thread being listed.  Don't delete it from the lists even
-     if we detect it exiting.  */
-  int refcount = 0;
-
   /* State of GDB control of inferior thread execution.
      See `struct thread_control_state'.  */
   thread_control_state control {};
@@ -346,6 +362,13 @@ public:
      fields point to self.  */
   struct thread_info *step_over_prev = NULL;
   struct thread_info *step_over_next = NULL;
+
+private:
+
+  /* If this is > 0, then it means there's code out there that relies
+     on this thread being listed.  Don't delete it from the lists even
+     if we detect it exiting.  */
+  int refcount = 0;
 };
 
 /* Create an empty thread list, or empty the existing one.  */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index c8c2d6e..1127884 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -8165,7 +8165,7 @@ save_stop_context (void)
       /* Take a strong reference so that the thread can't be deleted
 	 yet.  */
       sc->thread = inferior_thread ();
-      sc->thread->refcount++;
+      sc->thread->inc_refcount();
     }
   else
     sc->thread = NULL;
@@ -8182,7 +8182,7 @@ release_stop_context_cleanup (void *arg)
   struct stop_context *sc = (struct stop_context *) arg;
 
   if (sc->thread != NULL)
-    sc->thread->refcount--;
+    sc->thread->dec_refcount ();
   xfree (sc);
 }
 
diff --git a/gdb/thread.c b/gdb/thread.c
index cc3af7a..a40d015 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -192,6 +192,27 @@ clear_thread_inferior_resources (struct thread_info *tp)
   thread_cancel_execution_command (tp);
 }
 
+/* Set the TP's state as exited.  */
+
+static void
+set_thread_exited (struct thread_info *tp, int silent)
+{
+  /* Dead threads don't need to step-over.  Remove from queue.  */
+  if (tp->step_over_next != NULL)
+    thread_step_over_chain_remove (tp);
+
+  if (tp->state != THREAD_EXITED)
+    {
+      observer_notify_thread_exit (tp, silent);
+
+      /* Tag it as exited.  */
+      tp->state = THREAD_EXITED;
+
+      /* Clear breakpoints, etc. associated with this thread.  */
+      clear_thread_inferior_resources (tp);
+    }
+}
+
 void
 init_thread_list (void)
 {
@@ -205,7 +226,10 @@ init_thread_list (void)
   for (tp = thread_list; tp; tp = tpnext)
     {
       tpnext = tp->next;
-      delete tp;
+      if (tp->deletable ())
+	delete tp;
+      else
+	set_thread_exited (tp, 1);
     }
 
   thread_list = NULL;
@@ -430,25 +454,9 @@ delete_thread_1 (ptid_t ptid, int silent)
   if (!tp)
     return;
 
-  /* Dead threads don't need to step-over.  Remove from queue.  */
-  if (tp->step_over_next != NULL)
-    thread_step_over_chain_remove (tp);
+  set_thread_exited (tp, silent);
 
-  if (tp->state != THREAD_EXITED)
-    {
-      observer_notify_thread_exit (tp, silent);
-
-      /* Tag it as exited.  */
-      tp->state = THREAD_EXITED;
-
-      /* Clear breakpoints, etc. associated with this thread.  */
-      clear_thread_inferior_resources (tp);
-    }
-
-  /* If this is the current thread, or there's code out there that
-     relies on it existing (refcount > 0) we can't delete yet.  */
-  if (tp->refcount > 0
-      || ptid_equal (tp->ptid, inferior_ptid))
+  if (!tp->deletable ())
     {
        /* Will be really deleted some other time.  */
        return;
@@ -1546,7 +1554,7 @@ struct current_thread_cleanup
      'current_thread_cleanup_chain' below.  */
   struct current_thread_cleanup *next;
 
-  ptid_t inferior_ptid;
+  thread_info *thread;
   struct frame_id selected_frame_id;
   int selected_frame_level;
   int was_stopped;
@@ -1561,37 +1569,18 @@ struct current_thread_cleanup
    succeeds.  */
 static struct current_thread_cleanup *current_thread_cleanup_chain;
 
-/* A thread_ptid_changed observer.  Update all currently installed
-   current_thread_cleanup cleanups that want to switch back to
-   OLD_PTID to switch back to NEW_PTID instead.  */
-
-static void
-restore_current_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
-{
-  struct current_thread_cleanup *it;
-
-  for (it = current_thread_cleanup_chain; it != NULL; it = it->next)
-    {
-      if (ptid_equal (it->inferior_ptid, old_ptid))
-	it->inferior_ptid = new_ptid;
-    }
-}
-
 static void
 do_restore_current_thread_cleanup (void *arg)
 {
-  struct thread_info *tp;
   struct current_thread_cleanup *old = (struct current_thread_cleanup *) arg;
 
-  tp = find_thread_ptid (old->inferior_ptid);
-
-  /* If the previously selected thread belonged to a process that has
-     in the mean time been deleted (due to normal exit, detach, etc.),
-     then don't revert back to it, but instead simply drop back to no
-     thread selected.  */
-  if (tp
-      && find_inferior_ptid (tp->ptid) != NULL)
-    restore_current_thread (old->inferior_ptid);
+  /* If an entry of thread_info was previously selected, it won't be
+     deleted because we've increased its refcount.  The thread represented
+     by this thread_info entry may have already exited (due to normal exit,
+     detach, etc), so the thread_info.state is THREAD_EXITED.  */
+  if (old->thread != NULL
+      && find_inferior_ptid (old->thread->ptid) != NULL)
+    restore_current_thread (old->thread->ptid);
   else
     {
       restore_current_thread (null_ptid);
@@ -1619,9 +1608,9 @@ restore_current_thread_cleanup_dtor (void *arg)
 
   current_thread_cleanup_chain = current_thread_cleanup_chain->next;
 
-  tp = find_thread_ptid (old->inferior_ptid);
-  if (tp)
-    tp->refcount--;
+  if (old->thread != NULL)
+    old->thread->dec_refcount ();
+
   inf = find_inferior_id (old->inf_id);
   if (inf != NULL)
     inf->removable = old->was_removable;
@@ -1638,7 +1627,7 @@ set_thread_refcount (void *data)
     = (struct thread_array_cleanup *) data;
 
   for (k = 0; k != ta_cleanup->count; k++)
-    ta_cleanup->tp_array[k]->refcount--;
+    ta_cleanup->tp_array[k]->dec_refcount ();
 }
 
 struct cleanup *
@@ -1646,7 +1635,7 @@ make_cleanup_restore_current_thread (void)
 {
   struct current_thread_cleanup *old = XNEW (struct current_thread_cleanup);
 
-  old->inferior_ptid = inferior_ptid;
+  old->thread = NULL;
   old->inf_id = current_inferior ()->num;
   old->was_removable = current_inferior ()->removable;
 
@@ -1679,7 +1668,8 @@ make_cleanup_restore_current_thread (void)
       struct thread_info *tp = find_thread_ptid (inferior_ptid);
 
       if (tp)
-	tp->refcount++;
+	tp->inc_refcount ();
+      old->thread = tp;
     }
 
   current_inferior ()->removable = 0;
@@ -1796,7 +1786,7 @@ thread_apply_all_command (char *cmd, int from_tty)
       ALL_NON_EXITED_THREADS (tp)
         {
           tp_array[i] = tp;
-          tp->refcount++;
+          tp->inc_refcount ();
           i++;
         }
       /* Because we skipped exited threads, we may end up with fewer
@@ -2286,6 +2276,4 @@ Show printing of thread events (such as thread start and exit)."), NULL,
 
   create_internalvar_type_lazy ("_thread", &thread_funcs, NULL);
   create_internalvar_type_lazy ("_gthread", &gthread_funcs, NULL);
-
-  observer_attach_thread_ptid_changed (restore_current_thread_ptid_changed);
 }
-- 
1.9.1

[PATCH 0/2] Don't delete thread_info if refcount isn't zero

[Yao Qi]

After Pedro explained the refcount in thread_info, previously I completely
misunderstood it.  Here is the updated patch set.

Regression tested on x86_64-linux.

*** BLURB HERE ***

Yao Qi (2):
  Hoist code on marking thread as exited
  Don't delete thread_info if refcount isn't zero

 gdb/gdbthread.h |  33 ++++++++++++++---
 gdb/infrun.c    |   4 +--
 gdb/thread.c    | 107 +++++++++++++++++++++++---------------------------------
 3 files changed, 73 insertions(+), 71 deletions(-)

-- 
1.9.1

[PATCH 1/2] Hoist code on marking thread as exited

[Yao Qi]

This patch hoists code on marking thread as exited, so more code is shared
for two different paths (thread_info is deleted or is not deleted).

gdb:

2017-04-05  Yao Qi  <yao.qi@linaro.org>

	* thread.c (delete_thread_1): Hoist code on marking thread as
	exited.
---
 gdb/thread.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/gdb/thread.c b/gdb/thread.c
index fd9022f..cc3af7a 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -434,35 +434,26 @@ delete_thread_1 (ptid_t ptid, int silent)
   if (tp->step_over_next != NULL)
     thread_step_over_chain_remove (tp);
 
-  /* If this is the current thread, or there's code out there that
-     relies on it existing (refcount > 0) we can't delete yet.  Mark
-     it as exited, and notify it.  */
-  if (tp->refcount > 0
-      || ptid_equal (tp->ptid, inferior_ptid))
+  if (tp->state != THREAD_EXITED)
     {
-      if (tp->state != THREAD_EXITED)
-	{
-	  observer_notify_thread_exit (tp, silent);
+      observer_notify_thread_exit (tp, silent);
 
-	  /* Tag it as exited.  */
-	  tp->state = THREAD_EXITED;
+      /* Tag it as exited.  */
+      tp->state = THREAD_EXITED;
 
-	  /* Clear breakpoints, etc. associated with this thread.  */
-	  clear_thread_inferior_resources (tp);
-	}
+      /* Clear breakpoints, etc. associated with this thread.  */
+      clear_thread_inferior_resources (tp);
+    }
 
+  /* If this is the current thread, or there's code out there that
+     relies on it existing (refcount > 0) we can't delete yet.  */
+  if (tp->refcount > 0
+      || ptid_equal (tp->ptid, inferior_ptid))
+    {
        /* Will be really deleted some other time.  */
        return;
      }
 
-  /* Notify thread exit, but only if we haven't already.  */
-  if (tp->state != THREAD_EXITED)
-    observer_notify_thread_exit (tp, silent);
-
-  /* Tag it as exited.  */
-  tp->state = THREAD_EXITED;
-  clear_thread_inferior_resources (tp);
-
   if (tpprev)
     tpprev->next = tp->next;
   else
-- 
1.9.1

Re: [PATCH 1/2] Hoist code on marking thread as exited

[Pedro Alves]

On 04/05/2017 10:15 PM, Yao Qi wrote:
> This patch hoists code on marking thread as exited, so more code is shared
> for two different paths (thread_info is deleted or is not deleted).
> 
> gdb:
> 
> 2017-04-05  Yao Qi  <yao.qi@linaro.org>
> 
> 	* thread.c (delete_thread_1): Hoist code on marking thread as
> 	exited.

OK.

Thanks,
Pedro Alves

Re: [PATCH 2/2] Don't delete thread_info if refcount isn't zero

[Pedro Alves]

On 04/05/2017 10:15 PM, Yao Qi wrote:

> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -183,6 +183,27 @@ public:
>    explicit thread_info (inferior *inf, ptid_t ptid);
>    ~thread_info ();
>  
> +  bool deletable ()

Could be const:

  bool deletable () const

> +  {
> +    /* If this is the current thread, or there's code out there that
> +       relies on it existing (refcount > 0) we can't delete yet.  */
> +    return (refcount == 0 && !ptid_equal (ptid, inferior_ptid));
> +  }
> +
> +  /* Increase the refcount.  */
> +  void inc_refcount ()
> +  {
> +    gdb_assert (refcount >= 0);
> +    refcount++;
> +  }
> +
> +  /* Decrease the refcount.  */
> +  void dec_refcount ()
> +  {
> +    refcount--;
> +    gdb_assert (refcount >= 0);
> +  }

Nit: It's SO common to call this sort of methods "incref()" and
"decref()" that anything else looks a bit odd to me.

>    struct thread_info *step_over_prev = NULL;
>    struct thread_info *step_over_next = NULL;
> +
> +private:
> +
> +  /* If this is > 0, then it means there's code out there that relies
> +     on this thread being listed.  Don't delete it from the lists even
> +     if we detect it exiting.  */
> +  int refcount = 0;

Since this is now private, I think we should give it an "m_" prefix.

>  };
>  

>  
> +/* Set the TP's state as exited.  */
> +
> +static void
> +set_thread_exited (struct thread_info *tp, int silent)

Drop "struct" ?

>  static void
>  do_restore_current_thread_cleanup (void *arg)
>  {
> -  struct thread_info *tp;
>    struct current_thread_cleanup *old = (struct current_thread_cleanup *) arg;
>  
> -  tp = find_thread_ptid (old->inferior_ptid);
> -
> -  /* If the previously selected thread belonged to a process that has
> -     in the mean time been deleted (due to normal exit, detach, etc.),
> -     then don't revert back to it, but instead simply drop back to no
> -     thread selected.  */

This comment still makes sense, with a small tweak -- saying "deleted"
has always been a bit misleading here:

  /* If the previously selected thread belonged to a process that has
     in the mean time exited (or killed, detached, etc.), then don't revert
     back to it, but instead simply drop back to no thread selected.  */

I'll be happy with restoring this comment alongside your new comment.

The patch will LGTM with the nits/comments up to here addressed.

The rest of the review comments below could be addressed separately
(by anyone, not necessarily you), I'm just putting them out as
I thought of them.  I do think we should do it to avoid
hard-to-debug corner cases around find_inferior_ptid finding
an unrelated process that reused the old inferior's pid.

> -  if (tp
> -      && find_inferior_ptid (tp->ptid) != NULL)
> -    restore_current_thread (old->inferior_ptid);
> +  /* If an entry of thread_info was previously selected, it won't be
> +     deleted because we've increased its refcount.  The thread represented
> +     by this thread_info entry may have already exited (due to normal exit,
> +     detach, etc), so the thread_info.state is THREAD_EXITED.  */
> +  if (old->thread != NULL
> +      && find_inferior_ptid (old->thread->ptid) != NULL)
> +    restore_current_thread (old->thread->ptid);

Note this was a look up by inferior ptid, not by (stable) inferior number,
so we can genuinely find no inferior, even though the old inferior _object_
will always be around when we get here, given that we mark it non-removable
while the cleanup is installed [1].  Quite similar to increasing the
thread's refcount, really.

So this predicate would probably be better as:

   if (old->thread != NULL
      && old->thread != THREAD_EXITED
      && find_inferior_id (old->inf_id)->pid != 0)

We could also store the inferior pointer in "old" directly,
instead of the id, sparing the inferior look up:

   if (old->thread != NULL
      && old->thread != THREAD_EXITED
      && old->inf->pid != 0)


[1] - We should probably have a test that does something like:

define kill-and-remove
  kill inferiors 2
  remove-inferiors 2
end

# Start one inferior.
start

# Start another inferior.
add-inferior 2
inferior 2
start

# Kill and remove inferior 1 while thread 2.1 / inferior 2 is selected.
thread apply 1.1 kill-and-remove

Thanks,
Pedro Alves

Re: [PATCH 2/2] Don't delete thread_info if refcount isn't zero

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

>> -  tp = find_thread_ptid (old->inferior_ptid);
>> -
>> -  /* If the previously selected thread belonged to a process that has
>> -     in the mean time been deleted (due to normal exit, detach, etc.),
>> -     then don't revert back to it, but instead simply drop back to no
>> -     thread selected.  */
>
> This comment still makes sense, with a small tweak -- saying "deleted"
> has always been a bit misleading here:
>
>   /* If the previously selected thread belonged to a process that has
>      in the mean time exited (or killed, detached, etc.), then don't revert
>      back to it, but instead simply drop back to no thread selected.  */
>
> I'll be happy with restoring this comment alongside your new comment.

The comments describe something different from what the code does.  With
my patch, in make_cleanup_restore_current_thread, if we can find
thread_info by inferior_ptid, saved it in old->thread and increase the
refcount, so that it won't be deleted, but it can be marked as exited.
Then, in do_restore_current_thread_cleanup, 

+  if (old->thread != NULL
+      && find_inferior_ptid (old->thread->ptid) != NULL)
+    restore_current_thread (old->thread->ptid);
   else
     {
       restore_current_thread (null_ptid);

the first line means we previously selected one existing thread, we
still switch to that thread, which may be already marked as exited.  It
is nothing wrong as far as I can see.

-- 
Yao (齐尧)

Re: [PATCH 2/2] Don't delete thread_info if refcount isn't zero

[Pedro Alves]

On 04/07/2017 10:22 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>>> -  tp = find_thread_ptid (old->inferior_ptid);
>>> -
>>> -  /* If the previously selected thread belonged to a process that has
>>> -     in the mean time been deleted (due to normal exit, detach, etc.),
>>> -     then don't revert back to it, but instead simply drop back to no
>>> -     thread selected.  */
>>
>> This comment still makes sense, with a small tweak -- saying "deleted"
>> has always been a bit misleading here:
>>
>>   /* If the previously selected thread belonged to a process that has
>>      in the mean time exited (or killed, detached, etc.), then don't revert
>>      back to it, but instead simply drop back to no thread selected.  */
>>
>> I'll be happy with restoring this comment alongside your new comment.
> 
> The comments describe something different from what the code does.  

It doesn't.  See below.

> With
> my patch, in make_cleanup_restore_current_thread, if we can find
> thread_info by inferior_ptid, saved it in old->thread and increase the
> refcount, so that it won't be deleted, but it can be marked as exited.
> Then, in do_restore_current_thread_cleanup, 
> 
> +  if (old->thread != NULL
> +      && find_inferior_ptid (old->thread->ptid) != NULL)
> +    restore_current_thread (old->thread->ptid);
>    else
>      {
>        restore_current_thread (null_ptid);
> 
> the first line means we previously selected one existing thread, we
> still switch to that thread, which may be already marked as exited.  It
> is nothing wrong as far as I can see.

I'm not sure what you mean by "wrong".  I'm saying that the old comment
still makes sense.  That comment is talking about the 
"find_inferior_ptid (old->thread->ptid) != NULL" line, which is a 
lookup for an _inferior_ not a thread.  Both the inferior and thread_info
object are still around, but the process may have exited/been detached
meanwhile, and consequently the inferior's "pid" field is now
zero.  And in that case, we don't restore back the selected thread.

E.g., with:

 start
 thread apply all detach

#1 - the current thread (thread 1), has its refcount incremented.
#2 - detach gets rid of all threads of the process.  The "thread 1"
     object is left around, marked exited, because it was not
     deletable.
#3 - when we try to restore the previous thread, its
     _inferior_ has meanwhile gotten its "pid" field zeroed,
     so a look up for an inferior with the pid of the previous
     thread's "ptid" (which just looks at the pid) won't
     find any inferior.  (Unless it finds the wrong inferior
     that happened to reuse the pid meanwhile.)

Hopefully that makes the rest of my comments in my previous
message clearer.

Thanks,
Pedro Alves

Re: [PATCH 2/2] Don't delete thread_info if refcount isn't zero

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> I'm not sure what you mean by "wrong".  I'm saying that the old comment
> still makes sense.  That comment is talking about the 
> "find_inferior_ptid (old->thread->ptid) != NULL" line, which is a 
> lookup for an _inferior_ not a thread.  Both the inferior and thread_info

I thought it is about a thread.

> object are still around, but the process may have exited/been detached
> meanwhile, and consequently the inferior's "pid" field is now
> zero.  And in that case, we don't restore back the selected thread.

I move the comment close to "find_inferior_ptid (old->thread->ptid) != NULL"
line, to make it clearer.  Patch below is pushed in.

-- 
Yao (齐尧)
From 2e9d6d568506454eb8e5241439f91c05a99b4716 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Sat, 4 Mar 2017 22:31:40 +0000
Subject: [PATCH] Don't delete thread_info if refcount isn't zero

I build GDB with asan, and run test case hook-stop.exp, and threadapply.exp,
I got the following asan error,

=================================================================^M
^[[1m^[[31m==2291==ERROR: AddressSanitizer: heap-use-after-free on address 0x6160000999c4 at pc 0x000000826022 bp 0x7ffd28a8ff70 sp 0x7ffd28a8ff60^M
^[[1m^[[0m^[[1m^[[34mREAD of size 4 at 0x6160000999c4 thread T0^[[1m^[[0m^M
    #0 0x826021 in release_stop_context_cleanup ../../binutils-gdb/gdb/infrun.c:8203^M
    #1 0x72798a in do_my_cleanups ../../binutils-gdb/gdb/common/cleanups.c:154^M
    #2 0x727a32 in do_cleanups(cleanup*) ../../binutils-gdb/gdb/common/cleanups.c:176^M
    #3 0x826895 in normal_stop() ../../binutils-gdb/gdb/infrun.c:8381^M
    #4 0x815208 in fetch_inferior_event(void*) ../../binutils-gdb/gdb/infrun.c:4011^M
    #5 0x868aca in inferior_event_handler(inferior_event_type, void*) ../../binutils-gdb/gdb/inf-loop.c:44^M
....
^[[1m^[[32m0x6160000999c4 is located 68 bytes inside of 568-byte region [0x616000099980,0x616000099bb8)^M
^[[1m^[[0m^[[1m^[[35mfreed by thread T0 here:^[[1m^[[0m^M
    #0 0x7fb0bc1312ca in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x982ca)^M
    #1 0xb8c62f in xfree(void*) ../../binutils-gdb/gdb/common/common-utils.c:100^M
    #2 0x83df67 in free_thread ../../binutils-gdb/gdb/thread.c:207^M
    #3 0x83dfd2 in init_thread_list() ../../binutils-gdb/gdb/thread.c:223^M
    #4 0x805494 in kill_command ../../binutils-gdb/gdb/infcmd.c:2595^M
....

Detaching from program: /home/yao.qi/SourceCode/gnu/build-with-asan/gdb/testsuite/outputs/gdb.threads/threadapply/threadapply, process 2399^M
=================================================================^M
^[[1m^[[31m==2387==ERROR: AddressSanitizer: heap-use-after-free on address 0x6160000a98c0 at pc 0x00000083fd28 bp 0x7ffd401c3110 sp 0x7ffd401c3100^M
^[[1m^[[0m^[[1m^[[34mREAD of size 4 at 0x6160000a98c0 thread T0^[[1m^[[0m^M
    #0 0x83fd27 in thread_alive ../../binutils-gdb/gdb/thread.c:741^M
    #1 0x844277 in thread_apply_all_command ../../binutils-gdb/gdb/thread.c:1804^M
....
^M
^[[1m^[[32m0x6160000a98c0 is located 64 bytes inside of 568-byte region [0x6160000a9880,0x6160000a9ab8)^M
^[[1m^[[0m^[[1m^[[35mfreed by thread T0 here:^[[1m^[[0m^M
    #0 0x7f59a7e322ca in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x982ca)^M
    #1 0xb8c62f in xfree(void*) ../../binutils-gdb/gdb/common/common-utils.c:100^M
    #2 0x83df67 in free_thread ../../binutils-gdb/gdb/thread.c:207^M
    #3 0x83dfd2 in init_thread_list() ../../binutils-gdb/gdb/thread.c:223^M

This patch fixes the issue by deleting thread_info object if it is
deletable, otherwise, mark it as exited (by set_thread_exited).
Function set_thread_exited is shared from delete_thread_1.  This patch
also moves field "refcount" to private and methods incref and
decref.  Additionally, we stop using "ptid_t" in
"struct current_thread_cleanup" to reference threads, instead we use
"thread_info" directly.  Due to this change, we don't need
restore_current_thread_ptid_changed anymore.

gdb:

2017-04-10  Yao Qi  <yao.qi@linaro.org>

	PR gdb/19942
	* gdbthread.h (thread_info::deletable): New method.
	(thread_info::incref): New method.
	(thread_info::decref): New method.
	(thread_info::refcount): Move it to private.
	* infrun.c (save_stop_context): Call inc_refcount.
	(release_stop_context_cleanup): Likewise.
	* thread.c (set_thread_exited): New function.
	(init_thread_list): Delete "tp" only it is deletable, otherwise
	call set_thread_exited.
	(delete_thread_1): Call set_thread_exited.
	(current_thread_cleanup) <inferior_pid>: Remove.
	<thread>: New field.
	(restore_current_thread_ptid_changed): Removed.
	(do_restore_current_thread_cleanup): Adjust.
	(restore_current_thread_cleanup_dtor): Don't call
	find_thread_ptid.
	(set_thread_refcount): Use dec_refcount.
	(make_cleanup_restore_current_thread): Adjust.
	(thread_apply_all_command): Call inc_refcount.
	(_initialize_thread): Don't call
	observer_attach_thread_ptid_changed.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index a1f8b49..68d8f34 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,30 @@
 2017-04-10  Yao Qi  <yao.qi@linaro.org>
 
+	PR gdb/19942
+	* gdbthread.h (thread_info::deletable): New method.
+	(thread_info::incref): New method.
+	(thread_info::decref): New method.
+	(thread_info::refcount): Move it to private.
+	* infrun.c (save_stop_context): Call inc_refcount.
+	(release_stop_context_cleanup): Likewise.
+	* thread.c (set_thread_exited): New function.
+	(init_thread_list): Delete "tp" only it is deletable, otherwise
+	call set_thread_exited.
+	(delete_thread_1): Call set_thread_exited.
+	(current_thread_cleanup) <inferior_pid>: Remove.
+	<thread>: New field.
+	(restore_current_thread_ptid_changed): Removed.
+	(do_restore_current_thread_cleanup): Adjust.
+	(restore_current_thread_cleanup_dtor): Don't call
+	find_thread_ptid.
+	(set_thread_refcount): Use dec_refcount.
+	(make_cleanup_restore_current_thread): Adjust.
+	(thread_apply_all_command): Call inc_refcount.
+	(_initialize_thread): Don't call
+	observer_attach_thread_ptid_changed.
+
+2017-04-10  Yao Qi  <yao.qi@linaro.org>
+
 	* thread.c (delete_thread_1): Hoist code on marking thread as
 	exited.
 
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 9a16fe6..4cd7390 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -183,6 +183,27 @@ public:
   explicit thread_info (inferior *inf, ptid_t ptid);
   ~thread_info ();
 
+  bool deletable () const
+  {
+    /* If this is the current thread, or there's code out there that
+       relies on it existing (m_refcount > 0) we can't delete yet.  */
+    return (m_refcount == 0 && !ptid_equal (ptid, inferior_ptid));
+  }
+
+  /* Increase the refcount.  */
+  void incref ()
+  {
+    gdb_assert (m_refcount >= 0);
+    m_refcount++;
+  }
+
+  /* Decrease the refcount.  */
+  void decref ()
+  {
+    m_refcount--;
+    gdb_assert (m_refcount >= 0);
+  }
+
   struct thread_info *next = NULL;
   ptid_t ptid;			/* "Actual process id";
 				    In fact, this may be overloaded with 
@@ -254,11 +275,6 @@ public:
      but STATE will still be THREAD_RUNNING.  */
   enum thread_state state = THREAD_STOPPED;
 
-  /* If this is > 0, then it means there's code out there that relies
-     on this thread being listed.  Don't delete it from the lists even
-     if we detect it exiting.  */
-  int refcount = 0;
-
   /* State of GDB control of inferior thread execution.
      See `struct thread_control_state'.  */
   thread_control_state control {};
@@ -346,6 +362,13 @@ public:
      fields point to self.  */
   struct thread_info *step_over_prev = NULL;
   struct thread_info *step_over_next = NULL;
+
+private:
+
+  /* If this is > 0, then it means there's code out there that relies
+     on this thread being listed.  Don't delete it from the lists even
+     if we detect it exiting.  */
+  int m_refcount = 0;
 };
 
 /* Create an empty thread list, or empty the existing one.  */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index c8c2d6e..b5eb4ab 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -8165,7 +8165,7 @@ save_stop_context (void)
       /* Take a strong reference so that the thread can't be deleted
 	 yet.  */
       sc->thread = inferior_thread ();
-      sc->thread->refcount++;
+      sc->thread->incref ();
     }
   else
     sc->thread = NULL;
@@ -8182,7 +8182,7 @@ release_stop_context_cleanup (void *arg)
   struct stop_context *sc = (struct stop_context *) arg;
 
   if (sc->thread != NULL)
-    sc->thread->refcount--;
+    sc->thread->decref ();
   xfree (sc);
 }
 
diff --git a/gdb/thread.c b/gdb/thread.c
index 2e9da53..2d936cd 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -192,6 +192,27 @@ clear_thread_inferior_resources (struct thread_info *tp)
   thread_cancel_execution_command (tp);
 }
 
+/* Set the TP's state as exited.  */
+
+static void
+set_thread_exited (thread_info *tp, int silent)
+{
+  /* Dead threads don't need to step-over.  Remove from queue.  */
+  if (tp->step_over_next != NULL)
+    thread_step_over_chain_remove (tp);
+
+  if (tp->state != THREAD_EXITED)
+    {
+      observer_notify_thread_exit (tp, silent);
+
+      /* Tag it as exited.  */
+      tp->state = THREAD_EXITED;
+
+      /* Clear breakpoints, etc. associated with this thread.  */
+      clear_thread_inferior_resources (tp);
+    }
+}
+
 void
 init_thread_list (void)
 {
@@ -205,7 +226,10 @@ init_thread_list (void)
   for (tp = thread_list; tp; tp = tpnext)
     {
       tpnext = tp->next;
-      delete tp;
+      if (tp->deletable ())
+	delete tp;
+      else
+	set_thread_exited (tp, 1);
     }
 
   thread_list = NULL;
@@ -430,25 +454,9 @@ delete_thread_1 (ptid_t ptid, int silent)
   if (!tp)
     return;
 
-  /* Dead threads don't need to step-over.  Remove from queue.  */
-  if (tp->step_over_next != NULL)
-    thread_step_over_chain_remove (tp);
+  set_thread_exited (tp, silent);
 
-  if (tp->state != THREAD_EXITED)
-    {
-      observer_notify_thread_exit (tp, silent);
-
-      /* Tag it as exited.  */
-      tp->state = THREAD_EXITED;
-
-      /* Clear breakpoints, etc. associated with this thread.  */
-      clear_thread_inferior_resources (tp);
-    }
-
-  /* If this is the current thread, or there's code out there that
-     relies on it existing (refcount > 0) we can't delete yet.  */
-  if (tp->refcount > 0
-      || ptid_equal (tp->ptid, inferior_ptid))
+  if (!tp->deletable ())
     {
        /* Will be really deleted some other time.  */
        return;
@@ -1546,7 +1554,7 @@ struct current_thread_cleanup
      'current_thread_cleanup_chain' below.  */
   struct current_thread_cleanup *next;
 
-  ptid_t inferior_ptid;
+  thread_info *thread;
   struct frame_id selected_frame_id;
   int selected_frame_level;
   int was_stopped;
@@ -1561,37 +1569,21 @@ struct current_thread_cleanup
    succeeds.  */
 static struct current_thread_cleanup *current_thread_cleanup_chain;
 
-/* A thread_ptid_changed observer.  Update all currently installed
-   current_thread_cleanup cleanups that want to switch back to
-   OLD_PTID to switch back to NEW_PTID instead.  */
-
-static void
-restore_current_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
-{
-  struct current_thread_cleanup *it;
-
-  for (it = current_thread_cleanup_chain; it != NULL; it = it->next)
-    {
-      if (ptid_equal (it->inferior_ptid, old_ptid))
-	it->inferior_ptid = new_ptid;
-    }
-}
-
 static void
 do_restore_current_thread_cleanup (void *arg)
 {
-  struct thread_info *tp;
   struct current_thread_cleanup *old = (struct current_thread_cleanup *) arg;
 
-  tp = find_thread_ptid (old->inferior_ptid);
-
-  /* If the previously selected thread belonged to a process that has
-     in the mean time been deleted (due to normal exit, detach, etc.),
-     then don't revert back to it, but instead simply drop back to no
-     thread selected.  */
-  if (tp
-      && find_inferior_ptid (tp->ptid) != NULL)
-    restore_current_thread (old->inferior_ptid);
+  /* If an entry of thread_info was previously selected, it won't be
+     deleted because we've increased its refcount.  The thread represented
+     by this thread_info entry may have already exited (due to normal exit,
+     detach, etc), so the thread_info.state is THREAD_EXITED.  */
+  if (old->thread != NULL
+      /* If the previously selected thread belonged to a process that has
+	 in the mean time exited (or killed, detached, etc.), then don't revert
+	 back to it, but instead simply drop back to no thread selected.  */
+      && find_inferior_ptid (old->thread->ptid) != NULL)
+    restore_current_thread (old->thread->ptid);
   else
     {
       restore_current_thread (null_ptid);
@@ -1619,9 +1611,9 @@ restore_current_thread_cleanup_dtor (void *arg)
 
   current_thread_cleanup_chain = current_thread_cleanup_chain->next;
 
-  tp = find_thread_ptid (old->inferior_ptid);
-  if (tp)
-    tp->refcount--;
+  if (old->thread != NULL)
+    old->thread->decref ();
+
   inf = find_inferior_id (old->inf_id);
   if (inf != NULL)
     inf->removable = old->was_removable;
@@ -1638,7 +1630,7 @@ set_thread_refcount (void *data)
     = (struct thread_array_cleanup *) data;
 
   for (k = 0; k != ta_cleanup->count; k++)
-    ta_cleanup->tp_array[k]->refcount--;
+    ta_cleanup->tp_array[k]->decref ();
 }
 
 struct cleanup *
@@ -1646,7 +1638,7 @@ make_cleanup_restore_current_thread (void)
 {
   struct current_thread_cleanup *old = XNEW (struct current_thread_cleanup);
 
-  old->inferior_ptid = inferior_ptid;
+  old->thread = NULL;
   old->inf_id = current_inferior ()->num;
   old->was_removable = current_inferior ()->removable;
 
@@ -1679,7 +1671,8 @@ make_cleanup_restore_current_thread (void)
       struct thread_info *tp = find_thread_ptid (inferior_ptid);
 
       if (tp)
-	tp->refcount++;
+	tp->incref ();
+      old->thread = tp;
     }
 
   current_inferior ()->removable = 0;
@@ -1796,7 +1789,7 @@ thread_apply_all_command (char *cmd, int from_tty)
       ALL_NON_EXITED_THREADS (tp)
         {
           tp_array[i] = tp;
-          tp->refcount++;
+          tp->incref ();
           i++;
         }
       /* Because we skipped exited threads, we may end up with fewer
@@ -2286,6 +2279,4 @@ Show printing of thread events (such as thread start and exit)."), NULL,
 
   create_internalvar_type_lazy ("_thread", &thread_funcs, NULL);
   create_internalvar_type_lazy ("_gthread", &gthread_funcs, NULL);
-
-  observer_attach_thread_ptid_changed (restore_current_thread_ptid_changed);
 }

[pushed] GC gdb/thread.c:current_thread_cleanup_chain (Re: [PATCH 2/2] Don't delete thread_info if refcount isn't zero)

[Pedro Alves]

On 04/10/2017 02:40 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> I'm not sure what you mean by "wrong".  I'm saying that the old comment
>> still makes sense.  That comment is talking about the 
>> "find_inferior_ptid (old->thread->ptid) != NULL" line, which is a 
>> lookup for an _inferior_ not a thread.  Both the inferior and thread_info
> 
> I thought it is about a thread.
> 
>> object are still around, but the process may have exited/been detached
>> meanwhile, and consequently the inferior's "pid" field is now
>> zero.  And in that case, we don't restore back the selected thread.
> 
> I move the comment close to "find_inferior_ptid (old->thread->ptid) != NULL"
> line, to make it clearer.  Patch below is pushed in.

Thanks!

I noticed that ...

> -/* A thread_ptid_changed observer.  Update all currently installed
> -   current_thread_cleanup cleanups that want to switch back to
> -   OLD_PTID to switch back to NEW_PTID instead.  */
> -
> -static void
> -restore_current_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
> -{
> -  struct current_thread_cleanup *it;
> -
> -  for (it = current_thread_cleanup_chain; it != NULL; it = it->next)
> -    {
> -      if (ptid_equal (it->inferior_ptid, old_ptid))
> -	it->inferior_ptid = new_ptid;
> -    }
> -}


... this means current_thread_cleanup_chain is no longer useful.

So I pushed in the patch below.

From 996812e3d43f78b17b6454d2948cd825ec98c63b Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 10 Apr 2017 15:18:49 +0100
Subject: [PATCH] GC gdb/thread.c:current_thread_cleanup_chain

Commit 803bdfe43083475c7df3db38dc96f4e20d05457d ("Don't delete
thread_info if refcount isn't zero") eliminated
restore_current_thread_ptid_changed, so current_thread_cleanup_chain
is no longer necessary either.

gdb/ChangeLog:
2017-04-10  Pedro Alves  <palves@redhat.com>

	* thread.c (struct current_thread_cleanup) <next>: Delete field.
	(current_thread_cleanup_chain): Delete.
	(restore_current_thread_cleanup_dtor)
	(make_cleanup_restore_current_thread): Remove references to
	current_thread_cleanup_chain.
---
 gdb/ChangeLog |  8 ++++++++
 gdb/thread.c  | 17 -----------------
 2 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3cb6cd7..5e7736e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2017-04-10  Pedro Alves  <palves@redhat.com>
+
+	* thread.c (struct current_thread_cleanup) <next>: Delete field.
+	(current_thread_cleanup_chain): Delete.
+	(restore_current_thread_cleanup_dtor)
+	(make_cleanup_restore_current_thread): Remove references to
+	current_thread_cleanup_chain.
+
 2017-04-10  Alan Hayward  <alan.hayward@arm.com>
 
 	* msp430-tdep.c (msp430_pseudo_register_read): Never return
diff --git a/gdb/thread.c b/gdb/thread.c
index 2d936cd..3aca307 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1549,11 +1549,6 @@ restore_selected_frame (struct frame_id a_frame_id, int frame_level)
 
 struct current_thread_cleanup
 {
-  /* Next in list of currently installed 'struct
-     current_thread_cleanup' cleanups.  See
-     'current_thread_cleanup_chain' below.  */
-  struct current_thread_cleanup *next;
-
   thread_info *thread;
   struct frame_id selected_frame_id;
   int selected_frame_level;
@@ -1562,13 +1557,6 @@ struct current_thread_cleanup
   int was_removable;
 };
 
-/* A chain of currently installed 'struct current_thread_cleanup'
-   cleanups.  Restoring the previously selected thread looks up the
-   old thread in the thread list by ptid.  If the thread changes ptid,
-   we need to update the cleanup's thread structure so the look up
-   succeeds.  */
-static struct current_thread_cleanup *current_thread_cleanup_chain;
-
 static void
 do_restore_current_thread_cleanup (void *arg)
 {
@@ -1609,8 +1597,6 @@ restore_current_thread_cleanup_dtor (void *arg)
   struct thread_info *tp;
   struct inferior *inf;
 
-  current_thread_cleanup_chain = current_thread_cleanup_chain->next;
-
   if (old->thread != NULL)
     old->thread->decref ();
 
@@ -1642,9 +1628,6 @@ make_cleanup_restore_current_thread (void)
   old->inf_id = current_inferior ()->num;
   old->was_removable = current_inferior ()->removable;
 
-  old->next = current_thread_cleanup_chain;
-  current_thread_cleanup_chain = old;
-
   if (!ptid_equal (inferior_ptid, null_ptid))
     {
       struct frame_info *frame;
-- 
2.5.5

Re: [PATCH 2/2] Don't delete thread_info if refcount isn't zero

[Pedro Alves]

I'm taking a look at adding the test mentioned below, and managed to
trigger an internal error:

src/gdb/inferior.c:66: internal-error: void set_current_inferior(inferior*): Assertion `inf != NULL' failed.
A problem internal to GDB has been detected,KFAIL: gdb.threads/threadapply.exp: kill_and_remove: try kill-and-remove: thread apply 1.1 kill-and-remove (GDB internal error) 

I'll poke a bit more at it.

Thanks,
Pedro Alves

On 04/06/2017 11:18 AM, Pedro Alves wrote:

> The rest of the review comments below could be addressed separately
> (by anyone, not necessarily you), I'm just putting them out as
> I thought of them.  I do think we should do it to avoid
> hard-to-debug corner cases around find_inferior_ptid finding
> an unrelated process that reused the old inferior's pid.
> 
>> -  if (tp
>> -      && find_inferior_ptid (tp->ptid) != NULL)
>> -    restore_current_thread (old->inferior_ptid);
>> +  /* If an entry of thread_info was previously selected, it won't be
>> +     deleted because we've increased its refcount.  The thread represented
>> +     by this thread_info entry may have already exited (due to normal exit,
>> +     detach, etc), so the thread_info.state is THREAD_EXITED.  */
>> +  if (old->thread != NULL
>> +      && find_inferior_ptid (old->thread->ptid) != NULL)
>> +    restore_current_thread (old->thread->ptid);
> 
> Note this was a look up by inferior ptid, not by (stable) inferior number,
> so we can genuinely find no inferior, even though the old inferior _object_
> will always be around when we get here, given that we mark it non-removable
> while the cleanup is installed [1].  Quite similar to increasing the
> thread's refcount, really.
> 
> So this predicate would probably be better as:
> 
>    if (old->thread != NULL
>       && old->thread != THREAD_EXITED
>       && find_inferior_id (old->inf_id)->pid != 0)
> 
> We could also store the inferior pointer in "old" directly,
> instead of the id, sparing the inferior look up:
> 
>    if (old->thread != NULL
>       && old->thread != THREAD_EXITED
>       && old->inf->pid != 0)
> 
> 
> [1] - We should probably have a test that does something like:
> 
> define kill-and-remove
>   kill inferiors 2
>   remove-inferiors 2
> end
> 
> # Start one inferior.
> start
> 
> # Start another inferior.
> add-inferior 2
> inferior 2
> start
> 
> # Kill and remove inferior 1 while thread 2.1 / inferior 2 is selected.
> thread apply 1.1 kill-and-remove