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

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

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

Hi Ulrich and community,

Please find attached the patch {See:

0001-Fix-multi-thread-debug-bug-in-AIX.patch}

>I think the problem here may be that the lookup_minimal_symbol
>call in pdc_symbol_addrs has to be performed in the correct
>process address space.  This wasn't an issue before since the
>routine was only called for the first process anyway.

>Look at the equivalent routine on Linux, which is
>ps_pglobal_lookup in proc-service.c.  This does:

 >inferior *inf = ph->thread->inf;
 > scoped_restore_current_program_space restore_pspace;
 > set_current_program_space (inf->pspace);

> You'll need to do the equivalent (set the program space
> to the one appropriate for the inferior referenced by
> user_current_pid).

So, I tried setting this right in this patch and it did not work. I even tried setting the inferior_ptid and current_inferior to user_current_pid, but that also did not help.

The output still appears as follows for the same code in the previous mail.


[New Thread 258]

[New Thread 515]

fetch_regs_kernel_thread tid=1da0189 regno=64 arch64=0

[New inferior 2 (process 6553888)]

pdc_free (user_current_pid = 11272598, buf = 0x11016ee70)

pdc_free (user_current_pid = 11272598, buf = 0x11016eeb0)

pdc_free (user_current_pid = 11272598, buf = 0x11016eff0)

pdc_free (user_current_pid = 11272598, buf = 0x1104e2530)

pdc_free (user_current_pid = 11272598, buf = 0x1108af2d0)

pdc_symbol_addrs (user_current_pid = 11272598, symbols = 0xfffffffffffdf08, count = 1)

  symbols[0].name = "__n_pthreads"

  symbols[0].addr = 0xf0807334

 returning PDC_SUCCESS

pdc_read_data (user_current_pid = 11272598, buf = 0xfffffffffffdf00, addr = 0xf0807334, len = 4)

  status=0, returning SUCCESS

pdc_symbol_addrs (user_current_pid = 6553888, symbols = 0xfffffffffffe258, count = 1)

  symbols[0].name = "__n_pthreads"

 returning PDC_FAILURE

[New process 11272598]

[New inferior 3 (process 8323536)]

pdc_symbol_addrs (user_current_pid = 11272598, symbols = 0xfffffffffffdf08, count = 1)

  symbols[0].name = "__n_pthreads"

  symbols[0].addr = 0xf0807334

 returning PDC_SUCCESS

pdc_read_data (user_current_pid = 11272598, buf = 0xfffffffffffdf00, addr = 0xf0807334, len = 4)

  status=0, returning SUCCESS

pdc_symbol_addrs (user_current_pid = 8323536, symbols = 0xfffffffffffe258, count = 1)

  symbols[0].name = "__n_pthreads"

 returning PDC_FAILURE

I am parent

I am parent

So, we can see clearly it did try to check if our new inferiors can be threaded and it failed.

What I observed one thing while I tried to guess how Linux might be doing it is that once it detects a new inferior it continuously calls ps_pglobal_lookup in proc-service.c  using an observable till it succeeds in reading the symbol. Kindly see the below output of linux for the same program. {Output Credits:- Linux GDB}


[New Thread 0x7ffff7cff170 (LWP 259785)]

[New Thread 0x7ffff74ef170 (LWP 259786)]

[New inferior 2 (process 259787)]

[Thread debugging using libthread_db enabled]

Using host libthread_db library "/lib64/libthread_db.so.1".

[New inferior 3 (process 259788)]


The backtrace might be as follows:-


#11 0x0000000010063fe8 in std::_Function_handler<void (inferior*), void (*)(inferior*)>::_M_invoke(std::_Any_data const&, inferior*&&)

    at /usr/include/c++/8/bits/std_function.h:297

#12 0x00000000102c77b8 in std::function<void (inferior*)>::operator()(inferior*) const (

    __args#0=0x112cd920, this=<optimized out>) at /usr/include/c++/8/bits/std_function.h:687

#13 gdb::observers::observable<inferior*>::notify (args#0=0x112cd920, this=<optimized out>)

    at ./../gdbsupport/observable.h:166

#14 post_create_inferior  at infcmd.c:315

#15 0x00000000102e25a4 in follow_fork_inferior (detach_fork=false, follow_child=false) at infrun.c:683

#16 follow_fork () at infrun.c:79

#17 0x00000000102ec7b8 in handle_inferior_event  at infrun.c:5728

#18 0x00000000102ed9d8 in fetch_inferior_event () at infrun.c:4233

#19 0x00000000102c05a4 in inferior_event_handler  at inf-loop.c:41

#20 0x0000000010317e7c in handle_target_event

    at -nat.c:4216

#21 0x0000000010957e68 in handle_file_event

    at event-loop.cc:549

#22 0x00000000109588c4 in gdb_wait_for_event

 at event-loop.cc:670

#23 0x0000000010958cac in gdb_wait_for_event (block=0) at event-loop.cc:569

#24 gdb_do_one_event () at event-loop.cc:210

#25 0x000000001034d684 in start_event_loop () at main.c:411

#26 captured_command_loop () at main.c:471

#27 0x000000001034f8b0 in captured_main

 at main.c:1329

#28 gdb_main (args=<optimized out>) at main.c:1344

#29 0x000000001001a188 in main at gdb.c:32


Post stack number #11 they might be going to ps_pglobal_lookup () everytime till they can make the new inferior thread debugging possible. AIX on the other hand calls the pdc_symbol_adddress () only once for a new inferior after first inferior. For the first inferior as well, things succeed only in the fourth time as shown below for a 32-bit code.


