[PATCH v3] [RFC] Fix AIX thread exit events not being reported and UI to show kernel thread ID.

[PATCH v3] [RFC] Fix AIX thread exit events not being reported and UI to show kernel thread ID.

[Aditya Vidyadhar Kamath]

From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>

In AIX when a thread exits we were not showing that a thread exit event happened
and GDB continued to keep the terminated threads.

If we have terminated threads then the UI on info threads command will look like
(gdb) info threads
  Id   Target Id                                          Frame
* 1    Thread 1 (tid 26607979, running)    0xd0611d70 in _p_nsleep () from /usr/lib/libpthreads.a(_shr_xpg5.o)
  2    Thread 258 (tid 30998799, finished) aix-thread: ptrace (52, 30998799) returned -1 (errno = 3 The process does not exist.)

If we see the frame is not getting displayed correctly.

The reason for the same is that in AIX we were not managing thread states. In particular we do not know
when a thread terminates.

The reason being in sync_threadlists () the pbuf and gbuf lists remain the same though certain threads exit.

This patch is a fix to the same.

Also certain UI is changed.

On a new thread born and exit the UI in AIX will be similar to Linux with both user and kernel thread information.

[New Thread 258 (tid 32178533)]
[New Thread 515 (tid 30343651)]
[New Thread 772 (tid 33554909)]
[New Thread 1029 (tid 24969489)]
[New Thread 1286 (tid 18153945)]
[New Thread 1543 (tid 30736739)]
[Thread 258 (tid 32178533) exited]
[Thread 515 (tid 30343651) exited]
[Thread 772 (tid 33554909) exited]
[Thread 1029 (tid 24969489) exited]
[Thread 1286 (tid 18153945) exited]
[Thread 1543 (tid 30736739) exited]

and info threads will look like
(gdb) info threads
  Id   Target Id                           Frame
* 1    Thread 1 (tid 31326579) ([running]) 0xd0611d70 in _p_nsleep () from /usr/lib/libpthread.a(_shr_xpg5.o)
---
 gdb/aix-thread.c | 248 ++++++++++++++++-------------------------------
 1 file changed, 81 insertions(+), 167 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index c70bd82bc24..bd74c0c805b 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -55,6 +55,7 @@
 #include <sys/reg.h>
 #include <sched.h>
 #include <sys/pthdebug.h>
+#include <unordered_set>
 
 #if !HAVE_DECL_GETTHRDS
 extern int getthrds (pid_t, struct thrdsinfo64 *, int, tid_t *, int);
@@ -190,6 +191,12 @@ struct aix_thread_variables
   /* Whether the current architecture is 64-bit.
    Only valid when pd_able is true.  */
   int arch64;
+
+  /* Describes the number of thread exit events reported.  */
+  std::unordered_set<pthdb_pthread_t> exited_threads;
+
+  /* Describes if thread is still in queue and not in unknown state.  */
+  std::vector<pthdb_pthread_t> in_queue_threads;
 };
 
 /* Key to our per-inferior data.  */
@@ -737,47 +744,6 @@ state2str (pthdb_state_t state)
     }
 }
 
-/* qsort() comparison function for sorting pd_thread structs by pthid.  */
-
-static int
-pcmp (const void *p1v, const void *p2v)
-{
-  struct pd_thread *p1 = (struct pd_thread *) p1v;
-  struct pd_thread *p2 = (struct pd_thread *) p2v;
-  return p1->pthid < p2->pthid ? -1 : p1->pthid > p2->pthid;
-}
-
-/* ptid comparison function */
-
-static int
-ptid_cmp (ptid_t ptid1, ptid_t ptid2)
-{
-  if (ptid1.pid () < ptid2.pid ())
-    return -1;
-  else if (ptid1.pid () > ptid2.pid ())
-    return 1;
-  else if (ptid1.tid () < ptid2.tid ())
-    return -1;
-  else if (ptid1.tid () > ptid2.tid ())
-    return 1;
-  else if (ptid1.lwp () < ptid2.lwp ())
-    return -1;
-  else if (ptid1.lwp () > ptid2.lwp ())
-    return 1;
-  else
-    return 0;
-}
-
-/* qsort() comparison function for sorting thread_info structs by pid.  */
-
-static int
-gcmp (const void *t1v, const void *t2v)
-{
-  struct thread_info *t1 = *(struct thread_info **) t1v;
-  struct thread_info *t2 = *(struct thread_info **) t2v;
-  return ptid_cmp (t1->ptid, t2->ptid);
-}
-
 /* Search through the list of all kernel threads for the thread
    that has stopped on a SIGTRAP signal, and return its TID.
    Return 0 if none found.  */
