Re: [PATCH] Enable multi process debugging for AIX

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

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