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

[Aditya Kamath1 <Aditya.Kamath1@ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 9278 bytes --]

Hi Ulrich and community,

Please find the new patch [See:- 0001-Fix-multi-thread-bug-in-AIX.patch ].

I understood your previous email and what you are saying is correct. If we fix this top target, we can leave the process layer undisturbed.

Having said that, I have a few obstacles I am facing in order to achieve the same. Kindly not all outputs I paste in this mail are generated with "set debug aix-thread" command and "set detach-on-fork off" command.

We first try to get the symbol name where we need to attach a trap so that debugger can get notified that Hey you need to catch an event. This will be a thread create event in our case will be caught in a condition in aix-thread::wait layer and then we call pd_update () to sync_threadlists () to catch it. For this to happen the main thing is the symbol called "n_pthreads" needs to have an address in the symbol table. This symbol is checked when a new object file is generated via the pd​​_enable () where we use pthdb_session​_pthreaded () to check the same. If we are successful, we get into pd_update () and do our stuff plus push the top target as aix-thread.c..

So, all this happens correctly for program 1's parent {code attached below} as shown in the output below.

Starting program: /home/aditya/gdb_tests/ultimate-multi-thread-fork

pdc_symbol_addrs (user_current_pid = 17957132, symbols = 0xfffffffffffdbc8, count = 1)

  symbols[0].name = "__n_pthreads"

 returning PDC_FAILURE

pdc_symbol_addrs (user_current_pid = 17957132, symbols = 0xfffffffffffdbc8, count = 1)

  symbols[0].name = "__n_pthreads"

  symbols[0].addr = 0xf0807334

 returning PDC_SUCCESS

pdc_read_data (user_current_pid = 17957132, buf = 0xfffffffffffdbc0, addr = 0xf0807334, len = 4)

  status=0, returning SUCCESS

pdc_symbol_addrs (user_current_pid = 17957132, symbols = 0xfffffffffff88c8, count = 1)

  symbols[0].name = "__n_pthreads"

  symbols[0].addr = 0xf0807334



So, after this the first inferior works fine. When the second or the third inferior comes into picture from the new objfile () we to go pd_enable () then to pthdb_session​_pthreaded () .. Here we fail for the new inferior as shown the output below.


[New Thread 258]

[New Thread 515]

fetch_regs_kernel_thread tid=225018d regno=64 arch64=0

[New inferior 2 (process 8061286)]

pdc_free (user_current_pid = 17957132, buf = 0x11016f370)

pdc_free (user_current_pid = 17957132, buf = 0x11016f3b0)

pdc_free (user_current_pid = 17957132, buf = 0x11016f4f0)

pdc_free (user_current_pid = 17957132, buf = 0x1104e3a70)

pdc_free (user_current_pid = 17957132, buf = 0x1108af0d0)

pdc_symbol_addrs (user_current_pid = 17957132, symbols = 0xfffffffffffdef8, count = 1)

  symbols[0].name = "__n_pthreads"

 returning PDC_FAILURE

pdc_symbol_addrs (user_current_pid = 8061286, symbols = 0xfffffffffffe248, count = 1)

  symbols[0].name = "__n_pthreads"

 returning PDC_FAILURE

I am parent

[New process 17957132]

[New inferior 3 (process 17433000)]

pdc_symbol_addrs (user_current_pid = 17957132, symbols = 0xfffffffffffdef8, count = 1)

  symbols[0].name = "__n_pthreads"

 returning PDC_FAILURE

Since it could not read the symbol "_n_pthreads" it failed, and we could not set our top target for the new process as threads. So, I could not find why this happens. Because if the parent is pthreaded so will be the child as everything of the parent must be copied to the child. So, I should get my child also as pthreaded and "_n_pthread" symbol set to the address of the child's threads in the child process.

Thus, our top target remained as process layer. In target.c when our event is going to wait, our current inferior is the child, and its top target is process layer. In the process layer though it recognised the process correctly since our parent is threaded, we do not have ptid_t (pid) for it. Hence the line [New process 17957132] appeared in the output.

I did try doing searching in xcoffread.c but I felt I was in the wrong place searching for things which pthread debug library should define for us.

This is where I need guidance. Your help can be useful to solve this problem for AIX and the GDB community. Kindly guide me with your expertise and let me know what you think. I have given all the information possible of my understanding till here. Let me know if you need more information to guide me.

Waiting for a reply soon.

Have a nice day ahead.

Regards,
Aditya.

-----------------------------------
PROGRAM 1


#include <stdio.h>

