[PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Aditya Kamath1]

[-- Attachment #1: Type: text/plain, Size: 3602 bytes --]

Hi,

Folks using AIX are not able to debug multiple threads.

The reason:- Since a new thread addition causes a thread target to wait, in AIX once the event ptid is got with the waitpid(), we need to set the inferior_ptid variable. Every time we come into aix_thread_target::wait() we check if libpthdebug might be ready to be initialized.In doing so we call pd_activate(). Here the session needs to be successfully initialised failing to which just a pid is returned. We do not enter pd_update() in the former case to take care of the rest of the thread addition process. The pthdb_session_init() is dependent on inferior_ptid variable as per our observations to return PTHDB_SUCCESS.

Please find attached the patch. [See: Fix-for-multiple-thread-detection-in-AIX.patch]

This can be shown by the following program:-


#include <stdio.h>

#include <unistd.h>

#include <stdlib.h>

#include <pthread.h>

#include <assert.h>


pthread_barrier_t barrier;


#define NUM_THREADS 2


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);


  while (1); /* break here */

}


int

main (void)

{

  int i;


  alarm (300);


  pthread_barrier_init (&barrier, NULL, NUM_THREADS);


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

    {

      pthread_t thread;

      int res;


      res = pthread_create (&thread, NULL,

                            thread_function, NULL);

      assert (res == 0);

    }


  while (1)

    sleep (1);


  return 0;

}

Output without patch:-

(gdb) r

Starting program: /home/aditya/gdb_tests/continue-pending-status

^C

Program received signal SIGINT, Interrupt.

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

(gdb) info threads

  Id   Target Id         Frame

* 1    process 29557240  0xd0595fb0 in _p_nsleep () from /usr/lib/libpthread.a(shr_xpg5.o)

(gdb)



Output with patch:-


Reading symbols from /home/aditya/gdb_tests/continue-pending-status...

(gdb) r

Starting program: /home/aditya/gdb_tests/continue-pending-status

[New Thread 1]

^C[New Thread 258]

[New Thread 515]


Thread 1 received signal SIGINT, Interrupt.

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

(gdb) info threads

  Id   Target Id                           Frame

* 1    process 29557210                    0xd0595fb0 in _p_nsleep ()

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

  2    Thread 1 (tid 120197499, running)   0xd0595fb0 in _p_nsleep ()

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

  3    Thread 258 (tid 130486575, running) thread_function (arg=0x0)

    at continue-pending-status.c:36

  4    Thread 515 (tid 131666371, running) thread_function (arg=warning: (Internal error: pc 0x0 in read in psymtab, but not in symtab.)


0x0)

    at continue-pending-status.c:36

(gdb)

The gdb base test suite numbers with patch:-

# of expected passes            14094

# of unexpected failures        8770

# of unexpected successes       1

# of expected failures          10

# of known failures             46

# of unresolved testcases       121

# of untested testcases         84

# of unsupported tests          61

# of paths in test names        2

# of duplicate test names       5


Kindly let me know what you think.

Thanks and regards,
Aditya.



[-- Attachment #2: 0001-Fix-for-multiple-thread-detection-in-AIX.patch --]
[-- Type: application/octet-stream, Size: 928 bytes --]

From 7e8abe258f67abd3854fedf54e43da2b13541986 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Fri, 15 Jul 2022 08:40:25 -0500
Subject: [PATCH] Fix for multiple thread detection in AIX

---
 gdb/aix-thread.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index d47f5132592..f66b5904ae8 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -1088,6 +1088,10 @@ aix_thread_target::wait (ptid_t ptid, struct target_waitstatus *status,
      pid-only ptids.  */
   gdb_assert (ptid.is_pid ());
 
+  /* In pd_activate to get PTHB_SUCCESS in pthread debug session init
+     we need inferior_ptid set to update multiple threads. */
+  inferior_ptid = ptid;
+
   /* Check whether libpthdebug might be ready to be initialized.  */
   if (!pd_active && status->kind () == TARGET_WAITKIND_STOPPED
       && status->sig () == GDB_SIGNAL_TRAP)
-- 
2.31.1

Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Aditya Kamath1]

[-- Attachment #1: Type: text/plain, Size: 7807 bytes --]

Hi all,

I apologise for a mistake in the previous mail from my end. Please see the new patch. Kindly ignore the previous mail.

Folks using AIX are not able to debug multiple threads.

The reason:- Since a new thread addition causes a thread target to wait, in AIX once the event ptid is got with the waitpid(), we need to set the inferior_ptid variable. Every time we come into aix_thread_target::wait() we check if libpthdebug might be ready to be initialized.In doing so we call pd_activate(). Here the session needs to be successfully initialised failing to which just a pid is returned. We do not enter pd_update() in the former case to take care of the rest of the thread addition process. The pthdb_session_init() is dependent on inferior_ptid variable as per our observations to return PTHDB_SUCCESS.

Please find attached the patch. [See: Fix-for-multiple-thread-detection-in-AIX.patch]

This can be shown by the following program:-


#include <stdio.h>

#include <unistd.h>

#include <stdlib.h>

#include <pthread.h>

#include <assert.h>


pthread_barrier_t barrier;


#define NUM_THREADS 2


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);


  while (1); /* break here */

}


int

main (void)

{

  int i;


  alarm (300);


  pthread_barrier_init (&barrier, NULL, NUM_THREADS);


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

    {

      pthread_t thread;

      int res;


      res = pthread_create (&thread, NULL,

                            thread_function, NULL);

      assert (res == 0);

    }


  while (1)

    sleep (1);


  return 0;

}

Output without patch:-

(gdb) r

Starting program: /home/aditya/gdb_tests/continue-pending-status

^C

Program received signal SIGINT, Interrupt.

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

(gdb) info threads

  Id   Target Id         Frame

* 1    process 29557240  0xd0595fb0 in _p_nsleep () from /usr/lib/libpthread.a(shr_xpg5.o)

(gdb)



Output with patch:-


Reading symbols from /home/aditya/gdb_tests/continue-pending-status...

(gdb) r

Starting program: /home/aditya/gdb_tests/continue-pending-status

[New Thread 1]

^C[New Thread 258]

[New Thread 515]


Thread 1 received signal SIGINT, Interrupt.

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

(gdb) info threads

  Id   Target Id                           Frame

* 1    process 29557210                    0xd0595fb0 in _p_nsleep ()

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

  2    Thread 1 (tid 120197499, running)   0xd0595fb0 in _p_nsleep ()

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

  3    Thread 258 (tid 130486575, running) thread_function (arg=0x0)

    at continue-pending-status.c:36

  4    Thread 515 (tid 131666371, running) thread_function (arg=warning: (Internal error: pc 0x0 in read in psymtab, but not in symtab.)


0x0)

    at continue-pending-status.c:36

(gdb)

The gdb base test suite numbers with patch:-

# of expected passes            14094

# of unexpected failures        8770

# of unexpected successes       1

# of expected failures          10

# of known failures             46

# of unresolved testcases       121

# of untested testcases         84

# of unsupported tests          61

# of paths in test names        2

# of duplicate test names       5


Kindly let me know what you think.

Thanks and regards,
Aditya.
________________________________
From: Gdb-patches <gdb-patches-bounces+aditya.kamath1=ibm.com@sourceware.org> on behalf of Aditya Kamath1 via Gdb-patches <gdb-patches@sourceware.org>
Sent: 15 July 2022 21:21
To: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>; Ulrich Weigand <Ulrich.Weigand@de.ibm.com>; simark@simark.ca <simark@simark.ca>
Subject: [EXTERNAL] [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

Hi,

Folks using AIX are not able to debug multiple threads.

The reason:- Since a new thread addition causes a thread target to wait, in AIX once the event ptid is got with the waitpid(), we need to set the inferior_ptid variable. Every time we come into aix_thread_target::wait() we check if libpthdebug might be ready to be initialized.In doing so we call pd_activate(). Here the session needs to be successfully initialised failing to which just a pid is returned. We do not enter pd_update() in the former case to take care of the rest of the thread addition process. The pthdb_session_init() is dependent on inferior_ptid variable as per our observations to return PTHDB_SUCCESS.

Please find attached the patch. [See: Fix-for-multiple-thread-detection-in-AIX.patch]

This can be shown by the following program:-


#include <stdio.h>

#include <unistd.h>

#include <stdlib.h>

#include <pthread.h>

#include <assert.h>


pthread_barrier_t barrier;


#define NUM_THREADS 2


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);


  while (1); /* break here */

}


int

main (void)

{

  int i;


  alarm (300);


  pthread_barrier_init (&barrier, NULL, NUM_THREADS);


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

    {

      pthread_t thread;

      int res;


      res = pthread_create (&thread, NULL,

                            thread_function, NULL);

      assert (res == 0);

    }


  while (1)

    sleep (1);


  return 0;

}

Output without patch:-

(gdb) r

Starting program: /home/aditya/gdb_tests/continue-pending-status

^C

Program received signal SIGINT, Interrupt.

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

(gdb) info threads

  Id   Target Id         Frame

* 1    process 29557240  0xd0595fb0 in _p_nsleep () from /usr/lib/libpthread.a(shr_xpg5.o)

(gdb)



Output with patch:-


Reading symbols from /home/aditya/gdb_tests/continue-pending-status...

(gdb) r

Starting program: /home/aditya/gdb_tests/continue-pending-status

[New Thread 1]

^C[New Thread 258]

[New Thread 515]


Thread 1 received signal SIGINT, Interrupt.

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

(gdb) info threads

  Id   Target Id                           Frame

* 1    process 29557210                    0xd0595fb0 in _p_nsleep ()

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

  2    Thread 1 (tid 120197499, running)   0xd0595fb0 in _p_nsleep ()

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

  3    Thread 258 (tid 130486575, running) thread_function (arg=0x0)

    at continue-pending-status.c:36

  4    Thread 515 (tid 131666371, running) thread_function (arg=warning: (Internal error: pc 0x0 in read in psymtab, but not in symtab.)


0x0)

    at continue-pending-status.c:36

(gdb)

The gdb base test suite numbers with patch:-

# of expected passes            14094

# of unexpected failures        8770

# of unexpected successes       1

# of expected failures          10

# of known failures             46

# of unresolved testcases       121

# of untested testcases         84

# of unsupported tests          61

# of paths in test names        2

# of duplicate test names       5


Kindly let me know what you think.

Thanks and regards,
Aditya.



[-- Attachment #2: 0001-Fix-for-multiple-thread-detection-in-AIX.patch --]
[-- Type: application/octet-stream, Size: 1256 bytes --]

From a8c02f95f9b57085b9d7ec2648ed53a3b5f70bf8 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Fri, 15 Jul 2022 22:53:18 -0500
Subject: [PATCH] Fix for multiple thread detection in AIX

---
 gdb/aix-thread.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index d47f5132592..a83f8d38ca8 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -976,7 +976,7 @@ pd_enable (void)
   /* 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 (1);
+  pd_activate (inferior_ptid.pid());
 }
 
 /* Undo the effects of pd_enable().  */
@@ -1088,6 +1088,10 @@ aix_thread_target::wait (ptid_t ptid, struct target_waitstatus *status,
      pid-only ptids.  */
   gdb_assert (ptid.is_pid ());
 
+  /* In pd_activate to get PTHB_SUCCESS in pthread debug session init
+     we need inferior_ptid set to update multiple threads. */
+  inferior_ptid = ptid;
+
   /* Check whether libpthdebug might be ready to be initialized.  */
   if (!pd_active && status->kind () == TARGET_WAITKIND_STOPPED
       && status->sig () == GDB_SIGNAL_TRAP)
-- 
2.31.1

Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Ulrich Weigand]

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

>The reason:- Since a new thread addition causes a thread target to
>wait, in AIX once the event ptid is got with the waitpid(), we need to
>set the inferior_ptid variable. Every time we come into
>aix_thread_target::wait() we check if libpthdebug might be ready to be
>initialized.In doing so we call pd_activate(). Here the session needs
>to be successfully initialised failing to which just a pid is >returned.
>We do not enter pd_update() in the former case to take care of the rest
>of the thread addition process. The pthdb_session_init() is dependent
>on inferior_ptid variable as per our observations to return
>PTHDB_SUCCESS.

I think the change to pd_enable makes sense, passing 1 to pd_activate
seems clearly incorrect now.  Simon, you recently changed pd_activate
to take a PID instead of a boolean - any comments on this?

However, I do not see why the change to ::wait is necessary, or even
correct.  Note that when ::wait calls pd_activate or pd_update, it
already passes the correct pid.  I do not see any path from ::wait
to pd_enable.

Bye,
Ulrich

Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Aditya Kamath1]

Hi all,

This applies in a multithread case.

Why we need inferior_ptid reset in wait?? AIX uses pthread debug library which has to be intialised first. On an event of wait due to a thread creation, once we fetch the pid using waitpid(), the following condition satisfies..


/* Check whether libpthdebug might be ready to be initialized.*/

  if (!pd_active && status->kind () == TARGET_WAITKIND_STOPPED

  && status->sig () == GDB_SIGNAL_TRAP)

 and we enter pd_activate() to. On a successful pthread debug library initialisation we have to initiate a thread debug session using pthdb_session_init(). While we do that, we have call backs to read symbol, data etc. The problem is here where we are not able to read them successfully due to incorrect process ID information. It is not able to fetch the right address to read in pdc_read_data() where we read target_read_memory() and pdc_symbol_addrs().

Kindly read https://www.ibm.com/docs/en/aix/7.1?topic=programming-developing-multithreaded-program-debuggers for more on what I shared.
Developing multithreaded program debuggers<https://www.ibm.com/docs/en/aix/7.1?topic=programming-developing-multithreaded-program-debuggers>
The pthread debug library (libpthdebug.a) provides a set of functions that allows developers to provide debug capabilities for applications that use the pthread library.
www.ibm.com<http://www.ibm.com>

What we are thinking might be the solution??

Since we initialise the gdb with an observer to notify when there is a thread ptid change, on a wait event the inferior_ptid is set to null by switch_to_no_thread () with reinit_cache_frame (). Essentially with this we loose all our pid information needed to read our data and symbols as now our thread PTID is changed to null. So it is essential for us with the way AIX code is designed that we either reset our inferior_ptid in AIX thread wait code before we go out to the non-target dependent code like target.c, infrun.c to do it for us, or use

      thread_change_ptid (process_stratum_target *targ,

  ptid_t old_ptid, ptid_t new_ptid)


to inform GDB so that we can reset and get the right address for target_read_memory() in aix-thread.c file.


It will be great if you could tell us if we are thinking right at this moment.
________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 19 July 2022 17:51
To: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; simon.marchi@efficios.com <simon.marchi@efficios.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>; simark@simark.ca <simark@simark.ca>
Subject: Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

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