@@ -822,22 +788,16 @@ static void
 sync_threadlists (pid_t pid)
 {
   int cmd, status;
-  int pcount, psize, pi, gcount, gi;
-  struct pd_thread *pbuf;
-  struct thread_info **gbuf, **g, *thread;
   pthdb_pthread_t pdtid;
   pthread_t pthid;
   pthdb_tid_t tid;
   process_stratum_target *proc_target = current_inferior ()->process_target ();
   struct aix_thread_variables *data;
   data = get_thread_data_helper_for_pid (pid);
-
+  pthdb_state_t state;
+  data->in_queue_threads.clear ();
   /* Accumulate an array of libpthdebug threads sorted by pthread id.  */
 
-  pcount = 0;
-  psize = 1;
-  pbuf = XNEWVEC (struct pd_thread, psize);
-
   for (cmd = PTHDB_LIST_FIRST;; cmd = PTHDB_LIST_NEXT)
     {
       status = pthdb_pthread (data->pd_session, &pdtid, cmd);
@@ -848,118 +808,68 @@ sync_threadlists (pid_t pid)
       if (status != PTHDB_SUCCESS || pthid == PTHDB_INVALID_PTID)
 	continue;
 
-      if (pcount == psize)
-	{
-	  psize *= 2;
-	  pbuf = (struct pd_thread *) xrealloc (pbuf, 
-						psize * sizeof *pbuf);
-	}
-      pbuf[pcount].pdtid = pdtid;
-      pbuf[pcount].pthid = pthid;
-      pcount++;
-    }
-
-  for (pi = 0; pi < pcount; pi++)
-    {
-      status = pthdb_pthread_tid (data->pd_session, pbuf[pi].pdtid, &tid);
-      if (status != PTHDB_SUCCESS)
-	tid = PTHDB_INVALID_TID;
-      pbuf[pi].tid = tid;
-    }
-
-  qsort (pbuf, pcount, sizeof *pbuf, pcmp);
-
-  /* Accumulate an array of GDB threads sorted by pid.  */
-
-  /* gcount is GDB thread count and pcount is pthreadlib thread count.  */
+      ptid_t ptid (pid, 0, pthid);
+      status = pthdb_pthread_state (data->pd_session, pdtid, &state);
+      data->in_queue_threads.push_back (pdtid);
 
-  gcount = 0;
-  for (thread_info *tp : all_threads (proc_target, ptid_t (pid)))
-    gcount++;
-  g = gbuf = XNEWVEC (struct thread_info *, gcount);
-  for (thread_info *tp : all_threads (proc_target, ptid_t (pid)))
-    *g++ = tp;
-  qsort (gbuf, gcount, sizeof *gbuf, gcmp);
-
-  /* Apply differences between the two arrays to GDB's thread list.  */
-
-  for (pi = gi = 0; pi < pcount || gi < gcount;)
-    {
-      if (pi == pcount)
+      /* If this thread has reported and exited, do not add it again.  */
+      if (state == PST_TERM)
 	{
-	  delete_thread (gbuf[gi]);
-	  gi++;
+	  if (data->exited_threads.count (pdtid) != 0)
+	     continue;
 	}
-      else if (gi == gcount)
-	{
-	  aix_thread_info *priv = new aix_thread_info;
-	  priv->pdtid = pbuf[pi].pdtid;
-	  priv->tid = pbuf[pi].tid;
-
-	  thread = add_thread_with_info (proc_target,
-					 ptid_t (pid, 0, pbuf[pi].pthid),
-					 private_thread_info_up (priv));
 
-	  pi++;
-	}
-      else
+      /* If this thread has never been reported to GDB, add it.  */
+      if (!in_thread_list (proc_target, ptid))
 	{
-	  ptid_t pptid, gptid;
-	  int cmp_result;
-
-	  pptid = ptid_t (pid, 0, pbuf[pi].pthid);
-	  gptid = gbuf[gi]->ptid;
-	  pdtid = pbuf[pi].pdtid;
-	  tid = pbuf[pi].tid;
-
-	  cmp_result = ptid_cmp (pptid, gptid);
-
-	  if (cmp_result == 0)
-	    {
-	      aix_thread_info *priv = get_aix_thread_info (gbuf[gi]);
-
-	      priv->pdtid = pdtid;
-	      priv->tid = tid;
-	      pi++;
-	      gi++;
-	    }
-	  else if (cmp_result > 0)
+	  aix_thread_info *priv = new aix_thread_info;
+	  /* init priv */
+	  priv->pdtid = pdtid;
+	  status = pthdb_pthread_tid (data->pd_session, pdtid, &tid);
+	  priv->tid = tid;
+	  /* Check if this is the main thread.  If it is, then change
+	     its ptid and add its private data.  */
+	  if (get_signaled_thread (pid) == tid
+		&& in_thread_list (proc_target, ptid_t (pid)))
 	    {
-	      /* This is to make the main process thread now look
-		 like a thread.  */
-
-	      if (gptid.is_pid ())
-		{
-		  thread_info *tp = proc_target->find_thread (gptid);
-		  thread_change_ptid (proc_target, gptid, pptid);
-		  aix_thread_info *priv = new aix_thread_info;
-		  priv->pdtid = pbuf[pi].pdtid;
-		  priv->tid = pbuf[pi].tid;
-		  tp->priv.reset (priv);
-		  gi++;
-		  pi++;
-		}
-	      else
-		{
-		  delete_thread (gbuf[gi]);
-		  gi++;
-		}
+	      thread_info *tp = proc_target->find_thread (ptid_t (pid));
+	      thread_change_ptid (proc_target, ptid_t (pid), ptid);
+	      tp->priv.reset (priv);
 	    }
 	  else
-	    {
-	      thread = add_thread (proc_target, pptid);
+	    add_thread_with_info (proc_target, ptid,
+		private_thread_info_up (priv));
+	}
 
-	      aix_thread_info *priv = new aix_thread_info;
-	      thread->priv.reset (priv);
-	      priv->pdtid = pdtid;
-	      priv->tid = tid;
-	      pi++;
-	    }
+      /* The thread is terminated. Remove it.  */
+      if (state == PST_TERM)
+	{
+	  thread_info *thr = proc_target->find_thread (ptid);
+	  gdb_assert (thr != nullptr);
+	  delete_thread (thr);
+	  data->exited_threads.insert (pdtid);
 	}
     }
 
-  xfree (pbuf);
-  xfree (gbuf);
+    /* Sometimes there can be scenarios where the thread status is
+       unknown and we it will never iterate in the for loop above,
+       since cmd will be no longer be pointing to that threads.  One
+       such scenario is the gdb.threads/thread_events.exp testcase
+       where in the end after the threadfunc breakpoint is hit, the
+       thread exits and gets into a PST_UNKNOWN state.  So this thread
+       will not run in the above for loop.  Therefore the below for loop
+       is to manually delete such threads.  */
+    for (struct thread_info *it : all_threads ())
+      {
+	aix_thread_info *priv = get_aix_thread_info (it);
+	auto itr = std::find (data->in_queue_threads.begin (),
+				data->in_queue_threads.end (), priv->pdtid);
+	if (itr == data->in_queue_threads.end ())
+	  {
+	    delete_thread (it);
+	    data->exited_threads.insert (priv->pdtid);
+	  }
+      }
 }
 
 /* Iterate_over_threads() callback for locating a thread, using
@@ -1013,8 +923,8 @@ pd_update (pid_t pid)
 }
 
 /* Try to start debugging threads in the current process.
-   If successful and there exists and we can find an event thread, return a ptid
-   for that thread.  Otherwise, return a ptid-only ptid using PID.  */
+   If successful and there exists and we can find an event thread, set
+   pd_active for that thread.  Otherwise, return.  */
 
 static void
 pd_activate (pid_t pid)
@@ -2084,10 +1994,17 @@ aix_thread_target::thread_alive (ptid_t ptid)
 std::string
 aix_thread_target::pid_to_str (ptid_t ptid)
 {
-  if (ptid.tid () == 0)
-    return beneath ()->pid_to_str (ptid);
+  thread_info *thread_info = current_inferior ()->find_thread (ptid);
 
-  return string_printf (_("Thread %s"), pulongest (ptid.tid ()));
+  if (thread_info != NULL && thread_info->priv != NULL)
+    {
+      aix_thread_info *priv = get_aix_thread_info (thread_info);
+
+      return string_printf (_("Thread %s (tid %s)"), pulongest (ptid.tid ()),
+		pulongest (priv->tid));
+    }
+
+  return beneath ()->pid_to_str (ptid);
 }
 
 /* Return a printable representation of extra information about
@@ -2098,7 +2015,6 @@ aix_thread_target::extra_thread_info (struct thread_info *thread)
 {
   int status;
   pthdb_pthread_t pdtid;
-  pthdb_tid_t tid;
   pthdb_state_t state;
   pthdb_suspendstate_t suspendstate;
   pthdb_detachstate_t detachstate;
@@ -2115,33 +2031,31 @@ aix_thread_target::extra_thread_info (struct thread_info *thread)
   aix_thread_info *priv = get_aix_thread_info (thread);
 
   pdtid = priv->pdtid;
-  tid = priv->tid;
-
-  if (tid != PTHDB_INVALID_TID)
-    /* i18n: Like "thread-identifier %d, [state] running, suspended" */
-    buf.printf (_("tid %d"), (int)tid);
 
   status = pthdb_pthread_state (data->pd_session, pdtid, &state);
