[PATCH] Enable multi process debugging for AIX

[PATCH] Enable multi process debugging for AIX

[Aditya Kamath1]

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

Hi,


The attached proposed patch adds multi process debugging feature in AIX.”

Till now AIX supported debugging only one inferior at a time, now we can be able to debug multi process.

Users can use set follow fork mode in child or parent and set detach on fork on or off to enable/disable simultaneous debugging of parent/child.


An example program is written below as follows with the gdb output:

#include <stdio.h>

#include <sys/types.h>

#include <unistd.h>



void hello_from_child(){

  printf("Hello from child \n");

}



void hello_from_parent(){

  printf("Hello from Parent \n");

}



int main(){

  pid_t childpid;

  printf("I should not be printed after fork as I am executed \n");

  childpid = fork();

  if (childpid == 0)

   hello_from_child();

  else

    hello_from_parent();



  return 0;

}



GDB Output:-



(gdb) b hello_from_child

Breakpoint 1 at 0x10000584: file test_fork.c, line 6.

(gdb) set follow-fork-mode child

(gdb) set detach-on-fork off

(gdb) r

Starting program: /home/test_fork

I should not be printed after fork as I am executed

[Attaching after process 23331156 fork to child process 23134700]

[New inferior 2 (process 23134700)]

[Switching to process 23134700]



Thread 2.1 hit Breakpoint 1, hello_from_child ()

    at test_fork.c:6

6         printf("Hello from child \n");

(gdb) c

Continuing.

Hello from child

[Inferior 2 (process 23134700) exited normally]

(gdb)





The following are the results after running the "gdb.base" test suite:-



With patch



# of expected passes            26584
# of unexpected failures        3968
# of unexpected successes       1
# of expected failures          17
# of known failures             26
# of unresolved testcases       109
# of untested testcases         79
# of unsupported tests          62
# of paths in test names        1
# of duplicate test names       2



Without patch



# of expected passes 26244
# of unexpected failures 4230
# of unexpected successes 1
# of expected failures 17
# of known failures 26
# of unresolved testcases 110
# of untested testcases 79
# of unsupported tests 62
# of paths in test names 1
# of duplicate test names 4




Please find attached the patch [See file: 0001-Enable-multi-process-debugging-for-AIX.patch]


I have left comments in my patch to make it easy to understand and justify [code lines] the same. Let me know what you think.


Kindly give us feedback at the earliest.

Have a nice day ahead.


Thanks and regards,

Aditya.

Bye.


[-- Attachment #2: 0001-Enable-multi-process-debugging-for-AIX.patch --]
[-- Type: application/octet-stream, Size: 4303 bytes --]

From 1eab0bb05b1db09dfb98d31cc630722b40c081c4 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Tue, 19 Jul 2022 10:39:00 -0500
Subject: [PATCH] Enable multi process debugging for AIX

The attached proposed patch adds multi process debugging feature in AIX.

Till now AIX supported debugging only one inferior at a time,

now we can be able to debug multi process.

Users can use set follow fork mode in child or parent and

set detach on fork on or off to enable/disable simultaneous debugging of parent/child.
---
 gdb/aix-thread.c     |  4 ++++
 gdb/rs6000-aix-nat.c | 48 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 49 insertions(+), 3 deletions(-)

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)
diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c
index f604f7d503e..55844ca6dab 100644
--- a/gdb/rs6000-aix-nat.c
+++ b/gdb/rs6000-aix-nat.c
@@ -91,10 +91,16 @@ class rs6000_nat_target final : public inf_ptrace_target
 
   ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
 
+  /* Fork detection related functions, For adding multi process debugging
+     support. */ 
+  void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool) override;
+
+  void mourn_inferior () override;
+
 protected:
 
-  void post_startup_inferior (ptid_t ptid) override
-  { /* Nothing.  */ }
+  void post_startup_inferior (ptid_t ptid) override;
+  //{ /* Nothing.  */ }
 
 private:
   enum target_xfer_status
@@ -246,6 +252,22 @@ fetch_register (struct regcache *regcache, int regno)
     }
 }
 
+void rs6000_nat_target::post_startup_inferior(ptid_t ptid){
+  rs6000_ptrace64(PT_MULTI,ptid.pid(),NULL,1,NULL);
+}
+
+void
+rs6000_nat_target::follow_fork (inferior *child_inf, ptid_t child_ptid,
+                          target_waitkind fork_kind, bool follow_child,
+                                                      bool detach_fork)
+{
+  inf_ptrace_target::follow_fork(child_inf, child_ptid, fork_kind,
+                                        follow_child, detach_fork);
+}
+
+void rs6000_nat_target::mourn_inferior(){
+  inf_ptrace_target::mourn_inferior();
+}
 /* Store register REGNO back into the inferior.  */
 
 static void
@@ -539,8 +561,28 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
   if (status == 0x57c)
     ourstatus->set_loaded ();
   /* signal 0.  I have no idea why wait(2) returns with this status word.  */
-  else if (status == 0x7f)
+  /* 0x17f and 0x137f in hexadecimal are status returned if 
+     if we follow parent,
+     a switch is made to a child post parent execution
+     and child continues its execution [user switches to child and 
+     presses continue]. */
+  else if (status == 0x7f  || status == 0x17f || status == 0x137f)
     ourstatus->set_spurious ();
+  /* When a process completes execution and any fork process exits status. */
+  else if (WIFEXITED(status))
+    ourstatus->set_exited(0);
+  /* 57e is the status number in AIX for fork event.
+     If a breakpoint is attached to a parent or child then on fork,
+     or after fork, once a breakpoint hits and the next or continue is 
+     pressed, post breakpoint status is 1406 but we need not set status 
+     to set_forked(), hence the condition find_inferior_pid() to set 
+     fork status only if a child is born. */
+  else if (status == 0x57e && find_inferior_pid(this,pid)== nullptr)
+  {
+    ourstatus->set_forked (ptid_t(pid));
+    /* On a fork event return parent process ID to target wait */
+    return ptid_t(current_inferior()->pid);
+  }
   /* A normal waitstatus.  Let the usual macros deal with it.  */
   else
     *ourstatus = host_status_to_waitstatus (status);
-- 
2.31.1

Re: [PATCH] Enable multi process debugging for AIX

[Simon Marchi]

> From 1eab0bb05b1db09dfb98d31cc630722b40c081c4 Mon Sep 17 00:00:00 2001
> From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
> Date: Tue, 19 Jul 2022 10:39:00 -0500
> Subject: [PATCH] Enable multi process debugging for AIX
> 
> The attached proposed patch adds multi process debugging feature in AIX.
> 
> Till now AIX supported debugging only one inferior at a time,
> 
> now we can be able to debug multi process.
> 
> Users can use set follow fork mode in child or parent and
> 
> set detach on fork on or off to enable/disable simultaneous debugging of parent/child.

There are a lot of unclear things about how AIX reports a fork event to
the debugger.  Is there some documentation on that somewhere?

> ---
>  gdb/aix-thread.c     |  4 ++++
>  gdb/rs6000-aix-nat.c | 48 +++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 49 insertions(+), 3 deletions(-)
> 
> 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;

I would really prefer to avoid just setting inferior_ptid and hope
things work magically, this is a step backwards.  If some functions rely
on the value of inferior_ptid on entry, change them so they don't rely
on it anymore.

> +
>    /* Check whether libpthdebug might be ready to be initialized.  */
>    if (!pd_active && status->kind () == TARGET_WAITKIND_STOPPED
>        && status->sig () == GDB_SIGNAL_TRAP)
> diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c
> index f604f7d503e..55844ca6dab 100644
> --- a/gdb/rs6000-aix-nat.c
> +++ b/gdb/rs6000-aix-nat.c
> @@ -91,10 +91,16 @@ class rs6000_nat_target final : public inf_ptrace_target
>  
>    ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
>  
> +  /* Fork detection related functions, For adding multi process debugging
> +     support. */ 
> +  void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool) override;
> +
> +  void mourn_inferior () override;
> +
>  protected:
>  
> -  void post_startup_inferior (ptid_t ptid) override
> -  { /* Nothing.  */ }
> +  void post_startup_inferior (ptid_t ptid) override;
> +  //{ /* Nothing.  */ }
>  
>  private:
>    enum target_xfer_status
> @@ -246,6 +252,22 @@ fetch_register (struct regcache *regcache, int regno)
>      }
>  }
>  
> +void rs6000_nat_target::post_startup_inferior(ptid_t ptid){
> +  rs6000_ptrace64(PT_MULTI,ptid.pid(),NULL,1,NULL);
> +}
> +
> +void
> +rs6000_nat_target::follow_fork (inferior *child_inf, ptid_t child_ptid,
> +                          target_waitkind fork_kind, bool follow_child,
> +                                                      bool detach_fork)
> +{
> +  inf_ptrace_target::follow_fork(child_inf, child_ptid, fork_kind,
> +                                        follow_child, detach_fork);
> +}
> +
> +void rs6000_nat_target::mourn_inferior(){
> +  inf_ptrace_target::mourn_inferior();
> +}

Maybe I'm mistaken, but the two methods above seem unnecessary, since
inf_ptrace_target is the direct base of rs6000_nat_target.

>  /* Store register REGNO back into the inferior.  */
>  
>  static void
> @@ -539,8 +561,28 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>    if (status == 0x57c)
>      ourstatus->set_loaded ();
>    /* signal 0.  I have no idea why wait(2) returns with this status word.  */
> -  else if (status == 0x7f)
> +  /* 0x17f and 0x137f in hexadecimal are status returned if 
> +     if we follow parent,
> +     a switch is made to a child post parent execution
> +     and child continues its execution [user switches to child and 
> +     presses continue]. */
> +  else if (status == 0x7f  || status == 0x17f || status == 0x137f)
>      ourstatus->set_spurious ();
> +  /* When a process completes execution and any fork process exits status. */
> +  else if (WIFEXITED(status))
> +    ourstatus->set_exited(0);

Why is WIFEXITED now handled here?  It used to be handled in the
host_status_to_waitstatus call, which is still below.  Why doesn't that
work anymore?  Also, why pass a literal 0 as the exit code, can't it be
something else?

> +  /* 57e is the status number in AIX for fork event.
> +     If a breakpoint is attached to a parent or child then on fork,
> +     or after fork, once a breakpoint hits and the next or continue is 
> +     pressed, post breakpoint status is 1406 but we need not set status 
> +     to set_forked(), hence the condition find_inferior_pid() to set 
> +     fork status only if a child is born. */
> +  else if (status == 0x57e && find_inferior_pid(this,pid)== nullptr)

These magic numbers make the code really difficult to follow.  Is this
really the way AIX intends users to use the status that waitpid returns?
Isn't there a set of macros to inspect it?

Worst case, I suppose we can make our own, like:

  #define AIX_EVENT_FORK 0x57e

That would already help.

> +  {
> +    ourstatus->set_forked (ptid_t(pid));
> +    /* On a fork event return parent process ID to target wait */
> +    return ptid_t(current_inferior()->pid);

You should not use current_inferior() here, any inferior could be the
current one on entry.  Not necessarily the one that has forked.

How does AIX document the events sent to the ptracer in case of a fork?
We need to be able to figure out the pid of the parent and the pid of
the child before returning the fork event to the core.  On some platforms,
like FreeBSD, the kernel sends separate events for the parent and the
child, so we need a bit of special handling to consume both events:

  https://gitlab.com/gnutools/binutils-gdb/-/blob/9afca381e2e46ccee433ce09001506e7683b273f/gdb/fbsd-nat.c#L1003-1035

I don't know if that's the case here, but it sounds similar.

Simon

RE: [PATCH] Enable multi process debugging for AIX

[Aditya Kamath1]

Hi Simon, Ulrich and community,

I appreciate your patience to go through it and motivating us.

I have answered each of the issues point by point. Let me know your views on it and we shall then proceed towards the correct and optimal way of adding this to AIX.

Please see below my views, queries, reasons and justifications [in black and issues in red].

Have a nice day ahead.

Thanks and regards,
Aditya Kamath.

---------------------------------------------------------------
There are a lot of unclear things about how AIX reports a fork event to
the debugger. Is there some documentation on that somewhere?

Kindly use the link [ https://www.ibm.com/docs/en/aix/7.2?topic=p-ptrace-ptracex-ptrace64-subroutine ].. Here the PT_MULTI option has information about fork events.

----------------------------------------------------------

  inf_ptrace_target::follow_fork(child_inf, child_ptid, fork_kind,
> + follow_child, detach_fork);
> +}
> +
> +void rs6000_nat_target::mourn_inferior(){
> + inf_ptrace_target::mourn_inferior();
> +}

Maybe I'm mistaken, but the two methods above seem unnecessary, since
inf_ptrace_target is the direct base of rs6000_nat_target.


The generic code [infrun.c] calls the target_follow_fork() to set the follow fork and detach on fork modes. We need the functions to take care of what to do when we are in different follow or detach modes. If there is an alternate easy way let me know. Linux also uses the same.

----------------------------------------------------------

> + inferior_ptid = ptid;

I would really prefer to avoid just setting inferior_ptid and hope
things work magically, this is a step backwards. If some functions rely
on the value of inferior_ptid on entry, change them so they don't rely
on it anymore.

Why we need it?? 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

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.


-------------------------------------------------------------------------------------------------------------------

Why is WIFEXITED now handled here? It used to be handled in the
host_status_to_waitstatus call, which is still below. Why doesn't that
work anymore? Also, why pass a literal 0 as the exit code, can't it be
something else?

I agree with you on this. Will make this change.

-----------------------------------------------------------------------------------------------------

> + else if (status == 0x57e && find_inferior_pid(this,pid)== nullptr)

These magic numbers make the code really difficult to follow. Is this
really the way AIX intends users to use the status that waitpid returns?
Isn't there a set of macros to inspect it?

Worst case, I suppose we can make our own, like:

  #define AIX_EVENT_FORK 0x57e

Most likely we need to go with our own.. The only challenge being we might have multiple status code for set spurious which will be a challenge. Though let me know.. We can write our own.

----------------------------------------------------------------------------------------------------

You should not use current_inferior() here, any inferior could be the
current one on entry. Not necessarily the one that has forked.
How does AIX document the events sent to the ptracer in case of a fork?
We need to be able to figure out the pid of the parent and the pid of
the child before returning the fork event to the core. On some platforms,
like FreeBSD, the kernel sends separate events for the parent and the
child, so we need a bit of special handling to consume both events:

When PT_MULTI option is set with ptrace(), we are able to get the child process ID on a successful fork() event. I am not aware of the the parent pid handling. Since the parent and child are not simultaneously executing is the assumption on using current_inferior as only a parent will cause a wait even though I might be mistaken. We will try to figure that out, having said that if you have an idea from your experience or knowledge it will be helpful..

-------------------------------------------------------------------------------------------------


ptrace, ptracex, ptrace64 Subroutine<https://www.ibm.com/docs/en/aix/7.2?topic=p-ptrace-ptracex-ptrace64-subroutine>
For the 64-bit Process. Use ptracex where the debuggee is a 64-bit process and the operation requested uses the third (Address) parameter to reference the debuggee's address space or is sensitive to register size.Note that ptracex and ptrace64 will also support 32-bit debugees.. If returning or passing an int doesn't work for a 64-bit debuggee (for example, PT_READ_GPR), the buffer parameter ...
www.ibm.com




________________________________
From: Simon Marchi <simark@simark.ca>
Sent: 20 July 2022 00:21
To: Aditya Kamath1 <Aditya.Kamath1@ibm.com>; Ulrich Weigand <Ulrich.Weigand@de.ibm.com>; Aditya Kamath1 via Gdb-patches <gdb-patches@sourceware.org>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: [EXTERNAL] Re: [PATCH] Enable multi process debugging for AIX



> From 1eab0bb05b1db09dfb98d31cc630722b40c081c4 Mon Sep 17 00:00:00 2001
> From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
> Date: Tue, 19 Jul 2022 10:39:00 -0500
> Subject: [PATCH] Enable multi process debugging for AIX
>
> The attached proposed patch adds multi process debugging feature in AIX.
>
> Till now AIX supported debugging only one inferior at a time,
>
> now we can be able to debug multi process.
>
> Users can use set follow fork mode in child or parent and
>
> set detach on fork on or off to enable/disable simultaneous debugging of parent/child.

There are a lot of unclear things about how AIX reports a fork event to
the debugger.  Is there some documentation on that somewhere?

> ---
>  gdb/aix-thread.c     |  4 ++++
>  gdb/rs6000-aix-nat.c | 48 +++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 49 insertions(+), 3 deletions(-)
>
> 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;

I would really prefer to avoid just setting inferior_ptid and hope
things work magically, this is a step backwards.  If some functions rely
on the value of inferior_ptid on entry, change them so they don't rely
on it anymore.

> +
>    /* Check whether libpthdebug might be ready to be initialized.  */
>    if (!pd_active && status->kind () == TARGET_WAITKIND_STOPPED
>        && status->sig () == GDB_SIGNAL_TRAP)
> diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c
> index f604f7d503e..55844ca6dab 100644
> --- a/gdb/rs6000-aix-nat.c
> +++ b/gdb/rs6000-aix-nat.c
> @@ -91,10 +91,16 @@ class rs6000_nat_target final : public inf_ptrace_target
>
>    ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
>
> +  /* Fork detection related functions, For adding multi process debugging
> +     support. */
> +  void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool) override;
> +
> +  void mourn_inferior () override;
> +
>  protected:
>
> -  void post_startup_inferior (ptid_t ptid) override
> -  { /* Nothing.  */ }
> +  void post_startup_inferior (ptid_t ptid) override;
> +  //{ /* Nothing.  */ }
>
>  private:
>    enum target_xfer_status
> @@ -246,6 +252,22 @@ fetch_register (struct regcache *regcache, int regno)
>      }
>  }
>
> +void rs6000_nat_target::post_startup_inferior(ptid_t ptid){
> +  rs6000_ptrace64(PT_MULTI,ptid.pid(),NULL,1,NULL);
> +}
> +
> +void
> +rs6000_nat_target::follow_fork (inferior *child_inf, ptid_t child_ptid,
> +                          target_waitkind fork_kind, bool follow_child,
> +                                                      bool detach_fork)
> +{
> +  inf_ptrace_target::follow_fork(child_inf, child_ptid, fork_kind,
> +                                        follow_child, detach_fork);
> +}
> +
> +void rs6000_nat_target::mourn_inferior(){
> +  inf_ptrace_target::mourn_inferior();
> +}