>The reason:- Since a new thread addition causes a thread target to
>wait, in AIX once the event ptid is got with the waitpid(), we need to
>set the inferior_ptid variable. Every time we come into
>aix_thread_target::wait() we check if libpthdebug might be ready to be
>initialized.In doing so we call pd_activate(). Here the session needs
>to be successfully initialised failing to which just a pid is >returned.
>We do not enter pd_update() in the former case to take care of the rest
>of the thread addition process. The pthdb_session_init() is dependent
>on inferior_ptid variable as per our observations to return
>PTHDB_SUCCESS.

I think the change to pd_enable makes sense, passing 1 to pd_activate
seems clearly incorrect now.  Simon, you recently changed pd_activate
to take a PID instead of a boolean - any comments on this?

However, I do not see why the change to ::wait is necessary, or even
correct.  Note that when ::wait calls pd_activate or pd_update, it
already passes the correct pid.  I do not see any path from ::wait
to pd_enable.

Bye,
Ulrich

Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Aditya Kamath1]

[-- Attachment #1: Type: text/plain, Size: 9538 bytes --]

Hi all,

I have fixed the issue with multiple thread detection. Please find attached the patch and explanation.


The problem is in AIX we are not able to debug multiple threads or say multiple threads are not visible in a debug session though they exist in the inferior code.

Fact 1:- In AIX, before we enter into target wait thread code to fetch the pid of the inferior, we switch to no thread using switch_to_no_thread() using switch_to_inferior_no_thread() in inferior.c file.

Fact 2:- How do we add threads in AIX to a single inferior process??

  1.  We start off by initialising the pthread debug library using pthdb_session_pthreaded()
  2.  We then initialise a thread session using pthdb_session_init using pd_activate which will have callback functions to call to read/write memory buffer and symbols.
  3.  We then update our session variable if the  pthdb_session_init() was successful [PTHDB_SUCCESS].

More about this can be read in https://www.ibm.com/docs/en/aix/7.1?topic=programming-developing-multithreaded-program-debuggers
Developing multithreaded program debuggers<https://www.ibm.com/docs/en/aix/7.1?topic=programming-developing-multithreaded-program-debuggers>
The pthread debug library (libpthdebug.a) provides a set of functions that allows developers to provide debug capabilities for applications that use the pthread library.
www.ibm.com

The cause of the bug :- Since, for the GDB core we are switch_to_no_thread() i.e. we do not have a thread till we return the pid from the wait() there is no thread. So, when a call is made from pd_activate() in wait() of aix-thread.c, to pthdb_session_init() we are going to recieve PTHDB_NOT_THREADED.

Reason:- We end up reading the incorrect addresses and writing incorrect ones in pthdb_session_init() call back functions, as switched to no thread in gdb core.

What is the solution:- We should switch to the thread of the pid that has just returned from beneath->wait() using  switch_to_thread() which is what this patch does. Once this is done, before we check pthread debug library is initialised using:


if (!pd_active && status->kind () == TARGET_WAITKIND_STOPPED

      && status->sig () == GDB_SIGNAL_TRAP)

all our registers are well aligned with required information of what addresses and symbols to read/write etc in the  pthdb_session_init() callback functions. Thus this time when we call pthdb_session_init() since we are threaded or switched to a thread, our session is successfully established and pd_update () along with its friend sync_threadlists () will take care of adding threads.

The one more change is just a normal semantic one instead of one we need to pass process ID now.

Let me know what you think.

(See patch:- 0001-Fix-for-multiple-thread-detection-in-AIX.patch)

Thanks and regards,
Aditya.

​-------------------------------------------------------------------------

This can be shown by the following program:-


#include <stdio.h>

#include <unistd.h>

#include <stdlib.h>

#include <pthread.h>

#include <assert.h>


pthread_barrier_t barrier;


#define NUM_THREADS 2


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);


  while (1); /* break here */

}


int

main (void)

{

  int i;


  alarm (300);


  pthread_barrier_init (&barrier, NULL, NUM_THREADS);


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

    {

      pthread_t thread;

      int res;


      res = pthread_create (&thread, NULL,

                            thread_function, NULL);

      assert (res == 0);

    }


  while (1)

    sleep (1);


  return 0;

}

Output without patch:-

(gdb) r

Starting program: /home/aditya/gdb_tests/continue-pending-status

^C

Program received signal SIGINT, Interrupt.

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

(gdb) info threads

  Id   Target Id         Frame

* 1    process 29557240  0xd0595fb0 in _p_nsleep () from /usr/lib/libpthread.a(shr_xpg5.o)

(gdb)



Output with patch:-


Reading symbols from /home/aditya/gdb_tests/continue-pending-status...

(gdb) r

Starting program: /home/aditya/gdb_tests/continue-pending-status

[New Thread 1]

^C[New Thread 258]

[New Thread 515]


Thread 1 received signal SIGINT, Interrupt.

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

(gdb) info threads

  Id   Target Id                           Frame

* 1    process 29557210                    0xd0595fb0 in _p_nsleep ()

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

  2    Thread 1 (tid 120197499, running)   0xd0595fb0 in _p_nsleep ()

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

  3    Thread 258 (tid 130486575, running) thread_function (arg=0x0)

    at continue-pending-status.c:36

  4    Thread 515 (tid 131666371, running) thread_function (arg=warning: (Internal error: pc 0x0 in read in psymtab, but not in symtab.)


0x0)

    at continue-pending-status.c:36

(gdb)


________________________________
From: Aditya Kamath1 <Aditya.Kamath1@ibm.com>
Sent: 22 July 2022 22:33
To: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>; Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; simon.marchi@efficios.com <simon.marchi@efficios.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>; simark@simark.ca <simark@simark.ca>
Subject: Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

Hi all,

This applies in a multithread case.

Why we need inferior_ptid reset in wait?? AIX uses pthread debug library which has to be intialised first. On an event of wait due to a thread creation, once we fetch the pid using waitpid(), the following condition satisfies..


/* Check whether libpthdebug might be ready to be initialized.*/

  if (!pd_active && status->kind () == TARGET_WAITKIND_STOPPED

  && status->sig () == GDB_SIGNAL_TRAP)

 and we enter pd_activate() to. On a successful pthread debug library initialisation we have to initiate a thread debug session using pthdb_session_init(). While we do that, we have call backs to read symbol, data etc. The problem is here where we are not able to read them successfully due to incorrect process ID information. It is not able to fetch the right address to read in pdc_read_data() where we read target_read_memory() and pdc_symbol_addrs().

Kindly read https://www.ibm.com/docs/en/aix/7.1?topic=programming-developing-multithreaded-program-debuggers for more on what I shared.
Developing multithreaded program debuggers<https://www.ibm.com/docs/en/aix/7.1?topic=programming-developing-multithreaded-program-debuggers>
The pthread debug library (libpthdebug.a) provides a set of functions that allows developers to provide debug capabilities for applications that use the pthread library.
www.ibm.com<http://www.ibm.com>

What we are thinking might be the solution??

Since we initialise the gdb with an observer to notify when there is a thread ptid change, on a wait event the inferior_ptid is set to null by switch_to_no_thread () with reinit_cache_frame (). Essentially with this we loose all our pid information needed to read our data and symbols as now our thread PTID is changed to null. So it is essential for us with the way AIX code is designed that we either reset our inferior_ptid in AIX thread wait code before we go out to the non-target dependent code like target.c, infrun.c to do it for us, or use

      thread_change_ptid (process_stratum_target *targ,

  ptid_t old_ptid, ptid_t new_ptid)


to inform GDB so that we can reset and get the right address for target_read_memory() in aix-thread.c file.


It will be great if you could tell us if we are thinking right at this moment.
________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 19 July 2022 17:51
To: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; simon.marchi@efficios.com <simon.marchi@efficios.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>; simark@simark.ca <simark@simark.ca>
Subject: Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

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

>The reason:- Since a new thread addition causes a thread target to
>wait, in AIX once the event ptid is got with the waitpid(), we need to
>set the inferior_ptid variable. Every time we come into
>aix_thread_target::wait() we check if libpthdebug might be ready to be
>initialized.In doing so we call pd_activate(). Here the session needs
>to be successfully initialised failing to which just a pid is >returned.
>We do not enter pd_update() in the former case to take care of the rest
>of the thread addition process. The pthdb_session_init() is dependent
>on inferior_ptid variable as per our observations to return
>PTHDB_SUCCESS.

I think the change to pd_enable makes sense, passing 1 to pd_activate
seems clearly incorrect now.  Simon, you recently changed pd_activate
to take a PID instead of a boolean - any comments on this?

However, I do not see why the change to ::wait is necessary, or even
correct.  Note that when ::wait calls pd_activate or pd_update, it
already passes the correct pid.  I do not see any path from ::wait
to pd_enable.

Bye,
Ulrich


[-- Attachment #2: 0001-Fix-for-multiple-thread-detection-in-AIX.patch --]
[-- Type: application/octet-stream, Size: 1162 bytes --]

From 5c997cb3768948b07e9f505f1a9bbad269746ed7 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Mon, 25 Jul 2022 06:21:27 -0500
Subject: [PATCH] Fix-for-multiple-thread-detection-in-AIX

---
 gdb/aix-thread.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 4c9195a7f12..91466a17647 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -976,7 +976,7 @@ pd_enable (void)
   /* 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 (1);
+  pd_activate (inferior_ptid.pid());
 }
 
 /* Undo the effects of pd_enable().  */
@@ -1088,6 +1088,8 @@ aix_thread_target::wait (ptid_t ptid, struct target_waitstatus *status,
      pid-only ptids.  */
   gdb_assert (ptid.is_pid ());
 
+  switch_to_thread (current_inferior ()->process_target () ,ptid);
+
   /* Check whether libpthdebug might be ready to be initialized.  */
   if (!pd_active && status->kind () == TARGET_WAITKIND_STOPPED
       && status->sig () == GDB_SIGNAL_TRAP)
-- 
2.31.1

Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Ulrich Weigand]

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

>The cause of the bug :- Since, for the GDB core we are
>switch_to_no_thread() i.e. we do not have a thread till we return the
>pid from the wait() there is no thread. So, when a call is made from
>pd_activate() in wait() of aix-thread.c, to pthdb_session_init() we are
>going to recieve PTHDB_NOT_THREADED.

Thanks for the explanation.  I wasn't aware the use of
inferior_ptid happens in some of callback routines used
by the pthdb_session_init() library routine.

I still think the proposed fix isn't really ideal.  Can you instead
try to *temporarily* (i.e. using a scoped_restore) set up inferior_ptid
in pd_activate() before calling pthdb_session_init(), with a comment
explaining that this is needed for the callbacks?

[ pd_activate currently gets a pid.  If this isn't sufficient to set
up inferior_ptid, you might have to change the pd_activate interface
again to take a full ptid. ]


Bye,
Ulrich

Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Simon Marchi]

On 2022-07-25 08:21, Ulrich Weigand wrote:
> 
> Aditya Kamath1 <Aditya.Kamath1@ibm.com> wrote:
> 
>> The cause of the bug :- Since, for the GDB core we are
>> switch_to_no_thread() i.e. we do not have a thread till we return the
>> pid from the wait() there is no thread. So, when a call is made from
>> pd_activate() in wait() of aix-thread.c, to pthdb_session_init() we are
>> going to recieve PTHDB_NOT_THREADED.
> 
> Thanks for the explanation.  I wasn't aware the use of
> inferior_ptid happens in some of callback routines used
> by the pthdb_session_init() library routine.

Thanks, me neither, but it makes sense.

> I still think the proposed fix isn't really ideal.  Can you instead
> try to *temporarily* (i.e. using a scoped_restore) set up inferior_ptid
> in pd_activate() before calling pthdb_session_init(), with a comment
> explaining that this is needed for the callbacks?

That sounds like a good idea, this way, from the point of view of the
caller of pd_activate, pd_activate does not care about global state.
That's how we can do baby steps towards relying less on global state
implicitly.  The next step could be to try to make each individual
callback switch to the right global context, based on what they need.

You just need to be careful, some parts of GDB expect inferior_ptid, the
current thread, the current inferior and the current program space to be
sync'ed.  If you just set inferior_ptid,  you need to make sure to only
call functions that use inferior_ptid, not the other current stuff.
There is not practical way to know this, you have to carefully inspect
what is called.  To avoid this kind of problems, you can temporarily
switch thread (using scoped_restore_current_thread + switch_to_thread),
which will set all the current stuff mentioned above.  But sometimes
this isn't possible, especially in thw wait method, because there isn't
always a thread_info for the ptid you are handling yet, so you can't
switch to it.

Given the AIX target only supports one inferior for now, the current
inferior and program space should be correct.  But to support
multi-inferior, it will be important to keep that in mind.  You might
have to switch to the right inferior in addition to setting
inferior_ptid in pd_acticate.

