RE: [PATCH] Enable multi process debugging for AIX

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

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