Maybe I'm mistaken, but the two methods above seem unnecessary, since
inf_ptrace_target is the direct base of rs6000_nat_target.

>  /* Store register REGNO back into the inferior.  */
>
>  static void
> @@ -539,8 +561,28 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>    if (status == 0x57c)
>      ourstatus->set_loaded ();
>    /* signal 0.  I have no idea why wait(2) returns with this status word.  */
> -  else if (status == 0x7f)
> +  /* 0x17f and 0x137f in hexadecimal are status returned if
> +     if we follow parent,
> +     a switch is made to a child post parent execution
> +     and child continues its execution [user switches to child and
> +     presses continue]. */
> +  else if (status == 0x7f  || status == 0x17f || status == 0x137f)
>      ourstatus->set_spurious ();
> +  /* When a process completes execution and any fork process exits status. */
> +  else if (WIFEXITED(status))
> +    ourstatus->set_exited(0);

Why is WIFEXITED now handled here?  It used to be handled in the
host_status_to_waitstatus call, which is still below.  Why doesn't that
work anymore?  Also, why pass a literal 0 as the exit code, can't it be
something else?

> +  /* 57e is the status number in AIX for fork event.
> +     If a breakpoint is attached to a parent or child then on fork,
> +     or after fork, once a breakpoint hits and the next or continue is
> +     pressed, post breakpoint status is 1406 but we need not set status
> +     to set_forked(), hence the condition find_inferior_pid() to set
> +     fork status only if a child is born. */
> +  else if (status == 0x57e && find_inferior_pid(this,pid)== nullptr)

These magic numbers make the code really difficult to follow.  Is this
really the way AIX intends users to use the status that waitpid returns?
Isn't there a set of macros to inspect it?

Worst case, I suppose we can make our own, like:

  #define AIX_EVENT_FORK 0x57e

That would already help.

> +  {
> +    ourstatus->set_forked (ptid_t(pid));
> +    /* On a fork event return parent process ID to target wait */
> +    return ptid_t(current_inferior()->pid);

You should not use current_inferior() here, any inferior could be the
current one on entry.  Not necessarily the one that has forked.

How does AIX document the events sent to the ptracer in case of a fork?
We need to be able to figure out the pid of the parent and the pid of
the child before returning the fork event to the core.  On some platforms,
like FreeBSD, the kernel sends separate events for the parent and the
child, so we need a bit of special handling to consume both events:

  https://gitlab.com/gnutools/binutils-gdb/-/blob/9afca381e2e46ccee433ce09001506e7683b273f/gdb/fbsd-nat.c#L1003-1035

I don't know if that's the case here, but it sounds similar.

Simon

RE: [PATCH] Enable multi process debugging for AIX

[Aditya Kamath1]

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

Hi all,

Please find attached the patch. [See 0001-Enable-multi-process-debugging-for-AIX.patch]

The facts/steps to enable multi process debugging are :-


  *   Enable PT_MULTI option in ptrace () call along with the process ID parameter and the data parameter enabled. Usually one enables multi process debugging for all the process debugged in post_startup_inferior () function like in the Linux world.
  *   On a fork () event either the child can report a wait event first or the parent.
  *   A status of 57e in hexadecimal or 1406 in decimal is returned from the waitpid () with the process ID. This can be used to detect a fork () event.
  *   Since the child process will not have an inferior, we check with the find_inferior_pid () to see if the child reports first. If it is the next wait event is generated by the parent in AIX. This is how we absorb the parent and child event to fetch their process ID.
  *   The vice versa of the above can be done if the parent process reports a fork () event first.
  *   Once it is done we tell the gdb core that we forked with ourstatus by passing the child ptid.

More about this can be read in the document https://www.ibm.com/docs/en/aix/7.2?topic=p-ptrace-ptracex-ptrace64-subroutine .

I have left comments for the same and took into consideration the feedback given last time. Thanks to our multi thread fix we need not do any changes in aix-thread.c file any more like the last time.

Kindly give me feedback on the same.

Thanks and regards,
Aditya.

---------------------------------------------------

An example program is written below as follows with the gdb output:

#include <stdio.h>

#include <sys/types.h>

#include <unistd.h>



void hello_from_child(){

  printf("Hello from child \n");

}



void hello_from_parent(){

  printf("Hello from Parent \n");

}



int main(){

  pid_t childpid;

  printf("I should not be printed after fork as I am executed \n");

  childpid = fork();

  if (childpid == 0)

   hello_from_child();

  else

    hello_from_parent();



  return 0;

}



GDB Output:-



(gdb) b hello_from_child

Breakpoint 1 at 0x10000584: file test_fork.c, line 6.

(gdb) set follow-fork-mode child

(gdb) set detach-on-fork off

(gdb) r

Starting program: /home/test_fork

I should not be printed after fork as I am executed

[Attaching after process 23331156 fork to child process 23134700]

[New inferior 2 (process 23134700)]

[Switching to process 23134700]



Thread 2.1 hit Breakpoint 1, hello_from_child ()

    at test_fork.c:6

6         printf("Hello from child \n");

(gdb) c

Continuing.

Hello from child

[Inferior 2 (process 23134700) exited normally]

(gdb)





The following are the results after running the "gdb.base" test suite:-



With patch



# of expected passes            26584
# of unexpected failures        3968
# of unexpected successes       1
# of expected failures          17
# of known failures             26
# of unresolved testcases       109
# of untested testcases         79
# of unsupported tests          62
# of paths in test names        1
# of duplicate test names       2



Without patch



# of expected passes 26244
# of unexpected failures 4230
# of unexpected successes 1
# of expected failures 17
# of known failures 26
# of unresolved testcases 110
# of untested testcases 79
# of unsupported tests 62
# of paths in test names 1
# of duplicate test names 4

________________________________
From: Simon Marchi <simark@simark.ca>
Sent: 20 July 2022 00:21
To: Aditya Kamath1 <Aditya.Kamath1@ibm.com>; Ulrich Weigand <Ulrich.Weigand@de.ibm.com>; Aditya Kamath1 via Gdb-patches <gdb-patches@sourceware.org>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: [EXTERNAL] Re: [PATCH] Enable multi process debugging for AIX



> From 1eab0bb05b1db09dfb98d31cc630722b40c081c4 Mon Sep 17 00:00:00 2001
> From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
> Date: Tue, 19 Jul 2022 10:39:00 -0500
> Subject: [PATCH] Enable multi process debugging for AIX
>
> The attached proposed patch adds multi process debugging feature in AIX.
>
> Till now AIX supported debugging only one inferior at a time,
>
> now we can be able to debug multi process.
>
> Users can use set follow fork mode in child or parent and
>
> set detach on fork on or off to enable/disable simultaneous debugging of parent/child.

There are a lot of unclear things about how AIX reports a fork event to
the debugger.  Is there some documentation on that somewhere?

> ---
>  gdb/aix-thread.c     |  4 ++++
>  gdb/rs6000-aix-nat.c | 48 +++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 49 insertions(+), 3 deletions(-)
>
> 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;

I would really prefer to avoid just setting inferior_ptid and hope
things work magically, this is a step backwards.  If some functions rely
on the value of inferior_ptid on entry, change them so they don't rely
on it anymore.

> +
>    /* Check whether libpthdebug might be ready to be initialized.  */
>    if (!pd_active && status->kind () == TARGET_WAITKIND_STOPPED
>        && status->sig () == GDB_SIGNAL_TRAP)
> diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c
> index f604f7d503e..55844ca6dab 100644
> --- a/gdb/rs6000-aix-nat.c
> +++ b/gdb/rs6000-aix-nat.c
> @@ -91,10 +91,16 @@ class rs6000_nat_target final : public inf_ptrace_target
>
>    ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
>
> +  /* Fork detection related functions, For adding multi process debugging
> +     support. */
> +  void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool) override;
> +
> +  void mourn_inferior () override;
> +
>  protected:
>
> -  void post_startup_inferior (ptid_t ptid) override
> -  { /* Nothing.  */ }
> +  void post_startup_inferior (ptid_t ptid) override;
> +  //{ /* Nothing.  */ }
>
>  private:
>    enum target_xfer_status
> @@ -246,6 +252,22 @@ fetch_register (struct regcache *regcache, int regno)
>      }
>  }
>
> +void rs6000_nat_target::post_startup_inferior(ptid_t ptid){
> +  rs6000_ptrace64(PT_MULTI,ptid.pid(),NULL,1,NULL);
> +}
> +
> +void
> +rs6000_nat_target::follow_fork (inferior *child_inf, ptid_t child_ptid,
> +                          target_waitkind fork_kind, bool follow_child,
> +                                                      bool detach_fork)
> +{
> +  inf_ptrace_target::follow_fork(child_inf, child_ptid, fork_kind,
> +                                        follow_child, detach_fork);
> +}
> +
> +void rs6000_nat_target::mourn_inferior(){
> +  inf_ptrace_target::mourn_inferior();
> +}

Maybe I'm mistaken, but the two methods above seem unnecessary, since
inf_ptrace_target is the direct base of rs6000_nat_target.

>  /* Store register REGNO back into the inferior.  */
>
>  static void
> @@ -539,8 +561,28 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>    if (status == 0x57c)
>      ourstatus->set_loaded ();
>    /* signal 0.  I have no idea why wait(2) returns with this status word.  */
> -  else if (status == 0x7f)
> +  /* 0x17f and 0x137f in hexadecimal are status returned if
> +     if we follow parent,
> +     a switch is made to a child post parent execution
> +     and child continues its execution [user switches to child and
> +     presses continue]. */
> +  else if (status == 0x7f  || status == 0x17f || status == 0x137f)
>      ourstatus->set_spurious ();
> +  /* When a process completes execution and any fork process exits status. */
> +  else if (WIFEXITED(status))
> +    ourstatus->set_exited(0);

Why is WIFEXITED now handled here?  It used to be handled in the
host_status_to_waitstatus call, which is still below.  Why doesn't that
work anymore?  Also, why pass a literal 0 as the exit code, can't it be
something else?

> +  /* 57e is the status number in AIX for fork event.
> +     If a breakpoint is attached to a parent or child then on fork,
> +     or after fork, once a breakpoint hits and the next or continue is
> +     pressed, post breakpoint status is 1406 but we need not set status
> +     to set_forked(), hence the condition find_inferior_pid() to set
> +     fork status only if a child is born. */
> +  else if (status == 0x57e && find_inferior_pid(this,pid)== nullptr)

These magic numbers make the code really difficult to follow.  Is this
really the way AIX intends users to use the status that waitpid returns?
Isn't there a set of macros to inspect it?

Worst case, I suppose we can make our own, like:

  #define AIX_EVENT_FORK 0x57e

That would already help.

