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

[Aditya Kamath1 <Aditya.Kamath1@ibm.com>]

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

Respected community members,

Any update/feedback on this version of the patch. Kindly let me know.

Have a nice day ahead.

Thanks and regards,
Aditya.

From: Aditya Kamath1 <Aditya.Kamath1@ibm.com>
Date: Friday, 3 May 2024 at 8:44 PM
To: Aditya Vidyadhar Kamath <akamath996@gmail.com>, tom@tromey.com <tom@tromey.com>
Cc: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>, gdb-patches@sourceware.org <gdb-patches@sourceware.org>, Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>, jhb@freebsd.org <jhb@freebsd.org>
Subject: Re: [EXTERNAL] [PATCH v5] Fix AIX thread exit events not being reported and UI to show kernel thread ID.
Respected Ulrich and community members,

Hi,

>+        if (get_signaled_thread (pid) == tid
>+              && in_thread_list (proc_target, ptid_t (pid)))
>Didn't you want to change this?

In enthusiasm I missed it. Sorry for the same. Please check now.

>+      auto itr = in_queue_threads.find (priv->pdtid);
>+      if (itr == in_queue_threads.end ()
>Why don't you use .count like elsewhere?

This also is done.

Kindly let me know if v5 is okay.

Pasting results of v5 patch.

Gdb.threads/thread_events.exp test case result after this patch

=== gdb Summary ===
 gmake check RUNTESTFLAGS='gdb.threads/thread_events.exp  CC_FOR_TARGET="/opt/freeware/bin/gcc" CXX_FOR_TARGET="/opt/freeware/bin/g++" CXXFLAGS_FOR_TARGET="-O0 -w -g -gdwarf -maix64" CFLAGS_FOR_TARGET="-O0 -w -g -gdwarf -maix64"'
gmake check-single
# 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

Output of program 1(Program 1 Pasted below this email)
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 threadfunc
Breakpoint 2 at 0x10000718: file gdb.threads/thread_events.c, line 27.
(gdb) b after_join_func
Breakpoint 3 at 0x10000778: file gdb.threads/thread_events.c, line 34.
(gdb) c
Continuing.
[New Thread 258 (tid 24969631)]
[Switching to Thread 258 (tid 24969631)]

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

Thread 1 hit Breakpoint 3, after_join_func () at gdb.threads/thread_events.c:34
34        printf ("finished\n");
(gdb) info threads
  Id   Target Id                           Frame
* 1    Thread 1 (tid 34341137) ([running]) after_join_func () at gdb.threads/thread_events.c:34
(gdb) c
Continuing.
finished
[Thread 1 (tid 34341137) exited]
[Inferior 1 (process 11338122) exited normally]
(gdb) q

Program 1:

/*   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;
}



Output of program 2 [Program 2 pasted below]

Reading symbols from //gdb_tests/continue-pending-status_exit_test...
(gdb) r
Starting program: /gdb_tests/continue-pending-status_exit_test
Hello World
Hello World
More threads
Hello World
Hello World
Hello World
Hello World
[New Thread 258 (tid 24969633)]
[Thread 258 (tid 24969633) exited]
[New Thread 515 (tid 30540067)]
[Thread 515 (tid 30540067) exited]
[New Thread 772 (tid 30671107)]
[Thread 772 (tid 30671107) exited]
[New Thread 1029 (tid 31588767)]
[Thread 1029 (tid 31588767) exited]
[New Thread 1286 (tid 30998947)]
[Thread 1286 (tid 30998947) exited]
[New Thread 1543 (tid 28967413)]
[Thread 1543 (tid 28967413) 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 34341139) ([running]) 0xd0611d70 in _p_nsleep () from /usr/lib/libpthread.a(_shr_xpg5.o)
(gdb) q
A debugging session is active.

        Inferior 1 [process 11338126] will be killed.

Quit anyway? (y or n) y

Program 2:

#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;
}


From: Aditya Vidyadhar Kamath <akamath996@gmail.com>
Date: Friday, 3 May 2024 at 8:41 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 v5] 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)

Also a small change to testcase gdb.threads/thread_events.exp to make sure this test runs on AIX as well.
---
 gdb/aix-thread.c                            | 242 ++++++--------------
 gdb/testsuite/gdb.threads/thread_events.exp |   6 +-
 2 files changed, 78 insertions(+), 170 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index c70bd82bc24..a699a539598 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,9 @@ 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;
 };

 /* Key to our per-inferior data.  */
@@ -737,47 +741,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 +785,17 @@ 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;
+  std::set<pthdb_pthread_t> in_queue_threads;

   /* 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 +806,66 @@ 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.  */
-
-  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);
+      ptid_t ptid (pid, 0, pthid);
+      status = pthdb_pthread_state (data->pd_session, pdtid, &state);
+      in_queue_threads.insert (pdtid);

-  /* 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 (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);
+       if (in_queue_threads.count (priv->pdtid) == 0
+               && in_thread_list (proc_target, it->ptid))
+         {
+           delete_thread (it);
+           data->exited_threads.insert (priv->pdtid);
+         }
+      }
 }

 /* Iterate_over_threads() callback for locating a thread, using
@@ -1013,8 +919,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 +1990,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 +2011,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 +2027,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);

diff --git a/gdb/testsuite/gdb.threads/thread_events.exp b/gdb/testsuite/gdb.threads/thread_events.exp
index 100205e5f58..ca51abeb545 100644
--- a/gdb/testsuite/gdb.threads/thread_events.exp
+++ b/gdb/testsuite/gdb.threads/thread_events.exp
@@ -21,11 +21,9 @@
 # messages are printed and can be disabled) is dependent on the target
 # thread support code.

-# This test has only been verified with Linux targets, and would need
+# This test has been verified with Linux targets and AIX target, and would need
 # to be generalized to support other targets
-if {![istarget *-*-linux*]} {
-    return
-}
+require {is_any_target "*-*-linux*" "*-*-aix*"}

 # When using the RSP, we don't get notifications about new threads.
 # This is expected, so don't test for that.
--
2.41.0