#include <unistd.h>

#include <stdlib.h>

#include <pthread.h>

#include <assert.h>


pthread_barrier_t barrier;


#define NUM_THREADS 2


void *

dummy_thread_function (void *arg)

{

   printf ("Bye from dummy thread \n");

}


void *

thread_function (void *arg)

{

  /* This ensures that the breakpoint is only hit after both threads

     are created, so the test can always switch to the non-event

     thread when the breakpoint triggers.  */


  pthread_barrier_wait (&barrier);

  pid_t child;


  child = fork ();

  if (child > 0)

    printf ("I am parent \n");

  else{

    printf (" Iam child \n");

    child = fork ();

    if (child > 0)

      printf ("From child I became a parent \n");

    else

    {

      printf ("I am grandchild \n");

      pthread_t thread;

      pthread_create (&thread, NULL,

                      dummy_thread_function, NULL);

    }

  }

  while (1); /* break here */

}

int

main (void)

{

  int i;

  pthread_t thread[NUM_THREADS];


  alarm (300);


  pthread_barrier_init (&barrier, NULL, NUM_THREADS);


  for (i = 0; i < NUM_THREADS; i++)

    {

      int res;


      res = pthread_create (&thread[i], NULL,

                            thread_function, NULL);

      assert (res == 0);

    }


  while (1)

  {

    sleep (15);

  }


  return 0;

}











________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 08 December 2022 21:59
To: simark@simark.ca <simark@simark.ca>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: Re: [PATCH] 0001-Fix-multi-thread-debug-bug-in-AIX.patch

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

>>So this last bit seems to be the problem.  Could you elaborate on
>>what the exact call stack is?  I thought once the thread layer is
>>initialized, calls to ::wait should always go through it ...
>
>Kindly see the backtrace sections
>BT:- Thread_wait [which is on a thread event like new thread born or main process is pthreaded],
>BT:- Post thread wait in rs6000-aix-nat::wait  [which is the beneath ()->wait () in aix_thread_target::wait],
>BT:- If direct rs6000-aix-nat::wait [ where in output 3 and 4 {below in this email} you can see it will directly come to rs6000-aix-nat.c if the main process after having threads forks or uses a fork () call ] pasted below in this email.

I'm only replying to this is right now, because that seems to
be the fundamental problem that ultimately causes a lot of the
other issues you're seeing.

It seems the core problem is that you're not initializing the
thread layer correctly for any but the first inferior!  So all
other inferiors started with fork are assumed to be single-
threaded ...

If you look at a backtrace like this:

>BT:- If direct rs6000-aix-nat::wait
>
>Thread 1 hit Breakpoint 2, rs6000_nat_target::wait (this=0x1100a2e10 <_rs6000aixnat.rw_>, ptid=...,
>    ourstatus=0xffffffffffff360, options=...) at rs6000-aix-nat.c:695
>695           set_sigint_trap ();
>(gdb) bt
>#0  rs6000_nat_target::wait (this=0x1100a2e10 <_rs6000aixnat.rw_>, ptid=...,
>    ourstatus=0xffffffffffff360, options=...) at rs6000-aix-nat.c:695
>#1  0x0000000100340778 in target_wait (ptid=..., status=0xffffffffffff360, options=...) at target.c:2598

you see that the target.c code uses the current inferior's
"top_target" to find the appropriate target routines:

  target_ops *target = current_inferior ()->top_target ();
[...]
      ptid_t event_ptid = target->wait (ptid, status, options);

For a multi-threaded process "top_target" *should* point to
aix_thread_ops, which is achieved by this call in pd_enable:
  current_inferior ()->push_target (&aix_thread_ops);

However, note that this is applied only to *one* inferior.
You actually need to do this for *all* new inferiors as soon
as they are detected to become multi-threaded.

This does not happen because aix-thread.c currently has a static
global pd_able variable that applies to GDB as a whole.  Back in
the days where this was introduced, that was probably correct
since a single GDB session could only debug one single inferior
back then.  But for multiple inferiors, any of which can be
multi-threaded, this does not work.


I think you should first of all work on fixing this, and then
go back to validating your test scenarios without any of the
other changes - many of those likely will no longer be
necessary then.


Bye,
Ulrich


