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 Date: Friday, 3 May 2024 at 5:12 PM To: akamath996@gmail.com , Aditya Kamath1 , tom@tromey.com , jhb@FreeBSD.org Cc: gdb-patches@sourceware.org , Sangamesh Mallayya Subject: Re: [PATCH v3] [RFC] Fix AIX thread exit events not being reported and UI to show kernel thread ID. John Baldwin > 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