Re: [PATCH] Enable multi process debugging for AIX

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

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