pdc_symbol_addrs (user_current_pid = 11272598, symbols = 0xfffffffffffdbd8, count = 1)

  symbols[0].name = "__n_pthreads"

 returning PDC_FAILURE

pdc_symbol_addrs (user_current_pid = 11272598, symbols = 0xfffffffffffdbd8, count = 1)

  symbols[0].name = "__n_pthreads"

 returning PDC_FAILURE

pdc_symbol_addrs (user_current_pid = 11272598, symbols = 0xfffffffffffdbd8, count = 1)

  symbols[0].name = "__n_pthreads"

 returning PDC_FAILURE

pdc_symbol_addrs (user_current_pid = 11272598, symbols = 0xfffffffffffdbd8, count = 1)

  symbols[0].name = "__n_pthreads"

  symbols[0].addr = 0xf0807334

 returning PDC_SUCCESS

So, I guess if we manage to do something similar just like for the first inferior, we will get to the solution, but I did not understand how Linux might be reading the symbol again and again for a new inferior or AIX for that matter for the first inferior.  Kindly let me know how we can do something similar or are we missing something here that I have not kept in mind in our attempt to solve this for AIX and GDB community.

Let me know what you think. Waiting for a reply soon.

Have a nice day.

Regards,
Aditya.
________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 15 December 2022 21:23
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:

>[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

I think the problem here may be that the lookup_minimal_symbol
call in pdc_symbol_addrs has to be performed in the correct
process address space.  This wasn't an issue before since the
routine was only called for the first process anyway.

Look at the equivalent routine on Linux, which is
ps_pglobal_lookup in proc-service.c.  This does:

  inferior *inf = ph->thread->inf;
  scoped_restore_current_program_space restore_pspace;
  set_current_program_space (inf->pspace);

You'll need to do the equivalent (set the program space
to the one appropriate for the inferior referenced by
user_current_pid).

Bye,
Ulrich


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

From 8b1f34b7d62c9e6a0f216c0e3b36fce752952eee Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Sun, 18 Dec 2022 23:19:04 -0600
Subject: [PATCH] Fix multi thread debug bug in AIX

---
 gdb/aix-thread.c | 103 ++++++++++++++++++++++++++---------------------
 1 file changed, 58 insertions(+), 45 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index e556c153576..3a2afad25a1 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.  */
@@ -331,6 +332,9 @@ pdc_symbol_addrs (pthdb_user_t user_current_pid, pthdb_symbol_t *symbols, int co
   struct bound_minimal_symbol ms;
   int i;
   char *name;
+  scoped_restore_current_program_space restore_pspace; 
+  inferior *inf = find_inferior_pid (current_inferior ()->process_target (), user_current_pid);
+  set_current_program_space (inf->pspace);
 
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
@@ -508,14 +512,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 +642,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 +709,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 +740,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 +803,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 +832,22 @@ sync_threadlists (int pid)
 	    }
 	  else if (cmp_result > 0)
 	    {
-	      delete_thread (gbuf[gi]);
-	      gi++;
+              if (gptid.is_pid () && 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 +893,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 +932,7 @@ pd_activate (int pid)
     {
       return ptid_t (pid);
     }
-  pd_active = 1;
+  pd_active.push_back (pid);
   return pd_update (pid);
 }
 
@@ -935,26 +941,29 @@ 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
    application is pthreaded, and if so, prepare for thread debugging.  */
 
 static void
-pd_enable (void)
+pd_enable (inferior *inf)
 {
   int status;
   char *stub_name;
   struct bound_minimal_symbol ms;
+  pid_t pid = (inf == NULL?inferior_ptid.pid ():inf->pid);
 
   /* Don't initialize twice.  */
-  if (pd_able)
+  if (std::find (pd_able.begin (), pd_able.end (), pid) 
+	!= pd_able.end ())
     return;
 
   /* Check application word size.  */
@@ -962,7 +971,7 @@ pd_enable (void)
 
   /* Check whether the application is pthreaded.  */
   stub_name = NULL;
-  status = pthdb_session_pthreaded (inferior_ptid.pid (), PTHDB_FLAG_REGS,
+  status = pthdb_session_pthreaded (pid, PTHDB_FLAG_REGS,
 				    &pd_callbacks, &stub_name);
   if ((status != PTHDB_SUCCESS
        && status != PTHDB_NOT_PTHREADED) || !stub_name)
@@ -978,12 +987,12 @@ pd_enable (void)
 
   /* Prepare for thread debugging.  */
   current_inferior ()->push_target (&aix_thread_ops);
-  pd_able = 1;
+  pd_able.push_back (pid);
 
   /* If we're debugging a core file or an attached inferior, the
      pthread library may already have been initialized, so try to
      activate thread debugging.  */
-  pd_activate (inferior_ptid.pid ());
+  pd_activate (pid);
 }
 
 /* Undo the effects of pd_enable().  */
@@ -991,11 +1000,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);
 }
 
@@ -1010,7 +1022,7 @@ static void
 new_objfile (struct objfile *objfile)
 {
   if (objfile)
-    pd_enable ();
+    pd_enable (NULL);
   else
     pd_disable ();
 }
@@ -1020,7 +1032,7 @@ new_objfile (struct objfile *objfile)
 static void
 aix_thread_inferior_created (inferior *inf)
 {
-  pd_enable ();
+  pd_enable (inf);
 }
 
 /* Detach from the process attached to by aix_thread_attach().  */
@@ -1096,7 +1108,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