Re: RFA: nptl threading support patches

["J. Johnston" <jjohnstn at redhat dot com>]

Daniel Jacobowitz wrote:
> I can't approve any of this, but I have some comments.  First of all,
> this should be multiple patches, in at least a few trivial places. 
> Roughly one per paragraph below.

> Please don't post changes to generated files.  However, also please use
> the right versions of the tools to generate them!  You can not use
> autoconf 2.57 to generate the GDB configure script; the right version
> of autoconf 2.13 is available on sources.redhat.com.

Ok, I will resubmit in smaller patches and remove generated files.  I have to rethink
the scenario you mention whereby the main thread is the only unlocked thread
and exits under the Linuxthreads model.

> You did not mention testing.  I assume you tested with NPTL.  Did you
> test with LinuxThreads also?

Yes.  No additional Linuxthreads failures in gdb.threads, although killed.exp fails a
little uglier than it did before.  After looking at the exit problem, I will include the
testsuite results for gdb.threads for both nptl and linuxthreads in my resubmit.

> On Tue, Apr 22, 2003 at 11:10:08AM -0400, J. Johnston wrote:
>
> > The attached patch adds nptl threading support for gdb.
> >
> > There are a couple of key design changes to note with the nptl-enabled
> > kernel.  First of all, kill semantics have changed.  It is no longer
> > possible to send a signal to an lwp via kill.  Only a process id can be
> > used, rather than a thread lwp.  To keep the lin-lwp logic whereby SIGSTOP
> > is applied to a particular thread and some signals are pushed back, the
> > tkill syscall must be used.  The tkill syscall allows us to send a signal
> > to a particular thread.  A configuration check is made to see if we can
> > call syscall(__NR_tkill,..) and a runtime check is made to ensure that we 
> > can call
> > it successfully without getting ENOSYS.
>
>
> This is a separate patch, for instance.  It looks reasonable to me.
>
>
> > Another key behavior change regards thread exit.  For linuxthreads, exiting
> > causes a WIFEXITED event to occur at which point we can delete the current
> > thread and determine if any threads still exist.  In the new nptl model,
> > only the main thread generates this event and does so only once all the
> > threads have exited.  There is no event for the individual threads exiting
> > so we must check at various times to see if a thread has gone away.  For
> > example, when we stop all of the threads, this is a good time to see if
> > any have gone away.  When we get the exit signal, we check if we have the
> > main thread or not.  If it is the main thread, we have to stop all threads
> > (which will check whether they are alive or not), and then see how many
> > threads are left.  This logic works for either model.  If there are still
> > threads alive, we resume them and continue on without reporting the
> > exit.
>
>
> What was the verdict on whether this would break if I turned on exit
> events for every thread?  Sorry for not getting back to that kernel
> patch sooner, I'm way behind.  It looks like it would work.

Your patch won't affect this code as it was designed to handle either model, however,
a Red Hat Linux kernel has already shipped that has the behavior listed above.

> There are some subtleties.  I think that in your patch, the main thread
> reporting a WIFEXITED status will cause other threads to be resumed -
> even if they weren't running before, due to schedule locking.  I
> haven't tested that though.

As mentioned earlier, you raise a valid problem.  I have to look at this.

> > A change was made in thread-db.c to handle a FIXME that has been
> > there for a while.  When we get a create breakpoint in check_event, we
> > do not know what lwp has been created so we call td_ta_event_getmsg() to
> > get "a message" off the queue.  This message might not actually be the
> > message corresponding to the breakpoint that stopped us.  I have changed
> > the logic to perform a loop and read off all messages on the queue.  This
> > enables death event reporting to work if desired.  Without it, you can
> > get scenarios such as a death event preceding its create event, etc..
>
>
> This change looks reasonable to me.
>
>
> > The testsuite needed some changes because there is no longer a manager
> > thread with nptl so the testcase must account for either model when
> > doing info threads.  The schedlock testcase was changed because
> > nptl scheduling differs from linuxthreads.  One can't assume that
> > all threads will run equally in a small time slice, most notably when
> > stepping a particular thread.
>
>
> The changes to linux-dp.exp look reasonable to me.  That can be a patch
> all by itself.
>
> The changes to schedlock.exp look reasonable, and may fix the false
> failures it produces on LinuxThreads already depending on your luck
> with the scheduler.  It weakens the test - ideally it was supposed to
> make sure that GDB resumed every thread - but at this point I'll settle
> for resuming at least one other thread.  So that's a separate good
> patch too.
>
>
> > It is a lot to digest.  Let me know if/when it is ok to commit.
> >
> > -- Jeff J.
> >
> > 2003-04-17  Jeff Johnston  <jjohnstn at redhat dot com>
> >
> > 	* acconfig.h: Add HAVE_TKILL_SYSCALL definition check.
> > 	* config.in: HAVE_TKILL_SYSCALL and HAVE_SYSCALL checks added.
> > 	* configure.in: Add test for syscall function and check for
> > 	__NR_tkill macro in <syscall.h> to set HAVE_TKILL_SYSCALL.
> > 	* configure: Regenerated.
> > 	* thread-db.c (check_event): For create/death event breakpoints,
> > 	loop through all messages to ensure that we read the message
> > 	corresponding to the breakpoint we are at.
> > 	* lin-lwp.c [HAVE_TKILL_SYSCALL]: Include <unistd.h> and 
> > 	<sys/syscall.h>.
> > 	(kill_lwp): New function that uses tkill syscall or
> > 	uses kill, depending on whether threading model is nptl or not.
> > 	All callers of kill() changed to use kill_lwp().
> > 	(lin_lwp_wait): Make special check when WIFEXITED occurs to
> > 	see if all threads have already exited in the nptl model.
> > 	(stop_wait_callback): Check for threads already having exited and
> > 	delete such threads fromt the lwp list when discovered.
> > 	(stop_callback): Don't assert retcode of kill call.
> > 	* i386-linux-nat.c (ps_get_thread_area): New function needed by
> > 	nptl libthread_db.