Re: [PATCH v4 3/4] Remove obsolete getthrds and HAVE_PTRACE64 configure checks.

[Andrew Burgess <aburgess@redhat.com>]

Aditya Vidyadhar Kamath <akamath996@gmail.com> writes:

> From: Aditya Vidyadhar Kamath <aditya.kamath1@ibm.com>
>
> AIX 7.2 is the baseline supported AIX release.  On all AIX supported releases,
> getthrds is there <procinfo.h> declared, and ptrace64 is in <sys/ptrace.h>
> declared and  defined in libc.a.
>
> This patch removes the obsolete configure checks and cleans up.
>
> We are doing 2 things:
> 1:  Remove AC_CHECK_DECLS (getthrds) in configure.ac and config.in.
> 2:  Remove obsolete #ifdef HAVE_PTRACE64 in gdb/nat/gdb_ptrace.h
>     and use ptrace64 directly under PTRACE_TYPE_ARG5.

I ran this through an AI/LLV review agent which raised the following 3
points:

  Issue 1: addr_ptr is undefined after the patch
  
  The patch removes the #define addr_ptr macros (both the long long and int * variants, lines 370–376), but addr_ptr is still used in two places that are NOT modified by the patch:
  
      gdb/aix-thread.c:379: ptrace32 (int req, int id, addr_ptr addr, int data, int *buf)
      gdb/aix-thread.c:1120: ptrace32 (PTT_CONTINUE, tid[0], (addr_ptr) 1, ...)
  
  This will cause a build failure on AIX because addr_ptr is no longer defined as either long long or int *.

  Issue 2: 5-argument ptrace macro removed, but ptrace32() still calls ptrace() with 5 arguments
  
  The patch removes the 5-argument #define ptrace(request, pid, addr,
  data, buf) macro (lines 370–376). After this removal, the only ptrace
  macro in scope comes from gdb/nat/gdb_ptrace.h, which defines ptrace
  as a 4-argument macro:
  
  #define ptrace(request, pid, addr, data) \
       ptrace64 (request, pid, addr, data, 0)
  
  However, ptrace32() at line 383 still calls ptrace (req, id, addr,
  data, buf) with 5 arguments. This will cause a preprocessor error
  (wrong number of macro arguments) or at least a compilation error.

  Issue 3: rs6000-aix-nat.c still uses HAVE_PTRACE64
  
  rs6000-aix-nat.c uses #ifdef HAVE_PTRACE64 at lines 254 and 272. The
  patch removes HAVE_PTRACE64 from gdb/config.in but does NOT remove the
  AC_CHECK_FUNCS for ptrace64 from gdbsupport/common.m4:74. This means:
  
      If gdbsupport/common.m4 is not also updated, HAVE_PTRACE64 is still defined by the configure system, and the #undef removal from config.in is wrong (it will be regenerated with HAVE_PTRACE64 back).
      If the intent is to remove HAVE_PTRACE64 entirely, then rs6000-aix-nat.c must also be updated, and gdbsupport/common.m4 must have ptrace64 removed from the AC_CHECK_FUNCS list.
  
  Either way, the current patch is inconsistent.

For issue #3 I tried on an x86-64 GNU/Linux machine running autoheader
in the binutils-gdb/gdb/ directory, and indeed HAVE_PTRACE64 as added
back, so it does look like there is an inconsistency there.

To validate issues #1 and #2 I'd need to try building on an AIX machine,
which I assume is working for you, so maybe you can extend the commit
message to talk about these issues, e.g. "Remove the addr_ptr define,
GDB now picks up the definition from ....".  And similarly for the
ptrace macro: "GDB finds the 5 argument ptrace macro ....".

Though issues #1 and #2 might be obvious for someone familiar with AIX,
I do find the questions raised to be good ones, and even if the AI's
diagnosis of these as bugs is wrong, I think it would be worth
explaining these better in the commit message.


> ---
>  gdb/aix-thread.c     | 33 ++-------------------------------
>  gdb/config.in        |  7 -------
>  gdb/configure.ac     |  5 -----

If you change configure.ac then you need to regenerate the configure
file.  This, combined with issue #3 above, makes me thing you are hand
editing config.in, which is the wrong approach.  As the comment at the
top of that file says, config.in is auto-generated.

You'll need automake 1.15.1 and autoconf 2.69, then in the gdb/
directory just run `autoreconf`.

You probably should also do this in the gdbserver/ directory once issue
#3 is addressed.


