[PATCH RFA/RFC] Don't use lwp_from_thread() in thread_db_wait()

[PATCH RFA/RFC] Don't use lwp_from_thread() in thread_db_wait()

[Kevin Buettner]

I'm seeing the following failure when I run the gdb testsuite on an
SMP machine (GNU/Linux/x86):

FAIL: gdb.threads/pthreads.exp: continue to bkpt at common_routine in thread 2

Here's the relevant bit from the log file:

break common_routine thread 4
Breakpoint 6 at 0x804864e: file /saguaro1/intelp4-011128-branch/devo/gdb/testsuite/gdb.threads/pthreads.c, line 50.
(gdb) PASS: gdb.threads/pthreads.exp: set break at common_routine in thread 2
continue
Continuing.
Cannot find thread 1024: generic error
[followed by the FAIL message]

Now, consider the following portion of gdb's stack when this happens:

(top-gdb) bt
#0  ps_xfer_memory (ph=0x8392f80, addr=1073962208, buf=0xbfffef40 "", len=16, 
    write=0) at ../../devo/gdb/proc-service.c:81
#1  0x08111a36 in ps_pdread (ph=0x8392f80, addr=1073962208, buf=0xbfffef40, 
    size=16) at ../../devo/gdb/proc-service.c:194
#2  0x402aa3d9 in td_ta_map_id2thr (ta=0x83cde40, pt=1024, th=0xbfffef88)
    at td_ta_map_id2thr.c:41
#3  0x08112086 in lwp_from_thread (ptid={pid = 22395, lwp = 0, tid = 1024})
    at ../../devo/gdb/thread-db.c:261
#4  0x08112e20 in thread_db_wait (ptid={pid = 22395, lwp = 0, tid = 1024}, 
    ourstatus=0xbffff150) at ../../devo/gdb/thread-db.c:720
#5  0x080acbde in wait_for_inferior () at ../../devo/gdb/infrun.c:1246
#6  0x080ac915 in proceed (addr=4294967295, siggnal=TARGET_SIGNAL_DEFAULT, 
    step=0) at ../../devo/gdb/infrun.c:1045
#7  0x080a98b4 in continue_command (proc_count_exp=0x0, from_tty=1)
    at ../../devo/gdb/infcmd.c:536

As I see it, the problem is as follows...

thread_db_wait() wants to learn the lwp id of the thread that it
should wait for so that it can ask the lwp layer to wait on the lwp
corresponding to the thread in question.  In order to do this, it
calls lwp_from_thread().  lwp_from_thread needs help from the
libthread_db.so to figure this out, so it calls td_ta_map_id2thr(). 
BUT, this libthread_db function must interrogate the inferior
process's memory to look at the thread data structures.  To do this,
it calls back into gdb, using ps_pdread() to fetch the memory in
question.  Eventually, on Linux, ptrace() gets called to actually
fetch the memory.

The Linux/i386 (kernel) ptrace code contains the following check:

	ret = -ESRCH;
	if (!(child->ptrace & PT_PTRACED))
		goto out_tsk;
	if (child->state != TASK_STOPPED) {
		if (request != PTRACE_KILL)
			goto out_tsk;
	}

This says that ESRCH will be returned if the child process is not
being traced.  (Not relevant.)  It ALSO says to return ESRCH if the
process is not stopped.  This is of critical importance.

In the above trace, we are wanting to wait for the main thread to
stop, but in order to find out the necessary information so that we
can do this, the main thread must first be stopped!

The patch below fixes this problem for me and shows no regressions
in the testsuite.

I considered a number of other less palatable solutions.  One of them
involved implementing a linux/x86 specific version of
child_xfer_memory() which would (attempt to) explicitly stop the
process if an error occurred in attempting to do a memory read.  The
problem with this is that once stopped, what do we do with it?  Start
it again?  I'm sure that something could be worked out, but I studied
lin-lwp.c which had a fair amount of this kind of hair in it already,
and it scared me enough to opt for a simpler solution.  FWIW, the now
defunct lin-thread.c had #if 0'd out the corresponding bit of code
that I chose to disable in thread-db.c.  I'm guessing that the older
thread implementation had run into the same kind of problem in the
past.