> +  {
> +    ourstatus->set_forked (ptid_t(pid));
> +    /* On a fork event return parent process ID to target wait */
> +    return ptid_t(current_inferior()->pid);

You should not use current_inferior() here, any inferior could be the
current one on entry.  Not necessarily the one that has forked.

How does AIX document the events sent to the ptracer in case of a fork?
We need to be able to figure out the pid of the parent and the pid of
the child before returning the fork event to the core.  On some platforms,
like FreeBSD, the kernel sends separate events for the parent and the
child, so we need a bit of special handling to consume both events:

  https://gitlab.com/gnutools/binutils-gdb/-/blob/9afca381e2e46ccee433ce09001506e7683b273f/gdb/fbsd-nat.c#L1003-1035

I don't know if that's the case here, but it sounds similar.

Simon

[-- Attachment #2: 0001-Enable-multi-process-debugging-for-AIX.patch --]
[-- Type: application/octet-stream, Size: 4511 bytes --]

From 41134f21c63017f364353630d60f7fed106d9473 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Thu, 18 Aug 2022 13:50:22 -0500
Subject: [PATCH] Enable-multi-process-debugging-for-AIX

---
 gdb/rs6000-aix-nat.c | 80 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 77 insertions(+), 3 deletions(-)

diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c
index cb141427696..ad47df399d8 100644
--- a/gdb/rs6000-aix-nat.c
+++ b/gdb/rs6000-aix-nat.c
@@ -73,6 +73,8 @@
 # define ARCH64() (register_size (target_gdbarch (), 0) == 8)
 #endif
 
+# define AIX_FORK_EVENT 0x57e 
+
 class rs6000_nat_target final : public inf_ptrace_target
 {
 public:
@@ -91,10 +93,13 @@ class rs6000_nat_target final : public inf_ptrace_target
 
   ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
 
+  /* Fork detection related functions, For adding multi process debugging 
+     support.  */ 
+  void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool) override;
+
 protected:
 
-  void post_startup_inferior (ptid_t ptid) override
-  { /* Nothing.  */ }
+  void post_startup_inferior (ptid_t ptid) override;
 
 private:
   enum target_xfer_status
@@ -187,6 +192,39 @@ rs6000_ptrace64 (int req, int id, long long addr, int data, void *buf)
   return ret;
 }
 
+void rs6000_nat_target::post_startup_inferior (ptid_t ptid)
+{
+ 
+  /* In AIX to turn on multi process debugging in ptrace
+     PT_MULTI is the option to be passed,
+     with the process ID which can fork () and 
+     the data parameter [fourth parameter] must be 1.  */
+
+  rs6000_ptrace64 (PT_MULTI, ptid.pid(), NULL, 1, NULL);
+}
+
+void 
+rs6000_nat_target::follow_fork (inferior *child_inf, ptid_t child_ptid,
+                          target_waitkind fork_kind, bool follow_child,
+                                                      bool detach_fork)
+{
+ 
+  /* Once the fork event is detected the infrun.c code
+     calls the target_follow_fork to take care of 
+     follow child and detach the child activity which is
+     done using the function below.  */
+  
+  inf_ptrace_target::follow_fork (child_inf, child_ptid, fork_kind,
+                                        follow_child, detach_fork);
+
+  /* If we detach fork and follow child we do not want the child
+     process to geneate events that ptrace can trace.  Hence we 
+     detach it.  */
+
+  if (detach_fork && !follow_child)
+    rs6000_ptrace64 (PT_DETACH, child_ptid.pid (), NULL, NULL, NULL);
+}
+
 /* Fetch register REGNO from the inferior.  */
 
 static void
@@ -539,8 +577,44 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
   if (status == 0x57c)
     ourstatus->set_loaded ();
   /* signal 0.  I have no idea why wait(2) returns with this status word.  */
-  else if (status == 0x7f)
+  /* 0x17f and 0x137f in hexadecimal are status returned if 
+     if we follow parent,
+     a switch is made to a child post parent execution
+     and child continues its execution [user switches to child and 
+     presses continue].  */
+  else if (status == 0x7f || status == 0x17f || status == 0x137f)
     ourstatus->set_spurious ();
+  
+  /* In AIX when a fork event is detected the waitpid return 0x57e. 
+     Either the parent can raise a wait event first or the child. 
+     Since the child does not have an inferior and is a new process,
+     we use find_inferior_pid () to check if child process
+     reported the wait event first.  The next wait event will be 
+     raised by its parent process.  This is how we get the parent
+     and child process in AIX.  After this we set the status to 
+     a fork () event and return the parent ptid_t.  */
+ 
+  else if (status == AIX_FORK_EVENT && find_inferior_pid (this,pid) == nullptr)
+  {
+    int parent_pid;
+    parent_pid = waitpid (-1, &status, 0);
+    /* passing child pid as forked pid.  */
+    ourstatus->set_forked (ptid_t (pid));
+    return ptid_t (parent_pid);
+  }
+  
+  /* This is when the parent process reports wait event first after fork ()
+     and then the child.  */
+ 
+  else if (status == AIX_FORK_EVENT)
+  {
+    int parent_pid = pid;
+    int child_pid;
+    child_pid = waitpid (-1, &status, 0);
+    /* passing child pid as forked pid.  */
+    ourstatus->set_forked (ptid_t (child_pid));
+    return ptid_t (parent_pid);
+  }
   /* A normal waitstatus.  Let the usual macros deal with it.  */
   else
     *ourstatus = host_status_to_waitstatus (status);
-- 
2.31.1

RE: [PATCH] Enable multi process debugging for AIX

[Aditya Kamath1]

Hi all,

Kindly review the recent sent patch and let me know.

Have a nice day.

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: 19 August 2022 00:29
To: Simon Marchi <simark@simark.ca>; Ulrich Weigand <Ulrich.Weigand@de.ibm.com>; Aditya Kamath1 via Gdb-patches <gdb-patches@sourceware.org>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: [EXTERNAL] RE: [PATCH] Enable multi process debugging for AIX

Hi all,

Please find attached the patch. [See 0001-Enable-multi-process-debugging-for-AIX.patch]

The facts/steps to enable multi process debugging are :-


  *   Enable PT_MULTI option in ptrace () call along with the process ID parameter and the data parameter enabled. Usually one enables multi process debugging for all the process debugged in post_startup_inferior () function like in the Linux world.
  *   On a fork () event either the child can report a wait event first or the parent.
  *   A status of 57e in hexadecimal or 1406 in decimal is returned from the waitpid () with the process ID. This can be used to detect a fork () event.
  *   Since the child process will not have an inferior, we check with the find_inferior_pid () to see if the child reports first. If it is the next wait event is generated by the parent in AIX. This is how we absorb the parent and child event to fetch their process ID.
  *   The vice versa of the above can be done if the parent process reports a fork () event first.
  *   Once it is done we tell the gdb core that we forked with ourstatus by passing the child ptid.

More about this can be read in the document https://www.ibm.com/docs/en/aix/7.2?topic=p-ptrace-ptracex-ptrace64-subroutine .

I have left comments for the same and took into consideration the feedback given last time. Thanks to our multi thread fix we need not do any changes in aix-thread.c file any more like the last time.

Kindly give me feedback on the same.

Thanks and regards,
Aditya.

---------------------------------------------------

An example program is written below as follows with the gdb output:

#include <stdio.h>

#include <sys/types.h>

#include <unistd.h>



void hello_from_child(){

  printf("Hello from child \n");

}



void hello_from_parent(){

  printf("Hello from Parent \n");

}



int main(){

  pid_t childpid;

  printf("I should not be printed after fork as I am executed \n");

  childpid = fork();

  if (childpid == 0)

   hello_from_child();

  else

    hello_from_parent();



  return 0;

}



GDB Output:-



(gdb) b hello_from_child

Breakpoint 1 at 0x10000584: file test_fork.c, line 6.

(gdb) set follow-fork-mode child

(gdb) set detach-on-fork off

(gdb) r

Starting program: /home/test_fork

I should not be printed after fork as I am executed

[Attaching after process 23331156 fork to child process 23134700]

[New inferior 2 (process 23134700)]

[Switching to process 23134700]



Thread 2.1 hit Breakpoint 1, hello_from_child ()

    at test_fork.c:6

6         printf("Hello from child \n");

(gdb) c

Continuing.

Hello from child

[Inferior 2 (process 23134700) exited normally]

(gdb)





The following are the results after running the "gdb.base" test suite:-



With patch



# of expected passes            26584
# of unexpected failures        3968
# of unexpected successes       1
# of expected failures          17
# of known failures             26
# of unresolved testcases       109
# of untested testcases         79
# of unsupported tests          62
# of paths in test names        1
# of duplicate test names       2



Without patch



# of expected passes 26244
# of unexpected failures 4230
# of unexpected successes 1
# of expected failures 17
# of known failures 26
# of unresolved testcases 110
# of untested testcases 79
# of unsupported tests 62
# of paths in test names 1
# of duplicate test names 4

________________________________
From: Simon Marchi <simark@simark.ca>
Sent: 20 July 2022 00:21
To: Aditya Kamath1 <Aditya.Kamath1@ibm.com>; Ulrich Weigand <Ulrich.Weigand@de.ibm.com>; Aditya Kamath1 via Gdb-patches <gdb-patches@sourceware.org>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: [EXTERNAL] Re: [PATCH] Enable multi process debugging for AIX



> From 1eab0bb05b1db09dfb98d31cc630722b40c081c4 Mon Sep 17 00:00:00 2001
> From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
> Date: Tue, 19 Jul 2022 10:39:00 -0500
> Subject: [PATCH] Enable multi process debugging for AIX
>
> The attached proposed patch adds multi process debugging feature in AIX.
>
> Till now AIX supported debugging only one inferior at a time,
>
> now we can be able to debug multi process.
>
> Users can use set follow fork mode in child or parent and
>
> set detach on fork on or off to enable/disable simultaneous debugging of parent/child.

There are a lot of unclear things about how AIX reports a fork event to
the debugger.  Is there some documentation on that somewhere?

> ---
>  gdb/aix-thread.c     |  4 ++++
>  gdb/rs6000-aix-nat.c | 48 +++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 49 insertions(+), 3 deletions(-)
>
> 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;

I would really prefer to avoid just setting inferior_ptid and hope
things work magically, this is a step backwards.  If some functions rely
on the value of inferior_ptid on entry, change them so they don't rely
on it anymore.

> +
>    /* Check whether libpthdebug might be ready to be initialized.  */
>    if (!pd_active && status->kind () == TARGET_WAITKIND_STOPPED
>        && status->sig () == GDB_SIGNAL_TRAP)
> diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c
> index f604f7d503e..55844ca6dab 100644
> --- a/gdb/rs6000-aix-nat.c
> +++ b/gdb/rs6000-aix-nat.c
> @@ -91,10 +91,16 @@ class rs6000_nat_target final : public inf_ptrace_target
>
>    ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
>
> +  /* Fork detection related functions, For adding multi process debugging
> +     support. */
> +  void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool) override;
> +
> +  void mourn_inferior () override;
> +
>  protected:
>
> -  void post_startup_inferior (ptid_t ptid) override
> -  { /* Nothing.  */ }
> +  void post_startup_inferior (ptid_t ptid) override;
> +  //{ /* Nothing.  */ }
>
>  private:
>    enum target_xfer_status
> @@ -246,6 +252,22 @@ fetch_register (struct regcache *regcache, int regno)
>      }
>  }
>
> +void rs6000_nat_target::post_startup_inferior(ptid_t ptid){
> +  rs6000_ptrace64(PT_MULTI,ptid.pid(),NULL,1,NULL);
> +}
> +
> +void
> +rs6000_nat_target::follow_fork (inferior *child_inf, ptid_t child_ptid,
> +                          target_waitkind fork_kind, bool follow_child,
> +                                                      bool detach_fork)
> +{
> +  inf_ptrace_target::follow_fork(child_inf, child_ptid, fork_kind,
> +                                        follow_child, detach_fork);
> +}
> +
> +void rs6000_nat_target::mourn_inferior(){
> +  inf_ptrace_target::mourn_inferior();
> +}

Maybe I'm mistaken, but the two methods above seem unnecessary, since
inf_ptrace_target is the direct base of rs6000_nat_target.

>  /* Store register REGNO back into the inferior.  */
>
>  static void
> @@ -539,8 +561,28 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>    if (status == 0x57c)
>      ourstatus->set_loaded ();
>    /* signal 0.  I have no idea why wait(2) returns with this status word.  */
> -  else if (status == 0x7f)
> +  /* 0x17f and 0x137f in hexadecimal are status returned if
> +     if we follow parent,
> +     a switch is made to a child post parent execution
> +     and child continues its execution [user switches to child and
> +     presses continue]. */
> +  else if (status == 0x7f  || status == 0x17f || status == 0x137f)
>      ourstatus->set_spurious ();
> +  /* When a process completes execution and any fork process exits status. */
> +  else if (WIFEXITED(status))
> +    ourstatus->set_exited(0);

Why is WIFEXITED now handled here?  It used to be handled in the
host_status_to_waitstatus call, which is still below.  Why doesn't that
work anymore?  Also, why pass a literal 0 as the exit code, can't it be
something else?

> +  /* 57e is the status number in AIX for fork event.
> +     If a breakpoint is attached to a parent or child then on fork,
> +     or after fork, once a breakpoint hits and the next or continue is
> +     pressed, post breakpoint status is 1406 but we need not set status
> +     to set_forked(), hence the condition find_inferior_pid() to set
> +     fork status only if a child is born. */
> +  else if (status == 0x57e && find_inferior_pid(this,pid)== nullptr)

These magic numbers make the code really difficult to follow.  Is this
really the way AIX intends users to use the status that waitpid returns?
Isn't there a set of macros to inspect it?

Worst case, I suppose we can make our own, like:

  #define AIX_EVENT_FORK 0x57e

That would already help.