[-- Attachment #2: 0001-Fix-multi-thread-debug-bug-in-AIX.patch --]
[-- Type: application/octet-stream, Size: 7954 bytes --]

From aef6803b770f0555d16fa5bfbdda3be8c1901ad5 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Thu, 15 Dec 2022 06:25:28 -0600
Subject: [PATCH] Fix multi thread debug bug in AIX

This is a temporary fix. It is work in progress
---
 gdb/aix-thread.c | 90 +++++++++++++++++++++++++++---------------------
 1 file changed, 50 insertions(+), 40 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index e556c153576..0e4d43fb488 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -55,6 +55,7 @@
 #include <sys/reg.h>
 #include <sched.h>
 #include <sys/pthdebug.h>
+#include <vector>
 
 #if !HAVE_DECL_GETTHRDS
 extern int getthrds (pid_t, struct thrdsinfo64 *, int, tid_t *, int);
@@ -70,7 +71,7 @@ static bool debug_aix_thread;
 
 /* Return whether to treat PID as a debuggable thread id.  */
 
-#define PD_TID(ptid)	(pd_active && ptid.tid () != 0)
+#define PD_TID(ptid)	(std::find (pd_active.begin (), pd_active.end (), ptid.pid ()) != pd_active.end () && ptid.tid () != 0)
 
 /* Success and failure values returned by pthdb callbacks.  */
 
@@ -151,11 +152,11 @@ static CORE_ADDR pd_brk_addr;
 
 /* Whether the current application is debuggable by pthdb.  */
 
-static int pd_able = 0;
+static std::vector<pid_t> pd_able;
 
 /* Whether a threaded application is being debugged.  */
 
-static int pd_active = 0;
+static std::vector<pid_t> pd_active;
 
 /* Whether the current architecture is 64-bit.  
    Only valid when pd_able is true.  */
@@ -508,14 +509,13 @@ pdc_read_data (pthdb_user_t user_current_pid, void *buf,
   /* This is needed to eliminate the dependency of current thread
      which is null so that thread reads the correct target memory.  */
   {
-    scoped_restore_current_thread restore_current_thread;
+    scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
     /* Before the first inferior is added, we pass inferior_ptid.pid ()
        from pd_enable () which is 0.  There is no need to switch threads
        during first initialisation.  In the rest of the callbacks the
        current thread needs to be correct.  */
     if (user_current_pid != 0)
-      switch_to_thread (current_inferior ()->process_target (),
-			ptid_t (user_current_pid));
+      inferior_ptid = ptid_t (user_current_pid);
     status = target_read_memory (addr, (gdb_byte *) buf, len);
   }
   ret = status == 0 ? PDC_SUCCESS : PDC_FAILURE;
@@ -639,36 +639,23 @@ pcmp (const void *p1v, const void *p2v)
   return p1->pthid < p2->pthid ? -1 : p1->pthid > p2->pthid;
 }
 
-/* iterate_over_threads() callback for counting GDB threads.
-
-   Do not count the main thread (whose tid is zero).  This matches
-   the list of threads provided by the pthreaddebug library, which
-   does not include that main thread either, and thus allows us
-   to compare the two lists.  */
+/* iterate_over_threads() callback for counting GDB threads.  */
 
 static int
 giter_count (struct thread_info *thread, void *countp)
 {
-  if (PD_TID (thread->ptid))
-    (*(int *) countp)++;
+  (*(int *) countp)++;
   return 0;
 }
 
-/* iterate_over_threads() callback for accumulating GDB thread pids.
-
-   Do not include the main thread (whose tid is zero).  This matches
-   the list of threads provided by the pthreaddebug library, which
-   does not include that main thread either, and thus allows us
-   to compare the two lists.  */
+/* iterate_over_threads() callback for accumulating GDB thread pids.  */
 
 static int
 giter_accum (struct thread_info *thread, void *bufp)
 {
-  if (PD_TID (thread->ptid))
-    {
-      **(struct thread_info ***) bufp = thread;
-      (*(struct thread_info ***) bufp)++;
-    }
+  **(struct thread_info ***) bufp = thread;
+  (*(struct thread_info ***) bufp)++;
+
   return 0;
 }
 
@@ -719,7 +706,7 @@ get_signaled_thread (int pid)
 		    sizeof (thrinf), &ktid, 1) != 1)
 	break;
 
-      if (thrinf.ti_cursig == SIGTRAP)
+      if (thrinf.ti_cursig)
 	return thrinf.ti_tid;
     }
 
@@ -750,6 +737,9 @@ sync_threadlists (int pid)
   pthdb_pthread_t pdtid;
   pthread_t pthid;
   pthdb_tid_t tid;
+  process_stratum_target *proc_target
+            = current_inferior ()->process_target ();
+  thread_info  *tp;
 
   /* Accumulate an array of libpthdebug threads sorted by pthread id.  */
 
