diff mbox series

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

Message ID CH2PR15MB3544D487D375AAD3455FEC7AD6122@CH2PR15MB3544.namprd15.prod.outlook.com
State New
Headers
Series [v2] Fix AIX thread exit events not being reported and UI to show kernel thread ID. |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 fail Patch failed to apply
linaro-tcwg-bot/tcwg_gdb_build--master-arm fail Patch failed to apply

Commit Message

Aditya Kamath1 April 22, 2024, 2:33 p.m. UTC 
  Respected John, Tom, Ulrich, and community members,

Hi,

Thank you for the comments in the previous version of the patch.

>Next time can you please send the patch inline as a separate e-mail?  You can use a cover
>letter for the contents of this mail.  It is just easier to review in that case.

>One thing I think I'm not fully understanding: does a thread never get removed from the
>pth list once it has exited?  That is, is it on that list forever?  If so, I think
>you could simplify this quite a bit to remove the qsort and the gcount and associated
>list, etc.  Instead, I would keep track of exited threads whose exit has been reported
>via a new std::unordered_set<> class member in the target to avoid reporting duplicate
>events and then do something like:

Please see the patch pasted below this email. Thank you for this John. This makes it easy for us to maintain and keep track.

I have done a small clean-up of aix-thread.c to keep things simple.

Yes, a thread was never getting removed from pbuf. It was in it forever.

Output after applying this patch:-

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 19136989)]
[Thread 258 (tid 19136989) exited]
[New Thread 515 (tid 19464469)]
[Thread 515 (tid 19464469) exited]
[New Thread 772 (tid 34144651)]
[Thread 772 (tid 34144651) exited]
[New Thread 1029 (tid 25690595)]
[Thread 1029 (tid 25690595) exited]
[New Thread 1286 (tid 18547171)]
[Thread 1286 (tid 18547171) exited]
[New Thread 1543 (tid 32637297)]
[Thread 1543 (tid 32637297) exited]

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


Kindly let me know if this version is okay.

>> So, consider program 1 pasted below this email.

>Is there some existing gdb test case that is fixed by this patch?  If
>not then I think it would be better if the patch came with a new test.

Sure Tom. I will create another patch for this and send it soon.

For now I will attach the test program below.

Also, I have update the comments and removed the old ones.

>>I am planning to use
>>“status = pthdb_pthread_state (data->pd_session, pdtid, &state);”
>>
>>And if the state is PST_TERM then I want to remove the thread vector
>>from the pbuf list and make sure pcount is also reduced.
>>This can fix the issue.

>>This makes sense to me.

Thanks Ulrich. So as John suggested we will making use of unordered set and keeping track of exited threads using the state PST_TERM.

Have a nice day ahead.

Thanks and regards,
Aditya.

The patch:-

# cat patches/0001-Fix-AIX-thread-exit-events-not-being-reported-and-UI.patch
From ae736fe495c279d42e65f936bce0f9dab5ec703b Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath Aditya.Kamath1@ibm.com<mailto:Aditya.Kamath1@ibm.com>
Date: Mon, 22 Apr 2024 09:16:56 -0500
Subject: [PATCH] Fix AIX thread exit events not being reported and UI to show
 kernel thread ID.

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 | 217 ++++++++++++-----------------------------------
1 file changed, 52 insertions(+), 165 deletions(-)

--
2.41.0
Test 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;
}
diff mbox series

Patch

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

   /* 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 +805,44 @@  sync_threadlists (pid_t pid)
       if (status != PTHDB_SUCCESS || pthid == PTHDB_INVALID_PTID)
        continue;

-      if (pcount == psize)
+      ptid_t ptid (pid, 0, pthid);
+      status = pthdb_pthread_state (data->pd_session, pdtid, &state);
+      if (state == PST_TERM)
        {
-         psize *= 2;
-         pbuf = (struct pd_thread *) xrealloc (pbuf,
-                                               psize * sizeof *pbuf);
+         if (data->exited_threads.count (pdtid) != 0)
+            continue;
        }
-      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);

-  /* Apply differences between the two arrays to GDB's thread list.  */
-
-  for (pi = gi = 0; pi < pcount || gi < gcount;)
-    {
-      if (pi == pcount)
-       {
-         delete_thread (gbuf[gi]);
-         gi++;
-       }
-      else if (gi == gcount)
+      /* If this thread has never been reported to GDB, add it.  */
+      if (!in_thread_list (proc_target, ptid))
        {
          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
-       {
-         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)
+         /* 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++;
-           }
+      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);
}

/* Iterate_over_threads() callback for locating a thread, using
@@ -2084,10 +1967,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);
+
+  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 string_printf (_("Thread %s"), pulongest (ptid.tid ()));
+  return beneath ()->pid_to_str (ptid);
}

/* Return a printable representation of extra information about
@@ -2098,7 +1988,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 +2004,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);