> +  {
> +    ourstatus->set_forked (ptid_t(pid));
> +    /* On a fork event return parent process ID to target wait */
> +    return ptid_t(current_inferior()->pid);

You should not use current_inferior() here, any inferior could be the
current one on entry.  Not necessarily the one that has forked.

How does AIX document the events sent to the ptracer in case of a fork?
We need to be able to figure out the pid of the parent and the pid of
the child before returning the fork event to the core.  On some platforms,
like FreeBSD, the kernel sends separate events for the parent and the
child, so we need a bit of special handling to consume both events:

  https://gitlab.com/gnutools/binutils-gdb/-/blob/9afca381e2e46ccee433ce09001506e7683b273f/gdb/fbsd-nat.c#L1003-1035

I don't know if that's the case here, but it sounds similar.

Simon

Re: [PATCH] Enable multi process debugging for AIX

[Ulrich Weigand]

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

>Enable PT_MULTI option in ptrace () call along with the process ID
>parameter and the data parameter enabled. Usually one enables multi
>process debugging for all the process debugged in post_startup_inferior
>() function like in the Linux world.
>On a fork () event either the child can report a wait event first or
>the parent. 
>A status of 57e in hexadecimal or 1406 in decimal is returned from the
>waitpid () with the process ID. This can be used to detect a fork ()
>event.  
>Since the child process will not have an inferior, we check with the
>find_inferior_pid () to see if the child reports first. If it is the
>next wait event is generated by the parent in AIX. This is how we
>absorb the parent and child event to fetch their process ID. 
>The vice versa of the above can be done if the parent process reports a
>fork () event first. 
>Once it is done we tell the gdb core that we forked with ourstatus by
passing the child ptid. 

Simon already commented on this in his initial review, and I think this
is still not completely addressed.  Most importantly, your code simply
assumes that it is guaranteed that the two wait events for parent and
child will arrive immediately after one another, and the only uncertain
issue is the sequence between the two.

Is is actually guaranteed that this is the case?  Or could we also have
random other wait events (e.g. from other threads / processes) that can
be reported in between?   In that case, your implementation now
introduces a race condition where some event might get lost.

A proper fix might then have to be more involved, e.g. by storing those
event on some sort of "pending list" and only report a fork event to
common code once both sides have checked in, similar to the FreeBSD
approach Simon already pointed out to you.


As to this:
+# define AIX_FORK_EVENT 0x57e 

It would be much preferable to use the official names for these
constants.  For example, the document you mention here:

>More about this can be read in the document 
>https://www.ibm.com/docs/en/aix/7.2?topic=p-ptrace-ptracex-ptrace64-
subroutine

talks about:
  W_SFWTED
  Process stopped during execution of the fork subroutine.

Is this the 0x57e event?  If so, we should call it W_SFWTED in GDB as
well, and not some made-up name.

It would be great if you could find the official names for all the
other "magic" constants like 0x7f, 0x17f, 0x137f etc. as well.


Bye,
Ulrich

Re: [PATCH] Enable multi process debugging for AIX

[Simon Marchi]

> As to this:
> +# define AIX_FORK_EVENT 0x57e 
> 
> It would be much preferable to use the official names for these
> constants.  For example, the document you mention here:
> 
>> More about this can be read in the document 
>> https://www.ibm.com/docs/en/aix/7.2?topic=p-ptrace-ptracex-ptrace64-
> subroutine
> 
> talks about:
>   W_SFWTED
>   Process stopped during execution of the fork subroutine.
> 
> Is this the 0x57e event?  If so, we should call it W_SFWTED in GDB as
> well, and not some made-up name.
> 
> It would be great if you could find the official names for all the
> other "magic" constants like 0x7f, 0x17f, 0x137f etc. as well.

Is there no system header file that provides them?

Simon

Re: [PATCH] Enable multi process debugging for AIX

[Aditya Kamath1]

Hi Ulrich,

I understood what you and Simon are trying to tell. I get why this patch will fail.

I will get back to you all in couple of days with the solution.

Thanks and regards,
Aditya.
________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 22 August 2022 18:55
To: simark@simark.ca <simark@simark.ca>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: Re: [PATCH] Enable multi process debugging for AIX

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

>Enable PT_MULTI option in ptrace () call along with the process ID
>parameter and the data parameter enabled. Usually one enables multi
>process debugging for all the process debugged in post_startup_inferior
>() function like in the Linux world.
>On a fork () event either the child can report a wait event first or
>the parent.
>A status of 57e in hexadecimal or 1406 in decimal is returned from the
>waitpid () with the process ID. This can be used to detect a fork ()
>event.
>Since the child process will not have an inferior, we check with the
>find_inferior_pid () to see if the child reports first. If it is the
>next wait event is generated by the parent in AIX. This is how we
>absorb the parent and child event to fetch their process ID.
>The vice versa of the above can be done if the parent process reports a
>fork () event first.
>Once it is done we tell the gdb core that we forked with ourstatus by
passing the child ptid.

Simon already commented on this in his initial review, and I think this
is still not completely addressed.  Most importantly, your code simply
assumes that it is guaranteed that the two wait events for parent and
child will arrive immediately after one another, and the only uncertain
issue is the sequence between the two.

Is is actually guaranteed that this is the case?  Or could we also have
random other wait events (e.g. from other threads / processes) that can
be reported in between?   In that case, your implementation now
introduces a race condition where some event might get lost.

A proper fix might then have to be more involved, e.g. by storing those
event on some sort of "pending list" and only report a fork event to
common code once both sides have checked in, similar to the FreeBSD
approach Simon already pointed out to you.


As to this:
+# define AIX_FORK_EVENT 0x57e

It would be much preferable to use the official names for these
constants.  For example, the document you mention here:

>More about this can be read in the document
>https://www.ibm.com/docs/en/aix/7.2?topic=p-ptrace-ptracex-ptrace64-
subroutine

talks about:
  W_SFWTED
  Process stopped during execution of the fork subroutine.

Is this the 0x57e event?  If so, we should call it W_SFWTED in GDB as
well, and not some made-up name.

It would be great if you could find the official names for all the
other "magic" constants like 0x7f, 0x17f, 0x137f etc. as well.


Bye,
Ulrich

Re: [PATCH] Enable multi process debugging for AIX

[Aditya Kamath1]

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

Hi Ulrich and Simon,

Please find attached a new patch on this. [See: 0001-Enable-multi-process-debugging-for-AIX.patch]

I have addressed your issue of having a fork () events from other threads. There is one difference between AIX and the way freebsd can handle. freebsd gets the parent child relationship via a structure which has this info. This is obtained using LWP_INFO option in the ptrace () call. However, in AIX we do not have the same luxury of LWP_INFO. Here is how it works. A fork () event can be caused by the debugee only. Once the child is obtained, we know for a fact that either it is detached or stopped in the instruction where the parent was before fork () event. The child is not going to generate any fork event. Hence only this debugee which the parent will be able to fork again. As far as multiple thread fork is concerned, I have taken care of it in the code by a fact that a parent process will be in the inferior list.

Coming to the W_SFWTED, this also is taken care. Kindly see the did_aix_inferior_fork () function.


I have kept an assertion as well just in case I get some other event apart from fork () comes in while I figure out the parent child relationship. Ideally, I shouldn't get any other event in AIX.

Kindly let me know what is your feeback on this.

If its good kindly push the same.

Have a nice day ahead.

Thanks and regards,
Aditya.


________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 22 August 2022 18:55
To: simark@simark.ca <simark@simark.ca>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: Re: [PATCH] Enable multi process debugging for AIX

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

>Enable PT_MULTI option in ptrace () call along with the process ID
>parameter and the data parameter enabled. Usually one enables multi
>process debugging for all the process debugged in post_startup_inferior
>() function like in the Linux world.
>On a fork () event either the child can report a wait event first or
>the parent.
>A status of 57e in hexadecimal or 1406 in decimal is returned from the
>waitpid () with the process ID. This can be used to detect a fork ()
>event.
>Since the child process will not have an inferior, we check with the
>find_inferior_pid () to see if the child reports first. If it is the
>next wait event is generated by the parent in AIX. This is how we
>absorb the parent and child event to fetch their process ID.
>The vice versa of the above can be done if the parent process reports a
>fork () event first.
>Once it is done we tell the gdb core that we forked with ourstatus by
passing the child ptid.

Simon already commented on this in his initial review, and I think this
is still not completely addressed.  Most importantly, your code simply
assumes that it is guaranteed that the two wait events for parent and
child will arrive immediately after one another, and the only uncertain
issue is the sequence between the two.

Is is actually guaranteed that this is the case?  Or could we also have
random other wait events (e.g. from other threads / processes) that can
be reported in between?   In that case, your implementation now
introduces a race condition where some event might get lost.

A proper fix might then have to be more involved, e.g. by storing those
event on some sort of "pending list" and only report a fork event to
common code once both sides have checked in, similar to the FreeBSD
approach Simon already pointed out to you.


As to this:
+# define AIX_FORK_EVENT 0x57e

It would be much preferable to use the official names for these
constants.  For example, the document you mention here:

>More about this can be read in the document
>https://www.ibm.com/docs/en/aix/7.2?topic=p-ptrace-ptracex-ptrace64-
subroutine

talks about:
  W_SFWTED
  Process stopped during execution of the fork subroutine.

Is this the 0x57e event?  If so, we should call it W_SFWTED in GDB as
well, and not some made-up name.

It would be great if you could find the official names for all the
other "magic" constants like 0x7f, 0x17f, 0x137f etc. as well.


Bye,
Ulrich


[-- Attachment #2: 0001-Enable-multi-process-debugging-for-AIX.patch --]
[-- Type: application/octet-stream, Size: 6934 bytes --]

From 43a70d6621a300fe6fa8a13ef02e0c3fc7aaaa8c Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Wed, 19 Oct 2022 05:35:44 -0500
Subject: [PATCH] Enable multi-process debugging for AIX

---
 gdb/rs6000-aix-nat.c | 183 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 179 insertions(+), 4 deletions(-)

diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c
index cb141427696..39388968537 100644
--- a/gdb/rs6000-aix-nat.c
+++ b/gdb/rs6000-aix-nat.c
@@ -91,10 +91,13 @@ class rs6000_nat_target final : public inf_ptrace_target
 
   ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
 
+  /* Fork detection related functions, For adding multi process debugging
+     support.  */
+  void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool) override;
+
 protected:
 
-  void post_startup_inferior (ptid_t ptid) override
-  { /* Nothing.  */ }
+  void post_startup_inferior (ptid_t ptid) override;
 
 private:
   enum target_xfer_status
@@ -107,6 +110,83 @@ class rs6000_nat_target final : public inf_ptrace_target
 
 static rs6000_nat_target the_rs6000_nat_target;
 
+/* The below declaration is to track number of times, parent has
+   reported fork event before its children.  */
+
+static std::list<pid_t> aix_pending_parent;
+
+/* The below declaration is for a child process event that
+   is reported before its corresponding parent process in
+   the event of a fork ().  */
+
+static std::list<pid_t> aix_pending_children;
+
+static void
+aix_remember_child (pid_t pid)
+{
+  aix_pending_children.push_front (pid);
+}
+
+static void
+aix_remember_parent (pid_t pid)
+{
+  aix_pending_parent.push_front (pid);
+}
+
+/* In the below function we check if there was any child
+   process pending.  If it exists we return it from the
+   list, otherwise we return a null.  */
+
+static pid_t
+aix_is_child_pending ()
+{
+  auto it = aix_pending_children.begin ();
+  if (it != aix_pending_children.end ())
+  {
+    int child = *it;
+    aix_pending_children.erase (it);
+    return child;
+  }
+  return 0;
+}
+
+/* In the below function we check if there was any parent 
+   process pending.  If it exists we return it from the
+   list, otherwise we return a null.  */
+
+static pid_t
+aix_is_parent_pending ()
+{
+  auto it = aix_pending_parent.begin ();
+  if (it != aix_pending_parent.end ())
+  {
+    int parent = *it;
+    aix_pending_parent.erase (it);
+    return parent;
+  }
+  return 0;
+}
+
+/* This function checks if there was a fork () event.  */
+
+static bool
+did_aix_inferior_fork (int status)
+{
+  /* If multi-process debug mode is enabled, the status
+     location is set to W_SFWTED.  */
+
+  status = status & 0xff;
+
+  /* Eliminate the last few bits. If the W_SFWTED is set
+     which is equal to 0x7e, it is a fork event otherwise
+     it is not.  */
+
+  if (status ^ W_SFWTED)
+    return false;
+  else
+    return true;
+}
+
 /* Given REGNO, a gdb register number, return the corresponding
    number suitable for use as a ptrace() parameter.  Return -1 if
    there's no suitable mapping.  Also, set the int pointed to by
@@ -187,6 +267,47 @@ rs6000_ptrace64 (int req, int id, long long addr, int data, void *buf)
   return ret;
 }
 
+void rs6000_nat_target::post_startup_inferior (ptid_t ptid)
+{
+
+  /* In AIX to turn on multi process debugging in ptrace
+     PT_MULTI is the option to be passed,
+     with the process ID which can fork () and
+     the data parameter [fourth parameter] must be 1.  */
+
+  if (!ARCH64 ())
+    rs6000_ptrace32 (PT_MULTI, ptid.pid(), 0, 1, 0);
+  else
+    rs6000_ptrace64 (PT_MULTI, ptid.pid(), 0, 1, 0);
+}
+
+void
+rs6000_nat_target::follow_fork (inferior *child_inf, ptid_t child_ptid,
+                          target_waitkind fork_kind, bool follow_child,
+                                                      bool detach_fork)
+{
+
+  /* Once the fork event is detected the infrun.c code
+     calls the target_follow_fork to take care of
+     follow child and detach the child activity which is
+     done using the function below.  */
+
+  inf_ptrace_target::follow_fork (child_inf, child_ptid, fork_kind,
+                                        follow_child, detach_fork);
+
+  /* If we detach fork and follow child we do not want the child
+     process to geneate events that ptrace can trace.  Hence we
+     detach it.  */
+
+  if (detach_fork && !follow_child)
+  {
+    if (ARCH64 ())
+      rs6000_ptrace64 (PT_DETACH, child_ptid.pid (), 0, 0, 0);
+    else
+      rs6000_ptrace32 (PT_DETACH, child_ptid.pid (), 0, 0, 0);
+  }
+}
+
 /* Fetch register REGNO from the inferior.  */
 
 static void
@@ -538,9 +659,63 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
   /* stop after load" status.  */
   if (status == 0x57c)
     ourstatus->set_loaded ();
-  /* signal 0.  I have no idea why wait(2) returns with this status word.  */
-  else if (status == 0x7f)
+  /* 0x7f is signal 0.  */
+  /* 0x17f and 0x137f in hexadecimal are status returned if
+     if we follow parent,
+     a switch is made to a child post parent execution
+     and child continues its execution [user switches to child and
+     presses continue].  */
+  else if (status == 0x7f || status == 0x17f || status == 0x137f)
     ourstatus->set_spurious ();
+  /* Check for a fork () event.  */
+  else if (did_aix_inferior_fork (status))
+  {
+    /* Checking whether it is a parent or a child event.  */
+
+    if (find_inferior_pid (this, pid) == nullptr)
+      aix_remember_child (pid);
+    else
+      aix_remember_parent (pid);
+
+    while (1)
+    {
+      pid = waitpid (-1, &status, 0);
+      gdb_assert (did_aix_inferior_fork (status) == true);
+      
+      /* If the event is a child we check if there was a parent
+         event recorded before.  If yes we got the parent child
+         relationship.  If not we push this child and wait for 
+         the next fork () event.  */
+ 
+      if (find_inferior_pid (this, pid) == nullptr)
+      {
+        pid_t parent_pid = aix_is_parent_pending ();
+        if (parent_pid > 0)
+        {
+          ourstatus->set_forked (ptid_t (pid));
+          return ptid_t (parent_pid);
+        }
+        aix_remember_child (pid);
+      }
+
+      /* If the event is a parent we check if there was a child
+         event recorded before.  If yes we got the parent child
+         relationship.  If not we push this parent and wait for 
+         the next fork () event.  */
+
+      else
+      {
+        pid_t child_pid = aix_is_child_pending ();
+        if (child_pid > 0)
+        {
+          ourstatus->set_forked (ptid_t (child_pid));
+          return ptid_t (pid);
+        }
+        aix_remember_parent (pid);
+      } 
+    }
+  }
+
   /* A normal waitstatus.  Let the usual macros deal with it.  */
   else
     *ourstatus = host_status_to_waitstatus (status);
-- 
2.31.1

Re: [PATCH] Enable multi process debugging for AIX

[Aditya Kamath1]

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

Hi Ulrich and Simon,

Please find attached a new patch on this. [See: 0001-Enable-multi-process-debugging-for-AIX.patch]

I have addressed your issue of having a fork () events from other threads. There is one difference between AIX and the way freebsd can handle. freebsd gets the parent child relationship via a structure which has this info. This is obtained using LWP_INFO option in the ptrace () call. However, in AIX we do not have the same luxury of LWP_INFO. Here is how it works. A fork () event can be caused by the debugee only. Once the child is obtained, we know for a fact that either it is detached or stopped in the instruction where the parent was before fork () event. The child is not going to generate any fork event. Hence only this debugee which the parent will be able to fork again. As far as multiple thread fork is concerned, I have taken care of it in the code by a fact that a parent process will be in the inferior list.

Coming to the W_SFWTED, this also is taken care. Kindly see the did_aix_inferior_fork () function.


I have kept an assertion as well just in case I get some other event apart from fork () comes in while I figure out the parent child relationship. Ideally, I shouldn't get any other event in AIX.

Kindly let me know what is your feeback on this.

If its good kindly push the same.

Have a nice day ahead.

Thanks and regards,
Aditya.


________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 22 August 2022 18:55
To: simark@simark.ca <simark@simark.ca>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: Re: [PATCH] Enable multi process debugging for AIX

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

>Enable PT_MULTI option in ptrace () call along with the process ID
>parameter and the data parameter enabled. Usually one enables multi
>process debugging for all the process debugged in post_startup_inferior
>() function like in the Linux world.
>On a fork () event either the child can report a wait event first or
>the parent.
>A status of 57e in hexadecimal or 1406 in decimal is returned from the
>waitpid () with the process ID. This can be used to detect a fork ()
>event.
>Since the child process will not have an inferior, we check with the
>find_inferior_pid () to see if the child reports first. If it is the
>next wait event is generated by the parent in AIX. This is how we
>absorb the parent and child event to fetch their process ID.
>The vice versa of the above can be done if the parent process reports a
>fork () event first.
>Once it is done we tell the gdb core that we forked with ourstatus by
passing the child ptid.

Simon already commented on this in his initial review, and I think this
is still not completely addressed.  Most importantly, your code simply
assumes that it is guaranteed that the two wait events for parent and
child will arrive immediately after one another, and the only uncertain
issue is the sequence between the two.

Is is actually guaranteed that this is the case?  Or could we also have
random other wait events (e.g. from other threads / processes) that can
be reported in between?   In that case, your implementation now
introduces a race condition where some event might get lost.