+
+  /* Output should look like Thread %d (tid %d) ([state]).  */
+  /* Example:- Thread 1 (tid 34144587) ([running]).  */
+  /* where state can be running, idle, sleeping, finished,
+     suspended, detached, cancel pending, ready or unknown.  */
+
   if (status != PTHDB_SUCCESS)
     state = PST_NOTSUP;
-  buf.printf (", %s", state2str (state));
+  buf.printf ("[%s]", state2str (state));
 
   status = pthdb_pthread_suspendstate (data->pd_session, pdtid,
 				       &suspendstate);
   if (status == PTHDB_SUCCESS && suspendstate == PSS_SUSPENDED)
-    /* i18n: Like "Thread-Id %d, [state] running, suspended" */
-    buf.printf (_(", suspended"));
+    buf.printf (_("[suspended]"));
 
   status = pthdb_pthread_detachstate (data->pd_session, pdtid,
 				      &detachstate);
   if (status == PTHDB_SUCCESS && detachstate == PDS_DETACHED)
-    /* i18n: Like "Thread-Id %d, [state] running, detached" */
-    buf.printf (_(", detached"));
+    buf.printf (_("[detached]"));
 
   pthdb_pthread_cancelpend (data->pd_session, pdtid, &cancelpend);
   if (status == PTHDB_SUCCESS && cancelpend)
-    /* i18n: Like "Thread-Id %d, [state] running, cancel pending" */
-    buf.printf (_(", cancel pending"));
+    buf.printf (_("[cancel pending]"));
 
   buf.write ("", 1);
 
-- 
2.41.0

Re: [PATCH v3] [RFC] Fix AIX thread exit events not being reported and UI to show kernel thread ID.

[Aditya Kamath1]