This hunk in the patch:

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 4c9195a7f12..91466a17647 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -976,7 +976,7 @@ pd_enable (void)
   /* 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 (1);
+  pd_activate (inferior_ptid.pid());
 }

looks right to me (except the missing space before the parenthesis).  It
looks like an oversight in my "gdb: fix
{rs6000_nat_target,aix_thread_target}::wait to not use inferior_ptid"
patch, I forgot to update that call to pd_activate.  Note that the old
parameter to pd_activate was SET_INFPID, and if set, pd_update would
change the current thread to reflect the thread ptid, if thread
debugging was enabled.  The current code no longer does that.  If that
was important, we can re-introduce it here: make pd_enable switch to the
thread with the ptid returned by pd_activate.

Simon

RE: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Aditya Kamath1]

[-- Attachment #1: Type: text/plain, Size: 7635 bytes --]

Hi all,

I thank you for the feedback that was given. It was a nice insight.

Please find attached the new patch. [See Fix-for-multiple-thread-detection-in-AIX.patch ]

However, there are a few things that we had to look in further to what was suggested.

Here are my explanations to what was suggested.

> I still think the proposed fix isn't really ideal.  Can you instead
> try to *temporarily* (i.e. using a scoped_restore) set up inferior_ptid
> in pd_activate() before calling pthdb_session_init(), with a comment
> explaining that this is needed for the callbacks?

This is a nice idea Ulrich and Simon. However, let me take a case of a program creating 2 threads plus OfCourse having a main thread. Let's say the program creates the first thread. This solution works fantastic.

So, what is the problem with it??  We have our pd_active set to 1 in pd_activate(). So, the next time we get into the wait() of aix-thread.c on an event of a new thread, what happens is since pthread debug library is initialised we need not get into pd_activate() again to initialise. Therefore, this condition :


if (!pd_active && status->kind () == TARGET_WAITKIND_STOPPED

      && status->sig () == GDB_SIGNAL_TRAP)

was made... We directly go to the pd_update().. Since the sync_threadlists() also have pthread debug library functions and our current thread is also null,  we end up syncing threadlists with null thread which means our debugger will not reflect the new threads at all or if it does we will get an assertion saying "Assertion `ecs->event_thread->control.trap_expected' failed" simply because only the thread which is allowed to step should create a trap and we on a creation of new thread said to the gdb core that null thread is the one who raised the event of new thread creation and hence the trap..

So, what can be the solution?? It is great if we create a scope and temporarily switch our thread just before you pthdb_session init() in pd_activate() whicj takes care of session initialisation and just before the sync_threadlists() in pd_activate() post which sync threadlists() can give us the right thread who caused an event to the gdb core ..

Kindly see the patch for the same with comments and inferior_ptid.pid () space correction is also made which I needed to as per Simon.

Let me know what you think, if not let's push this so that AIX folks can debug with multiple threads.
----------------------------------------------------------------------------------------------

>>To avoid this kind of problems, you can temporarily
>>switch thread (using scoped_restore_current_thread + switch_to_thread),
>>which will set all the current stuff mentioned above.  But sometimes
>>this isn't possible, especially in thw wait method, because there isn't
>>always a thread_info for the ptid you are handling yet, so you can't
>>switch to it.

Since you all are more experienced than me, I am sure the future issues and solutions will be brightly more visible to all of you than me and I would love to learn that.. Having said that let me assume you might be thinking of a fork() event where in case we return the child process ID and we switch_to_thread(current_target, child_ptid) we might get an assertion saying inf->thread does not exist and rightly so.. That is where the APIs or functions like ourstatus->set_forked(child_ptid) come in picture where we can pass a new process info and then return a parent process ptid who has a thread from beneath->wait to aix_thread:wait() and that way we won't face this issue of having an inferior with no thread when we use switch_to_thread(current_target,ptid)  in AIX for the time being at least..

Hopefully we are thinking in the same terms and the solution for multiple threads is fair.

Have a nice day ahead.

Thank you,

Regards,
Aditya.







________________________________
From: Simon Marchi <simark@simark.ca>
Sent: 25 July 2022 21:00
To: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>; Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; simon.marchi@efficios.com <simon.marchi@efficios.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Subject: [EXTERNAL] Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch



On 2022-07-25 08:21, Ulrich Weigand wrote:
>
> Aditya Kamath1 <Aditya.Kamath1@ibm.com> wrote:
>
>> The cause of the bug :- Since, for the GDB core we are
>> switch_to_no_thread() i.e. we do not have a thread till we return the
>> pid from the wait() there is no thread. So, when a call is made from
>> pd_activate() in wait() of aix-thread.c, to pthdb_session_init() we are
>> going to recieve PTHDB_NOT_THREADED.
>
> Thanks for the explanation.  I wasn't aware the use of
> inferior_ptid happens in some of callback routines used
> by the pthdb_session_init() library routine.

Thanks, me neither, but it makes sense.

> I still think the proposed fix isn't really ideal.  Can you instead
> try to *temporarily* (i.e. using a scoped_restore) set up inferior_ptid
> in pd_activate() before calling pthdb_session_init(), with a comment
> explaining that this is needed for the callbacks?

That sounds like a good idea, this way, from the point of view of the
caller of pd_activate, pd_activate does not care about global state.
That's how we can do baby steps towards relying less on global state
implicitly.  The next step could be to try to make each individual
callback switch to the right global context, based on what they need.

You just need to be careful, some parts of GDB expect inferior_ptid, the
current thread, the current inferior and the current program space to be
sync'ed.  If you just set inferior_ptid,  you need to make sure to only
call functions that use inferior_ptid, not the other current stuff.
There is not practical way to know this, you have to carefully inspect
what is called.  To avoid this kind of problems, you can temporarily
switch thread (using scoped_restore_current_thread + switch_to_thread),
which will set all the current stuff mentioned above.  But sometimes
this isn't possible, especially in thw wait method, because there isn't
always a thread_info for the ptid you are handling yet, so you can't
switch to it.

Given the AIX target only supports one inferior for now, the current
inferior and program space should be correct.  But to support
multi-inferior, it will be important to keep that in mind.  You might
have to switch to the right inferior in addition to setting
inferior_ptid in pd_acticate.

This hunk in the patch:

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 4c9195a7f12..91466a17647 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -976,7 +976,7 @@ pd_enable (void)
   /* 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 (1);
+  pd_activate (inferior_ptid.pid());
 }

looks right to me (except the missing space before the parenthesis).  It
looks like an oversight in my "gdb: fix
{rs6000_nat_target,aix_thread_target}::wait to not use inferior_ptid"
patch, I forgot to update that call to pd_activate.  Note that the old
parameter to pd_activate was SET_INFPID, and if set, pd_update would
change the current thread to reflect the thread ptid, if thread
debugging was enabled.  The current code no longer does that.  If that
was important, we can re-introduce it here: make pd_enable switch to the
thread with the ptid returned by pd_activate.

Simon

[-- Attachment #2: 0001-Fix-for-multiple-thread-detection-in-AIX.patch --]
[-- Type: application/octet-stream, Size: 2076 bytes --]

From 91be783a0ede9fd1fd6f55315cd59803d2910d78 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Fri, 29 Jul 2022 03:36:21 -0500
Subject: [PATCH] Fix-for-multiple-thread-detection-in-AIX

---
 gdb/aix-thread.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 4c9195a7f12..68aad282ee7 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -887,8 +887,15 @@ pd_update (int pid)
   status = pthdb_session_update (pd_session);
   if (status != PTHDB_SUCCESS)
     return ptid_t (pid);
-
-  sync_threadlists (pid);
+  
+  /* This is needed to eliminate the dependency of current thread  
+     which is null so that thread lists sync with the correct current
+     thread. */
+  {
+    scoped_restore_current_thread restore_current_thread;
+    switch_to_thread (current_inferior ()->process_target () ,ptid_t(pid));
+    sync_threadlists (pid);
+  }
 
   /* Define "current thread" as one that just received a trap signal.  */
 
