Re: [PATCH] 0001-Fix-multi-thread-debug-bug-in-AIX.patch

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

Aditya Kamath1 <Aditya.Kamath1@ibm.com> wrote:

>Before we had an extra thread 'process 100' apart from 'thread 1'.
>So, in case someone interrupted a thread with ctrl+c.. In the
>pd_update () even if we don't have thread who signalled this interrupt
>when we return ptid_t (pid) it was fine. But now with no 'process 100'
>and only 'thread 1', we need to take care of interrupt as well,
>otherwise GDB core will take ptid_t(100) as a new process.
>So the change
>+      if (thrinf.ti_cursig == SIGTRAP || thrinf.ti_cursig == SIGINT)

Hmm.  So when "wait" returns, it needs to determine which thread
triggered the event that caused ptrace to stop.  On Linux, "wait"
will actually return the LWP of that thread, so it can be directly
used.  It seems on AIX, "wait" only returns a PID, and you do not
immediately know which thread caused the event?

In that case, I can see why you'd have to consider SIGINT as well
as SIGTRAP. However, it seems to me that even those two are not the
*only* cases that can cause "wait" to return - doesn't *any* signal
(potentially) trigger a ptrace intercept (causing wait to return)?

But that's probably a more general problem, and wouldn't occur in
this simple test case.

>In this case if we return a ptid_t (pid) then gdb core treats it
>as a new process since our parent thread is pthreaded GDB core
>only knows threads like ptid_t (pid, tid, pthid) and not
>ptid_t (pid). In order to avoid this, the proposed patch uses a
>function call find_the_return_ptid () to figure out the same.
>The changes like below are for this reason. 
>
>ourstatus->set_forked (ptid_t (pid));
>-             return ptid_t (parent_pid);
>+             return find_the_return_ptid (parent_pid);
>
>Kindly note that the control will always not come from aix-thread.c
>for such events. Hence, we cannot take care of the same there,
>though it will be a relief if we can do that. 

I'm not sure why it is necessary to handle this in the process layer
(rs6000-aix-nat.c) instead of the thread layer (aix-thread.c).
What specifically breaks if you do not have these rs6000-aix-nat.c
changes?

If you *do* need to handle LWPs (kernel thread IDs) in the process
layer (this can be a reasonable choice, and it done by several other
native targets), then it should be *consistent*, and *all* LWP handling
should be in the process layer. In particular, under no circumstances
does it make sense to duplicate the "find current/signalled thread"
code in *both* the process any thread layers.

>[Switching to process 16777620]

This outputs inferior_ptid ...

>0xd0595fb0 in _p_nsleep () from /usr/lib/libpthread.a(shr_xpg5.o)
>(gdb) info threads
>  Id   Target Id         Frame 
>* 1.1  process 16777620  0xd0595fb0 in _p_nsleep ()
>   from /usr/lib/libpthread.a(shr_xpg5.o)
>  1.2  process 16777620  0xd0595fb0 in _p_nsleep ()
>   from /usr/lib/libpthread.a(shr_xpg5.o)
>  1.3  process 16777620  0xd0595fb0 in _p_nsleep ()
>   from /usr/lib/libpthread.a(shr_xpg5.o)
>  2.1  process 8323570   0xd0594fc8 in ?? ()
>  3.1  process 17957172  0xd0594fc8 in ?? ()

... and this outputs the ptid values for those threads.

If it says "process ...", then those ptid values have not
properly been switched over to the (pid, lwp, tid) format.

You should verify that the sync_threadlists code handles
all multi-process cases correctly.  I haven't looked at
this in detail, but are you sure that here:

@@ -841,8 +829,22 @@ sync_threadlists (int pid)
 	    }
 	  else if (cmp_result > 0)
 	    {
-	      delete_thread (gbuf[gi]);
-	      gi++;
+	      if (gptid.is_pid ())
+		{
+		  thread_change_ptid (proc_target, gptid, pptid);

you never accidentally switch the *pid* part (if "gptid"
belows to a different pid than "pptid")?

Bye,
Ulrich