RE: [PATCH] Use current_inferior ()->pid for AIX

[Aditya Vidyadhar Kamath <ADITYA.VIDYADHAR.KAMATH@ibm.com>]

Any update on this?

Kindly let me know.

Thanks and regards,
Aditya.
________________________________
From: Gdb-patches <gdb-patches-bounces+aditya.vidyadhar.kamath=ibm.com@sourceware.org> on behalf of Aditya Vidyadhar Kamath via Gdb-patches <gdb-patches@sourceware.org>
Sent: Tuesday, April 12, 2022 7:02 PM
To: Simon Marchi <simon.marchi@polymtl.ca>; Joel Brobecker via Gdb-patches <gdb-patches@sourceware.org>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: [EXTERNAL] RE: [PATCH] Use current_inferior ()->pid for AIX

Hi,

A case by case analysis as suggested by you the last time has been done.

While the child process [the program to be debugged] is running inferior_ptid.pid() will not contain the child process ID. One has to use pid_t(current_inferior ()->pid) inorder to get the child process in AIX. GDB may allow you to execute many programmes at the same time. Numerous threads of execution can be initiated from multiple executables in each of multiple processes.

GDB uses an inferior object to represent the status of each programme run. An inferior usually correlates to a process, although it is more broad and may be used to targets without processes as well. Inferiors can be produced before a process runs and kept after it has completed. Inferiors have their own unique IDs, which are not the same as process ids. Each inferior may have several threads going through it. As a result, inferior ptid.pid may not need to contain the current inferior thread id. To obtain the inferior process ID, we must utilise current inferior()->pid in particular areas.

When the child process's pd_active variable is not set i.e. the process is not active one has to use the pid_t(current_inferior ()->pid) instead of inferior_ptid.pid() in AIX.

This can be demonstrated by the following code:


#include <stdio.h>

#include <stdlib.h>

int main()

{

   int i = 1;

   return 0;

}

Sample output without patch changes only in aix-thread.c at line 883 and not in rs6000-aix-nat.c:


inferior.c:303: internal-error: find_inferior_pid: Assertion `pid != 0' failed.

A problem internal to GDB has been detected,

further debugging may prove unreliable.

----- Backtrace -----

0x100e87543 ???

0x100e8770b ???

0x10003724b ???

0x100037697 ???

0x1000363f3 ???

0x1000593a3 ???

0x1000594ff ???

0x10053aa9b ???

0x1002f6e7b ???

0x1002f2ec3 ???

0x100b26d43 ???

0x100302a03 ???

0x10077e413 ???

0x10077b1a7 ???

0x100001dff ???

0x100002007 ???

0x10000421b ???

0x1000042ef ???

0x100000a9f ???

0x100000583 ???

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

inferior.c:303: internal-error: find_inferior_pid: Assertion `pid != 0' failed.


Sample output with patch changes in aix-thread.c and without patch changes in rs6000-aix-nat.c:


Child process unexpectedly missing: There are no child processes..

Program terminated with signal ?, Unknown signal.

The program no longer exists.

Output with patch:

(gdb) b main

Breakpoint 1 at 0x1000070c: file test.cc, line 3.

(gdb) r

Starting program: /home/gdb_tests/test

Breakpoint 1, main () at test.cc:3

3 int i = 1;


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

When there are multiple threads running in a debugging process, inferior_ptid will not have the exact current inferior thread. Instead we need to use ptid_t (current_inferior ()->pid) to get it in AIX.


This can be shown by the following program:


#include <stdio.h>

#include <unistd.h>

#include <stdlib.h>

#include <pthread.h>

#include <assert.h>

pthread_barrier_t barrier;

#define NUM_THREADS 2

void *

thread_function (void *arg)

{

pthread_barrier_wait (&barrier);

while (1); /* break here */

}

int

main (void)

{

int i;

alarm (300);

pthread_barrier_init (&barrier, NULL, NUM_THREADS);

for (i = 0; i < NUM_THREADS; i++)

{

pthread_t thread;

int res;

res = pthread_create (&thread, NULL, thread_function, NULL);

assert (res == 0);

}

while (1)

sleep (1);

return 0;

}



Sample output without patch changes in aix-thread.c at line 921 and 883 and not in rs6000-aix-nat.c:


inferior.c:303: internal-error: find_inferior_pid: Assertion `pid != 0' failed.

A problem internal to GDB has been detected,

further debugging may prove unreliable.

----- Backtrace -----

0x100e87543 ???

0x100e8770b ???

0x10003724b ???

0x100037697 ???

0x1000363f3 ???

0x1000593a3 ???

0x1000594ff ???

0x10053aa9b ???

0x1002f6e7b ???

0x1002f2ec3 ???

0x100b26d43 ???

0x100302a03 ???

0x10077e413 ???

0x10077b1a7 ???

0x100001dff ???

0x100002007 ???

0x10000421b ???

0x1000042ef ???

0x100000a9f ???

0x100000583 ???

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

inferior.c:303: internal-error: find_inferior_pid: Assertion `pid != 0' failed.


Output with patch:

(gdb) b main

Breakpoint 1 at 0x1000070c: file test.cc, line 3.

(gdb) r

Starting program: /home/gdb_tests/test

Breakpoint 1, main () at test.cc:3

3 int i = 1;



Summary of the gdb.base testsuites.



Without Patch
------------------------

# of expected passes

8096

# of unexpected failures

2160

# of unexpected successes

1

# of expected failures

4

# of known failures

5

# of unresolved testcases

113

# of untested testcases

83

# of unsupported tests

40

# of paths in test names

2

# of duplicate test names

13

With Patch
------------------------


# of expected passes

13822

# of unexpected failures

                    7399

# of unexpected successes

1

# of expected failures

11

# of known failures

6

# of unresolved testcases

78

# of untested testcases

88

# of unsupported tests

63

# of paths in test names

1

# of duplicate test names

2


(See attached file: 0001-Use-current_inferior-pid-for-AIX)


Thanks and regards,

Aditya.




________________________________
From: Simon Marchi <simon.marchi@polymtl.ca>
Sent: Tuesday, April 5, 2022 6:17 PM
To: Aditya Vidyadhar Kamath <ADITYA.VIDYADHAR.KAMATH@ibm.com>; Joel Brobecker via Gdb-patches <gdb-patches@sourceware.org>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: [EXTERNAL] Re: [PATCH] Use current_inferior ()->pid for AIX

Hi Aditya,

I don't think that using current_inferior throughout is a good solution
to your problems.  It will be case by case, unfortunately there's no
easy way out of this but to dig in GDB and understand its internals a
bit.