A proper fix might then have to be more involved, e.g. by storing those
event on some sort of "pending list" and only report a fork event to
common code once both sides have checked in, similar to the FreeBSD
approach Simon already pointed out to you.


As to this:
+# define AIX_FORK_EVENT 0x57e

It would be much preferable to use the official names for these
constants.  For example, the document you mention here:

>More about this can be read in the document
>https://www.ibm.com/docs/en/aix/7.2?topic=p-ptrace-ptracex-ptrace64-
subroutine

talks about:
  W_SFWTED
  Process stopped during execution of the fork subroutine.

Is this the 0x57e event?  If so, we should call it W_SFWTED in GDB as
well, and not some made-up name.

It would be great if you could find the official names for all the
other "magic" constants like 0x7f, 0x17f, 0x137f etc. as well.


Bye,
Ulrich


[-- Attachment #2: 0001-Enable-multi-process-debugging-for-AIX.patch --]
[-- Type: application/octet-stream, Size: 6934 bytes --]

From 43a70d6621a300fe6fa8a13ef02e0c3fc7aaaa8c Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Wed, 19 Oct 2022 05:35:44 -0500
Subject: [PATCH] Enable multi-process debugging for AIX

---
 gdb/rs6000-aix-nat.c | 183 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 179 insertions(+), 4 deletions(-)

diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c
index cb141427696..39388968537 100644
--- a/gdb/rs6000-aix-nat.c
+++ b/gdb/rs6000-aix-nat.c
@@ -91,10 +91,13 @@ class rs6000_nat_target final : public inf_ptrace_target
 
   ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
 
+  /* Fork detection related functions, For adding multi process debugging
+     support.  */
+  void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool) override;
+
 protected:
 
-  void post_startup_inferior (ptid_t ptid) override
-  { /* Nothing.  */ }
+  void post_startup_inferior (ptid_t ptid) override;
 
 private:
   enum target_xfer_status
@@ -107,6 +110,83 @@ class rs6000_nat_target final : public inf_ptrace_target
 
 static rs6000_nat_target the_rs6000_nat_target;
 
+/* The below declaration is to track number of times, parent has
+   reported fork event before its children.  */
+
+static std::list<pid_t> aix_pending_parent;
+
+/* The below declaration is for a child process event that
+   is reported before its corresponding parent process in
+   the event of a fork ().  */
+
+static std::list<pid_t> aix_pending_children;
+
+static void
+aix_remember_child (pid_t pid)
+{
+  aix_pending_children.push_front (pid);
+}
+
+static void
+aix_remember_parent (pid_t pid)
+{
+  aix_pending_parent.push_front (pid);
+}
+
+/* In the below function we check if there was any child
+   process pending.  If it exists we return it from the
+   list, otherwise we return a null.  */
+
+static pid_t
+aix_is_child_pending ()
+{
+  auto it = aix_pending_children.begin ();
+  if (it != aix_pending_children.end ())
+  {
+    int child = *it;
+    aix_pending_children.erase (it);
+    return child;
+  }
+  return 0;
+}
+
+/* In the below function we check if there was any parent 
+   process pending.  If it exists we return it from the
+   list, otherwise we return a null.  */
+
+static pid_t
+aix_is_parent_pending ()
+{
+  auto it = aix_pending_parent.begin ();
+  if (it != aix_pending_parent.end ())
+  {
+    int parent = *it;
+    aix_pending_parent.erase (it);
+    return parent;
+  }
+  return 0;
+}
+
+/* This function checks if there was a fork () event.  */
+
+static bool
+did_aix_inferior_fork (int status)
+{
+  /* If multi-process debug mode is enabled, the status
+     location is set to W_SFWTED.  */
+
+  status = status & 0xff;
+
+  /* Eliminate the last few bits. If the W_SFWTED is set
+     which is equal to 0x7e, it is a fork event otherwise
+     it is not.  */
+
+  if (status ^ W_SFWTED)
+    return false;
+  else
+    return true;
+}
+
 /* Given REGNO, a gdb register number, return the corresponding
    number suitable for use as a ptrace() parameter.  Return -1 if
    there's no suitable mapping.  Also, set the int pointed to by
@@ -187,6 +267,47 @@ rs6000_ptrace64 (int req, int id, long long addr, int data, void *buf)
   return ret;
 }
 
+void rs6000_nat_target::post_startup_inferior (ptid_t ptid)
+{
+
+  /* In AIX to turn on multi process debugging in ptrace
+     PT_MULTI is the option to be passed,
+     with the process ID which can fork () and
+     the data parameter [fourth parameter] must be 1.  */
+
+  if (!ARCH64 ())
+    rs6000_ptrace32 (PT_MULTI, ptid.pid(), 0, 1, 0);
+  else
+    rs6000_ptrace64 (PT_MULTI, ptid.pid(), 0, 1, 0);
+}
+
+void
+rs6000_nat_target::follow_fork (inferior *child_inf, ptid_t child_ptid,
+                          target_waitkind fork_kind, bool follow_child,
+                                                      bool detach_fork)
+{
+
+  /* Once the fork event is detected the infrun.c code
+     calls the target_follow_fork to take care of
+     follow child and detach the child activity which is
+     done using the function below.  */
+
+  inf_ptrace_target::follow_fork (child_inf, child_ptid, fork_kind,
+                                        follow_child, detach_fork);
+
+  /* If we detach fork and follow child we do not want the child
+     process to geneate events that ptrace can trace.  Hence we
+     detach it.  */
+
+  if (detach_fork && !follow_child)
+  {
+    if (ARCH64 ())
+      rs6000_ptrace64 (PT_DETACH, child_ptid.pid (), 0, 0, 0);
+    else
+      rs6000_ptrace32 (PT_DETACH, child_ptid.pid (), 0, 0, 0);
+  }
+}
+
 /* Fetch register REGNO from the inferior.  */
 
 static void
@@ -538,9 +659,63 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
   /* stop after load" status.  */
   if (status == 0x57c)
     ourstatus->set_loaded ();
-  /* signal 0.  I have no idea why wait(2) returns with this status word.  */
-  else if (status == 0x7f)
+  /* 0x7f is signal 0.  */
+  /* 0x17f and 0x137f in hexadecimal are status returned if
+     if we follow parent,
+     a switch is made to a child post parent execution
+     and child continues its execution [user switches to child and
+     presses continue].  */
+  else if (status == 0x7f || status == 0x17f || status == 0x137f)
     ourstatus->set_spurious ();
+  /* Check for a fork () event.  */
+  else if (did_aix_inferior_fork (status))
+  {
+    /* Checking whether it is a parent or a child event.  */
+
+    if (find_inferior_pid (this, pid) == nullptr)
+      aix_remember_child (pid);
+    else
+      aix_remember_parent (pid);
+
+    while (1)
+    {
+      pid = waitpid (-1, &status, 0);
+      gdb_assert (did_aix_inferior_fork (status) == true);
+      
+      /* If the event is a child we check if there was a parent
+         event recorded before.  If yes we got the parent child
+         relationship.  If not we push this child and wait for 
+         the next fork () event.  */
+ 
+      if (find_inferior_pid (this, pid) == nullptr)
+      {
+        pid_t parent_pid = aix_is_parent_pending ();
+        if (parent_pid > 0)
+        {
+          ourstatus->set_forked (ptid_t (pid));
+          return ptid_t (parent_pid);
+        }
+        aix_remember_child (pid);
+      }
+
+      /* If the event is a parent we check if there was a child
+         event recorded before.  If yes we got the parent child
+         relationship.  If not we push this parent and wait for 
+         the next fork () event.  */
+
+      else
+      {
+        pid_t child_pid = aix_is_child_pending ();
+        if (child_pid > 0)
+        {
+          ourstatus->set_forked (ptid_t (child_pid));
+          return ptid_t (pid);
+        }
+        aix_remember_parent (pid);
+      } 
+    }
+  }
+
   /* A normal waitstatus.  Let the usual macros deal with it.  */
   else
     *ourstatus = host_status_to_waitstatus (status);
-- 
2.31.1

Re: [PATCH] Enable multi process debugging for AIX

[Ulrich Weigand]

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

>I have addressed your issue of having a fork () events from other threads.

To clarify: I was not only concerned about *fork* events from other
threads, but *any* events from other threads.  For example, if a
multi-threaded process forks, is it possible that in the time between
the parent and child fork events arrive, some other thread hits a
breakpoint and that event is reported to GDB?

Or if GDB is already debugging multiple processes, could some event
from a completely different process arrive in between?

>I have kept an assertion as well just in case I get some other event
>apart from fork () comes in while I figure out the parent child relationship.
>Ideally, I shouldn't get any other event in AIX. 

I believe in the scenarios above, you would run into that assertion.

I still think that the proper fix is to *not* have any secondary
loop around waitpid.  Instead, just use the main GDB event loop;
it'll get back to you once the second event arrives, and in the
meantime other intervening events will be processed as usual.

>There is one difference between AIX and the way freebsd can handle.
>freebsd gets the parent child relationship via a structure which has
>this info. This is obtained using LWP_INFO option in the ptrace () call.
>However, in AIX we do not have the same luxury of LWP_INFO. 

Is there any other way of figuring out what the parent process of
the new child is?  Maybe you should just do that when you get the
child event.

>Coming to the W_SFWTED, this also is taken care. Kindly see the
>did_aix_inferior_fork () function. 

This logic seems a bit strange to me:

>+  /* If multi-process debug mode is enabled, the status
>+     location is set to W_SFWTED.  */
>+
>+  status = status & 0xff;
>+
>+  /* Eliminate the last few bits. If the W_SFWTED is set
>+     which is equal to 0x7e, it is a fork event otherwise
>+     it is not.  */
>+
>+  if (status ^ W_SFWTED)
>+    return false;
>+  else
>+    return true;

I understand W_SFWTED is not a bit mask, but simply a value.
So shouldn't you just test for "status == W_SFWTED"?  Also,
I'm not sure where the "& 0xff" comes into play.  What are
the contents of the high bits?  The AIX documentation doesn't
seem to say ...


Bye,
Ulrich

Re: [PATCH] Enable multi process debugging for AIX

[Aditya Kamath1]

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

Hi Ulrich,

>There is one difference between AIX and the way freebsd can handle.
>freebsd gets the parent child relationship via a structure which has
>this info. This is obtained using LWP_INFO option in the ptrace () call.
>However, in AIX we do not have the same luxury of LWP_INFO.

Is there any other way of figuring out what the parent process of
the new child is?  Maybe you should just do that when you get the
child event.

I fully did not get the above suggestion " Maybe you should just do that when you get the
child event ". Kindly let me know what you are trying to tell me in this context.

Have a nice day ahead.

Thanks and regards,
Aditya
________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 28 October 2022 16:29
To: simark@simark.ca <simark@simark.ca>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: Re: [PATCH] Enable multi process debugging for AIX

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

>I have addressed your issue of having a fork () events from other threads.

To clarify: I was not only concerned about *fork* events from other
threads, but *any* events from other threads.  For example, if a
multi-threaded process forks, is it possible that in the time between
the parent and child fork events arrive, some other thread hits a
breakpoint and that event is reported to GDB?

Or if GDB is already debugging multiple processes, could some event
from a completely different process arrive in between?

>I have kept an assertion as well just in case I get some other event
>apart from fork () comes in while I figure out the parent child relationship.
>Ideally, I shouldn't get any other event in AIX.

I believe in the scenarios above, you would run into that assertion.

I still think that the proper fix is to *not* have any secondary
loop around waitpid.  Instead, just use the main GDB event loop;
it'll get back to you once the second event arrives, and in the
meantime other intervening events will be processed as usual.

>There is one difference between AIX and the way freebsd can handle.
>freebsd gets the parent child relationship via a structure which has
>this info. This is obtained using LWP_INFO option in the ptrace () call.
>However, in AIX we do not have the same luxury of LWP_INFO.

Is there any other way of figuring out what the parent process of
the new child is?  Maybe you should just do that when you get the
child event.

>Coming to the W_SFWTED, this also is taken care. Kindly see the
>did_aix_inferior_fork () function.

This logic seems a bit strange to me:

>+  /* If multi-process debug mode is enabled, the status
>+     location is set to W_SFWTED.  */
>+
>+  status = status & 0xff;
>+
>+  /* Eliminate the last few bits. If the W_SFWTED is set
>+     which is equal to 0x7e, it is a fork event otherwise
>+     it is not.  */
>+
>+  if (status ^ W_SFWTED)
>+    return false;
>+  else
>+    return true;

I understand W_SFWTED is not a bit mask, but simply a value.
So shouldn't you just test for "status == W_SFWTED"?  Also,
I'm not sure where the "& 0xff" comes into play.  What are
the contents of the high bits?  The AIX documentation doesn't
seem to say ...


Bye,
Ulrich

Re: [PATCH] Enable multi process debugging for AIX

[Ulrich Weigand]

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

>>Is there any other way of figuring out what the parent process of
>>the new child is?  Maybe you should just do that when you get the
>>child event.
>
>I fully did not get the above suggestion " Maybe you should just do that when you get the
>child event ". Kindly let me know what you are trying to tell me in this context. 

The last iteration of you patch simply has a list of "pending parent"
and "pending child" PIDs.  If you get a fork event for the child, and
there is *any* parent PID in the "pending" list, you automatically
assume that this is the parent of *this* child.

I'm concerned that this may not always be true.  For example, if you
are already debugging multiple processes, and *two* of those fork at
the same time, you'll be getting four ptrace events in GDB - two
events for the two parent processes, and two events for the two new
child processes.  If this happens, you'll need to verify which child
is actually associated with *which* parent.

As you note, on other platforms this relationship is reported as part
of the ptrace event directly, and this is apparently not the case on
AIX.  However, the OS of course still knows what the parent process
of each of the child processes is - so I guess there should be *some*
way to find this out from within GDB.  (E.g. in Linux you could get
at that information by reading some /proc files.)

Bye,
Ulrich

Re: [PATCH] Enable multi process debugging for AIX

[Aditya Kamath1]

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

Hi Ulrich and Simon,

Please find attached the patch [See: 0001-Enable-multi-process-debugging-for-AIX.patch]

I have taken care of parent child relationship. See find_my_aix_parent () function. You can refer the link https://www.ibm.com/docs/en/aix/7.2?topic=g-getprocs-subroutine for the documentation of the same.

I have also taken care of what if break points hit in other threads while only a child or a parent event is reported in another thread as you mentioned Ulrich or any other event. [You can refer the output I have pasted below]. The while loop will take care.
getprocs Subroutine - IBM<https://www.ibm.com/docs/en/aix/7.2?topic=g-getprocs-subroutine>
Note: The ProcessBuffer parameter of getprocs subroutine contains two struct rusage fields named pi_ru and pi_cru.Each of these fields contains two struct timeval fields named ru_utime and ru_stime.The tv_usec field in both of the struct timeval contain nanoseconds instead of microseconds. These values cone from the struct user fields named U_ru and U_cru.
www.ibm.com


>I understand W_SFWTED is not a bit mask, but simply a value.
>So shouldn't you just test for "status == W_SFWTED"?  Also,
>I'm not sure where the "& 0xff" comes into play.  What are
>the contents of the high bits?  The AIX documentation doesn't
>seem to say ...

So, if there is a fork event then my W_SFWTED = 0x7e = 0x01111110 bits is set in my status flag. The other bits represent other things in the status flag. Therefore, I eliminate other bits with AND operation with 0xff and see only if my first 8 bits are set.. You were right with this "status == W_SFWTED". I corrected it.


