Re: [patch] Fix for PR gdb/10757

[Paul Pluzhnikov <ppluzhnikov at google dot com>]

On Sun, Oct 25, 2009 at 4:54 PM, Pedro Alves <pedro@codesourcery.com> wrote:

> FTR, I've took a peek at nptl/nptl_db, to try to understand
> a bit better why this happens (please do correct me if I'm
> wrong):
>
>  - First, it is worth remind us while nptl takes locks and barriers
>   to protect its internal structures, thread_db/ntpl_db doesn't,
>   since it runs in gdb's context.

I wonder if it *could* take some lock that would make all other threads
wait in pthread_create -- after all, thread_db does have access to inferior
via ps_pdwrite. That would certainly make the interface cleaner, at a cost
of an extra global lock on pthread_create path.

Regardless of whether that's a good idea or not, I am stuck on glibc-2.3.6
for the time being, and even if this was to go into glibc-2.11, the "broken"
glibc versions will be with us for a long time, and GDB should be able
to deal with them.

>  - nptl adds new threads to one of the __stack_user or stack_used
>   lists.  New nodes are added to the _head_ of the lists.
>
>  - nptl_db/td_ta_thr_iter.c:td_ta_thr_iter iterates over
>   these thread lists from head to tail.
>
> From these last two points alone, one can infer that an iteration
> over the thread list can miss new threads if they're added just while
> the list is being iterated.

Right.

[While nptl_db is fresh in your mind, would you mind looking at gdb/10838
to double-check my diagnosis there?
http://sourceware.org/bugzilla/show_bug.cgi?id=10838

This is related to current patch only in that linux/libthread_db is
also involved.]

> Another way to tackle this, would be iterate over
> /proc/pid/task/ and attach to all lwps that way, instead of
> relying on thread_db to do the initial iteration over all
> threads.  We'd still iterate using the thread_db interface
> once afterwards, so to learn the matching pthread
> ids of the lwps.

I should note that /proc may not be mounted. I am not sure whether GDB
currently requires /proc in other places.

> With /proc/pid/task, you'd still need to keep iterating until
> no new thread comes out, but, I think we could make the
> number of loops unbounded much safely, since we wouldn't be
> at risk of reading stale inferior data.  Assuming no kernel
> bugs, that is.  Note that this is something that would be
> a useful addition to gdb anyway, so that we could be able to
> attach to multi-threaded apps that use raw clone instead of
> pthreads.

I'll put that on my TODO list :-)

>> +      /* Require NUM_LOOP successive iterations which do not find any new threads.  */
>
> This line too long.

Fixed.

>> +      for (i = 0, loop = 0; loop < 4; ++i, ++loop)
>
> Err, s/4/num_loop/ ?  A comment explain _why_ we need this is missing
> as well.  The patch looks OK otherwise.

Fixed.

> Actually, I'd rename the `num_loop' parameter into
> say, `until_no_new', and do this instead:

Good idea, done.

I'll commit attached patch tomorrow if there are no further comments.
A similar patch is required for gdbserver as well.


Thanks,
-- 
Paul Pluzhnikov

Attachment: gdb-pr10757-20091026.txt
Description: Text document