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

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

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

Hi Ulrich and community,

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

>I think you'll have to allow for that modified form of the name as well.

I have allowed the same. Please see the solib-aix change. With this we are able to read all the symbols in any inferior successfully. One can verify this by using the set debug aix-thread.

If one executes a "info shared library" command, one can see the 4 libraries for any inferior. Kindly check output 1,pasted below in this email for program 1.


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

>This needs further debugging to understand what's going on once you allow
>that match.  That original problem should be fixed by that change, so
>there's probably something else as well ...

Yeah. As mentioned in my previous mail we are losing our threads information. Though in the output we do get a new thread my attempt to understand the root cause in the code failed miserably. Thread 1 belonging to process 1 is getting shown as 2.1 in output 2 pasted below. What's worse is the top target is also not setting properly in the process of having the right name for the shared library. I have the correct program space as well while reading the symbol. And since the top target is wrong, the new process appears though this is a threaded one in the output.

So, looking at this, I have missed out something may be minor or major causing the bug which I am unaware of in the code base. I have tried debugging the aix-thread.c.. But things look to be properly aligned as it should be there at least. Single inferior examples with multiple threads pass. But multi inferior with multi threads fail.

Kindly guide me on what I am missing here. It is surely something which I have not explored and unaware of. Your expertise can help us resolve this bug.

Thank you for the guidance so far in this bug.

Waiting for a reply soon.

Have a nice day ahead.

Thanks and 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 1


void *

thread_function (void *arg)

{


  pthread_barrier_wait (&barrier);

  pid_t child;


  child = fork ();

  if (child > 0)

    printf ("I am parent \n");

  else

  {

    child = fork ();

    if (child > 0)

      printf ("I am child \n");

    else

      printf ("I am grandchild \n");

  }

  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;

}

---------------------------------------------------------------------------------------------------------
Output 1:-


Reading symbols from /home/aditya/gdb_tests/ultimate-multi-thread-fork...

(gdb) set detach-on-fork off

(gdb) r

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

[New Thread 258]

[New inferior 2 (process 15925744)]

I am parent

^C[New process 11665696]


Thread 1.3 received signal SIGINT, Interrupt.

0xd0595fb0 in _p_nsleep () from /usr/lib/libpthread.a(shr_xpg5.o)

(gdb) info sharedlibrary

From        To          Syms Read   Shared Object Library

0xd05bc124  0xd05bf194  Yes (*)     /usr/lib/libpthreads.a(shr_comm.o)

0xd05bb240  0xd05bb9a1  Yes (*)     /usr/lib/libcrypt.a(shr.o)

0xd0576180  0xd05ba731  Yes (*)     /usr/lib/libpthread.a(shr_xpg5.o)

0xd0100e00  0xd0575123  Yes (*)     /usr/lib/libc.a(shr.o)

(*): Shared library is missing debugging information.

(gdb) inferior 2

[Switching to inferior 2 [process 15925744] (/home/aditya/gdb_tests/ultimate-multi-thread-fork)]

[Switching to thread 2.1 (Thread 258)]

#0  0xd0594fc8 in _sigsetmask () from /usr/lib/libpthread.a(shr_xpg5.o)

(gdb) info sharedlibrary

From        To          Syms Read   Shared Object Library

0xd05bc124  0xd05bf194  Yes (*)     /usr/lib/libpthreads.a(shr_comm.o)

0xd05bb240  0xd05bb9a1  Yes (*)     /usr/lib/libcrypt.a(shr.o)

0xd0576180  0xd05ba731  Yes (*)     /usr/lib/libpthread.a(shr_xpg5.o)

0xd0100e00  0xd0575123  Yes (*)     /usr/lib/libc.a(shr.o)

(*): Shared library is missing debugging information.

(gdb)

---------------------------------------------------------------------------------------------------------
Output 2:-

Reading symbols from /home/aditya/gdb_tests/ultimate-multi-thread-fork...

(gdb) set detach-on-fork off

(gdb) r

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

[New Thread 258]

[New inferior 2 (process 16122342)]

I am parent

^C[New process 11665700]


Thread 1.3 received signal SIGINT, Interrupt.

0xd0595fb0 in _p_nsleep () from /usr/lib/libpthread.a(shr_xpg5.o)

(gdb) info threads

  Id   Target Id                          Frame

* 1.3  process 11665700                   0xd0595fb0 in _p_nsleep ()

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

  2.1  Thread 258 (tid 28115287, running) 0xd0594fc8 in _sigsetmask ()

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

(gdb)

________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 09 January 2023 19:34
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:

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

Looks like this is because solib_aix_bfd_open *changes* the BFD filename here:

  /* Override the returned bfd's name with the name returned from solib_find
     along with appended parenthesized member name in order to allow commands
     listing all shared libraries to display.  Otherwise, we would only be
     displaying the name of the archive member object.  */
  std::string fname = string_printf ("%s%s",
                                     bfd_get_filename (archive_bfd.get ()),
                                     sep);
  bfd_set_filename (object_bfd.get (), fname.c_str ());

so when the same BFD gets checked a second time, you'll now see the changed
filename instead of the original one.

I think you'll have to allow for that modified form of the name as well.


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

This needs further debugging to understand what's going on once you allow
that match.  That original problem should be fixed by that change, so
there's probably something else as well ...

Bye,
Ulrich


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

From fb8d24df7c1362204ca0045361acd37439f46746 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Tue, 10 Jan 2023 05:58:28 -0600
Subject: [PATCH] Fix multi thread debug bug in AIX

---
 gdb/aix-thread.c | 103 ++++++++++++++++++++++++++---------------------
 gdb/solib-aix.c  |   6 +++
 2 files changed, 64 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
diff --git a/gdb/solib-aix.c b/gdb/solib-aix.c
index f483f54de13..181140b3345 100644
--- a/gdb/solib-aix.c
+++ b/gdb/solib-aix.c
@@ -618,6 +618,12 @@ solib_aix_bfd_open (const char *pathname)
       if (member_name == bfd_get_filename (object_bfd.get ()))
 	break;
 
+      std::string s = bfd_get_filename (object_bfd.get ());
+      if (s.find (member_name) != std::string::npos)
+      {
+	return object_bfd;
+      }
+
       object_bfd = gdb_bfd_openr_next_archived_file (archive_bfd.get (),
 						     object_bfd.get ());
     }
-- 
2.31.1