Let me know what you think.. Hope this patch works..

Have a nice day ahead.

Thanks and regards,
Aditya.

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

  pid_t child;


  child = fork ();

  if (child > 0)

    printf ("I am parent \n");

  else{

    printf (" Iam child \n");

    child = fork ();

    if (child > 0)

      printf ("From child I became a parent \n");

    else

      printf ("I am grandchild \n");

  }

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


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

(gdb) b thread_function

Breakpoint 1 at 0x100006f8: file /home/XYZ/gdb_tests/ultimate-multi-thread-fork.c, line 18.

(gdb) set detach-on-fork off

(gdb) r

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

[New Thread 1]

[New Thread 258]

[Switching to Thread 258]


Thread 3 hit Breakpoint 1, thread_function (arg=0x0)

    at /home/aditya/gdb_tests/ultimate-multi-thread-fork.c:18

18        pthread_barrier_wait (&barrier);

(gdb) c

Continuing.

[New Thread 515]

[Switching to Thread 515]


Thread 4 hit Breakpoint 1, thread_function (arg=warning: (Internal error: pc 0x0 in read in psymtab, but not in symtab.)


0x0)

    at /home/XYZ/gdb_tests/ultimate-multi-thread-fork.c:18

18        pthread_barrier_wait (&barrier);

(gdb) c

Continuing.

[New inferior 2 (process 18612656)]

warning: "/usr/lib/libpthreads.a": member "shr_comm.o" missing.

warning: "/usr/lib/libcrypt.a": member "shr.o" missing.

warning: "/usr/lib/libpthread.a": member "shr_xpg5.o" missing.

warning: "/usr/lib/libc.a": member "shr.o" missing.

warning: Could not load shared library symbols for 4 libraries, e.g. /usr/lib/libpthreads.a(shr_comm.o).

Use the "info sharedlibrary" command to see the complete listing.

Do you need "set solib-search-path" or "set sysroot"?

I am parent

[New inferior 3 (process 8454632)]

warning: "/usr/lib/libpthreads.a": member "shr_comm.o" missing.

warning: "/usr/lib/libcrypt.a": member "shr.o" missing.

warning: "/usr/lib/libpthread.a": member "shr_xpg5.o" missing.

warning: "/usr/lib/libc.a": member "shr.o" missing.

warning: Could not load shared library symbols for 4 libraries, e.g. /usr/lib/libpthreads.a(shr_comm.o).

Use the "info sharedlibrary" command to see the complete listing.

Do you need "set solib-search-path" or "set sysroot"?

I am parent

^C

Thread 1.1 received signal SIGINT, Interrupt.

[Switching to process 23462236]

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

(gdb)






________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 02 November 2022 14:26
To: simark@simark.ca <simark@simark.ca>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: Re: [PATCH] Enable multi process debugging for AIX

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

>>Is there any other way of figuring out what the parent process of
>>the new child is?  Maybe you should just do that when you get the
>>child event.
>
>I fully did not get the above suggestion " Maybe you should just do that when you get the
>child event ". Kindly let me know what you are trying to tell me in this context.

The last iteration of you patch simply has a list of "pending parent"
and "pending child" PIDs.  If you get a fork event for the child, and
there is *any* parent PID in the "pending" list, you automatically
assume that this is the parent of *this* child.

I'm concerned that this may not always be true.  For example, if you
are already debugging multiple processes, and *two* of those fork at
the same time, you'll be getting four ptrace events in GDB - two
events for the two parent processes, and two events for the two new
child processes.  If this happens, you'll need to verify which child
is actually associated with *which* parent.

As you note, on other platforms this relationship is reported as part
of the ptrace event directly, and this is apparently not the case on
AIX.  However, the OS of course still knows what the parent process
of each of the child processes is - so I guess there should be *some*
way to find this out from within GDB.  (E.g. in Linux you could get
at that information by reading some /proc files.)

Bye,
Ulrich


[-- Attachment #2: 0001-Enable-multi-process-debugging-for-AIX.patch --]
[-- Type: application/octet-stream, Size: 9396 bytes --]

From 8b7a0ee753f8bc84f872062c80126e6b04f57384 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Thu, 10 Nov 2022 03:55:56 -0600
Subject: [PATCH] Enable Multi process debugging for AIX.

---
 gdb/rs6000-aix-nat.c | 247 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 227 insertions(+), 20 deletions(-)

diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c
index cb141427696..36abf186ba0 100644
--- a/gdb/rs6000-aix-nat.c
+++ b/gdb/rs6000-aix-nat.c
@@ -54,6 +54,10 @@
 #include <sys/ldr.h>
 #include <sys/systemcfg.h>
 
+/* Header files for getting ppid in AIX of a child process.  */
+#include <procinfo.h>
+#include <sys/types.h>
+ 
 /* On AIX4.3+, sys/ldr.h provides different versions of struct ld_info for
    debugging 32-bit and 64-bit processes.  Define a typedef and macros for
    accessing fields in the appropriate structures.  */
@@ -91,10 +95,13 @@ class rs6000_nat_target final : public inf_ptrace_target
 
   ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
 
+  /* Fork detection related functions, For adding multi process debugging
+     support.  */
+  void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool) override;
+
 protected:
 
-  void post_startup_inferior (ptid_t ptid) override
-  { /* Nothing.  */ }
+  void post_startup_inferior (ptid_t ptid) override;
 
 private:
   enum target_xfer_status
@@ -107,6 +114,120 @@ class rs6000_nat_target final : public inf_ptrace_target
 
 static rs6000_nat_target the_rs6000_nat_target;
 
+/* The below declaration is to track number of times, parent has
+   reported fork event before its children.  */
+
+static std::list<pid_t> aix_pending_parent;
+
+/* The below declaration is for a child process event that
+   is reported before its corresponding parent process in
+   the event of a fork ().  */
+
+static std::list<pid_t> aix_pending_children;
+
+static void
+aix_remember_child (pid_t pid)
+{
+  aix_pending_children.push_front (pid);
+}
+
+static void
+aix_remember_parent (pid_t pid)
+{
+  aix_pending_parent.push_front (pid);
+}
+
+/* This function returns a parent of a child process.  */
+
+static pid_t
+find_my_aix_parent (pid_t child_pid)
+{
+  struct procsinfo ProcessBuffer1;
+  int ProcessSize1;
+  struct fdsinfo FileBuffer1;
+  int FileSize1;
+  pid_t IndexPointer1 = 0;
+  int Count = 1;
+  while (1)
+  {
+    if (getprocs (&ProcessBuffer1, sizeof (ProcessBuffer1),
+        &FileBuffer1, sizeof (FileBuffer1),  &IndexPointer1,
+        Count) != 1)
+      break; 
+     
+    if (child_pid == ProcessBuffer1.pi_pid)
+      return ProcessBuffer1.pi_ppid;
+  }
+  return 0;
+}
+
+/* In the below function we check if there was any child
+   process pending.  If it exists we return it from the
+   list, otherwise we return a null.  */
+
+static pid_t
+has_my_aix_child_reported (pid_t parent_pid)
+{
+  auto it = aix_pending_children.begin ();
+  pid_t child;
+  pid_t my_parent;
+  while (it != aix_pending_children.end ())
+  {
+    child = *it;
+    my_parent = find_my_aix_parent (child);
+    if (my_parent == parent_pid)
+    {
+      aix_pending_children.erase (it);
+      return child;
+    }
+    it++;
+  }
+  return 0;
+}
+
+/* In the below function we check if there was any parent 
+   process pending.  If it exists we return it from the
+   list, otherwise we return a null.  */
+
+static pid_t
+has_my_aix_parent_reported (pid_t child_pid)
+{
+  auto it = aix_pending_parent.begin ();
+  pid_t parent;
+  pid_t my_parent = find_my_aix_parent (child_pid);
+  while (it != aix_pending_parent.end ())
+  {
+    parent = *it;
+    if (my_parent == parent)
+    {
+      aix_pending_parent.erase (it);
+      return parent;
+    }
+    it++;
+  }
+  return 0;
+}
+
+/* This function checks if there was a fork () event.  */
+
+static bool
+did_aix_inferior_fork (int status)
+{
+  /* If multi-process debug mode is enabled, the status
+     location is set to W_SFWTED.  */
+
+  status = status & 0xff;
+
+  /* Eliminate the last few bits. If the W_SFWTED is set
+     which is equal to 0x7e, it is a fork event otherwise
+     it is not.  */
+
+  if (status != W_SFWTED)
+    return false;
+  else
+    return true;
+}
+
 /* Given REGNO, a gdb register number, return the corresponding
    number suitable for use as a ptrace() parameter.  Return -1 if
    there's no suitable mapping.  Also, set the int pointed to by
@@ -187,6 +308,47 @@ rs6000_ptrace64 (int req, int id, long long addr, int data, void *buf)
   return ret;
 }
 
+void rs6000_nat_target::post_startup_inferior (ptid_t ptid)
+{
+
+  /* In AIX to turn on multi process debugging in ptrace
+     PT_MULTI is the option to be passed,
+     with the process ID which can fork () and
+     the data parameter [fourth parameter] must be 1.  */
+
+  if (!ARCH64 ())
+    rs6000_ptrace32 (PT_MULTI, ptid.pid(), 0, 1, 0);
+  else
+    rs6000_ptrace64 (PT_MULTI, ptid.pid(), 0, 1, 0);
+}
+
+void
+rs6000_nat_target::follow_fork (inferior *child_inf, ptid_t child_ptid,
+                          target_waitkind fork_kind, bool follow_child,
+                                                      bool detach_fork)
+{
+
+  /* Once the fork event is detected the infrun.c code
+     calls the target_follow_fork to take care of
+     follow child and detach the child activity which is
+     done using the function below.  */
+
+  inf_ptrace_target::follow_fork (child_inf, child_ptid, fork_kind,
+                                        follow_child, detach_fork);
+
+  /* If we detach fork and follow child we do not want the child
+     process to geneate events that ptrace can trace.  Hence we
+     detach it.  */
+
+  if (detach_fork && !follow_child)
+  {
+    if (ARCH64 ())
+      rs6000_ptrace64 (PT_DETACH, child_ptid.pid (), 0, 0, 0);
+    else
+      rs6000_ptrace32 (PT_DETACH, child_ptid.pid (), 0, 0, 0);
+  }
+}
+
 /* Fetch register REGNO from the inferior.  */
 
 static void
@@ -501,10 +663,10 @@ ptid_t
 rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 			 target_wait_flags options)
 {
-  pid_t pid;
+  pid_t pid = -1;
   int status, save_errno;
 
-  do
+  while (1) 
     {
       set_sigint_trap ();
 
@@ -514,7 +676,7 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 	  save_errno = errno;
 	}
       while (pid == -1 && errno == EINTR);
-
+      
       clear_sigint_trap ();
 
       if (pid == -1)
@@ -530,22 +692,67 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
       /* Ignore terminated detached child processes.  */
       if (!WIFSTOPPED (status) && find_inferior_pid (this, pid) == nullptr)
 	pid = -1;
+      
+      if (pid != -1)
+      {
+        /* AIX has a couple of strange returns from wait().  */
+
+        /* stop after load" status.  */
+        if (status == 0x57c)
+          ourstatus->set_loaded ();
+        /* 0x7f is signal 0.  */
+        /* 0x17f and 0x137f in hexadecimal are status returned if
+           if we follow parent,
+           a switch is made to a child post parent execution
+           and child continues its execution [user switches to child and
+           presses continue].  */
+        else if (status == 0x7f || status == 0x17f || status == 0x137f)
+          ourstatus->set_spurious ();
+        /* Check for a fork () event.  */
+        else if (did_aix_inferior_fork (status))
+        {
+          /* Checking whether it is a parent or a child event.  */
+
+          /* If the event is a child we check if there was a parent
+             event recorded before.  If yes we got the parent child
+             relationship.  If not we push this child and wait for
+             the next fork () event.  */
+
+          if (find_inferior_pid (this, pid) == nullptr)
+            {
+              pid_t parent_pid = has_my_aix_parent_reported (pid);
+              if (parent_pid > 0)
+              {
+                ourstatus->set_forked (ptid_t (pid));
+                return ptid_t (parent_pid);
+              }
+              aix_remember_child (pid);
+            }
+
+            /* If the event is a parent we check if there was a child
+               event recorded before.  If yes we got the parent child
+               relationship.  If not we push this parent and wait for
+               the next fork () event.  */
+
+          else
+            {
+              pid_t child_pid = has_my_aix_child_reported (pid);
+              if (child_pid > 0)
+              {
+                ourstatus->set_forked (ptid_t (child_pid));
+                return ptid_t (pid);
+              }
+              aix_remember_parent (pid);
+            }   
+          continue;
+        }
+      /* A normal waitstatus.  Let the usual macros deal with it.  */
+      else
+        *ourstatus = host_status_to_waitstatus (status);
+      
+      return ptid_t (pid);
     }
-  while (pid == -1);
-
-  /* AIX has a couple of strange returns from wait().  */
-
-  /* stop after load" status.  */
-  if (status == 0x57c)
-    ourstatus->set_loaded ();
-  /* signal 0.  I have no idea why wait(2) returns with this status word.  */
-  else if (status == 0x7f)
-    ourstatus->set_spurious ();
-  /* A normal waitstatus.  Let the usual macros deal with it.  */
-  else
-    *ourstatus = host_status_to_waitstatus (status);
-
-  return ptid_t (pid);
+  }
 }
 \f
 
-- 
2.31.1

Re: [PATCH] Enable multi process debugging for AIX

[Ulrich Weigand]

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

>I have taken care of parent child relationship. See find_my_aix_parent () function. 
>You can refer the link https://www.ibm.com/docs/en/aix/7.2?topic=g-getprocs-subroutine
>for the documentation of the same. 
>
>I have also taken care of what if break points hit in other threads while only a 
>child or a parent event is reported in another thread as you mentioned Ulrich or
>any other event. [You can refer the output I have pasted below]. The while loop
>will take care. 

Thanks!  I don't see any further functional problems with this version any
more, but there are still a few options to simplify and improve performance.

See the inline comments below.

