Hi Ulrich,

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]. Kindly see my comments below so it becomes easy for you.

If all looks good kindly push this patch. If anything, else needed from my end kindly let me know.

Have a nice day ahead.

Thanks and regards,
Aditya.


------------------------
>You can just use  ... &child_pid, 1 ... here, then you don't
>need the separate variables

Done

>>+  auto it = std::find_if (aix_pending_children.begin (),
>>+                          aix_pending_children.end (),
>>+                          find_my_aix_parent);

>This is now wrong, unfortunately.  This only checks that
>"find_my_aix_parent" returns any non-null value, but it
>actually needs to check that it returns "parent_pid".

I apologise for the same. I missed this. thank you for pointing it out..

>>+    my_parent = *it;

>This line shouldn't be necessary.

Removed..

>>+        continue;

>Here (and elsewhere) make sure to use a tab instead of 8 spaces.
>Also, there should be no whitespace at the end of the line.

>>+      if (pid != -1)
>>+      {

>This if is now no longer necessary.

>>+      else
>>+        break;
>  >   }
>>-  while (pid == -1);
>>-
>>+  }

>Whitespace (indentation) looks wrong.

>>+  /* 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].  */

>Use a single comment block and try to make the lines of about
>equal length if possible.


All of the above done..

________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 15 November 2022 16:23
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>; Sanket Rathi <sanrathi@in.ibm.com>
Subject: Re: [PATCH] Enable multi process debugging for AIX

Aditya Kamath1 <Aditya.Kamath1@ibm.com> wrote:

>I have made all the changes you mentioned for optimisation. Please find attached the patch.

Unfortunately it looks like this introduced a bug to has_my_aix_child_reported,
see below.  Otherwise, there's only a few coding style issues left, then this
is ready to merge.


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

You can just use  ... &child_pid, 1 ... here, then you don't
need the separate variables.

>+static pid_t
>+has_my_aix_child_reported (pid_t parent_pid)
>+{
>+  pid_t child = 0;
>+  auto it = std::find_if (aix_pending_children.begin (),
>+                          aix_pending_children.end (),
>+                          find_my_aix_parent);

This is now wrong, unfortunately.  This only checks that
"find_my_aix_parent" returns any non-null value, but it
actually needs to check that it returns "parent_pid".

This should be something along the lines of:
  auto it = std::find_if (aix_pending_children.begin (),
                          aix_pending_children.end (),
                          [=] (pid_t child_pid)
                          {
                            return find_my_aix_parent (child_pid) == parent_pid;
                          });

>+has_my_aix_parent_reported (pid_t child_pid)
>+{
>+  pid_t my_parent = find_my_aix_parent (child_pid);
>+  auto it = std::find (aix_pending_parent.begin (),
>+                       aix_pending_parent.end (),
>+                       my_parent);
>+  if (it != aix_pending_parent.end ())
>+  {
>+    my_parent = *it;

This line shouldn't be necessary.

>       /* Ignore terminated detached child processes.  */
>       if (!WIFSTOPPED (status) && find_inferior_pid (this, pid) == nullptr)
>-      pid = -1;
>+        continue;

Here (and elsewhere) make sure to use a tab instead of 8 spaces.
Also, there should be no whitespace at the end of the line.

>+      if (pid != -1)
>+      {

This if is now no longer necessary.

>+      else
>+        break;
>     }
>-  while (pid == -1);
>-
>+  }

Whitespace (indentation) looks wrong.

>+  /* 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].  */

Use a single comment block and try to make the lines of about
equal length if possible.

Bye,
Ulrich