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

[Ulrich Weigand <Ulrich.Weigand@de.ibm.com>]

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

John Baldwin <jhb@FreeBSD.org<mailto:jhb@FreeBSD.org>> 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