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]

If all looks good kindly push this patch. AIX folks have been waiting for a while to use this feature. If anything, else needed from my end kindly let me know.

Have a nice day ahead.

Thanks and regards,
Aditya.
________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 14 November 2022 23:54
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:

>I have taken care of parent child relationship. See find_my_aix_parent () function.
>You can refer the link https://www.ibm.com/docs/en/aix/7.2?topic=g-getprocs-subroutine
>for the documentation of the same.
>
>I have also taken care of what if break points hit in other threads while only a
>child or a parent event is reported in another thread as you mentioned Ulrich or
>any other event. [You can refer the output I have pasted below]. The while loop
>will take care.

Thanks!  I don't see any further functional problems with this version any
more, but there are still a few options to simplify and improve performance.

See the inline comments below.

> +static pid_t
> +find_my_aix_parent (pid_t child_pid)
> +{
> +  struct procsinfo ProcessBuffer1;
> +  int ProcessSize1;
> +  struct fdsinfo FileBuffer1;
> +  int FileSize1;
> +  pid_t IndexPointer1 = 0;
> +  int Count = 1;
> +  while (1)
> +  {
> +    if (getprocs (&ProcessBuffer1, sizeof (ProcessBuffer1),
> +        &FileBuffer1, sizeof (FileBuffer1),  &IndexPointer1,
> +        Count) != 1)

If I understand the docs correctly, I think this can be
simplified: if you set IndexPointer1 initially to child_pid,
it should immediately return this process (if it exists),
so you shouldn't have to loop, right?

Also, as we don't use FileBuffer1, you can simply pass NULL.

> +static pid_t
> +has_my_aix_child_reported (pid_t parent_pid)
> +{
> +  auto it = aix_pending_children.begin ();
> +  pid_t child;
> +  pid_t my_parent;
> +  while (it != aix_pending_children.end ())
> +  {

I guess the loop can be handled with std::find_if ?

> +  while (it != aix_pending_parent.end ())

Here as well.

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

This whole function is now simply
  return (status & 0xff) == W_SFWTED;
(and probably doen't need to be a separate function ...)

> -  pid_t pid;
> +  pid_t pid = -1;

This doesn't seem necessary.

>    int status, save_errno;
>
> -  do
> +  while (1)
>      {
>        set_sigint_trap ();
>
> @@ -514,7 +676,7 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>          save_errno = errno;
>        }
>        while (pid == -1 && errno == EINTR);
> -
> +

Remove this whitespace change.

> @@ -530,22 +692,67 @@ 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;
> +
> +      if (pid != -1)
> +      {

Probably simpler to change the above "pid = -1" line to "continue".
Then we'll never get here unless pid != -1 already and don't need
another if.

> +        /* AIX has a couple of strange returns from wait().  */
> +
> +        /* stop after load" status.  */
> +        if (status == 0x57c)
> +          ourstatus->set_loaded ();
> +        /* 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 ();

I think those could probably be left after the loop as they are.
Only the fork-event specific code needs to go in the loop.


Bye,
Ulrich