Re: [PATCH] Enable multi process debugging for AIX

[Simon Marchi <simark@simark.ca>]

> 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