> From eb10e0ebc422d01f5ce96786f0bcf78a4f57d7ef Mon Sep 17 00:00:00 2001
> From: Aditya Vidyadhar Kamath
>  <adityakamath@li-ee942a4f-4ab2-47e9-8494-fb53eafbb9e1.ibm.com>
> Date: Wed, 30 Mar 2022 11:19:14 +0530
> Subject: [PATCH] Use current_inferior ()->pid for AIX.
>
> ---
>  gdb/aix-thread.c     | 15 ++++++++-------
>  gdb/rs6000-aix-nat.c |  8 ++++----
>  2 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
> index 85be4c15f1c..9331de3beb2 100644
> --- a/gdb/aix-thread.c
> +++ b/gdb/aix-thread.c
> @@ -708,7 +708,7 @@ get_signaled_thread (void)
>
>    while (1)
>      {
> -      if (getthrds (inferior_ptid.pid (), &thrinf,
> +      if (getthrds (current_inferior ()->pid, &thrinf,
>                    sizeof (thrinf), &ktid, 1) != 1)

This function doesn't seem to depend on the current inferior/thread, so
it could easily be changed to receive the pid as a parameter.

>        break;
>
> @@ -791,7 +791,7 @@ sync_threadlists (void)
>
>    /* Apply differences between the two arrays to GDB's thread list.  */
>
> -  infpid = inferior_ptid.pid ();
> +  infpid = current_inferior ()->pid;
>    for (pi = gi = 0; pi < pcount || gi < gcount;)
>      {
>        if (pi == pcount)
> @@ -883,11 +883,11 @@ pd_update (int set_infpid)
>    struct thread_info *thread = NULL;
>
>    if (!pd_active)
> -    return inferior_ptid;
> +    return ptid_t (current_inferior ()->pid);
>
>    status = pthdb_session_update (pd_session);
>    if (status != PTHDB_SUCCESS)
> -    return inferior_ptid;
> +    return ptid_t (current_inferior ()->pid);

Here, the pd_update function seems to want to return the event ptid
unmodified.  So, perhaps that ptid could be passed by the caller and
returned here.  Or, pd_update could be changed to return an
optional<ptid_t> and the caller deals with returning the right ptid.

>
>    sync_threadlists ();
>
> @@ -897,7 +897,7 @@ pd_update (int set_infpid)
>    if (tid != 0)
>      thread = iterate_over_threads (iter_tid, &tid);
>    if (!thread)
> -    ptid = inferior_ptid;
> +    ptid = ptid_t (current_inferior ()->pid);
>    else
>      {
>        ptid = thread->ptid;
> @@ -921,7 +921,7 @@ pd_activate (int set_infpid)
>                               &pd_session);
>    if (status != PTHDB_SUCCESS)
>      {
> -      return inferior_ptid;
> +      return ptid_t (current_inferior ()->pid);

This is the same idea as in pd_update.

>      }
>    pd_active = 1;
>    return pd_update (set_infpid);
> @@ -932,11 +932,12 @@ pd_activate (int set_infpid)
>  static void
>  pd_deactivate (void)
>  {
> +  ptid_t ptdrtn = ptid_t (current_inferior ()->pid)
>    if (!pd_active)
>      return;
>    pthdb_session_destroy (pd_session);
>
> -  pid_to_prc (&inferior_ptid);
> +  pid_to_prc (&ptdrtn);

Since pid_to_prc only writes to ptdrtn, it essentially a no-op.  I would
try just removing that call.  It was used to modify inferior_ptid after
the thread layer was disabled, which does not seem useful to me.

> diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c
> index 8563aea313a..0b8e4d2c687 100644
> --- a/gdb/rs6000-aix-nat.c
> +++ b/gdb/rs6000-aix-nat.c
> @@ -397,7 +397,7 @@ rs6000_nat_target::xfer_partial (enum target_object object,
>                                 ULONGEST offset, ULONGEST len,
>                                 ULONGEST *xfered_len)
>  {
> -  pid_t pid = inferior_ptid.pid ();
> +  pid_t pid = current_inferior ()->pid;
>    int arch64 = ARCH64 ();
>
>    switch (object)

Here, inferior_ptid should be valid, this change should not be
necessary.

> @@ -525,11 +525,11 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>
>          /* Claim it exited with unknown signal.  */
>          ourstatus->set_signalled (GDB_SIGNAL_UNKNOWN);
> -       return inferior_ptid;
> +       return ptid_t (current_inferior ()->pid);
>        }
>
>        /* Ignore terminated detached child processes.  */
> -      if (!WIFSTOPPED (status) && pid != inferior_ptid.pid ())
> +      if (!WIFSTOPPED (status) && pid != pid_t(current_inferior ()->pid))
>        pid = -1;
>      }
>    while (pid == -1);

The target_ops::wait method implemenattions should ne rely on the
current inferior either.  I don't think that this target supports
debugging multiple processes at the same time, but imagine that it does,
eventually.  When rs6000_nat_target::wait gets called, the current
inferior is one of the inferiors managed by that target, at random.  So
what the wait method needs to do is fetch an event from the OS, and
returning, regardless of which inferior it applies to (except if a ptid
argument is given).  So, you should never need to use either
inferior_ptid nor current_inferior in wait.

> @@ -658,7 +658,7 @@ rs6000_nat_target::xfer_shared_libraries
>    if (writebuf)
>      return TARGET_XFER_E_IO;
>
> -  gdb::byte_vector ldi_buf = rs6000_ptrace_ldinfo (inferior_ptid);
> +  gdb::byte_vector ldi_buf = rs6000_ptrace_ldinfo (ptid_t(current_inferior ()->pid));
>    result = rs6000_aix_ld_info_to_xml (target_gdbarch (), ldi_buf.data (),
>                                      readbuf, offset, len, 1);

This is used by xfer_partial, where inferior_ptid should be valid, so
this change shouldn't be necessary.

Simon