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

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

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