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

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

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

Hi Ulrich,

>If not, are shared libraries actually detected correctly in the
>second inferior (e.g. does "info sharedlibrary" show them correctly
>in the second inferior)?  If not, maybe solib-aix.c also needs to
>be reviewed whether it handles multiple inferiors correctly

You were right about this in the previous email. There are 4 shared libraries all present in their respective archived files, we try to access when a new inferior is created. They are:-

/usr/lib/libpthreads.a(shr_comm.o)

/usr/lib/libcrypt.a(shr.o)

/usr/lib/libpthread.a(shr_xpg5.o)

/usr/lib/libc.a(shr.o)

Consider the patch for the analysis as shown below:-


--- solib-aix.c_orig    2022-12-23 12:05:39 +0000

+++ solib-aix.c 2022-12-26 06:12:06 +0000

@@ -615,6 +615,7 @@

     (gdb_bfd_openr_next_archived_file (archive_bfd.get (), NULL));

   while (object_bfd != NULL)

     {

+      printf ("object_bfd is %s --- compared with --- member_name is %s in path %s \n", bfd_get_filename (object_bfd.get ()), member_name.c_str (), pathname);

       if (member_name == bfd_get_filename (object_bfd.get ()))

        break;

Here I have added a print statement to ensure we are able to find the member in the archive.

What's interesting is for the first inferior this works fine for all shared libraries. For the second one and every inferior thereafter the output is as shown below in the next paragraph,


object_bfd is shr.o --- compared with --- member_name is shr_comm.o in path /usr/lib/libpthreads.a(shr_comm.o)

object_bfd is /usr/lib/libpthreads.a(shr_comm.o) --- compared with --- member_name is shr_comm.o in path /usr/lib/libpthreads.a(shr_comm.o)

I was surprised that the bfd_get_filename (object_bfd.get ()) is returning the pathname instead of the object file descriptor. Everything until here seems to correct in the solib_aix_bfd_open () function and this makes it hard for me to understand what is going on. Even if I allow a pathname match to the member_name we end up losing all the information of our threads in the first process though we still have the process information.

One more thing I want to share is that the inferior is getting correctly aligned wherever current_inferior () is used to get the inferior's shared library list or information.

So right now, I do not have answers to why the pathname is getting returned and if I need to correct the same how I can. As a consequence, the Shared libraries are missing for new inferior.  Kindly guide me on how this can be solved.

The rest of the patch I am attempting to solve this remains unchanged.

Have a nice day ahead.

Thanks and regards,
Aditya.


________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 22 December 2022 18:20
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, 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.

So the way is works on Linux is that linux-thread-db.c registers
both an inferior_created observer and a new_objfile observer.

The inferior_created observer gets called immediately after the
new inferior is noticed - at this point, only the main executable
is detected by GDB, not any of the shared libraries.  So this will
only successfully detect a libpthread-linked executable if it was
*statically* linked.  For dynamically linked executables, the
new_objfile observer will later get called as well, once for each
shared library.  As soon as this happens for the libpthread shared
library, the fact that the inferior is multithreaded is detected.

At first glance, this should actually work the same on AIX - the
aix-thread.c file also registers both inferior_created and
new_objfile observers.  Not sure why this doesn't work ...
Do you see the new_objfile observer being called for all the
shared libraries in the second inferior?

If not, are shared libraries actually detected correctly in the
second inferior (e.g. does "info sharedlibrary" show them correctly
in the second inferior)?  If not, maybe solib-aix.c also needs to
be reviewed whether it handles multiple inferiors correctly ...

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