>  gdb/nat/gdb_ptrace.h | 13 ++++---------
>  4 files changed, 6 insertions(+), 52 deletions(-)
>
> diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
> index 65b9c5f7479..95aa18985ed 100644
> --- a/gdb/aix-thread.c
> +++ b/gdb/aix-thread.c
> @@ -1,4 +1,4 @@
> -/* Low level interface for debugging AIX 4.3+ pthreads.
> +/* Low level interface for debugging AIX 7.2+ pthreads.
>  
>     Copyright (C) 1999-2026 Free Software Foundation, Inc.
>     Written by Nick Duffek <nsd@redhat.com>.
> @@ -19,7 +19,7 @@
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
>  
> -/* This module uses the libpthdebug.a library provided by AIX 4.3+ for
> +/* This module uses the libpthdebug.a library provided by AIX 7.2+ for
>     debugging pthread applications.
>  
>     Some name prefix conventions:
> @@ -56,18 +56,9 @@
>  #include <sys/pthdebug.h>
>  #include <unordered_set>
>  
> -#if !HAVE_DECL_GETTHRDS
> -extern int getthrds (pid_t, struct thrdsinfo64 *, int, tid_t *, int);
> -#endif
> -
>  /* Whether to emit debugging output.  */
>  static bool debug_aix_thread;
>  
> -/* In AIX 5.1, functions use pthdb_tid_t instead of tid_t.  */
> -#ifndef PTHDB_VERSION_3
> -#define pthdb_tid_t	tid_t
> -#endif
> -
>  /* Success and failure values returned by pthdb callbacks.  */
>  
>  #define PDC_SUCCESS	PTHDB_SUCCESS
> @@ -315,15 +306,6 @@ ptrace_check (int req, int id, int ret)
>    return 0;  /* Not reached.  */
>  }
>  
> -/* Call ptracex (REQ, ID, ADDR, DATA, BUF) or
> -   ptrace64 (REQ, ID, ADDR, DATA, BUF) if HAVE_PTRACE64.
> -   Return success.  */
> -
> -#ifdef HAVE_PTRACE64
> -# define ptracex(request, pid, addr, data, buf) \
> -	 ptrace64 (request, pid, addr, data, buf)
> -#endif
> -
>  static int
>  ptrace64aix (int req, int id, long long addr, int data, int *buf)
>  {
> @@ -331,17 +313,6 @@ ptrace64aix (int req, int id, long long addr, int data, int *buf)
>    return ptrace_check (req, id, ptracex (req, id, addr, data, buf));
>  }
>  
> -/* Call ptrace (REQ, ID, ADDR, DATA, BUF) or
> -   ptrace64 (REQ, ID, ADDR, DATA, BUF) if HAVE_PTRACE64.
> -   Return success.  */
> -
> -#ifdef HAVE_PTRACE64
> -# define ptrace(request, pid, addr, data, buf) \
> -	 ptrace64 (request, pid, addr, data, buf)
> -# define addr_ptr long long
> -#else
> -# define addr_ptr int *
> -#endif
>  
>  static int
>  ptrace32 (int req, int id, addr_ptr addr, int data, int *buf)

The change above leaves 2 blank lines between the end of ptrace64aix and
ptrace32, just a single blank line is the normal GDB style.

> diff --git a/gdb/config.in b/gdb/config.in
> index 0e5a6637c2b..dab7a3e7239 100644
> --- a/gdb/config.in
> +++ b/gdb/config.in
> @@ -147,10 +147,6 @@
>  /* Define to 1 if you have the declaration of `ffs', and to 0 if you don't. */
>  #undef HAVE_DECL_FFS
>  
> -/* Define to 1 if you have the declaration of `getthrds', and to 0 if you
> -   don't. */
> -#undef HAVE_DECL_GETTHRDS
> -
>  /* Define to 1 if you have the declaration of `snprintf', and to 0 if you
>     don't. */
>  #undef HAVE_DECL_SNPRINTF
> @@ -397,9 +393,6 @@
>  /* Define to 1 if you have the `pthread_sigmask' function. */
>  #undef HAVE_PTHREAD_SIGMASK
>  
> -/* Define to 1 if you have the `ptrace64' function. */
> -#undef HAVE_PTRACE64
> -
>  /* Define if sys/ptrace.h defines the PTRACE_GETFPXREGS request. */
>  #undef HAVE_PTRACE_GETFPXREGS
>  
> diff --git a/gdb/configure.ac b/gdb/configure.ac
> index 0d88a280b16..684ce8e21fb 100644
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -1915,11 +1915,6 @@ if test "${build}" = "${host}" -a "${host}" = "${target}" ; then
>  	 CONFIG_SRCS="${CONFIG_SRCS} aix-thread.c"
>  	 CONFIG_OBS="${CONFIG_OBS} aix-thread.o"
>  	 LIBS="$LIBS -lpthdebug"
> -
> -	 # Older versions of AIX do not provide the declaration for
> -	 # the getthrds function (it appears that it was introduced
> -	 # with AIX 6.x).
> -	 AC_CHECK_DECLS(getthrds, [], [], [[#include <procinfo.h>]])
>        fi
>        ;;
>     esac
> diff --git a/gdb/nat/gdb_ptrace.h b/gdb/nat/gdb_ptrace.h
> index e0607164260..0d06cb8eb55 100644
> --- a/gdb/nat/gdb_ptrace.h
> +++ b/gdb/nat/gdb_ptrace.h
> @@ -127,15 +127,10 @@
>     zero.  */
>  
>  #ifdef PTRACE_TYPE_ARG5
> -# ifdef HAVE_PTRACE64
> -#  define ptrace(request, pid, addr, data) \
> -	  ptrace64 (request, pid, addr, data, 0)
> -#  undef PTRACE_TYPE_ARG3
> -#  define PTRACE_TYPE_ARG3 long long
> -# else
> -#  define ptrace(request, pid, addr, data) \
> -	  ptrace (request, pid, addr, data, 0)
> -# endif
> +# define ptrace(request, pid, addr, data) \
> +	 ptrace64 (request, pid, addr, data, 0)
> +# undef PTRACE_TYPE_ARG3
> +# define PTRACE_TYPE_ARG3 long long
>  #else
>  /* Wrapper that avoids adding a pointless cast to all callers.  */
>  # define ptrace(request, pid, addr, data) \

The comment block right about this talks about AIX and HP-UX, but
support for GDB was removed some time ago now.  As this is a cleanup
commit, it would be nice to update this comment to remove the HP-UX
references.


Thanks,
Andrew