* Re: [RFC] MERGEPID macro wrong ?
2001-05-30 0:50 ` Pierre Muller
@ 2001-05-30 17:22 ` Kevin Buettner
0 siblings, 0 replies; 4+ messages in thread
From: Kevin Buettner @ 2001-05-30 17:22 UTC (permalink / raw)
To: Pierre Muller, gdb-patches
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 6437 bytes --]
On May 30, 9:45am, Pierre Muller wrote:
> At 20:49 29/05/01 , Kevin Buettner a écrit:
> >... I think it would be better to change the MERGEPID define to read
> >as follows:
> >
> >#define MERGEPID(PID,LWP) ptid_build(PID, LWP, 0)
> >
> >Now that we have ptid_t with separate pid, tid, and lwp components we
> >should able to clean up a lot of code which used to overload PIDs, LWPs
> >and TIDs onto an int.
>
> I think that would be indeed nicer,
> because when you do a grep MERGEPID * in gdb dir you get the following:
>
> defs.h:/* Provide default definitions of PIDGET, TIDGET, and MERGEPID.
> defs.h:#define MERGEPID(PID, TID) ptid_build (PID, TID, 0)
> gnu-nat.c: proc->inf->pid, pid_to_thread_id (MERGEPID (proc->tid,
> 0)));
The gnu-nat.c use of MERGEPID is a recent addition. I didn't comment on
it at the time, but for this sort of thing I prefer
pid_to_ptid (proc->tid)
instead of
MERGEPID (proc->tid, 0)
. It turns out that these expressions do the same thing. The reason
I prefer using pid_to_ptid in this case is because (IMHO) it's much
more obvious what's going on. Also, invoking MERGEPID where the second
argument is explicitly zero implies that the code in question knows
something about what zero means in this context. (I think that
knowledge should be elsewhere.)
> lynx-nat.c: inferior_ptid = MERGEPID (PIDGET (inferior_ptid), thread);
Unfortunately, I don't know enough about LynxOS to comment on this one. I
will point out however, that the ``thread'' argument is obtained from
the status argument passed to wait().
> proc-service.c:#define BUILD_LWP(tid, pid) MERGEPID (pid, tid)
In this case, the ``tid'' parameter refers to an lwp id.
> procfs.c: return MERGEPID (pi->pid, proc_get_current_thread (pi));
> procfs.c: retval = MERGEPID (pi->pid, proc_get_current_thread
> (pi));
> procfs.c: temp_ptid = MERGEPID (pi->pid, temp_tid);
> procfs.c: temp_ptid = MERGEPID (pi->pid, temp_tid);
> procfs.c: inferior_ptid = MERGEPID (pi->pid, proc_get_current_thread (pi));
> procfs.c: ptid_t gdb_threadid = MERGEPID (pi->pid, thread->tid);
In each of the above uses in procfs.c, the second argument to MERGEPID
is also an lwp id.
> As the distinction between tid and lwp is still rather obsucre to me,
In some settings tids and lwps are identical. In others, threads are
implemented over lwps (kernel threads) in which there may be multiple
threads per lwp. It's also possible that there are a pool of lwps to
which threads are allocated. (Which implies that threads could
migrate from one lwp to another depending upon the whims of the thread
library.) In any event, I usually think of a tid as something
associated with a user space thread library and an lwp as something
associated with the kernel implemented facilities over which the user
space thread library is implemented. The book "Unix Internals: The
New Frontiers" by Uresh Vahalla has a very nice treatment of the matter.
> I am just wondering if all the above uses of MERGEPID
> are completely aware of this subtility!
> The BUILD_LWP macro seems to be a nice example showing that.
>
> Coming back to the ptid_t structure,
> I still wonder how threads and processes are handled...
Until recently, a tid and an lwp id occupied the same set of bits and
used a one bit flag to distinguish between the two. When I added
struct ptid, I decided that it would be better to use a different set
of bits for the lwp id and the tid. I don't think there's any code in
gdb which actually makes use of this yet, but it seems to me that
some of GDB's thread related code could be cleaned up if it were to
make use of both of these fields simultaneously. E.g, in
thread-db.c, there are quite a few calls to thread_from_lwp() and
lwp_from_thread(). I believe that most of these exist because
previously it was not possible to simultaneously store both the thread
id and lwp id in the same identifier. It seems to me that a lot of
code could be greatly simplified if we were to attempt to fully make
use of the fact that it is now possible to store both the tid and lwp
in a ptid_t.
E.g, here's what thread_db_resume() from thread-db.c looks like:
static void
thread_db_resume (ptid_t ptid, int step, enum target_signal signo)
{
struct cleanup *old_chain = save_inferior_ptid ();
if (GET_PID (ptid) == -1)
inferior_ptid = lwp_from_thread (inferior_ptid);
else if (is_thread (ptid))
ptid = lwp_from_thread (ptid);
target_beneath->to_resume (ptid, step, signo);
do_cleanups (old_chain);
}
This function starts off by invoking the cleanup machinery since
it is possible that thread_db_resume() modifies inferior_ptid:
struct cleanup *old_chain = save_inferior_ptid ();
It then checks for a magic -1 value that is supposed to indicate that
all threads should be resumed. Apparently though, this -1 value also
indicates to the lower level that the current inferior should be
treated specially. (See lin_lwp_resume() in lin-lwp.c.) Therefore,
it needs to convert inferior_ptid to something that the code in
lin-lwp.c can operate on. To do so, it invokes lwp_from_thread():
if (GET_PID (ptid) == -1)
inferior_ptid = lwp_from_thread (inferior_ptid);
If ptid didn't have the magic -1 pid component, it indicates that
just that thread should be resumed. But again, ptid might refer
to a thread instead of an lwp. So again, inferior_ptid needs to
be suitably adjusted so that it may be used by the lwp layer:
else if (is_thread (ptid))
ptid = lwp_from_thread (ptid);
Next, the real work of invoking the lwp layer takes place:
target_beneath->to_resume (ptid, step, signo);
Finally, the original value of inferior_ptid is restored:
do_cleanups (old_chain);
So... all but one of the above statements are involved with massaging
a ptid into a form that can be used by the lwp layer. Now that
ptid_t has separate lwp and tid components, this is a very silly
thing to do. Assuming that ptids were created with both of these
components set correctly, thread_db_resume() could be rewritten as
follows:
static void
thread_db_resume (ptid_t ptid, int step, enum target_signal signo)
{
target_beneath->to_resume (ptid, step, signo);
}
> Looking for instance in the win32-nat.c code, I see
> that each thread is considered as a different process, why ??
Someone else will have to answer this...
Kevin
^ permalink raw reply [flat|nested] 4+ messages in thread