[-- Attachment #1: Type: text/plain, Size: 19535 bytes --]

Respected community members,

This is the patch version 3 of the fix.

This patch clears the gdb.thread/thread_events.exp test case.

                === gdb Summary ===

# of expected passes            11
/current_gdb/binutils-gdb/gdb/gdb version  15.0.50.20240325-git -nw -nx -q -iex "set height 0" -iex "set width 0" -data-directory /current_gdb/binutils-gdb/gdb/data-directory

And clears program 1 attached below this email as well. See the GDB output for the same as well in AIX.

We have only one small problem.

Consider the gdb.threads/thread_events.exp test case. See program 2 below this email (same as the .c file in the test case).

Here is the output of GDB in AIX.

Reading symbols from testsuite/gdb.threads/thread_events...
(gdb) b main
Breakpoint 1 at 0x100007dc: file gdb.threads/thread_events.c, line 41.
(gdb) r
Starting program: /current_gdb/binutils-gdb/gdb/testsuite/gdb.threads/thread_events
Breakpoint 1, main (argc=1, argv=0x2ff22940) at gdb.threads/thread_events.c:41
41        if (pthread_create (&thread, NULL, threadfunc, NULL) != 0)
(gdb) b after_join_func
Breakpoint 2 at 0x10000778: file gdb.threads/thread_events.c, line 34.
(gdb) b threadfunc
Breakpoint 3 at 0x10000718: file gdb.threads/thread_events.c, line 27.
(gdb) c
Continuing.
[New Thread 258 (tid 25231759)]
[Switching to Thread 258 (tid 25231759)]

Thread 2 hit Breakpoint 3, threadfunc (arg=0x0) at gdb.threads/thread_events.c:27
27        printf ("in threadfunc\n");
(gdb)
Continuing.
in threadfunc
[Thread 258 (tid 25231759) exited]
[Switching to Thread 1 (tid 28115323)]

Thread 1 hit Breakpoint 2, after_join_func () at gdb.threads/thread_events.c:34
34        printf ("finished\n");
(gdb) info threads
./../gdbsupport/intrusive_list.h:468: internal-error: erase_element: Assertion `elem_node->prev != nullptr' failed.
A problem internal to GDB has been detected,


When I do info threads here this fails. Did this happen because we delete the thread?? I am not sure why this happened??

Kindly let me know what we missed or where I went wrong in the patch. Awaiting a reply.

Have a nice day ahead.

Thanks and regards,
Aditya.

Program 1

#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <pthread.h>
#include <assert.h>

pthread_barrier_t barrier;

#define NUM_THREADS 3

void *
thread_function (void *arg)
{
  /* This ensures that the breakpoint is only hit after both threads
     are created, so the test can always switch to the non-event
     thread when the breakpoint triggers.  */
//  pthread_barrier_wait (&barrier);

  printf ("Hello World \n"); /* break here */
}

int
main (void)
{
  int i;

  alarm (300);

  pthread_barrier_init (&barrier, NULL, NUM_THREADS);

  for (i = 0; i < NUM_THREADS; i++)
    {
      pthread_t thread;
      int res;

      res = pthread_create (&thread, NULL,
                            thread_function, NULL);
      assert (res == 0);
    }

  printf ("More threads \n");
  for (i = 0; i < NUM_THREADS; i++)
    {
      pthread_t thread;
      int res;

      res = pthread_create (&thread, NULL,
                thread_function, NULL);
      assert (res == 0);
    }
  while (1)
    sleep (1);

  return 0;
}


GDB output for program 1:

Reading symbols from //gdb_tests/continue-pending-status_exit_test...
(gdb) r
Starting program: /gdb_tests/continue-pending-status_exit_test
More threads
Hello World
Hello World
Hello World
Hello World
Hello World
Hello World
[New Thread 258 (tid 28115345)]
[Thread 258 (tid 28115345) exited]
[New Thread 515 (tid 27001289)]
[Thread 515 (tid 27001289) exited]
[New Thread 772 (tid 19136771)]
[Thread 772 (tid 19136771) exited]
[New Thread 1029 (tid 18612569)]
[Thread 1029 (tid 18612569) exited]
[New Thread 1286 (tid 31457595)]
[Thread 1286 (tid 31457595) exited]
[New Thread 1543 (tid 30736823)]
[Thread 1543 (tid 30736823) exited]

Thread 1 received signal SIGINT, Interrupt.
0xd0611d70 in _p_nsleep () from /usr/lib/libpthread.a(_shr_xpg5.o)
(gdb) info threads
  Id   Target Id                           Frame
* 1    Thread 1 (tid 25231793) ([running]) 0xd0611d70 in _p_nsleep () from /usr/lib/libpthread.a(_shr_xpg5.o)

Program 2:

This file was written by Chris Demetriou (cgd@google.com<mailto:cgd@google.com>).  */

/* Simple test to trigger thread events (thread start, thread exit).  */

#include <pthread.h>
#include <stdlib.h>
#include <stdio.h>

static void *
threadfunc (void *arg)
{
  printf ("in threadfunc\n");
  return NULL;
}

static void
after_join_func (void)
{
  printf ("finished\n");
}

int main (int argc, char *argv[])
{
  pthread_t thread;

  if (pthread_create (&thread, NULL, threadfunc, NULL) != 0)
    {
      printf ("pthread_create failed\n");
      exit (1);
    }

  if (pthread_join (thread, NULL) != 0)
    {
      printf ("pthread_join failed\n");
      exit (1);
    }

  after_join_func ();
  return 0;
}
From: Aditya Vidyadhar Kamath <akamath996@gmail.com>
Date: Thursday, 2 May 2024 at 8:01 PM
To: tom@tromey.com <tom@tromey.com>
Cc: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>, gdb-patches@sourceware.org <gdb-patches@sourceware.org>, Aditya Kamath1 <Aditya.Kamath1@ibm.com>, Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>, jhb@freebsd.org <jhb@freebsd.org>
Subject: [EXTERNAL] [PATCH v3] [RFC] Fix AIX thread exit events not being reported and UI to show kernel thread ID.
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>

In AIX when a thread exits we were not showing that a thread exit event happened
and GDB continued to keep the terminated threads.

If we have terminated threads then the UI on info threads command will look like
(gdb) info threads
  Id   Target Id                                          Frame
* 1    Thread 1 (tid 26607979, running)    0xd0611d70 in _p_nsleep () from /usr/lib/libpthreads.a(_shr_xpg5.o)
  2    Thread 258 (tid 30998799, finished) aix-thread: ptrace (52, 30998799) returned -1 (errno = 3 The process does not exist.)

If we see the frame is not getting displayed correctly.

The reason for the same is that in AIX we were not managing thread states. In particular we do not know
when a thread terminates.

The reason being in sync_threadlists () the pbuf and gbuf lists remain the same though certain threads exit.

This patch is a fix to the same.

Also certain UI is changed.

On a new thread born and exit the UI in AIX will be similar to Linux with both user and kernel thread information.

[New Thread 258 (tid 32178533)]
[New Thread 515 (tid 30343651)]
[New Thread 772 (tid 33554909)]
[New Thread 1029 (tid 24969489)]
[New Thread 1286 (tid 18153945)]
[New Thread 1543 (tid 30736739)]
[Thread 258 (tid 32178533) exited]
[Thread 515 (tid 30343651) exited]
[Thread 772 (tid 33554909) exited]
[Thread 1029 (tid 24969489) exited]
[Thread 1286 (tid 18153945) exited]
[Thread 1543 (tid 30736739) exited]

and info threads will look like
(gdb) info threads
  Id   Target Id                           Frame
* 1    Thread 1 (tid 31326579) ([running]) 0xd0611d70 in _p_nsleep () from /usr/lib/libpthread.a(_shr_xpg5.o)
---
 gdb/aix-thread.c | 248 ++++++++++++++++-------------------------------
 1 file changed, 81 insertions(+), 167 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index c70bd82bc24..bd74c0c805b 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -55,6 +55,7 @@
 #include <sys/reg.h>
 #include <sched.h>
 #include <sys/pthdebug.h>
+#include <unordered_set>

 #if !HAVE_DECL_GETTHRDS
 extern int getthrds (pid_t, struct thrdsinfo64 *, int, tid_t *, int);
@@ -190,6 +191,12 @@ struct aix_thread_variables
   /* Whether the current architecture is 64-bit.
    Only valid when pd_able is true.  */
   int arch64;
+
+  /* Describes the number of thread exit events reported.  */
+  std::unordered_set<pthdb_pthread_t> exited_threads;
+
+  /* Describes if thread is still in queue and not in unknown state.  */
+  std::vector<pthdb_pthread_t> in_queue_threads;
 };

 /* Key to our per-inferior data.  */
@@ -737,47 +744,6 @@ state2str (pthdb_state_t state)
     }
 }

-/* qsort() comparison function for sorting pd_thread structs by pthid.  */
-
-static int
-pcmp (const void *p1v, const void *p2v)
-{
-  struct pd_thread *p1 = (struct pd_thread *) p1v;
-  struct pd_thread *p2 = (struct pd_thread *) p2v;
-  return p1->pthid < p2->pthid ? -1 : p1->pthid > p2->pthid;
-}
-
-/* ptid comparison function */
-
-static int
-ptid_cmp (ptid_t ptid1, ptid_t ptid2)
-{
-  if (ptid1.pid () < ptid2.pid ())
-    return -1;
-  else if (ptid1.pid () > ptid2.pid ())
-    return 1;
-  else if (ptid1.tid () < ptid2.tid ())
-    return -1;
-  else if (ptid1.tid () > ptid2.tid ())
-    return 1;
-  else if (ptid1.lwp () < ptid2.lwp ())
-    return -1;
-  else if (ptid1.lwp () > ptid2.lwp ())
-    return 1;
-  else
-    return 0;
-}
-
-/* qsort() comparison function for sorting thread_info structs by pid.  */
-
-static int
-gcmp (const void *t1v, const void *t2v)
-{
-  struct thread_info *t1 = *(struct thread_info **) t1v;
-  struct thread_info *t2 = *(struct thread_info **) t2v;
-  return ptid_cmp (t1->ptid, t2->ptid);
-}
-
 /* Search through the list of all kernel threads for the thread
    that has stopped on a SIGTRAP signal, and return its TID.
    Return 0 if none found.  */
@@ -822,22 +788,16 @@ static void
 sync_threadlists (pid_t pid)
 {
   int cmd, status;
-  int pcount, psize, pi, gcount, gi;
-  struct pd_thread *pbuf;
-  struct thread_info **gbuf, **g, *thread;
   pthdb_pthread_t pdtid;
   pthread_t pthid;
   pthdb_tid_t tid;
   process_stratum_target *proc_target = current_inferior ()->process_target ();
   struct aix_thread_variables *data;
   data = get_thread_data_helper_for_pid (pid);
-
+  pthdb_state_t state;
+  data->in_queue_threads.clear ();
   /* Accumulate an array of libpthdebug threads sorted by pthread id.  */

-  pcount = 0;
-  psize = 1;
-  pbuf = XNEWVEC (struct pd_thread, psize);
-
   for (cmd = PTHDB_LIST_FIRST;; cmd = PTHDB_LIST_NEXT)
     {
       status = pthdb_pthread (data->pd_session, &pdtid, cmd);
@@ -848,118 +808,68 @@ sync_threadlists (pid_t pid)
       if (status != PTHDB_SUCCESS || pthid == PTHDB_INVALID_PTID)
         continue;

-      if (pcount == psize)
-       {
-         psize *= 2;
-         pbuf = (struct pd_thread *) xrealloc (pbuf,
-                                               psize * sizeof *pbuf);
-       }
-      pbuf[pcount].pdtid = pdtid;
-      pbuf[pcount].pthid = pthid;
-      pcount++;
-    }
-
-  for (pi = 0; pi < pcount; pi++)
-    {
-      status = pthdb_pthread_tid (data->pd_session, pbuf[pi].pdtid, &tid);
-      if (status != PTHDB_SUCCESS)
-       tid = PTHDB_INVALID_TID;
-      pbuf[pi].tid = tid;
-    }
-
-  qsort (pbuf, pcount, sizeof *pbuf, pcmp);
-
-  /* Accumulate an array of GDB threads sorted by pid.  */
-
-  /* gcount is GDB thread count and pcount is pthreadlib thread count.  */
+      ptid_t ptid (pid, 0, pthid);
+      status = pthdb_pthread_state (data->pd_session, pdtid, &state);
+      data->in_queue_threads.push_back (pdtid);

-  gcount = 0;
-  for (thread_info *tp : all_threads (proc_target, ptid_t (pid)))
-    gcount++;
-  g = gbuf = XNEWVEC (struct thread_info *, gcount);
-  for (thread_info *tp : all_threads (proc_target, ptid_t (pid)))
-    *g++ = tp;
-  qsort (gbuf, gcount, sizeof *gbuf, gcmp);
-
-  /* Apply differences between the two arrays to GDB's thread list.  */
-
-  for (pi = gi = 0; pi < pcount || gi < gcount;)
-    {
-      if (pi == pcount)
+      /* If this thread has reported and exited, do not add it again.  */
+      if (state == PST_TERM)
         {
-         delete_thread (gbuf[gi]);
-         gi++;
+         if (data->exited_threads.count (pdtid) != 0)
+            continue;
         }
-      else if (gi == gcount)
-       {
-         aix_thread_info *priv = new aix_thread_info;
-         priv->pdtid = pbuf[pi].pdtid;
-         priv->tid = pbuf[pi].tid;
-
-         thread = add_thread_with_info (proc_target,
-                                        ptid_t (pid, 0, pbuf[pi].pthid),
-                                        private_thread_info_up (priv));

-         pi++;
-       }
-      else
+      /* If this thread has never been reported to GDB, add it.  */
+      if (!in_thread_list (proc_target, ptid))
         {
-         ptid_t pptid, gptid;
-         int cmp_result;
-
-         pptid = ptid_t (pid, 0, pbuf[pi].pthid);
-         gptid = gbuf[gi]->ptid;
-         pdtid = pbuf[pi].pdtid;
-         tid = pbuf[pi].tid;
-
-         cmp_result = ptid_cmp (pptid, gptid);
-
-         if (cmp_result == 0)
-           {
-             aix_thread_info *priv = get_aix_thread_info (gbuf[gi]);
-
-             priv->pdtid = pdtid;
-             priv->tid = tid;
-             pi++;
-             gi++;
-           }
-         else if (cmp_result > 0)
+         aix_thread_info *priv = new aix_thread_info;
+         /* init priv */
+         priv->pdtid = pdtid;
+         status = pthdb_pthread_tid (data->pd_session, pdtid, &tid);
+         priv->tid = tid;
+         /* Check if this is the main thread.  If it is, then change
+            its ptid and add its private data.  */
+         if (get_signaled_thread (pid) == tid
+               && in_thread_list (proc_target, ptid_t (pid)))
             {
-             /* This is to make the main process thread now look
-                like a thread.  */
-
-             if (gptid.is_pid ())
-               {
-                 thread_info *tp = proc_target->find_thread (gptid);
-                 thread_change_ptid (proc_target, gptid, pptid);
-                 aix_thread_info *priv = new aix_thread_info;
-                 priv->pdtid = pbuf[pi].pdtid;
-                 priv->tid = pbuf[pi].tid;
-                 tp->priv.reset (priv);
-                 gi++;
-                 pi++;
-               }
-             else
-               {
-                 delete_thread (gbuf[gi]);
-                 gi++;
-               }
+             thread_info *tp = proc_target->find_thread (ptid_t (pid));
+             thread_change_ptid (proc_target, ptid_t (pid), ptid);
+             tp->priv.reset (priv);
             }
           else
-           {
-             thread = add_thread (proc_target, pptid);
+           add_thread_with_info (proc_target, ptid,
+               private_thread_info_up (priv));
+       }

-             aix_thread_info *priv = new aix_thread_info;
-             thread->priv.reset (priv);
-             priv->pdtid = pdtid;
-             priv->tid = tid;
-             pi++;
-           }
+      /* The thread is terminated. Remove it.  */
+      if (state == PST_TERM)
+       {
+         thread_info *thr = proc_target->find_thread (ptid);
+         gdb_assert (thr != nullptr);
+         delete_thread (thr);
+         data->exited_threads.insert (pdtid);
         }
     }

-  xfree (pbuf);
-  xfree (gbuf);
+    /* Sometimes there can be scenarios where the thread status is
+       unknown and we it will never iterate in the for loop above,
+       since cmd will be no longer be pointing to that threads.  One
+       such scenario is the gdb.threads/thread_events.exp testcase
+       where in the end after the threadfunc breakpoint is hit, the
+       thread exits and gets into a PST_UNKNOWN state.  So this thread
+       will not run in the above for loop.  Therefore the below for loop
+       is to manually delete such threads.  */
+    for (struct thread_info *it : all_threads ())
+      {
+       aix_thread_info *priv = get_aix_thread_info (it);
+       auto itr = std::find (data->in_queue_threads.begin (),
+                               data->in_queue_threads.end (), priv->pdtid);
+       if (itr == data->in_queue_threads.end ())
+         {
+           delete_thread (it);
+           data->exited_threads.insert (priv->pdtid);
+         }
+      }
 }

 /* Iterate_over_threads() callback for locating a thread, using
@@ -1013,8 +923,8 @@ pd_update (pid_t pid)
 }

 /* Try to start debugging threads in the current process.
-   If successful and there exists and we can find an event thread, return a ptid
-   for that thread.  Otherwise, return a ptid-only ptid using PID.  */
+   If successful and there exists and we can find an event thread, set
+   pd_active for that thread.  Otherwise, return.  */

 static void
 pd_activate (pid_t pid)
@@ -2084,10 +1994,17 @@ aix_thread_target::thread_alive (ptid_t ptid)
 std::string
 aix_thread_target::pid_to_str (ptid_t ptid)
 {
-  if (ptid.tid () == 0)
-    return beneath ()->pid_to_str (ptid);
+  thread_info *thread_info = current_inferior ()->find_thread (ptid);

-  return string_printf (_("Thread %s"), pulongest (ptid.tid ()));
+  if (thread_info != NULL && thread_info->priv != NULL)
+    {
+      aix_thread_info *priv = get_aix_thread_info (thread_info);
+
+      return string_printf (_("Thread %s (tid %s)"), pulongest (ptid.tid ()),
+               pulongest (priv->tid));
+    }
+
+  return beneath ()->pid_to_str (ptid);
 }

 /* Return a printable representation of extra information about
@@ -2098,7 +2015,6 @@ aix_thread_target::extra_thread_info (struct thread_info *thread)
 {
   int status;
   pthdb_pthread_t pdtid;
-  pthdb_tid_t tid;
   pthdb_state_t state;
   pthdb_suspendstate_t suspendstate;
   pthdb_detachstate_t detachstate;
@@ -2115,33 +2031,31 @@ aix_thread_target::extra_thread_info (struct thread_info *thread)
   aix_thread_info *priv = get_aix_thread_info (thread);

   pdtid = priv->pdtid;
-  tid = priv->tid;
-
-  if (tid != PTHDB_INVALID_TID)
-    /* i18n: Like "thread-identifier %d, [state] running, suspended" */
-    buf.printf (_("tid %d"), (int)tid);

   status = pthdb_pthread_state (data->pd_session, pdtid, &state);
+
+  /* Output should look like Thread %d (tid %d) ([state]).  */
+  /* Example:- Thread 1 (tid 34144587) ([running]).  */
+  /* where state can be running, idle, sleeping, finished,
+     suspended, detached, cancel pending, ready or unknown.  */
+
   if (status != PTHDB_SUCCESS)
     state = PST_NOTSUP;
-  buf.printf (", %s", state2str (state));
+  buf.printf ("[%s]", state2str (state));

   status = pthdb_pthread_suspendstate (data->pd_session, pdtid,
                                        &suspendstate);
   if (status == PTHDB_SUCCESS && suspendstate == PSS_SUSPENDED)
-    /* i18n: Like "Thread-Id %d, [state] running, suspended" */
-    buf.printf (_(", suspended"));
+    buf.printf (_("[suspended]"));

   status = pthdb_pthread_detachstate (data->pd_session, pdtid,
                                       &detachstate);
   if (status == PTHDB_SUCCESS && detachstate == PDS_DETACHED)
-    /* i18n: Like "Thread-Id %d, [state] running, detached" */
-    buf.printf (_(", detached"));
+    buf.printf (_("[detached]"));

   pthdb_pthread_cancelpend (data->pd_session, pdtid, &cancelpend);
   if (status == PTHDB_SUCCESS && cancelpend)
-    /* i18n: Like "Thread-Id %d, [state] running, cancel pending" */
-    buf.printf (_(", cancel pending"));
+    buf.printf (_("[cancel pending]"));

   buf.write ("", 1);

--
2.41.0

Re: [PATCH v3] [RFC] Fix AIX thread exit events not being reported and UI to show kernel thread ID.

[Ulrich Weigand]

Aditya Vidyadhar Kamath <akamath996@gmail.com> wrote:

>+  /* Describes the number of thread exit events reported.  */
>+  std::unordered_set<pthdb_pthread_t> exited_threads;
>+
>+  /* Describes if thread is still in queue and not in unknown state.  */
>+  std::vector<pthdb_pthread_t> in_queue_threads;

I don't really like these new global variables.  exited_threads
seems to be only ever added to, so it will just keep growing
forever?  in_queue_threads seems to be fully local to the
sync_threadlists routine, so why is it not just a local
variable there?

As a more general question, I'm wondering why you're completely
changing the way sync_threadlists works.  I think the overall
idea of sync_threadslists, i.e. to compare the thread list of
the OS with GDB's thread list and then update the latter to
match the former, is still valid.  I thought you'd simply change
the way the "pbuf" list is generated to filter out those threads
that the OS considers in terminated state.

Bye,
Ulrich

Re: [PATCH v3] [RFC] Fix AIX thread exit events not being reported and UI to show kernel thread ID.

[John Baldwin]

On 5/2/24 9:30 AM, Ulrich Weigand wrote:
> Aditya Vidyadhar Kamath <akamath996@gmail.com> wrote:
> 
>> +  /* Describes the number of thread exit events reported.  */
>> +  std::unordered_set<pthdb_pthread_t> exited_threads;
>> +
>> +  /* Describes if thread is still in queue and not in unknown state.  */
>> +  std::vector<pthdb_pthread_t> in_queue_threads;
> 
> I don't really like these new global variables.  exited_threads
> seems to be only ever added to, so it will just keep growing
> forever?  in_queue_threads seems to be fully local to the
> sync_threadlists routine, so why is it not just a local
> variable there?

I think exited_threads is not global but per-inferior?

in_queue_threads does indeed seem to be something that could be local
to the function.

> As a more general question, I'm wondering why you're completely
> changing the way sync_threadlists works.  I think the overall
> idea of sync_threadslists, i.e. to compare the thread list of
> the OS with GDB's thread list and then update the latter to
> match the former, is still valid.  I thought you'd simply change
> the way the "pbuf" list is generated to filter out those threads
> that the OS considers in terminated state.

I think in this case it's a bit different as the internal list from
pbuf "grows forever" (specifically, exited threads do not get removed
from the thread library's internal list it seems, just remain forever
as an exited thread).  In that case, you don't have to extract two
thread lists so that you can walk the union of the two lists looking
for mismatches.  Instead, the pbuf list will always be a superset of
GDB's list, so it is sufficient to walk the pbuf list and decide how
to handle each thread.  (I must say that I'm surprised by the behavior
of exited threads staying in AIX's thread library list forever, but
it does seem to be the case.)

-- 
John Baldwin

RE: [PATCH v3] [RFC] Fix AIX thread exit events not being reported and UI to show kernel thread ID.

[Aditya Kamath1]

[-- Attachment #1: Type: text/plain, Size: 5768 bytes --]

Respected Ulrich, John and community members,

Hi,

>I don't really like these new global variables.  exited_threads
>seems to be only ever added to, so it will just keep growing
>forever?  in_queue_threads seems to be fully local to the
>sync_threadlists routine, so why is it not just a local
>variable there?

>As a more general question, I'm wondering why you're completely
>changing the way sync_threadlists works.  I think the overall
>idea of sync_threadslists, i.e. to compare the thread list of
>the OS with GDB's thread list and then update the latter to
>match the former, is still valid.  I thought you'd simply change
>the way the "pbuf" list is generated to filter out those threads
>that the OS considers in terminated state.

>I think exited_threads is not global but per-inferior?

>in_queue_threads does indeed seem to be something that could be local
to the function.

So I agree with keeping in_queue_threads local.

Yes, the idea is to use pbuf and gbuf to match threads and be either add or delete depending on the threads depending on the lists.

Here is the problem as John said.

There is no way pbuf is able tell us whether a thread exits or not. Though a thread reached the PST_TERM state it still ends up being in the for loop (cmd = FIRST to last) and therefore the pbuf list. So in AIX we are not able to show if a thread actually exits or not. Simply because both pbuf and gbuf lists are same and nothing will happen.

There are two scenarios that is causing problems if we stick to the pbuf and gbuf way of doing sync_threadlists (). For both of them assume there is one main thread and one thread with a simple printf (“Hello world \n”);

1:  In the first scenario the thread reaches PST_TERM state and prints its printf contents as well before even our pd_activate () is successful and are able to tell GDB that “Hey a new thread has arrived” via pd_update () or sync_threadlists (). In this scenario even if we have the code to exclude the thread from the pbuf because it is in terminated state, it is of no use, since we never even informed user that our thread was born. So pbuf and gbuf will just have a main thread only throughput the debug session. Initially I proposed a fix such that we skip the PST_TERM condition in case the thread was never added to GDB. But then this complicates the whole logic which we proposed the use of exited_threads set.

2: In the exit scenario also, let us say we have made sure GDB is aware that a new thread is born and is in gbuf. This time when  GDB comes to sync_threadlists (), the for loop (cmd=First to last) runs only once for the main thread. Reason being the thread with simple printf has finished its job and no longer exists in the OS. So it has reached an unknown state (PST_UNKNOWN). This again will cause problems while having pbuf to do a PST_TERM check and then have its list and then match with gbuf list. Which is why I proposed for in_queue_threads vector.

So this is what is happening in AIX and these are factors I kept in mind before proposing this patch.

I understand things are not optimal in the code.

Kindly let me know what you think can be the best way out. I am open to having pbuf, gbuf way or maintain lists per inferior as well contain thread information.

Have a nice day ahead.

Thanks and regards,
Aditya.

From: John Baldwin <jhb@FreeBSD.org>
Date: Thursday, 2 May 2024 at 10:11 PM
To: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>, akamath996@gmail.com <akamath996@gmail.com>, tom@tromey.com <tom@tromey.com>
Cc: gdb-patches@sourceware.org <gdb-patches@sourceware.org>, Aditya Kamath1 <Aditya.Kamath1@ibm.com>, Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: [EXTERNAL] Re: [PATCH v3] [RFC] Fix AIX thread exit events not being reported and UI to show kernel thread ID.
On 5/2/24 9:30 AM, Ulrich Weigand wrote:
> Aditya Vidyadhar Kamath <akamath996@gmail.com> wrote:
>
>> +  /* Describes the number of thread exit events reported.  */
>> +  std::unordered_set<pthdb_pthread_t> exited_threads;
>> +
>> +  /* Describes if thread is still in queue and not in unknown state.  */
>> +  std::vector<pthdb_pthread_t> in_queue_threads;
>
> I don't really like these new global variables.  exited_threads
> seems to be only ever added to, so it will just keep growing
> forever?  in_queue_threads seems to be fully local to the
> sync_threadlists routine, so why is it not just a local
> variable there?

I think exited_threads is not global but per-inferior?

in_queue_threads does indeed seem to be something that could be local
to the function.

> As a more general question, I'm wondering why you're completely
> changing the way sync_threadlists works.  I think the overall
> idea of sync_threadslists, i.e. to compare the thread list of
> the OS with GDB's thread list and then update the latter to
> match the former, is still valid.  I thought you'd simply change
> the way the "pbuf" list is generated to filter out those threads
> that the OS considers in terminated state.

I think in this case it's a bit different as the internal list from
pbuf "grows forever" (specifically, exited threads do not get removed
from the thread library's internal list it seems, just remain forever
as an exited thread).  In that case, you don't have to extract two
thread lists so that you can walk the union of the two lists looking
for mismatches.  Instead, the pbuf list will always be a superset of
GDB's list, so it is sufficient to walk the pbuf list and decide how
to handle each thread.  (I must say that I'm surprised by the behavior
of exited threads staying in AIX's thread library list forever, but
it does seem to be the case.)

--
John Baldwin

Re: [PATCH v3] [RFC] Fix AIX thread exit events not being reported and UI to show kernel thread ID.

[Ulrich Weigand]

Aditya Kamath1 <Aditya.Kamath1@ibm.com> wrote:

>There is no way pbuf is able tell us whether a thread exits or not.
>Though a thread reached the PST_TERM state it still ends up being in
>the for loop (cmd = FIRST to last) and therefore the pbuf list.

My point was that you could simply check whether the state is
PST_TERM and if so, just not add this thread to the pbuf to begin
with.  After that, just run through the pbuf vs. gbuf comparison
as it is today.

Bye,
Ulrich

Re: [PATCH v3] [RFC] Fix AIX thread exit events not being reported and UI to show kernel thread ID.

[John Baldwin]

On 5/2/24 11:28 AM, Ulrich Weigand wrote:
> Aditya Kamath1 <Aditya.Kamath1@ibm.com> wrote:
> 
>> There is no way pbuf is able tell us whether a thread exits or not.
>> Though a thread reached the PST_TERM state it still ends up being in
>> the for loop (cmd = FIRST to last) and therefore the pbuf list.
> 
> My point was that you could simply check whether the state is
> PST_TERM and if so, just not add this thread to the pbuf to begin
> with.  After that, just run through the pbuf vs. gbuf comparison
> as it is today.

I think the one edge case with that Aditya was trying to handle is that
today AIX doesn't force a stop when a new thread appears.  Thus, a
thread can be created and exit in between two stops.  If you ignore
PST_TERM threads always, then GDB will never "notice" this thread, so
the output doesn't match that of Linux.  The current patch Aditya has
will notice this case and report back-to-back "new thread" and "thread
exited" messages for these threads when it rescans the thread lists at
the stop after the thread was created and exited.

Even if you don't do that, the fact that pbuf will always report some
sort of status for all threads (including exited threads that have been
seen before), does mean that a single loop over the list of threads from
the thread library is sufficient to enumerate all possible threads.  When
comparing the before and after versions of the code side by side I find
the newer version easier to understand as a single loop over the list
reported by libthread_db even if the resulting diff is a bit larger.

-- 
John Baldwin

Re: [PATCH v3] [RFC] Fix AIX thread exit events not being reported and UI to show kernel thread ID.

[Ulrich Weigand]

[-- Attachment #1: Type: text/plain, Size: 3176 bytes --]

John Baldwin <jhb@FreeBSD.org<mailto:jhb@FreeBSD.org>> wrote:


>I think the one edge case with that Aditya was trying to handle is that

>today AIX doesn't force a stop when a new thread appears.  Thus, a

>thread can be created and exit in between two stops.  If you ignore

>PST_TERM threads always, then GDB will never "notice" this thread, so

>the output doesn't match that of Linux.  The current patch Aditya has

>will notice this case and report back-to-back "new thread" and "thread

>exited" messages for these threads when it rescans the thread lists at

>the stop after the thread was created and exited.


I see.  I guess this is why the exited_threads variable is needed?

It does seem odd to allow the inferior to cause unbounded memory

consumption in GDB - but maybe this isn't a significant concern if

the OS already limits the number of threads during lifetime of a

process somehow ...


>Even if you don't do that, the fact that pbuf will always report some

>sort of status for all threads (including exited threads that have been

>seen before), does mean that a single loop over the list of threads from

>the thread library is sufficient to enumerate all possible threads.  When

>comparing the before and after versions of the code side by side I find

>the newer version easier to understand as a single loop over the list

>reported by libthread_db even if the resulting diff is a bit larger.


The current patch does have a second loop:

    for (struct thread_info *it : all_threads ())

      {

        aix_thread_info *priv = get_aix_thread_info (it);

        auto itr = std::find (data->in_queue_threads.begin (),

                                data->in_queue_threads.end (), priv->pdtid);

        if (itr == data->in_queue_threads.end ())

          {

            delete_thread (it);

            data->exited_threads.insert (priv->pdtid);

          }

      }

which is quadratic in the number of threads; I think avoiding this

was one of the reasons for using two sorted lists in the current

implementation.  However, I guess this can be fixed by using a

different data structure for in_queue_threads, probably best an

unordered_set as well (or maybe set).


If the new implementation is easier to read, I don't object to it.


One more question to Aditya:


          /* Check if this is the main thread.  If it is, then change

             its ptid and add its private data.  */

          if (get_signaled_thread (pid) == tid

                && in_thread_list (proc_target, ptid_t (pid)))


I don't understand this use of get_signaled_thread - this does *not*

always return the main thread, but rather the one where GDB happened

to stop (which may or may not be the main thread).


The old code assumed the thread with the smallest ptid is the main

thread - is there a reason for not using that same check?


(If there is a reason for using get_signaled_thread that I don't

see right now, at least it should be moved outside the loop to

avoid another quadratic runtime complexity.)


Bye,

Ulrich

Re: [PATCH v3] [RFC] Fix AIX thread exit events not being reported and UI to show kernel thread ID.

[Aditya Kamath1]

[-- Attachment #1: Type: text/plain, Size: 5917 bytes --]

Respected Ulrich, John and community members,

>I see.  I guess this is why the exited_threads variable is needed?

>It does seem odd to allow the inferior to cause unbounded memory

>consumption in GDB - but maybe this isn't a significant concern if

>the OS already limits the number of threads during lifetime of a

>process somehow ...



Yes. We want to check if the thread has reported and exited. If it is, we do not add it again [This is the responsibility of the first if condition in the for loop]. Also, if this thread has never been seen before and may or may not be in PST_TERM then add it as a new thread and in the next if condition for the PST_TERM matches. If it does delete this thread.



Usually there is a limit for resources that a process can use. Unless a user, sets them to unlimited via ulimit command, every process will have limited resources to work with.



>However, I guess this can be fixed by using a

>different data structure for in_queue_threads, probably best an

>unordered_set as well (or maybe set).

>If the new implementation is easier to read, I don't object to it.



I will take care of this.





>I don't understand this use of get_signaled_thread - this does *not*

>always return the main thread, but rather the one where GDB happened

>to stop (which may or may not be the main thread).

>The old code assumed the thread with the smallest ptid is the main

>thread - is there a reason for not using that same check?



Yeah, you are right. There is one stop when the main thread is getting threaded. It came here and then I wanted to be sure that the ptid we are changing is the main thread’s only. Because there can be the new threads also. Because I was aware that the signalled thread here will be the main thread for the testcase I was testing, I had put it. But you are correct. It need not be the case everytime. And the smallest ptid guy is the main thread.



I am thinking having just if (in_thread_list (proc_target, ptid_t (pid))) will be enough then.



I will make this change.

Will make all the suggested changes and get back with a new version of the patch.

Have a nice day ahead.

Thanks and regards,
Aditya.

From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Date: Friday, 3 May 2024 at 5:12 PM
To: akamath996@gmail.com <akamath996@gmail.com>, Aditya Kamath1 <Aditya.Kamath1@ibm.com>, tom@tromey.com <tom@tromey.com>, jhb@FreeBSD.org <jhb@FreeBSD.org>
Cc: gdb-patches@sourceware.org <gdb-patches@sourceware.org>, Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: Re: [PATCH v3] [RFC] Fix AIX thread exit events not being reported and UI to show kernel thread ID.

John Baldwin <jhb@FreeBSD.org<mailto:jhb@FreeBSD.org>> wrote:



>I think the one edge case with that Aditya was trying to handle is that

>today AIX doesn't force a stop when a new thread appears.  Thus, a

>thread can be created and exit in between two stops.  If you ignore

>PST_TERM threads always, then GDB will never "notice" this thread, so

>the output doesn't match that of Linux.  The current patch Aditya has

>will notice this case and report back-to-back "new thread" and "thread

>exited" messages for these threads when it rescans the thread lists at

>the stop after the thread was created and exited.



I see.  I guess this is why the exited_threads variable is needed?

It does seem odd to allow the inferior to cause unbounded memory

consumption in GDB - but maybe this isn't a significant concern if

the OS already limits the number of threads during lifetime of a

process somehow ...



>Even if you don't do that, the fact that pbuf will always report some

>sort of status for all threads (including exited threads that have been

>seen before), does mean that a single loop over the list of threads from

>the thread library is sufficient to enumerate all possible threads.  When

>comparing the before and after versions of the code side by side I find

>the newer version easier to understand as a single loop over the list

>reported by libthread_db even if the resulting diff is a bit larger.



The current patch does have a second loop:

    for (struct thread_info *it : all_threads ())

      {

        aix_thread_info *priv = get_aix_thread_info (it);

        auto itr = std::find (data->in_queue_threads.begin (),

                                data->in_queue_threads.end (), priv->pdtid);

        if (itr == data->in_queue_threads.end ())

          {

            delete_thread (it);

            data->exited_threads.insert (priv->pdtid);

          }

      }

which is quadratic in the number of threads; I think avoiding this

was one of the reasons for using two sorted lists in the current

implementation.  However, I guess this can be fixed by using a

different data structure for in_queue_threads, probably best an

unordered_set as well (or maybe set).



If the new implementation is easier to read, I don't object to it.



One more question to Aditya:



          /* Check if this is the main thread.  If it is, then change

             its ptid and add its private data.  */

          if (get_signaled_thread (pid) == tid

                && in_thread_list (proc_target, ptid_t (pid)))



I don't understand this use of get_signaled_thread - this does *not*

always return the main thread, but rather the one where GDB happened

to stop (which may or may not be the main thread).



The old code assumed the thread with the smallest ptid is the main

thread - is there a reason for not using that same check?



(If there is a reason for using get_signaled_thread that I don't

see right now, at least it should be moved outside the loop to

avoid another quadratic runtime complexity.)



Bye,

Ulrich