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

[John Baldwin <jhb@FreeBSD.org>]

On 4/29/24 4:48 AM, Aditya Kamath1 wrote:
> This is an older version of the patch. Sorry for resending this. Kindly ignore the same. I have sent the latest version in another git send-email.

Please use -v <n> (e.g. '-v 2') with a new version of n each time you use git send-email
to send a new version of a patch.  This makes includes the 'n' value in the e-mail
subject making it easier to see which version is the latest version.
  
> Have a nice day ahead.
> 
> Thanks and regards,
> Aditya.
> 
> From: Aditya Vidyadhar Kamath <akamath996@gmail.com>
> Date: Monday, 29 April 2024 at 5:11 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>
> Subject: [EXTERNAL] [PATCH] 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 | 91 ++++++++++++++++++++++++++++++------------------
>   1 file changed, 57 insertions(+), 34 deletions(-)
> 
> diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
> index c70bd82bc24..5ccccf02879 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 <vector>
> 
>   #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 born events have been reported.  */
> +  int threads_reported = 0;
>   };
> 
>   /* Key to our per-inferior data.  */
> @@ -740,11 +744,9 @@ state2str (pthdb_state_t state)
>   /* qsort() comparison function for sorting pd_thread structs by pthid.  */
> 
>   static int
> -pcmp (const void *p1v, const void *p2v)
> +pcmp (struct pd_thread p1v, struct pd_thread 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;
> +  return p1v.pthid < p2v.pthid;
>   }
> 
>   /* ptid comparison function */
> @@ -823,7 +825,6 @@ 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;
> @@ -831,12 +832,13 @@ sync_threadlists (pid_t pid)
>     process_stratum_target *proc_target = current_inferior ()->process_target ();
>     struct aix_thread_variables *data;
>     data = get_thread_data_helper_for_pid (pid);
> +  std::vector<pd_thread> pbuf;
> +  struct pd_thread aix_pd_thread;
> 
>     /* 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)
>       {
> @@ -848,27 +850,36 @@ 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;
> +      aix_pd_thread.pdtid = pdtid;
> +      aix_pd_thread.pthid = pthid;
> +      pbuf.push_back (aix_pd_thread);
>         pcount++;
>       }
> 
> -  for (pi = 0; pi < pcount; pi++)
> +  for (auto it = pbuf.begin (); it != pbuf.end ();)
>       {
> -      status = pthdb_pthread_tid (data->pd_session, pbuf[pi].pdtid, &tid);
> +      status = pthdb_pthread_tid (data->pd_session, (*it).pdtid, &tid);
>         if (status != PTHDB_SUCCESS)
>           tid = PTHDB_INVALID_TID;
> -      pbuf[pi].tid = tid;
> -    }
> 
> -  qsort (pbuf, pcount, sizeof *pbuf, pcmp);
> +      (*it).tid = tid;
> +      pthdb_state_t state;
> +
> +      status = pthdb_pthread_state (data->pd_session, (*it).pdtid, &state);
> 
> +      /* If the thread is terminated and has not reported its existence
> +        or new thread event, then wait for it to report.  We want users
> +        to know threads were added and then delete. So first add them to
> +        pbuf and the next time sync_threadlists is called delete them since
> +        data->threads_reported is guarenteed to be greater than 0.  */
> +
> +      if (state == PST_TERM && data->threads_reported > 0)
> +       pbuf.erase (it);
> +      else
> +       ++it;
> +    }
> +
> +  std::sort (pbuf.begin (), pbuf.end (), pcmp);
>     /* Accumulate an array of GDB threads sorted by pid.  */
> 
>     /* gcount is GDB thread count and pcount is pthreadlib thread count.  */
> @@ -881,6 +892,7 @@ sync_threadlists (pid_t pid)
>       *g++ = tp;
>     qsort (gbuf, gcount, sizeof *gbuf, gcmp);
> 
> +  pcount = pbuf.size ();
>     /* Apply differences between the two arrays to GDB's thread list.  */
> 
>     for (pi = gi = 0; pi < pcount || gi < gcount;)
> @@ -889,6 +901,12 @@ sync_threadlists (pid_t pid)
>           {
>             delete_thread (gbuf[gi]);
>             gi++;
> +         data->threads_reported--;
> +
> +         /* Since main thread is reported.  */
> +
> +         if (data->threads_reported == 0)
> +           data->threads_reported = 1;
>           }
>         else if (gi == gcount)
>           {
> @@ -901,6 +919,7 @@ sync_threadlists (pid_t pid)
>                                            private_thread_info_up (priv));
> 
>             pi++;
> +         data->threads_reported++;
>           }
>         else
>           {
> @@ -913,7 +932,6 @@ sync_threadlists (pid_t pid)
>             tid = pbuf[pi].tid;
> 
>             cmp_result = ptid_cmp (pptid, gptid);
> -
>             if (cmp_result == 0)
>               {
>                 aix_thread_info *priv = get_aix_thread_info (gbuf[gi]);
> @@ -943,6 +961,10 @@ sync_threadlists (pid_t pid)
>                   {
>                     delete_thread (gbuf[gi]);
>                     gi++;
> +                 data->threads_reported--;
> +
> +                 if (data->threads_reported == 0)
> +                   data->threads_reported = 1;
>                   }
>               }
>             else
> @@ -954,11 +976,11 @@ sync_threadlists (pid_t pid)
>                 priv->pdtid = pdtid;
>                 priv->tid = tid;
>                 pi++;
> +             data->threads_reported++;
>               }
>           }
>       }
> 
> -  xfree (pbuf);
>     xfree (gbuf);
>   }
> 
> @@ -2084,10 +2106,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 +2127,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 +2143,28 @@ 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);
>     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

-- 
John Baldwin