RE: [PATCH] Enable multi process debugging for AIX

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

Hi Simon, Ulrich and community,

I appreciate your patience to go through it and motivating us.

I have answered each of the issues point by point. Let me know your views on it and we shall then proceed towards the correct and optimal way of adding this to AIX.

Please see below my views, queries, reasons and justifications [in black and issues in red].

Have a nice day ahead.

Thanks and regards,
Aditya Kamath.

---------------------------------------------------------------
There are a lot of unclear things about how AIX reports a fork event to
the debugger. Is there some documentation on that somewhere?

Kindly use the link [ https://www.ibm.com/docs/en/aix/7.2?topic=p-ptrace-ptracex-ptrace64-subroutine ].. Here the PT_MULTI option has information about fork events.

----------------------------------------------------------

  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.


The generic code [infrun.c] calls the target_follow_fork() to set the follow fork and detach on fork modes. We need the functions to take care of what to do when we are in different follow or detach modes. If there is an alternate easy way let me know. Linux also uses the same.

----------------------------------------------------------

> + 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.

Why we need it?? AIX uses pthread debug library which has to be intialised first. On an event of wait due to a thread creation, once we fetch the pid using waitpid(), the following condition satisfies..


/* Check whether libpthdebug might be ready to be initialized.*/

  if (!pd_active && status->kind () == TARGET_WAITKIND_STOPPED

  && status->sig () == GDB_SIGNAL_TRAP)

 and we enter pd_activate() to. On a successful pthread debug library initialisation we have to initiate a thread debug session using pthdb_session_init(). While we do that, we have call backs to read symbol, data etc. The problem is here where we are not able to read them successfully due to incorrect process ID information. It is not able to fetch the right address to read in pdc_read_data() where we read target_read_memory() and pdc_symbol_addrs().

Kindly read https://www.ibm.com/docs/en/aix/7.1?topic=programming-developing-multithreaded-program-debuggers for more on what I shared.
Developing multithreaded program debuggers<https://www.ibm.com/docs/en/aix/7.1?topic=programming-developing-multithreaded-program-debuggers>
The pthread debug library (libpthdebug.a) provides a set of functions that allows developers to provide debug capabilities for applications that use the pthread library.
www.ibm.com

What we are thinking might be the solution??

Since we initialise the gdb with an observer to notify when there is a thread ptid change, on a wait event the inferior_ptid is set to null by switch_to_no_thread () with reinit_cache_frame (). Essentially with this we loose all our pid information needed to read our data and symbols as now our thread PTID is changed to null. So it is essential for us with the way AIX code is designed that we either reset our inferior_ptid in AIX thread wait code before we go out to the non-target dependent code like target.c, infrun.c to do it for us, or use

      thread_change_ptid (process_stratum_target *targ,

  ptid_t old_ptid, ptid_t new_ptid)


to inform GDB so that we can reset and get the right address for target_read_memory() in aix-thread.c file.


It will be great if you could tell us if we are thinking right at this moment.


-------------------------------------------------------------------------------------------------------------------

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?

I agree with you on this. Will make this change.

-----------------------------------------------------------------------------------------------------

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

Most likely we need to go with our own.. The only challenge being we might have multiple status code for set spurious which will be a challenge. Though let me know.. We can write our own.

----------------------------------------------------------------------------------------------------

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:

When PT_MULTI option is set with ptrace(), we are able to get the child process ID on a successful fork() event. I am not aware of the the parent pid handling. Since the parent and child are not simultaneously executing is the assumption on using current_inferior as only a parent will cause a wait even though I might be mistaken. We will try to figure that out, having said that if you have an idea from your experience or knowledge it will be helpful..

-------------------------------------------------------------------------------------------------


ptrace, ptracex, ptrace64 Subroutine<https://www.ibm.com/docs/en/aix/7.2?topic=p-ptrace-ptracex-ptrace64-subroutine>
For the 64-bit Process. Use ptracex where the debuggee is a 64-bit process and the operation requested uses the third (Address) parameter to reference the debuggee's address space or is sensitive to register size.Note that ptracex and ptrace64 will also support 32-bit debugees.. If returning or passing an int doesn't work for a 64-bit debuggee (for example, PT_READ_GPR), the buffer parameter ...
www.ibm.com




________________________________
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