@@ -810,8 +800,6 @@ sync_threadlists (int pid)
 	  priv->pdtid = pbuf[pi].pdtid;
 	  priv->tid = pbuf[pi].tid;
 
-	  process_stratum_target *proc_target
-	    = current_inferior ()->process_target ();
 	  thread = add_thread_with_info (proc_target,
 					 ptid_t (pid, 0, pbuf[pi].pthid),
 					 priv);
@@ -841,8 +829,23 @@ sync_threadlists (int pid)
 	    }
 	  else if (cmp_result > 0)
 	    {
-	      delete_thread (gbuf[gi]);
-	      gi++;
+              if (gptid.is_pid ())
+                {
+                  gdb_assert (gptid.pid () == pptid.pid ());
+                  thread_change_ptid (proc_target, gptid, pptid);
+                  aix_thread_info *priv = new aix_thread_info;
+                  priv->pdtid = pbuf[pi].pdtid;
+                  priv->tid = pbuf[pi].tid;
+                  tp = find_thread_ptid (proc_target, pptid);
+                  tp->priv.reset (priv);
+                  pi++;
+                  gi++;
+                }
+              else
+                {
+                  delete_thread (gbuf[gi]);
+                  gi++;
+                }
 	    }
 	  else
 	    {
@@ -888,7 +891,8 @@ pd_update (int pid)
   pthdb_tid_t tid;
   struct thread_info *thread = NULL;
 
-  if (!pd_active)
+  if (std::find (pd_active.begin (), pd_active.end (), pid)
+    == pd_active.end ())
     return ptid_t (pid);
 
   status = pthdb_session_update (pd_session);
@@ -926,7 +930,7 @@ pd_activate (int pid)
     {
       return ptid_t (pid);
     }
-  pd_active = 1;
+  pd_active.push_back (pid);
   return pd_update (pid);
 }
 
@@ -935,12 +939,13 @@ pd_activate (int pid)
 static void
 pd_deactivate (void)
 {
-  if (!pd_active)
+  if (std::find (pd_active.begin (), pd_active.end (), inferior_ptid.pid ())
+    == pd_active.end ())
     return;
   pthdb_session_destroy (pd_session);
   
   pid_to_prc (&inferior_ptid);
-  pd_active = 0;
+  pd_active.erase (std::find (pd_active.begin (), pd_active.end (), inferior_ptid.pid ()));
 }
 
 /* An object file has just been loaded.  Check whether the current
@@ -954,7 +959,8 @@ pd_enable (void)
   struct bound_minimal_symbol ms;
 
   /* Don't initialize twice.  */
-  if (pd_able)
+  if (std::find (pd_able.begin (), pd_able.end (), inferior_ptid.pid ()) 
+	!= pd_able.end ())
     return;
 
   /* Check application word size.  */
@@ -978,7 +984,7 @@ pd_enable (void)
 
   /* Prepare for thread debugging.  */
   current_inferior ()->push_target (&aix_thread_ops);
-  pd_able = 1;
+  pd_able.push_back (inferior_ptid.pid ());
 
   /* If we're debugging a core file or an attached inferior, the
      pthread library may already have been initialized, so try to
@@ -991,11 +997,14 @@ pd_enable (void)
 static void
 pd_disable (void)
 {
-  if (!pd_able)
+  if (std::find (pd_able.begin (), pd_able.end (), 
+	inferior_ptid.pid ()) == pd_able.end ())
     return;
-  if (pd_active)
+  if (std::find (pd_active.begin (), pd_active.end (),
+        inferior_ptid.pid ()) != pd_active.end ())
     pd_deactivate ();
-  pd_able = 0;
+  pd_able.erase (std::find (pd_able.begin (), pd_able.end (), 
+	inferior_ptid.pid ()));
   current_inferior ()->unpush_target (&aix_thread_ops);
 }
 
@@ -1096,7 +1105,8 @@ aix_thread_target::wait (ptid_t ptid, struct target_waitstatus *status,
   gdb_assert (ptid.is_pid ());
 
   /* Check whether libpthdebug might be ready to be initialized.  */
-  if (!pd_active && status->kind () == TARGET_WAITKIND_STOPPED
+  if (std::find (pd_active.begin (), pd_active.end (), ptid.pid ()) == pd_active.end ()
+      && status->kind () == TARGET_WAITKIND_STOPPED
       && status->sig () == GDB_SIGNAL_TRAP)
     {
       process_stratum_target *proc_target
-- 
2.31.1