@@ -911,10 +918,17 @@ static ptid_t
 pd_activate (int pid)
 {
   int status;
-		
-  status = pthdb_session_init (PD_USER, arch64 ? PEM_64BIT : PEM_32BIT,
-			       PTHDB_FLAG_REGS, &pd_callbacks, 
-			       &pd_session);
+	
+  /* This is needed to eliminate the dependency of callbacks on current 
+     thread and inferior_ptid. */
+	
+  {
+    scoped_restore_current_thread restore_current_thread;
+    switch_to_thread (current_inferior ()->process_target () ,ptid_t(pid));
+    status = pthdb_session_init (PD_USER, arch64 ? PEM_64BIT : PEM_32BIT,
+			         PTHDB_FLAG_REGS, &pd_callbacks, 
+			         &pd_session);
+  }
   if (status != PTHDB_SUCCESS)
     {
       return ptid_t (pid);
@@ -976,7 +990,7 @@ pd_enable (void)
   /* 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 (1);
+  pd_activate (inferior_ptid.pid ());
 }
 
 /* Undo the effects of pd_enable().  */
-- 
2.31.1

RE: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Aditya Kamath1]

Hi all,

Kindly let me know feedback for the patch in the previous mail.

Thanks and regards,
Aditya
________________________________
From: Gdb-patches <gdb-patches-bounces+aditya.kamath1=ibm.com@sourceware.org> on behalf of Aditya Kamath1 via Gdb-patches <gdb-patches@sourceware.org>
Sent: 29 July 2022 14:53
To: Simon Marchi <simark@simark.ca>; Ulrich Weigand <Ulrich.Weigand@de.ibm.com>; Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; simon.marchi@efficios.com <simon.marchi@efficios.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Subject: [EXTERNAL] RE: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

Hi all,

I thank you for the feedback that was given. It was a nice insight.

Please find attached the new patch. [See Fix-for-multiple-thread-detection-in-AIX.patch ]

However, there are a few things that we had to look in further to what was suggested.

Here are my explanations to what was suggested.

> I still think the proposed fix isn't really ideal.  Can you instead
> try to *temporarily* (i.e. using a scoped_restore) set up inferior_ptid
> in pd_activate() before calling pthdb_session_init(), with a comment
> explaining that this is needed for the callbacks?

This is a nice idea Ulrich and Simon. However, let me take a case of a program creating 2 threads plus OfCourse having a main thread. Let's say the program creates the first thread. This solution works fantastic.

So, what is the problem with it??  We have our pd_active set to 1 in pd_activate(). So, the next time we get into the wait() of aix-thread.c on an event of a new thread, what happens is since pthread debug library is initialised we need not get into pd_activate() again to initialise. Therefore, this condition :


if (!pd_active && status->kind () == TARGET_WAITKIND_STOPPED

      && status->sig () == GDB_SIGNAL_TRAP)

was made... We directly go to the pd_update().. Since the sync_threadlists() also have pthread debug library functions and our current thread is also null,  we end up syncing threadlists with null thread which means our debugger will not reflect the new threads at all or if it does we will get an assertion saying "Assertion `ecs->event_thread->control.trap_expected' failed" simply because only the thread which is allowed to step should create a trap and we on a creation of new thread said to the gdb core that null thread is the one who raised the event of new thread creation and hence the trap..

So, what can be the solution?? It is great if we create a scope and temporarily switch our thread just before you pthdb_session init() in pd_activate() whicj takes care of session initialisation and just before the sync_threadlists() in pd_activate() post which sync threadlists() can give us the right thread who caused an event to the gdb core ..

Kindly see the patch for the same with comments and inferior_ptid.pid () space correction is also made which I needed to as per Simon.

Let me know what you think, if not let's push this so that AIX folks can debug with multiple threads.
----------------------------------------------------------------------------------------------

>>To avoid this kind of problems, you can temporarily
>>switch thread (using scoped_restore_current_thread + switch_to_thread),
>>which will set all the current stuff mentioned above.  But sometimes
>>this isn't possible, especially in thw wait method, because there isn't
>>always a thread_info for the ptid you are handling yet, so you can't
>>switch to it.

Since you all are more experienced than me, I am sure the future issues and solutions will be brightly more visible to all of you than me and I would love to learn that.. Having said that let me assume you might be thinking of a fork() event where in case we return the child process ID and we switch_to_thread(current_target, child_ptid) we might get an assertion saying inf->thread does not exist and rightly so.. That is where the APIs or functions like ourstatus->set_forked(child_ptid) come in picture where we can pass a new process info and then return a parent process ptid who has a thread from beneath->wait to aix_thread:wait() and that way we won't face this issue of having an inferior with no thread when we use switch_to_thread(current_target,ptid)  in AIX for the time being at least..

Hopefully we are thinking in the same terms and the solution for multiple threads is fair.

Have a nice day ahead.

Thank you,

Regards,
Aditya.







________________________________
From: Simon Marchi <simark@simark.ca>
Sent: 25 July 2022 21:00
To: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>; Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; simon.marchi@efficios.com <simon.marchi@efficios.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Subject: [EXTERNAL] Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch



On 2022-07-25 08:21, Ulrich Weigand wrote:
>
> Aditya Kamath1 <Aditya.Kamath1@ibm.com> wrote:
>
>> The cause of the bug :- Since, for the GDB core we are
>> switch_to_no_thread() i.e. we do not have a thread till we return the
>> pid from the wait() there is no thread. So, when a call is made from
>> pd_activate() in wait() of aix-thread.c, to pthdb_session_init() we are
>> going to recieve PTHDB_NOT_THREADED.
>
> Thanks for the explanation.  I wasn't aware the use of
> inferior_ptid happens in some of callback routines used
> by the pthdb_session_init() library routine.

Thanks, me neither, but it makes sense.

> I still think the proposed fix isn't really ideal.  Can you instead
> try to *temporarily* (i.e. using a scoped_restore) set up inferior_ptid
> in pd_activate() before calling pthdb_session_init(), with a comment
> explaining that this is needed for the callbacks?

That sounds like a good idea, this way, from the point of view of the
caller of pd_activate, pd_activate does not care about global state.
That's how we can do baby steps towards relying less on global state
implicitly.  The next step could be to try to make each individual
callback switch to the right global context, based on what they need.

You just need to be careful, some parts of GDB expect inferior_ptid, the
current thread, the current inferior and the current program space to be
sync'ed.  If you just set inferior_ptid,  you need to make sure to only
call functions that use inferior_ptid, not the other current stuff.
There is not practical way to know this, you have to carefully inspect
what is called.  To avoid this kind of problems, you can temporarily
switch thread (using scoped_restore_current_thread + switch_to_thread),
which will set all the current stuff mentioned above.  But sometimes
this isn't possible, especially in thw wait method, because there isn't
always a thread_info for the ptid you are handling yet, so you can't
switch to it.

Given the AIX target only supports one inferior for now, the current
inferior and program space should be correct.  But to support
multi-inferior, it will be important to keep that in mind.  You might
have to switch to the right inferior in addition to setting
inferior_ptid in pd_acticate.

This hunk in the patch:

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 4c9195a7f12..91466a17647 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -976,7 +976,7 @@ pd_enable (void)
   /* 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 (1);
+  pd_activate (inferior_ptid.pid());
 }

looks right to me (except the missing space before the parenthesis).  It
looks like an oversight in my "gdb: fix
{rs6000_nat_target,aix_thread_target}::wait to not use inferior_ptid"
patch, I forgot to update that call to pd_activate.  Note that the old
parameter to pd_activate was SET_INFPID, and if set, pd_update would
change the current thread to reflect the thread ptid, if thread
debugging was enabled.  The current code no longer does that.  If that
was important, we can re-introduce it here: make pd_enable switch to the
thread with the ptid returned by pd_activate.

Simon

Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Ulrich Weigand]

Hi Aditya,

thanks for the update. I'd like to still understand a bit better exactly where the current thread is required to be set, so we can keep those scopes as small as possible. Is is really all of sync_threadlists, or is it only the first part, where the pthdb_ calls are made? If so, we should move the scope to cover only that part.


Note that another, even better approach would be to move those scopes *into the callback routines* that actually need it. That would require a means to pass information into those callbacks - maybe we can use pthdb_user_t for this purpose, e.g. by passing the PID there instead of always just a constant PD_USER? This would be equivalent to what's done on Linux - you can check the callbacks used for the Linux thread library in proc-service.c. Note that only ps_xfer_memory and ps_pglobal_lookup set up scopes there.


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand | Phone: +49-7031/16-3727
  Distinguished Engineer, Open source compilers and toolchain
  IBM Deutschland Research & Development GmbH
  Vors. des Aufsichtsrats: Gregor Pillen | Geschäftsführung: David Faller
  Sitz d. Ges.: Böblingen | Registergericht: Amtsgericht Stuttgart, HRB 243294

-----Original Message-----
From: Aditya Kamath1 <Aditya.Kamath1@ibm.com<mailto:Aditya%20Kamath1%20%3cAditya.Kamath1@ibm.com%3e>>
To: Simon Marchi <simark@simark.ca<mailto:Simon%20Marchi%20%3csimark@simark.ca%3e>>, Ulrich Weigand <Ulrich.Weigand@de.ibm.com<mailto:Ulrich%20Weigand%20%3cUlrich.Weigand@de.ibm.com%3e>>, Sangamesh Mallayya <sangamesh.swamy@in.ibm.com<mailto:Sangamesh%20Mallayya%20%3csangamesh.swamy@in.ibm.com%3e>>, simon.marchi@efficios.com <simon.marchi@efficios.com<mailto:%22simon.marchi@efficios.com%22%20%3csimon.marchi@efficios.com%3e>>, gdb-patches@sourceware.org <gdb-patches@sourceware.org<mailto:%22gdb-patches@sourceware.org%22%20%3cgdb-patches@sourceware.org%3e>>
Subject: Re: [EXTERNAL] Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch
Date: Fri, 29 Jul 2022 09:23:55 +0000

Hi all,

I thank you for the feedback that was given. It was a nice insight.

Please find attached the new patch. [See Fix-for-multiple-thread-detection-in-AIX.patch ]

However, there are a few things that we had to look in further to what was suggested.

Here are my explanations to what was suggested.

> I still think the proposed fix isn't really ideal.  Can you instead
> try to *temporarily* (i.e. using a scoped_restore) set up inferior_ptid
> in pd_activate() before calling pthdb_session_init(), with a comment
> explaining that this is needed for the callbacks?

This is a nice idea Ulrich and Simon. However, let me take a case of a program creating 2 threads plus OfCourse having a main thread. Let's say the program creates the first thread. This solution works fantastic.

So, what is the problem with it??  We have our pd_active set to 1 in pd_activate(). So, the next time we get into the wait() of aix-thread.c on an event of a new thread, what happens is since pthread debug library is initialised we need not get into pd_activate() again to initialise. Therefore, this condition :


if (!pd_active && status->kind () == TARGET_WAITKIND_STOPPED

      && status->sig () == GDB_SIGNAL_TRAP)

was made... We directly go to the pd_update().. Since the sync_threadlists() also have pthread debug library functions and our current thread is also null,  we end up syncing threadlists with null thread which means our debugger will not reflect the new threads at all or if it does we will get an assertion saying "Assertion `ecs->event_thread->control.trap_expected' failed" simply because only the thread which is allowed to step should create a trap and we on a creation of new thread said to the gdb core that null thread is the one who raised the event of new thread creation and hence the trap..

So, what can be the solution?? It is great if we create a scope and temporarily switch our thread just before you pthdb_session init() in pd_activate() whicj takes care of session initialisation and just before the sync_threadlists() in pd_activate() post which sync threadlists() can give us the right thread who caused an event to the gdb core ..

Kindly see the patch for the same with comments and inferior_ptid.pid () space correction is also made which I needed to as per Simon.

Let me know what you think, if not let's push this so that AIX folks can debug with multiple threads.
----------------------------------------------------------------------------------------------

>>To avoid this kind of problems, you can temporarily
>>switch thread (using scoped_restore_current_thread + switch_to_thread),
>>which will set all the current stuff mentioned above.  But sometimes
>>this isn't possible, especially in thw wait method, because there isn't
>>always a thread_info for the ptid you are handling yet, so you can't
>>switch to it.

Since you all are more experienced than me, I am sure the future issues and solutions will be brightly more visible to all of you than me and I would love to learn that.. Having said that let me assume you might be thinking of a fork() event where in case we return the child process ID and we switch_to_thread(current_target, child_ptid) we might get an assertion saying inf->thread does not exist and rightly so.. That is where the APIs or functions like ourstatus->set_forked(child_ptid) come in picture where we can pass a new process info and then return a parent process ptid who has a thread from beneath->wait to aix_thread:wait() and that way we won't face this issue of having an inferior with no thread when we use switch_to_thread(current_target,ptid)  in AIX for the time being at least..

Hopefully we are thinking in the same terms and the solution for multiple threads is fair.

Have a nice day ahead.

Thank you,

Regards,
Aditya.







________________________________
From: Simon Marchi <simark@simark.ca>
Sent: 25 July 2022 21:00
To: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>; Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; simon.marchi@efficios.com <simon.marchi@efficios.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Subject: [EXTERNAL] Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch



On 2022-07-25 08:21, Ulrich Weigand wrote:
>
> Aditya Kamath1 <Aditya.Kamath1@ibm.com> wrote:
>
>> The cause of the bug :- Since, for the GDB core we are
>> switch_to_no_thread() i.e. we do not have a thread till we return the
>> pid from the wait() there is no thread. So, when a call is made from
>> pd_activate() in wait() of aix-thread.c, to pthdb_session_init() we are
>> going to recieve PTHDB_NOT_THREADED.
>
> Thanks for the explanation.  I wasn't aware the use of
> inferior_ptid happens in some of callback routines used
> by the pthdb_session_init() library routine.

Thanks, me neither, but it makes sense.

> I still think the proposed fix isn't really ideal.  Can you instead
> try to *temporarily* (i.e. using a scoped_restore) set up inferior_ptid
> in pd_activate() before calling pthdb_session_init(), with a comment
> explaining that this is needed for the callbacks?

That sounds like a good idea, this way, from the point of view of the
caller of pd_activate, pd_activate does not care about global state.
That's how we can do baby steps towards relying less on global state
implicitly.  The next step could be to try to make each individual
callback switch to the right global context, based on what they need.

You just need to be careful, some parts of GDB expect inferior_ptid, the
current thread, the current inferior and the current program space to be
sync'ed.  If you just set inferior_ptid,  you need to make sure to only
call functions that use inferior_ptid, not the other current stuff.
There is not practical way to know this, you have to carefully inspect
what is called.  To avoid this kind of problems, you can temporarily
switch thread (using scoped_restore_current_thread + switch_to_thread),
which will set all the current stuff mentioned above.  But sometimes
this isn't possible, especially in thw wait method, because there isn't
always a thread_info for the ptid you are handling yet, so you can't
switch to it.

Given the AIX target only supports one inferior for now, the current
inferior and program space should be correct.  But to support
multi-inferior, it will be important to keep that in mind.  You might
have to switch to the right inferior in addition to setting
inferior_ptid in pd_acticate.

This hunk in the patch:

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 4c9195a7f12..91466a17647 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -976,7 +976,7 @@ pd_enable (void)
   /* 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 (1);
+  pd_activate (inferior_ptid.pid());
 }

looks right to me (except the missing space before the parenthesis).  It
looks like an oversight in my "gdb: fix
{rs6000_nat_target,aix_thread_target}::wait to not use inferior_ptid"
patch, I forgot to update that call to pd_activate.  Note that the old
parameter to pd_activate was SET_INFPID, and if set, pd_update would
change the current thread to reflect the thread ptid, if thread
debugging was enabled.  The current code no longer does that.  If that
was important, we can re-introduce it here: make pd_enable switch to the
thread with the ptid returned by pd_activate.

Simon

Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Aditya Kamath1]

[-- Attachment #1: Type: text/plain, Size: 11094 bytes --]

Hi Simon and Ulrich,

Thank you for the suggestions.

Please find attached the patch. [ See 0001-Fix-for-multiple-thread-detection-in-AIX.patch]

I have moved the scope + switch_to_current_thread to pdc_read_data() where the dependency is needed in the call-backs as suggested by Ulrich. I also eliminated PD_USER variable as it is no longer needed. As far as the user_current_pid != 0 condition is concerned, we need this check during initialisation during the birth of the first inferior. We passed a parameter inferior_ptid.pid () in pd_enable () which is 0 at that time.

Since we do not need PD_USER and having pthdb_user_t user as the name might be look vague to someone trying to understand, I have changed it to user_current_pid.

I think this solves the problem. Let me know what you guys think. If it looks good we can push this so that folks using AIX can now see multi thread debugging.

Have a nice day.

Thanks and regards,
Aditya.
________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 03 August 2022 21:52
To: simark@simark.ca <simark@simark.ca>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; simon.marchi@efficios.com <simon.marchi@efficios.com>; Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Subject: Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

Hi Aditya,

thanks for the update. I'd like to still understand a bit better exactly where the current thread is required to be set, so we can keep those scopes as small as possible. Is is really all of sync_threadlists, or is it only the first part, where the pthdb_ calls are made? If so, we should move the scope to cover only that part.


Note that another, even better approach would be to move those scopes *into the callback routines* that actually need it. That would require a means to pass information into those callbacks - maybe we can use pthdb_user_t for this purpose, e.g. by passing the PID there instead of always just a constant PD_USER? This would be equivalent to what's done on Linux - you can check the callbacks used for the Linux thread library in proc-service.c. Note that only ps_xfer_memory and ps_pglobal_lookup set up scopes there.


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand | Phone: +49-7031/16-3727
  Distinguished Engineer, Open source compilers and toolchain
  IBM Deutschland Research & Development GmbH
  Vors. des Aufsichtsrats: Gregor Pillen | Geschäftsführung: David Faller
  Sitz d. Ges.: Böblingen | Registergericht: Amtsgericht Stuttgart, HRB 243294

-----Original Message-----
From: Aditya Kamath1 <Aditya.Kamath1@ibm.com<mailto:Aditya%20Kamath1%20%3cAditya.Kamath1@ibm.com%3e>>
To: Simon Marchi <simark@simark.ca<mailto:Simon%20Marchi%20%3csimark@simark.ca%3e>>, Ulrich Weigand <Ulrich.Weigand@de.ibm.com<mailto:Ulrich%20Weigand%20%3cUlrich.Weigand@de.ibm.com%3e>>, Sangamesh Mallayya <sangamesh.swamy@in.ibm.com<mailto:Sangamesh%20Mallayya%20%3csangamesh.swamy@in.ibm.com%3e>>, simon.marchi@efficios.com <simon.marchi@efficios.com<mailto:%22simon.marchi@efficios.com%22%20%3csimon.marchi@efficios.com%3e>>, gdb-patches@sourceware.org <gdb-patches@sourceware.org<mailto:%22gdb-patches@sourceware.org%22%20%3cgdb-patches@sourceware.org%3e>>
Subject: Re: [EXTERNAL] Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch
Date: Fri, 29 Jul 2022 09:23:55 +0000

Hi all,

I thank you for the feedback that was given. It was a nice insight.

Please find attached the new patch. [See Fix-for-multiple-thread-detection-in-AIX.patch ]

However, there are a few things that we had to look in further to what was suggested.

Here are my explanations to what was suggested.

> I still think the proposed fix isn't really ideal.  Can you instead
> try to *temporarily* (i.e. using a scoped_restore) set up inferior_ptid
> in pd_activate() before calling pthdb_session_init(), with a comment
> explaining that this is needed for the callbacks?

This is a nice idea Ulrich and Simon. However, let me take a case of a program creating 2 threads plus OfCourse having a main thread. Let's say the program creates the first thread. This solution works fantastic.

So, what is the problem with it??  We have our pd_active set to 1 in pd_activate(). So, the next time we get into the wait() of aix-thread.c on an event of a new thread, what happens is since pthread debug library is initialised we need not get into pd_activate() again to initialise. Therefore, this condition :


if (!pd_active && status->kind () == TARGET_WAITKIND_STOPPED

      && status->sig () == GDB_SIGNAL_TRAP)

was made... We directly go to the pd_update().. Since the sync_threadlists() also have pthread debug library functions and our current thread is also null,  we end up syncing threadlists with null thread which means our debugger will not reflect the new threads at all or if it does we will get an assertion saying "Assertion `ecs->event_thread->control.trap_expected' failed" simply because only the thread which is allowed to step should create a trap and we on a creation of new thread said to the gdb core that null thread is the one who raised the event of new thread creation and hence the trap..

So, what can be the solution?? It is great if we create a scope and temporarily switch our thread just before you pthdb_session init() in pd_activate() whicj takes care of session initialisation and just before the sync_threadlists() in pd_activate() post which sync threadlists() can give us the right thread who caused an event to the gdb core ..

Kindly see the patch for the same with comments and inferior_ptid.pid () space correction is also made which I needed to as per Simon.

Let me know what you think, if not let's push this so that AIX folks can debug with multiple threads.
----------------------------------------------------------------------------------------------

>>To avoid this kind of problems, you can temporarily
>>switch thread (using scoped_restore_current_thread + switch_to_thread),
>>which will set all the current stuff mentioned above.  But sometimes
>>this isn't possible, especially in thw wait method, because there isn't
>>always a thread_info for the ptid you are handling yet, so you can't
>>switch to it.

Since you all are more experienced than me, I am sure the future issues and solutions will be brightly more visible to all of you than me and I would love to learn that.. Having said that let me assume you might be thinking of a fork() event where in case we return the child process ID and we switch_to_thread(current_target, child_ptid) we might get an assertion saying inf->thread does not exist and rightly so.. That is where the APIs or functions like ourstatus->set_forked(child_ptid) come in picture where we can pass a new process info and then return a parent process ptid who has a thread from beneath->wait to aix_thread:wait() and that way we won't face this issue of having an inferior with no thread when we use switch_to_thread(current_target,ptid)  in AIX for the time being at least..

Hopefully we are thinking in the same terms and the solution for multiple threads is fair.

Have a nice day ahead.

Thank you,

Regards,
Aditya.







________________________________
From: Simon Marchi <simark@simark.ca>
Sent: 25 July 2022 21:00
To: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>; Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; simon.marchi@efficios.com <simon.marchi@efficios.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Subject: [EXTERNAL] Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch



On 2022-07-25 08:21, Ulrich Weigand wrote:
>
> Aditya Kamath1 <Aditya.Kamath1@ibm.com> wrote:
>
>> The cause of the bug :- Since, for the GDB core we are
>> switch_to_no_thread() i.e. we do not have a thread till we return the
>> pid from the wait() there is no thread. So, when a call is made from
>> pd_activate() in wait() of aix-thread.c, to pthdb_session_init() we are
>> going to recieve PTHDB_NOT_THREADED.
>
> Thanks for the explanation.  I wasn't aware the use of
> inferior_ptid happens in some of callback routines used
> by the pthdb_session_init() library routine.

Thanks, me neither, but it makes sense.

> I still think the proposed fix isn't really ideal.  Can you instead
> try to *temporarily* (i.e. using a scoped_restore) set up inferior_ptid
> in pd_activate() before calling pthdb_session_init(), with a comment
> explaining that this is needed for the callbacks?

That sounds like a good idea, this way, from the point of view of the
caller of pd_activate, pd_activate does not care about global state.
That's how we can do baby steps towards relying less on global state
implicitly.  The next step could be to try to make each individual
callback switch to the right global context, based on what they need.

You just need to be careful, some parts of GDB expect inferior_ptid, the
current thread, the current inferior and the current program space to be
sync'ed.  If you just set inferior_ptid,  you need to make sure to only
call functions that use inferior_ptid, not the other current stuff.
There is not practical way to know this, you have to carefully inspect
what is called.  To avoid this kind of problems, you can temporarily
switch thread (using scoped_restore_current_thread + switch_to_thread),
which will set all the current stuff mentioned above.  But sometimes
this isn't possible, especially in thw wait method, because there isn't
always a thread_info for the ptid you are handling yet, so you can't
switch to it.

Given the AIX target only supports one inferior for now, the current
inferior and program space should be correct.  But to support
multi-inferior, it will be important to keep that in mind.  You might
have to switch to the right inferior in addition to setting
inferior_ptid in pd_acticate.

This hunk in the patch:

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 4c9195a7f12..91466a17647 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -976,7 +976,7 @@ pd_enable (void)
   /* 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 (1);
+  pd_activate (inferior_ptid.pid());
 }

looks right to me (except the missing space before the parenthesis).  It
looks like an oversight in my "gdb: fix
{rs6000_nat_target,aix_thread_target}::wait to not use inferior_ptid"
patch, I forgot to update that call to pd_activate.  Note that the old
parameter to pd_activate was SET_INFPID, and if set, pd_update would
change the current thread to reflect the thread ptid, if thread
debugging was enabled.  The current code no longer does that.  If that
was important, we can re-introduce it here: make pd_enable switch to the
thread with the ptid returned by pd_activate.

Simon

[-- Attachment #2: 0001-Fix-for-multiple-thread-detection-in-AIX.patch --]
[-- Type: application/octet-stream, Size: 8354 bytes --]

From 8104e50aa29a3e99ccd12e369dd7dfdb1b3eeedf Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Thu, 4 Aug 2022 09:59:59 -0500
Subject: [PATCH] Fix-for-multiple-thread-detection-in-AIX.

In AIX multiple threads were not added. This patch is a fix for the same

When we create a pthread debug session we have callbacks to read symbols and memory.

One of those call backs is pdc_read_data.

Before we come into aix-thread wait() we switch to no thread and therefore the current thread is null.

When we get into pdc_read_data we have a dependency that we need to be in the correct current thread that has caused an event of new thread,

inorder to read memory.

Hence we switch to the correct thread.

This is done by passing the pid in the pthdb_user_t user_current_pid parameter in every call back.
---
 gdb/aix-thread.c | 72 ++++++++++++++++++++++++++----------------------
 1 file changed, 39 insertions(+), 33 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 4c9195a7f12..2586e59507c 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -72,11 +72,6 @@ static bool debug_aix_thread;
 
 #define PD_TID(ptid)	(pd_active && ptid.tid () != 0)
 
-/* pthdb_user_t value that we pass to pthdb functions.  0 causes
-   PTHDB_BAD_USER errors, so use 1.  */
-
-#define PD_USER	1
-
 /* Success and failure values returned by pthdb callbacks.  */
 
 #define PDC_SUCCESS	PTHDB_SUCCESS
@@ -331,7 +326,7 @@ pid_to_prc (ptid_t *ptidp)
    the address of SYMBOLS[<i>].name.  */
 
 static int
-pdc_symbol_addrs (pthdb_user_t user, pthdb_symbol_t *symbols, int count)
+pdc_symbol_addrs (pthdb_user_t user_current_pid, pthdb_symbol_t *symbols, int count)
 {
   struct bound_minimal_symbol ms;
   int i;
@@ -339,8 +334,8 @@ pdc_symbol_addrs (pthdb_user_t user, pthdb_symbol_t *symbols, int count)
 
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_symbol_addrs (user = %ld, symbols = 0x%lx, count = %d)\n",
-		user, (long) symbols, count);
+		"pdc_symbol_addrs (user_current_pid = %ld, symbols = 0x%lx, count = %d)\n",
+		user_current_pid, (long) symbols, count);
 
   for (i = 0; i < count; i++)
     {
@@ -353,7 +348,7 @@ pdc_symbol_addrs (pthdb_user_t user, pthdb_symbol_t *symbols, int count)
 	symbols[i].addr = 0;
       else
 	{
-	  ms = lookup_minimal_symbol (name, NULL, NULL);
+         ms = lookup_minimal_symbol (name, NULL, NULL);
 	  if (ms.minsym == NULL)
 	    {
 	      if (debug_aix_thread)
@@ -378,7 +373,7 @@ pdc_symbol_addrs (pthdb_user_t user, pthdb_symbol_t *symbols, int count)
    If successful return 0, else non-zero is returned.  */
 
 static int
-pdc_read_regs (pthdb_user_t user, 
+pdc_read_regs (pthdb_user_t user_current_pid, 
 	       pthdb_tid_t tid,
 	       unsigned long long flags,
 	       pthdb_context_t *context)
@@ -450,7 +445,7 @@ pdc_read_regs (pthdb_user_t user,
    If successful return 0, else non-zero is returned.  */
 
 static int
-pdc_write_regs (pthdb_user_t user,
+pdc_write_regs (pthdb_user_t user_current_pid,
 		pthdb_tid_t tid,
 		unsigned long long flags,
 		pthdb_context_t *context)
@@ -500,17 +495,27 @@ pdc_write_regs (pthdb_user_t user,
 /* pthdb callback: read LEN bytes from process ADDR into BUF.  */
 
 static int
-pdc_read_data (pthdb_user_t user, void *buf, 
+pdc_read_data (pthdb_user_t user_current_pid, void *buf, 
 	       pthdb_addr_t addr, size_t len)
 {
   int status, ret;
 
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_read_data (user = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
-		user, (long) buf, hex_string (addr), len);
+		"pdc_read_data (user_current_pid = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
+		user_current_pid, (long) buf, hex_string (addr), len);
 
-  status = target_read_memory (addr, (gdb_byte *) buf, len);
+  /* 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;
+    /* 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));
+    status = target_read_memory (addr, (gdb_byte *) buf, len);
+  }
   ret = status == 0 ? PDC_SUCCESS : PDC_FAILURE;
 
   if (debug_aix_thread)
@@ -522,15 +527,15 @@ pdc_read_data (pthdb_user_t user, void *buf,
 /* pthdb callback: write LEN bytes from BUF to process ADDR.  */
 
 static int
-pdc_write_data (pthdb_user_t user, void *buf, 
+pdc_write_data (pthdb_user_t user_current_pid, void *buf, 
 		pthdb_addr_t addr, size_t len)
 {
   int status, ret;
 
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_write_data (user = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
-		user, (long) buf, hex_string (addr), len);
+		"pdc_write_data (user_current_pid = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
+		user_current_pid, (long) buf, hex_string (addr), len);
 
   status = target_write_memory (addr, (gdb_byte *) buf, len);
   ret = status == 0 ? PDC_SUCCESS : PDC_FAILURE;
@@ -545,12 +550,12 @@ pdc_write_data (pthdb_user_t user, void *buf,
    in BUFP.  */
 
 static int
-pdc_alloc (pthdb_user_t user, size_t len, void **bufp)
+pdc_alloc (pthdb_user_t user_current_pid, size_t len, void **bufp)
 {
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_alloc (user = %ld, len = %ld, bufp = 0x%lx)\n",
-		user, len, (long) bufp);
+		"pdc_alloc (user_current_pid = %ld, len = %ld, bufp = 0x%lx)\n",
+		user_current_pid, len, (long) bufp);
   *bufp = xmalloc (len);
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog, 
@@ -567,12 +572,12 @@ pdc_alloc (pthdb_user_t user, size_t len, void **bufp)
    pointer to the result in BUFP.  */
 
 static int
-pdc_realloc (pthdb_user_t user, void *buf, size_t len, void **bufp)
+pdc_realloc (pthdb_user_t user_current_pid, void *buf, size_t len, void **bufp)
 {
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_realloc (user = %ld, buf = 0x%lx, len = %ld, bufp = 0x%lx)\n",
-		user, (long) buf, len, (long) bufp);
+		"pdc_realloc (user_current_pid = %ld, buf = 0x%lx, len = %ld, bufp = 0x%lx)\n",
+		user_current_pid, (long) buf, len, (long) bufp);
   *bufp = xrealloc (buf, len);
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog, 
@@ -584,11 +589,11 @@ pdc_realloc (pthdb_user_t user, void *buf, size_t len, void **bufp)
    realloc callback.  */
 
 static int
-pdc_dealloc (pthdb_user_t user, void *buf)
+pdc_dealloc (pthdb_user_t user_current_pid, void *buf)
 {
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog, 
-		"pdc_free (user = %ld, buf = 0x%lx)\n", user,
+		"pdc_free (user_current_pid = %ld, buf = 0x%lx)\n", user_current_pid,
 		(long) buf);
   xfree (buf);
   return PDC_SUCCESS;
@@ -887,7 +892,6 @@ pd_update (int pid)
   status = pthdb_session_update (pd_session);
   if (status != PTHDB_SUCCESS)
     return ptid_t (pid);
-
   sync_threadlists (pid);
 
   /* Define "current thread" as one that just received a trap signal.  */
@@ -911,10 +915,11 @@ static ptid_t
 pd_activate (int pid)
 {
   int status;
-		
-  status = pthdb_session_init (PD_USER, arch64 ? PEM_64BIT : PEM_32BIT,
-			       PTHDB_FLAG_REGS, &pd_callbacks, 
-			       &pd_session);
+	
+  status = pthdb_session_init (pid, arch64 ? PEM_64BIT : PEM_32BIT,
+	  		           PTHDB_FLAG_REGS, &pd_callbacks, 
+			           &pd_session);
+  
   if (status != PTHDB_SUCCESS)
     {
       return ptid_t (pid);
@@ -955,8 +960,9 @@ pd_enable (void)
 
   /* Check whether the application is pthreaded.  */
   stub_name = NULL;
-  status = pthdb_session_pthreaded (PD_USER, PTHDB_FLAG_REGS,
+  status = pthdb_session_pthreaded (inferior_ptid.pid (), PTHDB_FLAG_REGS,
 				    &pd_callbacks, &stub_name);
+  
   if ((status != PTHDB_SUCCESS
        && status != PTHDB_NOT_PTHREADED) || !stub_name)
     return;
@@ -976,7 +982,7 @@ pd_enable (void)
   /* 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 (1);
+  pd_activate (inferior_ptid.pid ());
 }
 
 /* Undo the effects of pd_enable().  */
-- 
2.31.1

Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Aditya Kamath1]

[-- Attachment #1: Type: text/plain, Size: 12718 bytes --]

Hi Simon and Ulrich,

Kindly ignore the previous patch. There was spacing issues from my end in the same. I apologise for it.

Thank you for the suggestions.

Please find attached the patch. [ See 0001-Fix-for-multiple-thread-detection-in-AIX.patch]

I have moved the scope + switch_to_current_thread to pdc_read_data() where the dependency is needed in the call-backs as suggested by Ulrich. I also eliminated PD_USER variable as it is no longer needed. As far as the user_current_pid != 0 condition is concerned, we need this check during initialisation during the birth of the first inferior. We passed a parameter inferior_ptid.pid () in pd_enable () which is 0 at that time.

Since we do not need PD_USER and having pthdb_user_t user as the name might be look vague to someone trying to understand, I have changed it to user_current_pid.

I think this solves the problem. Let me know what you all think. If it looks good, we can push this patch, so that folks using AIX can now see multi thread debugging.

Have a nice day ahead.

Thanks and regards,
Aditya.
________________________________
From: Gdb-patches <gdb-patches-bounces+aditya.kamath1=ibm.com@sourceware.org> on behalf of Aditya Kamath1 via Gdb-patches <gdb-patches@sourceware.org>
Sent: 04 August 2022 20:45
To: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>; simark@simark.ca <simark@simark.ca>; simon.marchi@efficios.com <simon.marchi@efficios.com>; Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Subject: [EXTERNAL] Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

Hi Simon and Ulrich,

Thank you for the suggestions.

Please find attached the patch. [ See 0001-Fix-for-multiple-thread-detection-in-AIX.patch]

I have moved the scope + switch_to_current_thread to pdc_read_data() where the dependency is needed in the call-backs as suggested by Ulrich. I also eliminated PD_USER variable as it is no longer needed. As far as the user_current_pid != 0 condition is concerned, we need this check during initialisation during the birth of the first inferior. We passed a parameter inferior_ptid.pid () in pd_enable () which is 0 at that time.

Since we do not need PD_USER and having pthdb_user_t user as the name might be look vague to someone trying to understand, I have changed it to user_current_pid.

I think this solves the problem. Let me know what you guys think. If it looks good we can push this so that folks using AIX can now see multi thread debugging.

Have a nice day.

Thanks and regards,
Aditya.
________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 03 August 2022 21:52
To: simark@simark.ca <simark@simark.ca>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; simon.marchi@efficios.com <simon.marchi@efficios.com>; Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Subject: Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

Hi Aditya,

thanks for the update. I'd like to still understand a bit better exactly where the current thread is required to be set, so we can keep those scopes as small as possible. Is is really all of sync_threadlists, or is it only the first part, where the pthdb_ calls are made? If so, we should move the scope to cover only that part.


Note that another, even better approach would be to move those scopes *into the callback routines* that actually need it. That would require a means to pass information into those callbacks - maybe we can use pthdb_user_t for this purpose, e.g. by passing the PID there instead of always just a constant PD_USER? This would be equivalent to what's done on Linux - you can check the callbacks used for the Linux thread library in proc-service.c. Note that only ps_xfer_memory and ps_pglobal_lookup set up scopes there.


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand | Phone: +49-7031/16-3727
  Distinguished Engineer, Open source compilers and toolchain
  IBM Deutschland Research & Development GmbH
  Vors. des Aufsichtsrats: Gregor Pillen | Geschäftsführung: David Faller
  Sitz d. Ges.: Böblingen | Registergericht: Amtsgericht Stuttgart, HRB 243294

-----Original Message-----
From: Aditya Kamath1 <Aditya.Kamath1@ibm.com<mailto:Aditya%20Kamath1%20%3cAditya.Kamath1@ibm.com%3e>>
To: Simon Marchi <simark@simark.ca<mailto:Simon%20Marchi%20%3csimark@simark.ca%3e>>, Ulrich Weigand <Ulrich.Weigand@de.ibm.com<mailto:Ulrich%20Weigand%20%3cUlrich.Weigand@de.ibm.com%3e>>, Sangamesh Mallayya <sangamesh.swamy@in.ibm.com<mailto:Sangamesh%20Mallayya%20%3csangamesh.swamy@in.ibm.com%3e>>, simon.marchi@efficios.com <simon.marchi@efficios.com<mailto:%22simon.marchi@efficios.com%22%20%3csimon.marchi@efficios.com%3e>>, gdb-patches@sourceware.org <gdb-patches@sourceware.org<mailto:%22gdb-patches@sourceware.org%22%20%3cgdb-patches@sourceware.org%3e>>
Subject: Re: [EXTERNAL] Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch
Date: Fri, 29 Jul 2022 09:23:55 +0000

Hi all,

I thank you for the feedback that was given. It was a nice insight.

Please find attached the new patch. [See Fix-for-multiple-thread-detection-in-AIX.patch ]

However, there are a few things that we had to look in further to what was suggested.

Here are my explanations to what was suggested.

> I still think the proposed fix isn't really ideal.  Can you instead
> try to *temporarily* (i.e. using a scoped_restore) set up inferior_ptid
> in pd_activate() before calling pthdb_session_init(), with a comment
> explaining that this is needed for the callbacks?

This is a nice idea Ulrich and Simon. However, let me take a case of a program creating 2 threads plus OfCourse having a main thread. Let's say the program creates the first thread. This solution works fantastic.

So, what is the problem with it??  We have our pd_active set to 1 in pd_activate(). So, the next time we get into the wait() of aix-thread.c on an event of a new thread, what happens is since pthread debug library is initialised we need not get into pd_activate() again to initialise. Therefore, this condition :


if (!pd_active && status->kind () == TARGET_WAITKIND_STOPPED

      && status->sig () == GDB_SIGNAL_TRAP)

was made... We directly go to the pd_update().. Since the sync_threadlists() also have pthread debug library functions and our current thread is also null,  we end up syncing threadlists with null thread which means our debugger will not reflect the new threads at all or if it does we will get an assertion saying "Assertion `ecs->event_thread->control.trap_expected' failed" simply because only the thread which is allowed to step should create a trap and we on a creation of new thread said to the gdb core that null thread is the one who raised the event of new thread creation and hence the trap..

So, what can be the solution?? It is great if we create a scope and temporarily switch our thread just before you pthdb_session init() in pd_activate() whicj takes care of session initialisation and just before the sync_threadlists() in pd_activate() post which sync threadlists() can give us the right thread who caused an event to the gdb core ..

Kindly see the patch for the same with comments and inferior_ptid.pid () space correction is also made which I needed to as per Simon.

Let me know what you think, if not let's push this so that AIX folks can debug with multiple threads.
----------------------------------------------------------------------------------------------

>>To avoid this kind of problems, you can temporarily
>>switch thread (using scoped_restore_current_thread + switch_to_thread),
>>which will set all the current stuff mentioned above.  But sometimes
>>this isn't possible, especially in thw wait method, because there isn't
>>always a thread_info for the ptid you are handling yet, so you can't
>>switch to it.

Since you all are more experienced than me, I am sure the future issues and solutions will be brightly more visible to all of you than me and I would love to learn that.. Having said that let me assume you might be thinking of a fork() event where in case we return the child process ID and we switch_to_thread(current_target, child_ptid) we might get an assertion saying inf->thread does not exist and rightly so.. That is where the APIs or functions like ourstatus->set_forked(child_ptid) come in picture where we can pass a new process info and then return a parent process ptid who has a thread from beneath->wait to aix_thread:wait() and that way we won't face this issue of having an inferior with no thread when we use switch_to_thread(current_target,ptid)  in AIX for the time being at least..

Hopefully we are thinking in the same terms and the solution for multiple threads is fair.

Have a nice day ahead.

Thank you,

Regards,
Aditya.







________________________________
From: Simon Marchi <simark@simark.ca>
Sent: 25 July 2022 21:00
To: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>; Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; simon.marchi@efficios.com <simon.marchi@efficios.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Subject: [EXTERNAL] Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch



On 2022-07-25 08:21, Ulrich Weigand wrote:
>
> Aditya Kamath1 <Aditya.Kamath1@ibm.com> wrote:
>
>> The cause of the bug :- Since, for the GDB core we are
>> switch_to_no_thread() i.e. we do not have a thread till we return the
>> pid from the wait() there is no thread. So, when a call is made from
>> pd_activate() in wait() of aix-thread.c, to pthdb_session_init() we are
>> going to recieve PTHDB_NOT_THREADED.
>
> Thanks for the explanation.  I wasn't aware the use of
> inferior_ptid happens in some of callback routines used
> by the pthdb_session_init() library routine.

Thanks, me neither, but it makes sense.

> I still think the proposed fix isn't really ideal.  Can you instead
> try to *temporarily* (i.e. using a scoped_restore) set up inferior_ptid
> in pd_activate() before calling pthdb_session_init(), with a comment
> explaining that this is needed for the callbacks?

That sounds like a good idea, this way, from the point of view of the
caller of pd_activate, pd_activate does not care about global state.
That's how we can do baby steps towards relying less on global state
implicitly.  The next step could be to try to make each individual
callback switch to the right global context, based on what they need.

You just need to be careful, some parts of GDB expect inferior_ptid, the
current thread, the current inferior and the current program space to be
sync'ed.  If you just set inferior_ptid,  you need to make sure to only
call functions that use inferior_ptid, not the other current stuff.
There is not practical way to know this, you have to carefully inspect
what is called.  To avoid this kind of problems, you can temporarily
switch thread (using scoped_restore_current_thread + switch_to_thread),
which will set all the current stuff mentioned above.  But sometimes
this isn't possible, especially in thw wait method, because there isn't
always a thread_info for the ptid you are handling yet, so you can't
switch to it.

Given the AIX target only supports one inferior for now, the current
inferior and program space should be correct.  But to support
multi-inferior, it will be important to keep that in mind.  You might
have to switch to the right inferior in addition to setting
inferior_ptid in pd_acticate.

This hunk in the patch:

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 4c9195a7f12..91466a17647 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -976,7 +976,7 @@ pd_enable (void)
   /* 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 (1);
+  pd_activate (inferior_ptid.pid());
 }

looks right to me (except the missing space before the parenthesis).  It
looks like an oversight in my "gdb: fix
{rs6000_nat_target,aix_thread_target}::wait to not use inferior_ptid"
patch, I forgot to update that call to pd_activate.  Note that the old
parameter to pd_activate was SET_INFPID, and if set, pd_update would
change the current thread to reflect the thread ptid, if thread
debugging was enabled.  The current code no longer does that.  If that
was important, we can re-introduce it here: make pd_enable switch to the
thread with the ptid returned by pd_activate.

Simon

[-- Attachment #2: 0001-Fix-for-multiple-thread-detection-in-AIX.patch --]
[-- Type: application/octet-stream, Size: 7689 bytes --]

From 7f345f9e66f57a1f72e385b6689bcd67d7c78fd1 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Thu, 4 Aug 2022 23:56:09 -0500
Subject: [PATCH] Fix-for-multiple-thread-detection-in-AIX.

In AIX multiple threads were not added. This patch is a fix for the same

When we create a pthread debug session we have callbacks to read symbols and memory.

One of those call backs is pdc_read_data.

Before we come into aix-thread wait() we switch to no thread and therefore the current thread is null.

When we get into pdc_read_data we have a dependency that we need to be in the correct current thread that has caused an event of new thread,

inorder to read memory.

Hence we switch to the correct thread.

This is done by passing the pid in the pthdb_user_t user_current_pid parameter in every call back.
---
 gdb/aix-thread.c | 63 ++++++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 4c9195a7f12..81097c436e3 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -72,11 +72,6 @@ static bool debug_aix_thread;
 
 #define PD_TID(ptid)	(pd_active && ptid.tid () != 0)
 
-/* pthdb_user_t value that we pass to pthdb functions.  0 causes
-   PTHDB_BAD_USER errors, so use 1.  */
-
-#define PD_USER	1
-
 /* Success and failure values returned by pthdb callbacks.  */
 
 #define PDC_SUCCESS	PTHDB_SUCCESS
@@ -331,7 +326,7 @@ pid_to_prc (ptid_t *ptidp)
    the address of SYMBOLS[<i>].name.  */
 
 static int
-pdc_symbol_addrs (pthdb_user_t user, pthdb_symbol_t *symbols, int count)
+pdc_symbol_addrs (pthdb_user_t user_current_pid, pthdb_symbol_t *symbols, int count)
 {
   struct bound_minimal_symbol ms;
   int i;
@@ -339,8 +334,8 @@ pdc_symbol_addrs (pthdb_user_t user, pthdb_symbol_t *symbols, int count)
 
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_symbol_addrs (user = %ld, symbols = 0x%lx, count = %d)\n",
-		user, (long) symbols, count);
+		"pdc_symbol_addrs (user_current_pid = %ld, symbols = 0x%lx, count = %d)\n",
+		user_current_pid, (long) symbols, count);
 
   for (i = 0; i < count; i++)
     {
@@ -378,7 +373,7 @@ pdc_symbol_addrs (pthdb_user_t user, pthdb_symbol_t *symbols, int count)
    If successful return 0, else non-zero is returned.  */
 
 static int
-pdc_read_regs (pthdb_user_t user, 
+pdc_read_regs (pthdb_user_t user_current_pid, 
 	       pthdb_tid_t tid,
 	       unsigned long long flags,
 	       pthdb_context_t *context)
@@ -450,7 +445,7 @@ pdc_read_regs (pthdb_user_t user,
    If successful return 0, else non-zero is returned.  */
 
 static int
-pdc_write_regs (pthdb_user_t user,
+pdc_write_regs (pthdb_user_t user_current_pid,
 		pthdb_tid_t tid,
 		unsigned long long flags,
 		pthdb_context_t *context)
@@ -500,17 +495,27 @@ pdc_write_regs (pthdb_user_t user,
 /* pthdb callback: read LEN bytes from process ADDR into BUF.  */
 
 static int
-pdc_read_data (pthdb_user_t user, void *buf, 
+pdc_read_data (pthdb_user_t user_current_pid, void *buf, 
 	       pthdb_addr_t addr, size_t len)
 {
   int status, ret;
 
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_read_data (user = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
-		user, (long) buf, hex_string (addr), len);
+		"pdc_read_data (user_current_pid = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
+		user_current_pid, (long) buf, hex_string (addr), len);
 
-  status = target_read_memory (addr, (gdb_byte *) buf, len);
+  /* 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;
+    /* 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)); 
+    status = target_read_memory (addr, (gdb_byte *) buf, len);
+  }
   ret = status == 0 ? PDC_SUCCESS : PDC_FAILURE;
 
   if (debug_aix_thread)
@@ -522,15 +527,15 @@ pdc_read_data (pthdb_user_t user, void *buf,
 /* pthdb callback: write LEN bytes from BUF to process ADDR.  */
 
 static int
-pdc_write_data (pthdb_user_t user, void *buf, 
-		pthdb_addr_t addr, size_t len)
+pdc_write_data (pthdb_user_t user_current_pid, void *buf, 
+               pthdb_addr_t addr, size_t len)
 {
   int status, ret;
 
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_write_data (user = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
-		user, (long) buf, hex_string (addr), len);
+		"pdc_write_data (user_current_pid = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
+		user_current_pid, (long) buf, hex_string (addr), len);
 
   status = target_write_memory (addr, (gdb_byte *) buf, len);
   ret = status == 0 ? PDC_SUCCESS : PDC_FAILURE;
@@ -545,12 +550,12 @@ pdc_write_data (pthdb_user_t user, void *buf,
    in BUFP.  */
 
 static int
-pdc_alloc (pthdb_user_t user, size_t len, void **bufp)
+pdc_alloc (pthdb_user_t user_current_pid, size_t len, void **bufp)
 {
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_alloc (user = %ld, len = %ld, bufp = 0x%lx)\n",
-		user, len, (long) bufp);
+		"pdc_alloc (user_current_pid = %ld, len = %ld, bufp = 0x%lx)\n",
+		user_current_pid, len, (long) bufp);
   *bufp = xmalloc (len);
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog, 
@@ -567,12 +572,12 @@ pdc_alloc (pthdb_user_t user, size_t len, void **bufp)
    pointer to the result in BUFP.  */
 
 static int
-pdc_realloc (pthdb_user_t user, void *buf, size_t len, void **bufp)
+pdc_realloc (pthdb_user_t user_current_pid, void *buf, size_t len, void **bufp)
 {
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_realloc (user = %ld, buf = 0x%lx, len = %ld, bufp = 0x%lx)\n",
-		user, (long) buf, len, (long) bufp);
+		"pdc_realloc (user_current_pid = %ld, buf = 0x%lx, len = %ld, bufp = 0x%lx)\n",
+		user_current_pid, (long) buf, len, (long) bufp);
   *bufp = xrealloc (buf, len);
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog, 
@@ -584,11 +589,11 @@ pdc_realloc (pthdb_user_t user, void *buf, size_t len, void **bufp)
    realloc callback.  */
 
 static int
-pdc_dealloc (pthdb_user_t user, void *buf)
+pdc_dealloc (pthdb_user_t user_current_pid, void *buf)
 {
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog, 
-		"pdc_free (user = %ld, buf = 0x%lx)\n", user,
+		"pdc_free (user_current_pid = %ld, buf = 0x%lx)\n", user_current_pid,
 		(long) buf);
   xfree (buf);
   return PDC_SUCCESS;
@@ -912,7 +917,7 @@ pd_activate (int pid)
 {
   int status;
 		
-  status = pthdb_session_init (PD_USER, arch64 ? PEM_64BIT : PEM_32BIT,
+  status = pthdb_session_init (pid, arch64 ? PEM_64BIT : PEM_32BIT,
 			       PTHDB_FLAG_REGS, &pd_callbacks, 
 			       &pd_session);
   if (status != PTHDB_SUCCESS)
@@ -955,7 +960,7 @@ pd_enable (void)
 
   /* Check whether the application is pthreaded.  */
   stub_name = NULL;
-  status = pthdb_session_pthreaded (PD_USER, PTHDB_FLAG_REGS,
+  status = pthdb_session_pthreaded (inferior_ptid.pid (), PTHDB_FLAG_REGS,
 				    &pd_callbacks, &stub_name);
   if ((status != PTHDB_SUCCESS
        && status != PTHDB_NOT_PTHREADED) || !stub_name)
@@ -976,7 +981,7 @@ pd_enable (void)
   /* 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 (1);
+  pd_activate (inferior_ptid.pid());
 }
 
 /* Undo the effects of pd_enable().  */
-- 
2.31.1

Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Ulrich Weigand]

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

>I have moved the scope + switch_to_current_thread to pdc_read_data()
>where the dependency is needed in the call-backs as suggested by
>Ulrich. I also eliminated PD_USER variable as it is no longer needed.
>As far as the user_current_pid != 0 condition is concerned, we need
>this check during initialisation during the birth of the first
>inferior. We passed a parameter inferior_ptid.pid () in pd_enable ()
>which is 0 at that time.

This looks good to me, with just some coding-style / formatting
issues:

+    /* 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. */

Please use two spaces after each '.' in comments.  Also, watch the
maximum line length of 80.

+    if (user_current_pid != 0) 
+      switch_to_thread (current_inferior ()->process_target ()
,ptid_t(user_current_pid)); 


There should be a space after the "," and after "ptid_t" before the '('
.

Patch should be good to go with these changes.

Thanks,
Ulrich

Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Aditya Kamath1]

[-- Attachment #1: Type: text/plain, Size: 1947 bytes --]

Hi Simon and Ulrich,

Thank you so much for the Feeback.  I have made the space adjustments. Please find attached the patch [See Fix-for-multiple-thread-detection-in-AIX.patch]

If there is no issues, I will push the patch in git. Kindly let me know.

Have a nice day ahead.

Thanks and regards,
Aditya.
________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 05 August 2022 17:23
To: simark@simark.ca <simark@simark.ca>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; simon.marchi@efficios.com <simon.marchi@efficios.com>; Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Subject: Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

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

>I have moved the scope + switch_to_current_thread to pdc_read_data()
>where the dependency is needed in the call-backs as suggested by
>Ulrich. I also eliminated PD_USER variable as it is no longer needed.
>As far as the user_current_pid != 0 condition is concerned, we need
>this check during initialisation during the birth of the first
>inferior. We passed a parameter inferior_ptid.pid () in pd_enable ()
>which is 0 at that time.

This looks good to me, with just some coding-style / formatting
issues:

+    /* 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. */

Please use two spaces after each '.' in comments.  Also, watch the
maximum line length of 80.

+    if (user_current_pid != 0)
+      switch_to_thread (current_inferior ()->process_target ()
,ptid_t(user_current_pid));


There should be a space after the "," and after "ptid_t" before the '('
.

Patch should be good to go with these changes.

Thanks,
Ulrich


[-- Attachment #2: 0001-Fix-for-multiple-thread-detection-in-AIX.patch --]
[-- Type: application/octet-stream, Size: 7698 bytes --]

From 427231baf143e67522b47ae9310bdf4d197cff5a Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Fri, 5 Aug 2022 09:07:37 -0500
Subject: [PATCH] Fix-for-multiple-thread-detection-in-AIX.

In AIX multiple threads were not added. This patch is a fix for the same

When we create a pthread debug session we have callbacks to read symbols and memory.

One of those call backs is pdc_read_data.

Before we come into aix-thread wait() we switch to no thread and therefore the current thread is null.

When we get into pdc_read_data we have a dependency that we need to be in the correct current thread that has caused an event of new thread,

inorder to read memory.

Hence we switch to the correct thread.

This is done by passing the pid in the pthdb_user_t user_current_pid parameter in every call back.
---
 gdb/aix-thread.c | 63 ++++++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 4c9195a7f12..70ff69b2449 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -72,11 +72,6 @@ static bool debug_aix_thread;
 
 #define PD_TID(ptid)	(pd_active && ptid.tid () != 0)
 
-/* pthdb_user_t value that we pass to pthdb functions.  0 causes
-   PTHDB_BAD_USER errors, so use 1.  */
-
-#define PD_USER	1
-
 /* Success and failure values returned by pthdb callbacks.  */
 
 #define PDC_SUCCESS	PTHDB_SUCCESS
@@ -331,7 +326,7 @@ pid_to_prc (ptid_t *ptidp)
    the address of SYMBOLS[<i>].name.  */
 
 static int
-pdc_symbol_addrs (pthdb_user_t user, pthdb_symbol_t *symbols, int count)
+pdc_symbol_addrs (pthdb_user_t user_current_pid, pthdb_symbol_t *symbols, int count)
 {
   struct bound_minimal_symbol ms;
   int i;
@@ -339,8 +334,8 @@ pdc_symbol_addrs (pthdb_user_t user, pthdb_symbol_t *symbols, int count)
 
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_symbol_addrs (user = %ld, symbols = 0x%lx, count = %d)\n",
-		user, (long) symbols, count);
+		"pdc_symbol_addrs (user_current_pid = %ld, symbols = 0x%lx, count = %d)\n",
+		user_current_pid, (long) symbols, count);
 
   for (i = 0; i < count; i++)
     {
@@ -378,7 +373,7 @@ pdc_symbol_addrs (pthdb_user_t user, pthdb_symbol_t *symbols, int count)
    If successful return 0, else non-zero is returned.  */
 
 static int
-pdc_read_regs (pthdb_user_t user, 
+pdc_read_regs (pthdb_user_t user_current_pid, 
 	       pthdb_tid_t tid,
 	       unsigned long long flags,
 	       pthdb_context_t *context)
@@ -450,7 +445,7 @@ pdc_read_regs (pthdb_user_t user,
    If successful return 0, else non-zero is returned.  */
 
 static int
-pdc_write_regs (pthdb_user_t user,
+pdc_write_regs (pthdb_user_t user_current_pid,
 		pthdb_tid_t tid,
 		unsigned long long flags,
 		pthdb_context_t *context)
@@ -500,17 +495,27 @@ pdc_write_regs (pthdb_user_t user,
 /* pthdb callback: read LEN bytes from process ADDR into BUF.  */
 
 static int
-pdc_read_data (pthdb_user_t user, void *buf, 
+pdc_read_data (pthdb_user_t user_current_pid, void *buf, 
 	       pthdb_addr_t addr, size_t len)
 {
   int status, ret;
 
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_read_data (user = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
-		user, (long) buf, hex_string (addr), len);
+		"pdc_read_data (user_current_pid = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
+		user_current_pid, (long) buf, hex_string (addr), len);
 
-  status = target_read_memory (addr, (gdb_byte *) buf, len);
+  /* 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;
+    /* 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)); 
+    status = target_read_memory (addr, (gdb_byte *) buf, len);
+  }
   ret = status == 0 ? PDC_SUCCESS : PDC_FAILURE;
 
   if (debug_aix_thread)
@@ -522,15 +527,15 @@ pdc_read_data (pthdb_user_t user, void *buf,
 /* pthdb callback: write LEN bytes from BUF to process ADDR.  */
 
 static int
-pdc_write_data (pthdb_user_t user, void *buf, 
-		pthdb_addr_t addr, size_t len)
+pdc_write_data (pthdb_user_t user_current_pid, void *buf, 
+               pthdb_addr_t addr, size_t len)
 {
   int status, ret;
 
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_write_data (user = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
-		user, (long) buf, hex_string (addr), len);
+		"pdc_write_data (user_current_pid = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
+		user_current_pid, (long) buf, hex_string (addr), len);
 
   status = target_write_memory (addr, (gdb_byte *) buf, len);
   ret = status == 0 ? PDC_SUCCESS : PDC_FAILURE;
@@ -545,12 +550,12 @@ pdc_write_data (pthdb_user_t user, void *buf,
    in BUFP.  */
 
 static int
-pdc_alloc (pthdb_user_t user, size_t len, void **bufp)
+pdc_alloc (pthdb_user_t user_current_pid, size_t len, void **bufp)
 {
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_alloc (user = %ld, len = %ld, bufp = 0x%lx)\n",
-		user, len, (long) bufp);
+		"pdc_alloc (user_current_pid = %ld, len = %ld, bufp = 0x%lx)\n",
+		user_current_pid, len, (long) bufp);
   *bufp = xmalloc (len);
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog, 
@@ -567,12 +572,12 @@ pdc_alloc (pthdb_user_t user, size_t len, void **bufp)
    pointer to the result in BUFP.  */
 
 static int
-pdc_realloc (pthdb_user_t user, void *buf, size_t len, void **bufp)
+pdc_realloc (pthdb_user_t user_current_pid, void *buf, size_t len, void **bufp)
 {
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_realloc (user = %ld, buf = 0x%lx, len = %ld, bufp = 0x%lx)\n",
-		user, (long) buf, len, (long) bufp);
+		"pdc_realloc (user_current_pid = %ld, buf = 0x%lx, len = %ld, bufp = 0x%lx)\n",
+		user_current_pid, (long) buf, len, (long) bufp);
   *bufp = xrealloc (buf, len);
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog, 
@@ -584,11 +589,11 @@ pdc_realloc (pthdb_user_t user, void *buf, size_t len, void **bufp)
    realloc callback.  */
 
 static int
-pdc_dealloc (pthdb_user_t user, void *buf)
+pdc_dealloc (pthdb_user_t user_current_pid, void *buf)
 {
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog, 
-		"pdc_free (user = %ld, buf = 0x%lx)\n", user,
+		"pdc_free (user_current_pid = %ld, buf = 0x%lx)\n", user_current_pid,
 		(long) buf);
   xfree (buf);
   return PDC_SUCCESS;
@@ -912,7 +917,7 @@ pd_activate (int pid)
 {
   int status;
 		
-  status = pthdb_session_init (PD_USER, arch64 ? PEM_64BIT : PEM_32BIT,
+  status = pthdb_session_init (pid, arch64 ? PEM_64BIT : PEM_32BIT,
 			       PTHDB_FLAG_REGS, &pd_callbacks, 
 			       &pd_session);
   if (status != PTHDB_SUCCESS)
@@ -955,7 +960,7 @@ pd_enable (void)
 
   /* Check whether the application is pthreaded.  */
   stub_name = NULL;
-  status = pthdb_session_pthreaded (PD_USER, PTHDB_FLAG_REGS,
+  status = pthdb_session_pthreaded (inferior_ptid.pid (), PTHDB_FLAG_REGS,
 				    &pd_callbacks, &stub_name);
   if ((status != PTHDB_SUCCESS
        && status != PTHDB_NOT_PTHREADED) || !stub_name)
@@ -976,7 +981,7 @@ pd_enable (void)
   /* 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 (1);
+  pd_activate (inferior_ptid.pid ());
 }
 
 /* Undo the effects of pd_enable().  */
-- 
2.31.1

Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Ulrich Weigand]

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

>Thank you so much for the Feeback.  I have made the space adjustments.
>Please find attached the patch [See Fix-for-multiple-thread-detection-
>in-AIX.patch]
>
>If there is no issues, I will push the patch in git. Kindly let me
>know. 

This is OK.

Thanks,
Ulrich

Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Aditya Kamath1]

[-- Attachment #1: Type: text/plain, Size: 1121 bytes --]

Hi Simon and Ulrich,

Unfortunately, I came to know I do not have access to push. It will be great if you help us to push this. I will be thankful to you for the same.

Once again, thank you for all the guidance.

Attaching the patch [See: Fix-for-multiple-thread-detection-in-AIX.patch] for your reference.

Have a nice day.

Thanks and regards,
Aditya.
________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 05 August 2022 19:48
To: simark@simark.ca <simark@simark.ca>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; simon.marchi@efficios.com <simon.marchi@efficios.com>; Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Subject: Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

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

>Thank you so much for the Feeback.  I have made the space adjustments.
>Please find attached the patch [See Fix-for-multiple-thread-detection-
>in-AIX.patch]
>
>If there is no issues, I will push the patch in git. Kindly let me
>know.

This is OK.

Thanks,
Ulrich


[-- Attachment #2: 0001-Fix-for-multiple-thread-detection-in-AIX.patch --]
[-- Type: application/octet-stream, Size: 7698 bytes --]

From 427231baf143e67522b47ae9310bdf4d197cff5a Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Fri, 5 Aug 2022 09:07:37 -0500
Subject: [PATCH] Fix-for-multiple-thread-detection-in-AIX.

In AIX multiple threads were not added. This patch is a fix for the same

When we create a pthread debug session we have callbacks to read symbols and memory.

One of those call backs is pdc_read_data.

Before we come into aix-thread wait() we switch to no thread and therefore the current thread is null.

When we get into pdc_read_data we have a dependency that we need to be in the correct current thread that has caused an event of new thread,

inorder to read memory.

Hence we switch to the correct thread.

This is done by passing the pid in the pthdb_user_t user_current_pid parameter in every call back.
---
 gdb/aix-thread.c | 63 ++++++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 4c9195a7f12..70ff69b2449 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -72,11 +72,6 @@ static bool debug_aix_thread;
 
 #define PD_TID(ptid)	(pd_active && ptid.tid () != 0)
 
-/* pthdb_user_t value that we pass to pthdb functions.  0 causes
-   PTHDB_BAD_USER errors, so use 1.  */
-
-#define PD_USER	1
-
 /* Success and failure values returned by pthdb callbacks.  */
 
 #define PDC_SUCCESS	PTHDB_SUCCESS
@@ -331,7 +326,7 @@ pid_to_prc (ptid_t *ptidp)
    the address of SYMBOLS[<i>].name.  */
 
 static int
-pdc_symbol_addrs (pthdb_user_t user, pthdb_symbol_t *symbols, int count)
+pdc_symbol_addrs (pthdb_user_t user_current_pid, pthdb_symbol_t *symbols, int count)
 {
   struct bound_minimal_symbol ms;
   int i;
@@ -339,8 +334,8 @@ pdc_symbol_addrs (pthdb_user_t user, pthdb_symbol_t *symbols, int count)
 
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_symbol_addrs (user = %ld, symbols = 0x%lx, count = %d)\n",
-		user, (long) symbols, count);
+		"pdc_symbol_addrs (user_current_pid = %ld, symbols = 0x%lx, count = %d)\n",
+		user_current_pid, (long) symbols, count);
 
   for (i = 0; i < count; i++)
     {
@@ -378,7 +373,7 @@ pdc_symbol_addrs (pthdb_user_t user, pthdb_symbol_t *symbols, int count)
    If successful return 0, else non-zero is returned.  */
 
 static int
-pdc_read_regs (pthdb_user_t user, 
+pdc_read_regs (pthdb_user_t user_current_pid, 
 	       pthdb_tid_t tid,
 	       unsigned long long flags,
 	       pthdb_context_t *context)
@@ -450,7 +445,7 @@ pdc_read_regs (pthdb_user_t user,
    If successful return 0, else non-zero is returned.  */
 
 static int
-pdc_write_regs (pthdb_user_t user,
+pdc_write_regs (pthdb_user_t user_current_pid,
 		pthdb_tid_t tid,
 		unsigned long long flags,
 		pthdb_context_t *context)
@@ -500,17 +495,27 @@ pdc_write_regs (pthdb_user_t user,
 /* pthdb callback: read LEN bytes from process ADDR into BUF.  */
 
 static int
-pdc_read_data (pthdb_user_t user, void *buf, 
+pdc_read_data (pthdb_user_t user_current_pid, void *buf, 
 	       pthdb_addr_t addr, size_t len)
 {
   int status, ret;
 
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_read_data (user = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
-		user, (long) buf, hex_string (addr), len);
+		"pdc_read_data (user_current_pid = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
+		user_current_pid, (long) buf, hex_string (addr), len);
 
-  status = target_read_memory (addr, (gdb_byte *) buf, len);
+  /* 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;
+    /* 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)); 
+    status = target_read_memory (addr, (gdb_byte *) buf, len);
+  }
   ret = status == 0 ? PDC_SUCCESS : PDC_FAILURE;
 
   if (debug_aix_thread)
@@ -522,15 +527,15 @@ pdc_read_data (pthdb_user_t user, void *buf,
 /* pthdb callback: write LEN bytes from BUF to process ADDR.  */
 
 static int
-pdc_write_data (pthdb_user_t user, void *buf, 
-		pthdb_addr_t addr, size_t len)
+pdc_write_data (pthdb_user_t user_current_pid, void *buf, 
+               pthdb_addr_t addr, size_t len)
 {
   int status, ret;
 
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_write_data (user = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
-		user, (long) buf, hex_string (addr), len);
+		"pdc_write_data (user_current_pid = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
+		user_current_pid, (long) buf, hex_string (addr), len);
 
   status = target_write_memory (addr, (gdb_byte *) buf, len);
   ret = status == 0 ? PDC_SUCCESS : PDC_FAILURE;
@@ -545,12 +550,12 @@ pdc_write_data (pthdb_user_t user, void *buf,
    in BUFP.  */
 
 static int
-pdc_alloc (pthdb_user_t user, size_t len, void **bufp)
+pdc_alloc (pthdb_user_t user_current_pid, size_t len, void **bufp)
 {
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_alloc (user = %ld, len = %ld, bufp = 0x%lx)\n",
-		user, len, (long) bufp);
+		"pdc_alloc (user_current_pid = %ld, len = %ld, bufp = 0x%lx)\n",
+		user_current_pid, len, (long) bufp);
   *bufp = xmalloc (len);
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog, 
@@ -567,12 +572,12 @@ pdc_alloc (pthdb_user_t user, size_t len, void **bufp)
    pointer to the result in BUFP.  */
 
 static int
-pdc_realloc (pthdb_user_t user, void *buf, size_t len, void **bufp)
+pdc_realloc (pthdb_user_t user_current_pid, void *buf, size_t len, void **bufp)
 {
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_realloc (user = %ld, buf = 0x%lx, len = %ld, bufp = 0x%lx)\n",
-		user, (long) buf, len, (long) bufp);
+		"pdc_realloc (user_current_pid = %ld, buf = 0x%lx, len = %ld, bufp = 0x%lx)\n",
+		user_current_pid, (long) buf, len, (long) bufp);
   *bufp = xrealloc (buf, len);
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog, 
@@ -584,11 +589,11 @@ pdc_realloc (pthdb_user_t user, void *buf, size_t len, void **bufp)
    realloc callback.  */
 
 static int
-pdc_dealloc (pthdb_user_t user, void *buf)
+pdc_dealloc (pthdb_user_t user_current_pid, void *buf)
 {
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog, 
-		"pdc_free (user = %ld, buf = 0x%lx)\n", user,
+		"pdc_free (user_current_pid = %ld, buf = 0x%lx)\n", user_current_pid,
 		(long) buf);
   xfree (buf);
   return PDC_SUCCESS;
@@ -912,7 +917,7 @@ pd_activate (int pid)
 {
   int status;
 		
-  status = pthdb_session_init (PD_USER, arch64 ? PEM_64BIT : PEM_32BIT,
+  status = pthdb_session_init (pid, arch64 ? PEM_64BIT : PEM_32BIT,
 			       PTHDB_FLAG_REGS, &pd_callbacks, 
 			       &pd_session);
   if (status != PTHDB_SUCCESS)
@@ -955,7 +960,7 @@ pd_enable (void)
 
   /* Check whether the application is pthreaded.  */
   stub_name = NULL;
-  status = pthdb_session_pthreaded (PD_USER, PTHDB_FLAG_REGS,
+  status = pthdb_session_pthreaded (inferior_ptid.pid (), PTHDB_FLAG_REGS,
 				    &pd_callbacks, &stub_name);
   if ((status != PTHDB_SUCCESS
        && status != PTHDB_NOT_PTHREADED) || !stub_name)
@@ -976,7 +981,7 @@ pd_enable (void)
   /* 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 (1);
+  pd_activate (inferior_ptid.pid ());
 }
 
 /* Undo the effects of pd_enable().  */
-- 
2.31.1

Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Aditya Kamath1]

[-- Attachment #1: Type: text/plain, Size: 1769 bytes --]

Hi all,

Kindly help me push the patch as I do not have the access for the same.

Attaching once again. [See: Fix-for-multiple-thread-detection-in-AIX.patch].

Have a nice day ahead.

Thanks and regards,
Aditya.
________________________________
From: Aditya Kamath1 <Aditya.Kamath1@ibm.com>
Sent: 05 August 2022 19:54
To: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>; simark@simark.ca <simark@simark.ca>; simon.marchi@efficios.com <simon.marchi@efficios.com>; Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Subject: Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

Hi Simon and Ulrich,

Unfortunately, I came to know I do not have access to push. It will be great if you help us to push this. I will be thankful to you for the same.

Once again, thank you for all the guidance.

Attaching the patch [See: Fix-for-multiple-thread-detection-in-AIX.patch] for your reference.

Have a nice day.

Thanks and regards,
Aditya.
________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 05 August 2022 19:48
To: simark@simark.ca <simark@simark.ca>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; simon.marchi@efficios.com <simon.marchi@efficios.com>; Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Subject: Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

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

>Thank you so much for the Feeback.  I have made the space adjustments.
>Please find attached the patch [See Fix-for-multiple-thread-detection-
>in-AIX.patch]
>
>If there is no issues, I will push the patch in git. Kindly let me
>know.

This is OK.

Thanks,
Ulrich


[-- Attachment #2: 0001-Fix-for-multiple-thread-detection-in-AIX.patch --]
[-- Type: application/octet-stream, Size: 7698 bytes --]

From 427231baf143e67522b47ae9310bdf4d197cff5a Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Fri, 5 Aug 2022 09:07:37 -0500
Subject: [PATCH] Fix-for-multiple-thread-detection-in-AIX.

In AIX multiple threads were not added. This patch is a fix for the same

When we create a pthread debug session we have callbacks to read symbols and memory.

One of those call backs is pdc_read_data.

Before we come into aix-thread wait() we switch to no thread and therefore the current thread is null.

When we get into pdc_read_data we have a dependency that we need to be in the correct current thread that has caused an event of new thread,

inorder to read memory.

Hence we switch to the correct thread.

This is done by passing the pid in the pthdb_user_t user_current_pid parameter in every call back.
---
 gdb/aix-thread.c | 63 ++++++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 4c9195a7f12..70ff69b2449 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -72,11 +72,6 @@ static bool debug_aix_thread;
 
 #define PD_TID(ptid)	(pd_active && ptid.tid () != 0)
 
-/* pthdb_user_t value that we pass to pthdb functions.  0 causes
-   PTHDB_BAD_USER errors, so use 1.  */
-
-#define PD_USER	1
-
 /* Success and failure values returned by pthdb callbacks.  */
 
 #define PDC_SUCCESS	PTHDB_SUCCESS
@@ -331,7 +326,7 @@ pid_to_prc (ptid_t *ptidp)
    the address of SYMBOLS[<i>].name.  */
 
 static int
-pdc_symbol_addrs (pthdb_user_t user, pthdb_symbol_t *symbols, int count)
+pdc_symbol_addrs (pthdb_user_t user_current_pid, pthdb_symbol_t *symbols, int count)
 {
   struct bound_minimal_symbol ms;
   int i;
@@ -339,8 +334,8 @@ pdc_symbol_addrs (pthdb_user_t user, pthdb_symbol_t *symbols, int count)
 
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_symbol_addrs (user = %ld, symbols = 0x%lx, count = %d)\n",
-		user, (long) symbols, count);
+		"pdc_symbol_addrs (user_current_pid = %ld, symbols = 0x%lx, count = %d)\n",
+		user_current_pid, (long) symbols, count);
 
   for (i = 0; i < count; i++)
     {
@@ -378,7 +373,7 @@ pdc_symbol_addrs (pthdb_user_t user, pthdb_symbol_t *symbols, int count)
    If successful return 0, else non-zero is returned.  */
 
 static int
-pdc_read_regs (pthdb_user_t user, 
+pdc_read_regs (pthdb_user_t user_current_pid, 
 	       pthdb_tid_t tid,
 	       unsigned long long flags,
 	       pthdb_context_t *context)
@@ -450,7 +445,7 @@ pdc_read_regs (pthdb_user_t user,
    If successful return 0, else non-zero is returned.  */
 
 static int
-pdc_write_regs (pthdb_user_t user,
+pdc_write_regs (pthdb_user_t user_current_pid,
 		pthdb_tid_t tid,
 		unsigned long long flags,
 		pthdb_context_t *context)
@@ -500,17 +495,27 @@ pdc_write_regs (pthdb_user_t user,
 /* pthdb callback: read LEN bytes from process ADDR into BUF.  */
 
 static int
-pdc_read_data (pthdb_user_t user, void *buf, 
+pdc_read_data (pthdb_user_t user_current_pid, void *buf, 
 	       pthdb_addr_t addr, size_t len)
 {
   int status, ret;
 
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_read_data (user = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
-		user, (long) buf, hex_string (addr), len);
+		"pdc_read_data (user_current_pid = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
+		user_current_pid, (long) buf, hex_string (addr), len);
 
-  status = target_read_memory (addr, (gdb_byte *) buf, len);
+  /* 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;
+    /* 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)); 
+    status = target_read_memory (addr, (gdb_byte *) buf, len);
+  }
   ret = status == 0 ? PDC_SUCCESS : PDC_FAILURE;
 
   if (debug_aix_thread)
@@ -522,15 +527,15 @@ pdc_read_data (pthdb_user_t user, void *buf,
 /* pthdb callback: write LEN bytes from BUF to process ADDR.  */
 
 static int
-pdc_write_data (pthdb_user_t user, void *buf, 
-		pthdb_addr_t addr, size_t len)
+pdc_write_data (pthdb_user_t user_current_pid, void *buf, 
+               pthdb_addr_t addr, size_t len)
 {
   int status, ret;
 
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_write_data (user = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
-		user, (long) buf, hex_string (addr), len);
+		"pdc_write_data (user_current_pid = %ld, buf = 0x%lx, addr = %s, len = %ld)\n",
+		user_current_pid, (long) buf, hex_string (addr), len);
 
   status = target_write_memory (addr, (gdb_byte *) buf, len);
   ret = status == 0 ? PDC_SUCCESS : PDC_FAILURE;
@@ -545,12 +550,12 @@ pdc_write_data (pthdb_user_t user, void *buf,
    in BUFP.  */
 
 static int
-pdc_alloc (pthdb_user_t user, size_t len, void **bufp)
+pdc_alloc (pthdb_user_t user_current_pid, size_t len, void **bufp)
 {
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_alloc (user = %ld, len = %ld, bufp = 0x%lx)\n",
-		user, len, (long) bufp);
+		"pdc_alloc (user_current_pid = %ld, len = %ld, bufp = 0x%lx)\n",
+		user_current_pid, len, (long) bufp);
   *bufp = xmalloc (len);
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog, 
@@ -567,12 +572,12 @@ pdc_alloc (pthdb_user_t user, size_t len, void **bufp)
    pointer to the result in BUFP.  */
 
 static int
-pdc_realloc (pthdb_user_t user, void *buf, size_t len, void **bufp)
+pdc_realloc (pthdb_user_t user_current_pid, void *buf, size_t len, void **bufp)
 {
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog,
-		"pdc_realloc (user = %ld, buf = 0x%lx, len = %ld, bufp = 0x%lx)\n",
-		user, (long) buf, len, (long) bufp);
+		"pdc_realloc (user_current_pid = %ld, buf = 0x%lx, len = %ld, bufp = 0x%lx)\n",
+		user_current_pid, (long) buf, len, (long) bufp);
   *bufp = xrealloc (buf, len);
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog, 
@@ -584,11 +589,11 @@ pdc_realloc (pthdb_user_t user, void *buf, size_t len, void **bufp)
    realloc callback.  */
 
 static int
-pdc_dealloc (pthdb_user_t user, void *buf)
+pdc_dealloc (pthdb_user_t user_current_pid, void *buf)
 {
   if (debug_aix_thread)
     gdb_printf (gdb_stdlog, 
-		"pdc_free (user = %ld, buf = 0x%lx)\n", user,
+		"pdc_free (user_current_pid = %ld, buf = 0x%lx)\n", user_current_pid,
 		(long) buf);
   xfree (buf);
   return PDC_SUCCESS;
@@ -912,7 +917,7 @@ pd_activate (int pid)
 {
   int status;
 		
-  status = pthdb_session_init (PD_USER, arch64 ? PEM_64BIT : PEM_32BIT,
+  status = pthdb_session_init (pid, arch64 ? PEM_64BIT : PEM_32BIT,
 			       PTHDB_FLAG_REGS, &pd_callbacks, 
 			       &pd_session);
   if (status != PTHDB_SUCCESS)
@@ -955,7 +960,7 @@ pd_enable (void)
 
   /* Check whether the application is pthreaded.  */
   stub_name = NULL;
-  status = pthdb_session_pthreaded (PD_USER, PTHDB_FLAG_REGS,
+  status = pthdb_session_pthreaded (inferior_ptid.pid (), PTHDB_FLAG_REGS,
 				    &pd_callbacks, &stub_name);
   if ((status != PTHDB_SUCCESS
        && status != PTHDB_NOT_PTHREADED) || !stub_name)
@@ -976,7 +981,7 @@ pd_enable (void)
   /* 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 (1);
+  pd_activate (inferior_ptid.pid ());
 }
 
 /* Undo the effects of pd_enable().  */
-- 
2.31.1

Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Ulrich Weigand]

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

>Kindly help me push the patch as I do not have the access for the
same. 

I've checked this in now.

Thanks,
Ulrich

Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

[Aditya Kamath1]

Thanks
________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 09 August 2022 19:11
To: simark@simark.ca <simark@simark.ca>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; simon.marchi@efficios.com <simon.marchi@efficios.com>; Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Subject: Re: [PATCH] Fix-for-multiple-thread-detection-in-AIX.patch

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

>Kindly help me push the patch as I do not have the access for the
same.

I've checked this in now.

Thanks,
Ulrich