Re: [PATCH] Enable multi process debugging for AIX

[Ulrich Weigand <Ulrich.Weigand@de.ibm.com>]

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