> +static pid_t
> +find_my_aix_parent (pid_t child_pid)
> +{
> +  struct procsinfo ProcessBuffer1;
> +  int ProcessSize1;
> +  struct fdsinfo FileBuffer1;
> +  int FileSize1;
> +  pid_t IndexPointer1 = 0;
> +  int Count = 1;
> +  while (1)
> +  {
> +    if (getprocs (&ProcessBuffer1, sizeof (ProcessBuffer1),
> +        &FileBuffer1, sizeof (FileBuffer1),  &IndexPointer1,
> +        Count) != 1)

If I understand the docs correctly, I think this can be
simplified: if you set IndexPointer1 initially to child_pid,
it should immediately return this process (if it exists),
so you shouldn't have to loop, right?

Also, as we don't use FileBuffer1, you can simply pass NULL.

> +static pid_t
> +has_my_aix_child_reported (pid_t parent_pid)
> +{
> +  auto it = aix_pending_children.begin ();
> +  pid_t child;
> +  pid_t my_parent;
> +  while (it != aix_pending_children.end ())
> +  {

I guess the loop can be handled with std::find_if ?

> +  while (it != aix_pending_parent.end ())

Here as well.

> +static bool
> +did_aix_inferior_fork (int status)
> +{
> +  /* If multi-process debug mode is enabled, the status
> +     location is set to W_SFWTED.  */
> +
> +  status = status & 0xff;
> +
> +  /* Eliminate the last few bits. If the W_SFWTED is set
> +     which is equal to 0x7e, it is a fork event otherwise
> +     it is not.  */
> +
> +  if (status != W_SFWTED)
> +    return false;
> +  else
> +    return true;

This whole function is now simply
  return (status & 0xff) == W_SFWTED;
(and probably doen't need to be a separate function ...)

> -  pid_t pid;
> +  pid_t pid = -1;

This doesn't seem necessary.

>    int status, save_errno;
>  
> -  do
> +  while (1) 
>      {
>        set_sigint_trap ();
>  
> @@ -514,7 +676,7 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>  	  save_errno = errno;
>  	}
>        while (pid == -1 && errno == EINTR);
> -
> +      

Remove this whitespace change.

> @@ -530,22 +692,67 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>        /* Ignore terminated detached child processes.  */
>        if (!WIFSTOPPED (status) && find_inferior_pid (this, pid) == nullptr)
>  	pid = -1;
> +      
> +      if (pid != -1)
> +      {

Probably simpler to change the above "pid = -1" line to "continue".
Then we'll never get here unless pid != -1 already and don't need
another if.

> +        /* AIX has a couple of strange returns from wait().  */
> +
> +        /* stop after load" status.  */
> +        if (status == 0x57c)
> +          ourstatus->set_loaded ();
> +        /* 0x7f is signal 0.  */
> +        /* 0x17f and 0x137f in hexadecimal are status returned if
> +           if we follow parent,
> +           a switch is made to a child post parent execution
> +           and child continues its execution [user switches to child and
> +           presses continue].  */
> +        else if (status == 0x7f || status == 0x17f || status == 0x137f)
> +          ourstatus->set_spurious ();

I think those could probably be left after the loop as they are.
Only the fork-event specific code needs to go in the loop.


Bye,
Ulrich

Re: [PATCH] Enable multi process debugging for AIX

[Aditya Kamath1]

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

Hi Ulrich,

I have made all the changes you mentioned for optimisation. Please find attached the patch. [See: 0001-Enable-multi-process-debugging-for-AIX.patch]

If all looks good kindly push this patch. AIX folks have been waiting for a while to use this feature. If anything, else needed from my end kindly let me know.

Have a nice day ahead.

Thanks and regards,
Aditya.
________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 14 November 2022 23:54
To: simark@simark.ca <simark@simark.ca>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: Re: [PATCH] Enable multi process debugging for AIX

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

>I have taken care of parent child relationship. See find_my_aix_parent () function.
>You can refer the link https://www.ibm.com/docs/en/aix/7.2?topic=g-getprocs-subroutine
>for the documentation of the same.
>
>I have also taken care of what if break points hit in other threads while only a
>child or a parent event is reported in another thread as you mentioned Ulrich or
>any other event. [You can refer the output I have pasted below]. The while loop
>will take care.

Thanks!  I don't see any further functional problems with this version any
more, but there are still a few options to simplify and improve performance.

See the inline comments below.

> +static pid_t
> +find_my_aix_parent (pid_t child_pid)
> +{
> +  struct procsinfo ProcessBuffer1;
> +  int ProcessSize1;
> +  struct fdsinfo FileBuffer1;
> +  int FileSize1;
> +  pid_t IndexPointer1 = 0;
> +  int Count = 1;
> +  while (1)
> +  {
> +    if (getprocs (&ProcessBuffer1, sizeof (ProcessBuffer1),
> +        &FileBuffer1, sizeof (FileBuffer1),  &IndexPointer1,
> +        Count) != 1)

If I understand the docs correctly, I think this can be
simplified: if you set IndexPointer1 initially to child_pid,
it should immediately return this process (if it exists),
so you shouldn't have to loop, right?

Also, as we don't use FileBuffer1, you can simply pass NULL.

> +static pid_t
> +has_my_aix_child_reported (pid_t parent_pid)
> +{
> +  auto it = aix_pending_children.begin ();
> +  pid_t child;
> +  pid_t my_parent;
> +  while (it != aix_pending_children.end ())
> +  {

I guess the loop can be handled with std::find_if ?

> +  while (it != aix_pending_parent.end ())

Here as well.

> +static bool
> +did_aix_inferior_fork (int status)
> +{
> +  /* If multi-process debug mode is enabled, the status
> +     location is set to W_SFWTED.  */
> +
> +  status = status & 0xff;
> +
> +  /* Eliminate the last few bits. If the W_SFWTED is set
> +     which is equal to 0x7e, it is a fork event otherwise
> +     it is not.  */
> +
> +  if (status != W_SFWTED)
> +    return false;
> +  else
> +    return true;

This whole function is now simply
  return (status & 0xff) == W_SFWTED;
(and probably doen't need to be a separate function ...)

> -  pid_t pid;
> +  pid_t pid = -1;

This doesn't seem necessary.

>    int status, save_errno;
>
> -  do
> +  while (1)
>      {
>        set_sigint_trap ();
>
> @@ -514,7 +676,7 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>          save_errno = errno;
>        }
>        while (pid == -1 && errno == EINTR);
> -
> +

Remove this whitespace change.

> @@ -530,22 +692,67 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>        /* Ignore terminated detached child processes.  */
>        if (!WIFSTOPPED (status) && find_inferior_pid (this, pid) == nullptr)
>        pid = -1;
> +
> +      if (pid != -1)
> +      {

Probably simpler to change the above "pid = -1" line to "continue".
Then we'll never get here unless pid != -1 already and don't need
another if.

> +        /* AIX has a couple of strange returns from wait().  */
> +
> +        /* stop after load" status.  */
> +        if (status == 0x57c)
> +          ourstatus->set_loaded ();
> +        /* 0x7f is signal 0.  */
> +        /* 0x17f and 0x137f in hexadecimal are status returned if
> +           if we follow parent,
> +           a switch is made to a child post parent execution
> +           and child continues its execution [user switches to child and
> +           presses continue].  */
> +        else if (status == 0x7f || status == 0x17f || status == 0x137f)
> +          ourstatus->set_spurious ();

I think those could probably be left after the loop as they are.
Only the fork-event specific code needs to go in the loop.


Bye,
Ulrich


[-- Attachment #2: 0001-Enable-multi-process-debugging-for-AIX.patch --]
[-- Type: application/octet-stream, Size: 8140 bytes --]

From 30cf237c83defbbdfe89c90cbc665e2590f86e87 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Tue, 15 Nov 2022 01:05:49 -0600
Subject: [PATCH] Enable multi process debugging for AIX

With this patch users with AIX operating system will be able to debug multiple process simulataneously be it a parent or a child process.
---
 gdb/rs6000-aix-nat.c | 191 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 183 insertions(+), 8 deletions(-)

diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c
index cb141427696..acac0dd8429 100644
--- a/gdb/rs6000-aix-nat.c
+++ b/gdb/rs6000-aix-nat.c
@@ -54,6 +54,10 @@
 #include <sys/ldr.h>
 #include <sys/systemcfg.h>
 
+/* Header files for getting ppid in AIX of a child process.  */
+#include <procinfo.h>
+#include <sys/types.h>
+ 
 /* On AIX4.3+, sys/ldr.h provides different versions of struct ld_info for
    debugging 32-bit and 64-bit processes.  Define a typedef and macros for
    accessing fields in the appropriate structures.  */
@@ -91,10 +95,13 @@ class rs6000_nat_target final : public inf_ptrace_target
 
   ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
 
+  /* Fork detection related functions, For adding multi process debugging
+     support.  */
+  void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool) override;
+
 protected:
 
-  void post_startup_inferior (ptid_t ptid) override
-  { /* Nothing.  */ }
+  void post_startup_inferior (ptid_t ptid) override;
 
 private:
   enum target_xfer_status
@@ -107,6 +114,86 @@ class rs6000_nat_target final : public inf_ptrace_target
 
 static rs6000_nat_target the_rs6000_nat_target;
 
+/* The below declaration is to track number of times, parent has
+   reported fork event before its children.  */
+
+static std::list<pid_t> aix_pending_parent;
+
+/* The below declaration is for a child process event that
+   is reported before its corresponding parent process in
+   the event of a fork ().  */
+
+static std::list<pid_t> aix_pending_children;
+
+static void
+aix_remember_child (pid_t pid)
+{
+  aix_pending_children.push_front (pid);
+}
+
+static void
+aix_remember_parent (pid_t pid)
+{
+  aix_pending_parent.push_front (pid);
+}
+
+/* This function returns a parent of a child process.  */
+
+static pid_t
+find_my_aix_parent (pid_t child_pid)
+{
+  struct procsinfo ProcessBuffer1;
+  pid_t IndexPointer1 = child_pid;
+  int Count = 1;
+  
+  if (getprocs (&ProcessBuffer1, sizeof (ProcessBuffer1),
+      NULL, 0, &IndexPointer1, Count) != 1)
+    return 0; 
+     
+  else   
+    return ProcessBuffer1.pi_ppid;
+  
+}
+
+/* In the below function we check if there was any child
+   process pending.  If it exists we return it from the
+   list, otherwise we return a null.  */
+
+static pid_t
+has_my_aix_child_reported (pid_t parent_pid)
+{
+  pid_t child = 0;
+  auto it = std::find_if (aix_pending_children.begin (),
+                          aix_pending_children.end (),
+                          find_my_aix_parent);
+  if (it != aix_pending_children.end ())
+  {
+    child = *it; 
+    aix_pending_children.erase (it);
+  }   
+  return child;
+}
+
+/* In the below function we check if there was any parent 
+   process pending.  If it exists we return it from the
+   list, otherwise we return a null.  */
+
+static pid_t
+has_my_aix_parent_reported (pid_t child_pid)
+{
+  pid_t my_parent = find_my_aix_parent (child_pid);
+  auto it = std::find (aix_pending_parent.begin (),
+                       aix_pending_parent.end (),
+                       my_parent);
+  if (it != aix_pending_parent.end ())
+  {
+    my_parent = *it;
+    aix_pending_parent.erase (it);
+    return my_parent;
+  }
+  return 0;
+}
+
 /* Given REGNO, a gdb register number, return the corresponding
    number suitable for use as a ptrace() parameter.  Return -1 if
    there's no suitable mapping.  Also, set the int pointed to by
@@ -187,6 +274,47 @@ rs6000_ptrace64 (int req, int id, long long addr, int data, void *buf)
   return ret;
 }
 
+void rs6000_nat_target::post_startup_inferior (ptid_t ptid)
+{
+
+  /* In AIX to turn on multi process debugging in ptrace
+     PT_MULTI is the option to be passed,
+     with the process ID which can fork () and
+     the data parameter [fourth parameter] must be 1.  */
+
+  if (!ARCH64 ())
+    rs6000_ptrace32 (PT_MULTI, ptid.pid(), 0, 1, 0);
+  else
+    rs6000_ptrace64 (PT_MULTI, ptid.pid(), 0, 1, 0);
+}
+
+void
+rs6000_nat_target::follow_fork (inferior *child_inf, ptid_t child_ptid,
+                          target_waitkind fork_kind, bool follow_child,
+                                                      bool detach_fork)
+{
+
+  /* Once the fork event is detected the infrun.c code
+     calls the target_follow_fork to take care of
+     follow child and detach the child activity which is
+     done using the function below.  */
+
+  inf_ptrace_target::follow_fork (child_inf, child_ptid, fork_kind,
+                                        follow_child, detach_fork);
+
+  /* If we detach fork and follow child we do not want the child
+     process to geneate events that ptrace can trace.  Hence we
+     detach it.  */
+
+  if (detach_fork && !follow_child)
+  {
+    if (ARCH64 ())
+      rs6000_ptrace64 (PT_DETACH, child_ptid.pid (), 0, 0, 0);
+    else
+      rs6000_ptrace32 (PT_DETACH, child_ptid.pid (), 0, 0, 0);
+  }
+}
+
 /* Fetch register REGNO from the inferior.  */
 
 static void
@@ -504,7 +632,7 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
   pid_t pid;
   int status, save_errno;
 
-  do
+  while (1) 
     {
       set_sigint_trap ();
 
@@ -529,17 +657,64 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 
       /* Ignore terminated detached child processes.  */
       if (!WIFSTOPPED (status) && find_inferior_pid (this, pid) == nullptr)
-	pid = -1;
+        continue;	
+      
+      if (pid != -1)
+      {
+        /* Check for a fork () event.  */
+        if ((status & 0xff) == W_SFWTED)
+        {
+          /* Checking whether it is a parent or a child event.  */
+
+          /* If the event is a child we check if there was a parent
+             event recorded before.  If yes we got the parent child
+             relationship.  If not we push this child and wait for
+             the next fork () event.  */
+
+          if (find_inferior_pid (this, pid) == nullptr)
+            {
+              pid_t parent_pid = has_my_aix_parent_reported (pid);
+              if (parent_pid > 0)
+              {
+                ourstatus->set_forked (ptid_t (pid));
+                return ptid_t (parent_pid);
+              }
+              aix_remember_child (pid);
+            }
+
+            /* If the event is a parent we check if there was a child
+               event recorded before.  If yes we got the parent child
+               relationship.  If not we push this parent and wait for
+               the next fork () event.  */
+
+          else
+            {
+              pid_t child_pid = has_my_aix_child_reported (pid);
+              if (child_pid > 0)
+              {
+                ourstatus->set_forked (ptid_t (child_pid));
+                return ptid_t (pid);
+              }
+              aix_remember_parent (pid);
+            }   
+          continue;
+        }
+      else
+        break;
     }
-  while (pid == -1);
-
+  }
   /* AIX has a couple of strange returns from wait().  */
 
   /* stop after load" status.  */
   if (status == 0x57c)
     ourstatus->set_loaded ();
-  /* signal 0.  I have no idea why wait(2) returns with this status word.  */
-  else if (status == 0x7f)
+  /* 0x7f is signal 0.  */
+  /* 0x17f and 0x137f in hexadecimal are status returned if
+     if we follow parent,
+     a switch is made to a child post parent execution
+     and child continues its execution [user switches to child and
+     presses continue].  */
+  else if (status == 0x7f || status == 0x17f || status == 0x137f)
     ourstatus->set_spurious ();
   /* A normal waitstatus.  Let the usual macros deal with it.  */
   else
-- 
2.31.1

Re: [PATCH] Enable multi process debugging for AIX

[Ulrich Weigand]

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

>I have made all the changes you mentioned for optimisation. Please find attached the patch.

Unfortunately it looks like this introduced a bug to has_my_aix_child_reported,
see below.  Otherwise, there's only a few coding style issues left, then this
is ready to merge.


>+static pid_t
>+find_my_aix_parent (pid_t child_pid)
>+{
>+  struct procsinfo ProcessBuffer1;
>+  pid_t IndexPointer1 = child_pid;
>+  int Count = 1;
>+  
>+  if (getprocs (&ProcessBuffer1, sizeof (ProcessBuffer1),
>+      NULL, 0, &IndexPointer1, Count) != 1)

You can just use  ... &child_pid, 1 ... here, then you don't
need the separate variables.

>+static pid_t
>+has_my_aix_child_reported (pid_t parent_pid)
>+{
>+  pid_t child = 0;
>+  auto it = std::find_if (aix_pending_children.begin (),
>+                          aix_pending_children.end (),
>+                          find_my_aix_parent);

This is now wrong, unfortunately.  This only checks that
"find_my_aix_parent" returns any non-null value, but it
actually needs to check that it returns "parent_pid".

This should be something along the lines of:
  auto it = std::find_if (aix_pending_children.begin (),
                          aix_pending_children.end (),
                          [=] (pid_t child_pid)
                          {
                            return find_my_aix_parent (child_pid) == parent_pid;
                          });

>+has_my_aix_parent_reported (pid_t child_pid)
>+{
>+  pid_t my_parent = find_my_aix_parent (child_pid);
>+  auto it = std::find (aix_pending_parent.begin (),
>+                       aix_pending_parent.end (),
>+                       my_parent);
>+  if (it != aix_pending_parent.end ())
>+  {
>+    my_parent = *it;

This line shouldn't be necessary.

>       /* Ignore terminated detached child processes.  */
>       if (!WIFSTOPPED (status) && find_inferior_pid (this, pid) == nullptr)
>-	pid = -1;
>+        continue;	

Here (and elsewhere) make sure to use a tab instead of 8 spaces.
Also, there should be no whitespace at the end of the line.

>+      if (pid != -1)
>+      {

This if is now no longer necessary.

>+      else
>+        break;
>     }
>-  while (pid == -1);
>-
>+  }

Whitespace (indentation) looks wrong.

>+  /* 0x7f is signal 0.  */
>+  /* 0x17f and 0x137f in hexadecimal are status returned if
>+     if we follow parent,
>+     a switch is made to a child post parent execution
>+     and child continues its execution [user switches to child and
>+     presses continue].  */

Use a single comment block and try to make the lines of about
equal length if possible.

Bye,
Ulrich

Re: [PATCH] Enable multi process debugging for AIX

[Aditya Kamath1]

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

Hi Ulrich,

Hi Ulrich,

I have made all the changes you mentioned for optimisation. Please find attached the patch. [See: 0001-Enable-multi-process-debugging-for-AIX.patch]. Kindly see my comments below so it becomes easy for you.

If all looks good kindly push this patch. If anything, else needed from my end kindly let me know.

Have a nice day ahead.

Thanks and regards,
Aditya.


------------------------
>You can just use  ... &child_pid, 1 ... here, then you don't
>need the separate variables

Done

>>+  auto it = std::find_if (aix_pending_children.begin (),
>>+                          aix_pending_children.end (),
>>+                          find_my_aix_parent);

>This is now wrong, unfortunately.  This only checks that
>"find_my_aix_parent" returns any non-null value, but it
>actually needs to check that it returns "parent_pid".

I apologise for the same. I missed this. thank you for pointing it out..

>>+    my_parent = *it;

>This line shouldn't be necessary.

Removed..

>>+        continue;

>Here (and elsewhere) make sure to use a tab instead of 8 spaces.
>Also, there should be no whitespace at the end of the line.

>>+      if (pid != -1)
>>+      {

>This if is now no longer necessary.

>>+      else
>>+        break;
>  >   }
>>-  while (pid == -1);
>>-
>>+  }

>Whitespace (indentation) looks wrong.

>>+  /* 0x7f is signal 0.  */
>>+  /* 0x17f and 0x137f in hexadecimal are status returned if
>>+     if we follow parent,
>>+     a switch is made to a child post parent execution
>>+     and child continues its execution [user switches to child and
>>+     presses continue].  */

>Use a single comment block and try to make the lines of about
>equal length if possible.


All of the above done..

________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 15 November 2022 16:23
To: simark@simark.ca <simark@simark.ca>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; Sanket Rathi <sanrathi@in.ibm.com>
Subject: Re: [PATCH] Enable multi process debugging for AIX

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

>I have made all the changes you mentioned for optimisation. Please find attached the patch.

Unfortunately it looks like this introduced a bug to has_my_aix_child_reported,
see below.  Otherwise, there's only a few coding style issues left, then this
is ready to merge.


>+static pid_t
>+find_my_aix_parent (pid_t child_pid)
>+{
>+  struct procsinfo ProcessBuffer1;
>+  pid_t IndexPointer1 = child_pid;
>+  int Count = 1;
>+
>+  if (getprocs (&ProcessBuffer1, sizeof (ProcessBuffer1),
>+      NULL, 0, &IndexPointer1, Count) != 1)

You can just use  ... &child_pid, 1 ... here, then you don't
need the separate variables.

>+static pid_t
>+has_my_aix_child_reported (pid_t parent_pid)
>+{
>+  pid_t child = 0;
>+  auto it = std::find_if (aix_pending_children.begin (),
>+                          aix_pending_children.end (),
>+                          find_my_aix_parent);

This is now wrong, unfortunately.  This only checks that
"find_my_aix_parent" returns any non-null value, but it
actually needs to check that it returns "parent_pid".

This should be something along the lines of:
  auto it = std::find_if (aix_pending_children.begin (),
                          aix_pending_children.end (),
                          [=] (pid_t child_pid)
                          {
                            return find_my_aix_parent (child_pid) == parent_pid;
                          });

>+has_my_aix_parent_reported (pid_t child_pid)
>+{
>+  pid_t my_parent = find_my_aix_parent (child_pid);
>+  auto it = std::find (aix_pending_parent.begin (),
>+                       aix_pending_parent.end (),
>+                       my_parent);
>+  if (it != aix_pending_parent.end ())
>+  {
>+    my_parent = *it;

This line shouldn't be necessary.

>       /* Ignore terminated detached child processes.  */
>       if (!WIFSTOPPED (status) && find_inferior_pid (this, pid) == nullptr)
>-      pid = -1;
>+        continue;

Here (and elsewhere) make sure to use a tab instead of 8 spaces.
Also, there should be no whitespace at the end of the line.

>+      if (pid != -1)
>+      {

This if is now no longer necessary.

>+      else
>+        break;
>     }
>-  while (pid == -1);
>-
>+  }

Whitespace (indentation) looks wrong.

>+  /* 0x7f is signal 0.  */
>+  /* 0x17f and 0x137f in hexadecimal are status returned if
>+     if we follow parent,
>+     a switch is made to a child post parent execution
>+     and child continues its execution [user switches to child and
>+     presses continue].  */

Use a single comment block and try to make the lines of about
equal length if possible.

Bye,
Ulrich


[-- Attachment #2: 0001-Enable-multi-process-debugging-for-AIX.patch --]
[-- Type: application/octet-stream, Size: 8100 bytes --]

From 98cdefd6fcb1a4b9ccaacb1e2ec9cffb9a9b1adc Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Tue, 15 Nov 2022 05:50:52 -0600
Subject: [PATCH] Enable multi process debugging for AIX

With this patch users with AIX operating system will be able to debug multiple process simulataneously be it a parent or a child process.
---
 gdb/rs6000-aix-nat.c | 187 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 179 insertions(+), 8 deletions(-)

diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c
index cb141427696..be006c34867 100644
--- a/gdb/rs6000-aix-nat.c
+++ b/gdb/rs6000-aix-nat.c
@@ -54,6 +54,10 @@
 #include <sys/ldr.h>
 #include <sys/systemcfg.h>
 
+/* Header files for getting ppid in AIX of a child process.  */
+#include <procinfo.h>
+#include <sys/types.h>
+ 
 /* On AIX4.3+, sys/ldr.h provides different versions of struct ld_info for
    debugging 32-bit and 64-bit processes.  Define a typedef and macros for
    accessing fields in the appropriate structures.  */
@@ -91,10 +95,13 @@ class rs6000_nat_target final : public inf_ptrace_target
 
   ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
 
+  /* Fork detection related functions, For adding multi process debugging
+     support.  */
+  void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool) override;
+
 protected:
 
-  void post_startup_inferior (ptid_t ptid) override
-  { /* Nothing.  */ }
+  void post_startup_inferior (ptid_t ptid) override;
 
 private:
   enum target_xfer_status
@@ -107,6 +114,86 @@ class rs6000_nat_target final : public inf_ptrace_target
 
 static rs6000_nat_target the_rs6000_nat_target;
 
+/* The below declaration is to track number of times, parent has
+   reported fork event before its children.  */
+
+static std::list<pid_t> aix_pending_parent;
+
+/* The below declaration is for a child process event that
+   is reported before its corresponding parent process in
+   the event of a fork ().  */
+
+static std::list<pid_t> aix_pending_children;
+
+static void
+aix_remember_child (pid_t pid)
+{
+  aix_pending_children.push_front (pid);
+}
+
+static void
+aix_remember_parent (pid_t pid)
+{
+  aix_pending_parent.push_front (pid);
+}
+
+/* This function returns a parent of a child process.  */
+
+static pid_t
+find_my_aix_parent (pid_t child_pid)
+{
+  struct procsinfo ProcessBuffer1;
+  
+  if (getprocs (&ProcessBuffer1, sizeof (ProcessBuffer1),
+      NULL, 0, &child_pid, 1) != 1)
+    return 0; 
+     
+  else   
+    return ProcessBuffer1.pi_ppid;
+  
+}
+
+/* In the below function we check if there was any child
+   process pending.  If it exists we return it from the
+   list, otherwise we return a null.  */
+
+static pid_t
+has_my_aix_child_reported (pid_t parent_pid)
+{
+  pid_t child = 0;
+  auto it = std::find_if (aix_pending_children.begin (),
+                          aix_pending_children.end (),
+                          [=] (pid_t child_pid)
+                          {
+                            return find_my_aix_parent (child_pid) == parent_pid;
+                          }); 
+  if (it != aix_pending_children.end ())
+  {
+    child = *it; 
+    aix_pending_children.erase (it);
+  }   
+  return child;
+}
+
+/* In the below function we check if there was any parent 
+   process pending.  If it exists we return it from the
+   list, otherwise we return a null.  */
+
+static pid_t
+has_my_aix_parent_reported (pid_t child_pid)
+{
+  pid_t my_parent = find_my_aix_parent (child_pid);
+  auto it = std::find (aix_pending_parent.begin (),
+                       aix_pending_parent.end (),
+                       my_parent);
+  if (it != aix_pending_parent.end ())
+  {
+    aix_pending_parent.erase (it);
+    return my_parent;
+  }
+  return 0;
+}
+
 /* Given REGNO, a gdb register number, return the corresponding
    number suitable for use as a ptrace() parameter.  Return -1 if
    there's no suitable mapping.  Also, set the int pointed to by
@@ -187,6 +274,47 @@ rs6000_ptrace64 (int req, int id, long long addr, int data, void *buf)
   return ret;
 }
 
+void rs6000_nat_target::post_startup_inferior (ptid_t ptid)
+{
+
+  /* In AIX to turn on multi process debugging in ptrace
+     PT_MULTI is the option to be passed,
+     with the process ID which can fork () and
+     the data parameter [fourth parameter] must be 1.  */
+
+  if (!ARCH64 ())
+    rs6000_ptrace32 (PT_MULTI, ptid.pid(), 0, 1, 0);
+  else
+    rs6000_ptrace64 (PT_MULTI, ptid.pid(), 0, 1, 0);
+}
+
+void
+rs6000_nat_target::follow_fork (inferior *child_inf, ptid_t child_ptid,
+                          target_waitkind fork_kind, bool follow_child,
+                                                      bool detach_fork)
+{
+
+  /* Once the fork event is detected the infrun.c code
+     calls the target_follow_fork to take care of
+     follow child and detach the child activity which is
+     done using the function below.  */
+
+  inf_ptrace_target::follow_fork (child_inf, child_ptid, fork_kind,
+                                        follow_child, detach_fork);
+
+  /* If we detach fork and follow child we do not want the child
+     process to geneate events that ptrace can trace.  Hence we
+     detach it.  */
+
+  if (detach_fork && !follow_child)
+  {
+    if (ARCH64 ())
+      rs6000_ptrace64 (PT_DETACH, child_ptid.pid (), 0, 0, 0);
+    else
+      rs6000_ptrace32 (PT_DETACH, child_ptid.pid (), 0, 0, 0);
+  }
+}
+
 /* Fetch register REGNO from the inferior.  */
 
 static void
@@ -504,7 +632,7 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
   pid_t pid;
   int status, save_errno;
 
-  do
+  while (1) 
     {
       set_sigint_trap ();
 
@@ -529,17 +657,60 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 
       /* Ignore terminated detached child processes.  */
       if (!WIFSTOPPED (status) && find_inferior_pid (this, pid) == nullptr)
-	pid = -1;
+	continue;	
+      
+      /* Check for a fork () event.  */
+      if ((status & 0xff) == W_SFWTED)
+      {
+        /* Checking whether it is a parent or a child event.  */
+
+        /* If the event is a child we check if there was a parent
+           event recorded before.  If yes we got the parent child
+           relationship.  If not we push this child and wait for
+           the next fork () event.  */
+
+        if (find_inferior_pid (this, pid) == nullptr)
+          {
+            pid_t parent_pid = has_my_aix_parent_reported (pid);
+            if (parent_pid > 0)
+            {
+              ourstatus->set_forked (ptid_t (pid));
+              return ptid_t (parent_pid);
+            }
+            aix_remember_child (pid);
+          }
+
+          /* If the event is a parent we check if there was a child
+             event recorded before.  If yes we got the parent child
+             relationship.  If not we push this parent and wait for
+             the next fork () event.  */
+
+        else
+          {
+            pid_t child_pid = has_my_aix_child_reported (pid);
+            if (child_pid > 0)
+            {
+              ourstatus->set_forked (ptid_t (child_pid));
+              return ptid_t (pid);
+            }
+            aix_remember_parent (pid);
+          }   
+        continue;
+      }
+      else
+        break;
     }
-  while (pid == -1);
-
   /* AIX has a couple of strange returns from wait().  */
 
   /* stop after load" status.  */
   if (status == 0x57c)
     ourstatus->set_loaded ();
-  /* signal 0.  I have no idea why wait(2) returns with this status word.  */
-  else if (status == 0x7f)
+  /* 0x7f is signal 0.  */
+  /* 0x17f and 0x137f in hexadecimal are status returned and */ 
+  /* then if we follow parent, a switch is made to a child */
+  /* post parent execution and child continues its execution */ 
+  /* [user switches to child and presses continue].  */
+  else if (status == 0x7f || status == 0x17f || status == 0x137f)
     ourstatus->set_spurious ();
   /* A normal waitstatus.  Let the usual macros deal with it.  */
   else
-- 
2.31.1

Re: [PATCH] Enable multi process debugging for AIX

[Ulrich Weigand]

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

>I have made all the changes you mentioned for optimisation. Please find attached the patch.

This is OK, thanks.  I've checked the patch in.

Note that there were still a few coding style issues
(indentation, comment style, tabs vs. spaces, white
space at the end of the line), which I've fixed
before committing - please have a look.

Bye,
Ulrich

Re: [PATCH] Enable multi process debugging for AIX

[Aditya Kamath1]

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

Hi,

Thank you for pushing the patch. Thank you, Ulrich and Simon, for all your guidance, knowledge sharing and patience throughout this feature addition for AIX.

>Note that there were still a few coding style issues
>(indentation, comment style, tabs vs. spaces, white
>space at the end of the line), which I've fixed
>before committing - please have a look.

Thanks for this. I have seen it. Will keep this in mind and implement in the future work for GDB.

Have a nice day ahead.

Thanks and regards,
Aditya.
________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 15 November 2022 18:13
To: simark@simark.ca <simark@simark.ca>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>; Sanket Rathi <sanrathi@in.ibm.com>
Subject: Re: [PATCH] Enable multi process debugging for AIX

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

>I have made all the changes you mentioned for optimisation. Please find attached the patch.

This is OK, thanks.  I've checked the patch in.

Note that there were still a few coding style issues
(indentation, comment style, tabs vs. spaces, white
space at the end of the line), which I've fixed
before committing - please have a look.

Bye,
Ulrich