Comments?  Okay to commit?

	* thread-db.c (thread_db_wait): Don't attempt to use
	lwp_from_thread().  Doing so assumes that the main thread
	is already stopped and this might not be the case.  Instead,
	simply wait for any thread.

Index: thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/thread-db.c,v
retrieving revision 1.21
diff -u -p -r1.21 thread-db.c
--- thread-db.c	2002/02/24 21:53:02	1.21
+++ thread-db.c	2002/03/11 23:25:36
@@ -719,10 +719,31 @@ thread_db_wait (ptid_t ptid, struct targ
 {
   extern ptid_t trap_ptid;
 
-  if (GET_PID (ptid) != -1 && is_thread (ptid))
-    ptid = lwp_from_thread (ptid);
+  /* Note: kevinb/2002-03-11: We used to do the following here:
 
-  ptid = target_beneath->to_wait (ptid, ourstatus);
+	if (GET_PID (ptid) != -1 && is_thread (ptid))
+	  ptid = lwp_from_thread (ptid);
+
+	ptid = target_beneath->to_wait (ptid, ourstatus);
+
+     The problem with calling lwp_from_thread() at this point is that
+     the main thread is not necessarily stopped.  This is a problem
+     because lwp_from_thread() requires help from the thread_db to
+     obtain the thread to lwp mapping.  In order to perform this
+     operation, the thread_db library calls back into GDB to do a
+     memory read of the main thread.  On GNU/Linux, a memory read
+     is performed via ptrace(), which requires that the process be
+     stopped.  (ESRCH is returned otherwise.)  Even if it were
+     permissible to read the memory of a running process, it would
+     probably not be a good idea to rely on such results.
+     
+     So, instead of attempting to fetch the LWP id and invoke a
+     lower layer's target_wait() with a ptid constructed from this
+     LWP, we simply wait for any thread and let infrun.c's thread
+     hopping machinery sort out whether the desired thread has been
+     stopped or not.  */
+
+  ptid = target_beneath->to_wait (pid_to_ptid (-1), ourstatus);
 
   if (proc_handle.pid == 0)
     /* The current child process isn't the actual multi-threaded

Re: [PATCH RFA/RFC] Don't use lwp_from_thread() in thread_db_wait()

[Daniel Jacobowitz]

On Mon, Mar 11, 2002 at 04:45:54PM -0700, Kevin Buettner wrote:
> I'm seeing the following failure when I run the gdb testsuite on an
> SMP machine (GNU/Linux/x86):
> 
> FAIL: gdb.threads/pthreads.exp: continue to bkpt at common_routine in thread 2

Thank you for investigating this!  I've been seeing it somewhat
erratically for months, but not had any idea where to start.

> thread_db_wait() wants to learn the lwp id of the thread that it
> should wait for so that it can ask the lwp layer to wait on the lwp
> corresponding to the thread in question.  In order to do this, it
> calls lwp_from_thread().  lwp_from_thread needs help from the
> libthread_db.so to figure this out, so it calls td_ta_map_id2thr(). 
> BUT, this libthread_db function must interrogate the inferior
> process's memory to look at the thread data structures.  To do this,
> it calls back into gdb, using ps_pdread() to fetch the memory in
> question.  Eventually, on Linux, ptrace() gets called to actually
> fetch the memory.

There's another solution that I see.  At the top of thread-db is the
comment:

/* FIXME: There is certainly some room for improvements:
   - Cache LWP ids.

This would be a tremendous performance win, and fix this problem.  But
that may be a slightly longer term solution.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [PATCH RFA/RFC] Don't use lwp_from_thread() in thread_db_wait()

[Kevin Buettner]

On Mar 11,  9:47pm, Daniel Jacobowitz wrote:

> On Mon, Mar 11, 2002 at 04:45:54PM -0700, Kevin Buettner wrote:
> > I'm seeing the following failure when I run the gdb testsuite on an
> > SMP machine (GNU/Linux/x86):
> > 
> > FAIL: gdb.threads/pthreads.exp: continue to bkpt at common_routine in thread 2
> 
> Thank you for investigating this!  I've been seeing it somewhat
> erratically for months, but not had any idea where to start.
> 
> > thread_db_wait() wants to learn the lwp id of the thread that it
> > should wait for so that it can ask the lwp layer to wait on the lwp
> > corresponding to the thread in question.  In order to do this, it
> > calls lwp_from_thread().  lwp_from_thread needs help from the
> > libthread_db.so to figure this out, so it calls td_ta_map_id2thr(). 
> > BUT, this libthread_db function must interrogate the inferior
> > process's memory to look at the thread data structures.  To do this,
> > it calls back into gdb, using ps_pdread() to fetch the memory in
> > question.  Eventually, on Linux, ptrace() gets called to actually
> > fetch the memory.
> 
> There's another solution that I see.  At the top of thread-db is the
> comment:
> 
> /* FIXME: There is certainly some room for improvements:
>    - Cache LWP ids.
> 
> This would be a tremendous performance win, and fix this problem.  But
> that may be a slightly longer term solution.

I think that an LWP id cache is only useful so long as all of the
threads are stopped.  This is because the mappings could change in the
course of running the program.  So, for this particular case, where
the threads are running and we want to wait for one of them to stop,
the cache wouldn't be useful to us.

Of course, if we have knowledge that a particular thread
implementation never changes its mappings or perhaps only changes its
mappings for certain threads, we might be able to use such a cache
across the stop/start transitions.  However, I think that Mark had
intended for thread-db.c to be a fairly generic solution that's not
wedded to any one particular thread implementation.  In particular, it
should be possible to use it with an M:N model in which a thread may
migrate from one LWP to another.

That said, the LWP<->TID mapping operations are farily expensive
(since they involve fairly sizable target memory reads), and I agree
that an LWP cache would be beneficial even if it needs to be
invalidated when the program starts again.

Kevin

Re: [PATCH RFA/RFC] Don't use lwp_from_thread() in thread_db_wait()

[Daniel Jacobowitz]

On Mon, Mar 11, 2002 at 08:16:19PM -0700, Kevin Buettner wrote:
> I think that an LWP id cache is only useful so long as all of the
> threads are stopped.  This is because the mappings could change in the
> course of running the program.  So, for this particular case, where
> the threads are running and we want to wait for one of them to stop,
> the cache wouldn't be useful to us.
> 
> Of course, if we have knowledge that a particular thread
> implementation never changes its mappings or perhaps only changes its
> mappings for certain threads, we might be able to use such a cache
> across the stop/start transitions.  However, I think that Mark had
> intended for thread-db.c to be a fairly generic solution that's not
> wedded to any one particular thread implementation.  In particular, it
> should be possible to use it with an M:N model in which a thread may
> migrate from one LWP to another.

This implies that part of the caching should be in lin-lwp.c rather
than in thread-db.c... that knowledge belongs with the lower level
threading layer.  Does that make sense?

We could also, for instance, update the cache via thread event
reporting...

> That said, the LWP<->TID mapping operations are farily expensive
> (since they involve fairly sizable target memory reads), and I agree
> that an LWP cache would be beneficial even if it needs to be
> invalidated when the program starts again.

Definitely.  I profiled this segment of GDB once not long ago; this and
the is_alive checks dominated the profile.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [PATCH RFA/RFC] Don't use lwp_from_thread() in thread_db_wait()

[Kevin Buettner]

On Mar 11, 10:23pm, Daniel Jacobowitz wrote:

> On Mon, Mar 11, 2002 at 08:16:19PM -0700, Kevin Buettner wrote:
> > I think that an LWP id cache is only useful so long as all of the
> > threads are stopped.  This is because the mappings could change in the
> > course of running the program.  So, for this particular case, where
> > the threads are running and we want to wait for one of them to stop,
> > the cache wouldn't be useful to us.
> > 
> > Of course, if we have knowledge that a particular thread
> > implementation never changes its mappings or perhaps only changes its
> > mappings for certain threads, we might be able to use such a cache
> > across the stop/start transitions.  However, I think that Mark had
> > intended for thread-db.c to be a fairly generic solution that's not
> > wedded to any one particular thread implementation.  In particular, it
> > should be possible to use it with an M:N model in which a thread may
> > migrate from one LWP to another.
> 
> This implies that part of the caching should be in lin-lwp.c rather
> than in thread-db.c... that knowledge belongs with the lower level
> threading layer.  Does that make sense?

I think I see what you're driving at, though I don't think it belongs
in lin-lwp.c.  lin-lwp.c should, I hope, be usable as is by a number
of different thread implementations.  Instead, I think what you have
in mind should reside in some sort of policy adjuct to thread-db.c
which understands the kinds of relationships that can exist between
thread ids and lwp ids.  If it knows that the thread implementation
uses a 1:1 model as linuxthreads does now, it can use agressive
caching.  (By which I mean that the cache is allowed to persist
between stops in the debugger).  If it uses a M:N model, it must cache
more conservatively.  (I.e, the cache must be invalidated whenever the
inferior is resumed.)  I think this code could be reasonably generic
and it shouldn't be too hard to implement.  The difficult part will be
to figure out which kind of thread library you have.  After all, if
someone provided a dropin replacement for linuxthreads which
implemented M:N threading, how would you tell the difference?

> We could also, for instance, update the cache via thread event
> reporting...

If the thread events tell GDB when a thread has migrated from one
LWP to another, then this would work too.

...

But, for the problem at hand (i.e, the bug that my patch is intended
to fix), I think it's important that we first make it work without
caching.  As I see it, the cache ought to exist to enhance
performance, not guarantee basic correctness.  If we can't make it
work without some sort of caching or enhanced thread event reporting,
we need to understand exactly why first.

Kevin

Re: [PATCH RFA/RFC] Don't use lwp_from_thread() in thread_db_wait()

[Daniel Jacobowitz]

On Tue, Mar 12, 2002 at 12:52:16AM -0700, Kevin Buettner wrote:
> On Mar 11, 10:23pm, Daniel Jacobowitz wrote:
> 
> > On Mon, Mar 11, 2002 at 08:16:19PM -0700, Kevin Buettner wrote:
> > > I think that an LWP id cache is only useful so long as all of the
> > > threads are stopped.  This is because the mappings could change in the
> > > course of running the program.  So, for this particular case, where
> > > the threads are running and we want to wait for one of them to stop,
> > > the cache wouldn't be useful to us.
> > > 
> > > Of course, if we have knowledge that a particular thread
> > > implementation never changes its mappings or perhaps only changes its
> > > mappings for certain threads, we might be able to use such a cache
> > > across the stop/start transitions.  However, I think that Mark had
> > > intended for thread-db.c to be a fairly generic solution that's not
> > > wedded to any one particular thread implementation.  In particular, it
> > > should be possible to use it with an M:N model in which a thread may
> > > migrate from one LWP to another.
> > 
> > This implies that part of the caching should be in lin-lwp.c rather
> > than in thread-db.c... that knowledge belongs with the lower level
> > threading layer.  Does that make sense?
> 
> I think I see what you're driving at, though I don't think it belongs
> in lin-lwp.c.  lin-lwp.c should, I hope, be usable as is by a number
> of different thread implementations.  Instead, I think what you have
> in mind should reside in some sort of policy adjuct to thread-db.c
> which understands the kinds of relationships that can exist between
> thread ids and lwp ids.  If it knows that the thread implementation
> uses a 1:1 model as linuxthreads does now, it can use agressive
> caching.  (By which I mean that the cache is allowed to persist
> between stops in the debugger).  If it uses a M:N model, it must cache
> more conservatively.  (I.e, the cache must be invalidated whenever the
> inferior is resumed.)  I think this code could be reasonably generic
> and it shouldn't be too hard to implement.  The difficult part will be

I like this a lot.

> to figure out which kind of thread library you have.  After all, if
> someone provided a dropin replacement for linuxthreads which
> implemented M:N threading, how would you tell the difference?

... great care, and maybe a ``set'' option to override?  Unfortunately,
IBM's ngpt seems to be mostly drop-in, barring some symbol versioning
complexity.  We can probably find a way to distinguish...  what's worse
is that NGPT can build as a libpthread.so, but not a libthread_db.so,
so we may have to handle mismatches :(

> > We could also, for instance, update the cache via thread event
> > reporting...
> 
> If the thread events tell GDB when a thread has migrated from one
> LWP to another, then this would work too.

Yes, that could probably be arranged.  Someday we should talk to a
vendor of an M:N threads package and see what we have to work with.  I
don't know of any offhand besides NGPT.

> ...
> 
> But, for the problem at hand (i.e, the bug that my patch is intended
> to fix), I think it's important that we first make it work without
> caching.  As I see it, the cache ought to exist to enhance
> performance, not guarantee basic correctness.  If we can't make it
> work without some sort of caching or enhanced thread event reporting,
> we need to understand exactly why first.

I agree.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [PATCH RFA/RFC] Don't use lwp_from_thread() in thread_db_wait()

[David Taylor]

> Date: Tue, 12 Mar 2002 11:23:31 -0500
> From: Daniel Jacobowitz <drow@mvista.com>

> Yes, that could probably be arranged.  Someday we should talk to a
> vendor of an M:N threads package and see what we have to work with.  I
> don't know of any offhand besides NGPT.

If I understand you correctly, then: Solaris.

Re: [PATCH RFA/RFC] Don't use lwp_from_thread() in thread_db_wait()

[Daniel Jacobowitz]

On Wed, Mar 13, 2002 at 12:37:08PM -0500, David Taylor wrote:
> > Date: Tue, 12 Mar 2002 11:23:31 -0500
> > From: Daniel Jacobowitz <drow@mvista.com>
> 
> > Yes, that could probably be arranged.  Someday we should talk to a
> > vendor of an M:N threads package and see what we have to work with.  I
> > don't know of any offhand besides NGPT.
> 
> If I understand you correctly, then: Solaris.

Is it really?

To clarify, LinuxThreads has one thread per process; IBM's NGPT has
multiple threads per process, but still multiple processes.  I was
under the impression that Solaris LWPs would have all threads in one
process.

(except of course the terminology gets fuzzy here.  One "process" in
Solaris includes multiple LWPs which can be executing at the same time. 
If my understanding above is correct it might be more appropriate to
call Solaris one-thread-per-LWP).

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [PATCH RFA/RFC] Don't use lwp_from_thread() in thread_db_wait()

[Andrew Cagney]

> On Wed, Mar 13, 2002 at 12:37:08PM -0500, David Taylor wrote:
> 
>> > Date: Tue, 12 Mar 2002 11:23:31 -0500
>> > From: Daniel Jacobowitz <drow@mvista.com>
> 
>> 
> 
>> > Yes, that could probably be arranged.  Someday we should talk to a
>> > vendor of an M:N threads package and see what we have to work with.  I
>> > don't know of any offhand besides NGPT.
> 
>> 
>> If I understand you correctly, then: Solaris.
> 
> 
> Is it really?

Yep.

> To clarify, LinuxThreads has one thread per process; IBM's NGPT has
> multiple threads per process, but still multiple processes.  I was
> under the impression that Solaris LWPs would have all threads in one
> process.

That was their first attempt (if I remember right).

> (except of course the terminology gets fuzzy here.  One "process" in
> Solaris includes multiple LWPs which can be executing at the same time. 
> If my understanding above is correct it might be more appropriate to
> call Solaris one-thread-per-LWP).

enjoy,
Andrew

Re: [PATCH RFA/RFC] Don't use lwp_from_thread() in thread_db_wait()

[David Taylor]

> Date: Wed, 13 Mar 2002 12:46:32 -0500
> From: Daniel Jacobowitz <drow@mvista.com>
> 
> On Wed, Mar 13, 2002 at 12:37:08PM -0500, David Taylor wrote:
> > > Date: Tue, 12 Mar 2002 11:23:31 -0500
> > > From: Daniel Jacobowitz <drow@mvista.com>
> > 
> > > Yes, that could probably be arranged.  Someday we should talk to a
> > > vendor of an M:N threads package and see what we have to work with.  I
> > > don't know of any offhand besides NGPT.
> > 
> > If I understand you correctly, then: Solaris.
> 
> Is it really?
> 
> To clarify, LinuxThreads has one thread per process; IBM's NGPT has
> multiple threads per process, but still multiple processes.  I was
> under the impression that Solaris LWPs would have all threads in one
> process.
> 
> (except of course the terminology gets fuzzy here.  One "process" in
> Solaris includes multiple LWPs which can be executing at the same time. 
> If my understanding above is correct it might be more appropriate to
> call Solaris one-thread-per-LWP).

Perhaps I misunderstood.  Or perhaps it's just a terminology issue.

Solaris >= 2.5.1 has a two level threads implementation --

	. threads, sometimes called user threads
	. light weight processes, sometimes called kernel threads

They are all part of one process.  You can have M user threads mapped
onto N LWPs.

There are two sorts of signals in Solaris -- synchronous (some LWP
executing some thread caused the signal -- e.g., SIGILL, SIGSEGV) and
asynchronous (e.g., SIGINT, SIGQUIT, SIGSTOP, SIGKILL).  Synchronous
signals are delivered to the thread/lwp that caused the signal;
asynchronous signals are delivered to the asychronous signal LWP
(ASLWP), arbitrary thread.

Re: [PATCH RFA/RFC] Don't use lwp_from_thread() in thread_db_wait()

[Daniel Jacobowitz]

On Mon, Mar 11, 2002 at 04:45:54PM -0700, Kevin Buettner wrote:
> I'm seeing the following failure when I run the gdb testsuite on an
> SMP machine (GNU/Linux/x86):
> 
> FAIL: gdb.threads/pthreads.exp: continue to bkpt at common_routine in thread 2

[snip great analysis]

> Comments?  Okay to commit?
> 
> 	* thread-db.c (thread_db_wait): Don't attempt to use
> 	lwp_from_thread().  Doing so assumes that the main thread
> 	is already stopped and this might not be the case.  Instead,
> 	simply wait for any thread.

Has anyone had a chance to look at this patch?

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [PATCH RFA/RFC] Don't use lwp_from_thread() in thread_db_wait()

[Michael Snyder]

Daniel Jacobowitz wrote:
> 
> On Mon, Mar 11, 2002 at 04:45:54PM -0700, Kevin Buettner wrote:
> > I'm seeing the following failure when I run the gdb testsuite on an
> > SMP machine (GNU/Linux/x86):
> >
> > FAIL: gdb.threads/pthreads.exp: continue to bkpt at common_routine in thread 2
> 
> [snip great analysis]
> 
> > Comments?  Okay to commit?
> >
> >       * thread-db.c (thread_db_wait): Don't attempt to use
> >       lwp_from_thread().  Doing so assumes that the main thread
> >       is already stopped and this might not be the case.  Instead,
> >       simply wait for any thread.
> 
> Has anyone had a chance to look at this patch?

No, I've been a schmuck -- I promised Kevin I would look at it, and I
haven't.