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

[Aditya Kamath1]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://sourceware.org/pipermail/gdb-patches/attachments/20240502/4fdb0dd9/attachment-0001.htm>