Re: [PATCH] Enable multi process debugging for AIX

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

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