Re: [PATCH 3/5] powerpc64-aix inf-ptrace patch

[Mark Kettenis <mark.kettenis@xs4all.nl>]

> From: Raunaq 12 <raunaq12@in.ibm.com>
> Date: Tue, 23 Jul 2013 11:04:17 +0530
> 
> I have restricted the macro definitions to only gdb_ptrace.h.
> Something like this as seen below -
> 
> #ifdef PTRACE_TYPE_ARG5
> # ifdef BFD64

As Tom and I already told you, this #ifdef BFD64 is the wrong check.
You should add an autoconf test for ptrace64() and use #ifdef HAVE_PTRACE64.

> #  define ptrace(request, pid, addr, data) \
>           ptrace64 (request, (long long)pid, (long long)addr, data, 0)
> # else
> #  define ptrace(request, pid, addr, data) \
>           ptrace(request, (int)pid, (PTRACE_TYPE_ARG3)addr, data, 0)
> # endif
> #endif
> 
> I have type cast pid here to avoid additional compiler warnings.

Really?  What is the fundamental type of pid_t on AIX?  It's an int on
any sane UNIX system, so expect it to be an int on AIX as well.  So
casting to an int is pointless, and passing an int where a wider
signed integer type is expected should not produce a warning.

Redundant casts are bad.  They hide bugs.  Please drop them.

> > So unless I'm missing something, you should not need to make any
> > changes to inf-ptrace.c itself.
> 
> But, inf-ptrace.c makes ptrace calls as seen below -
> 
> 	ptrace (PT_DETACH, fpid, (PTRACE_TYPE_ARG3)1, 0)
> 
> If I make the macro definitions in gdb_ptrace.h as seen above, then
> the casts for the 'Address'(3rd parameter in ptrace call) should
> be removed from ptrace calls in inf-ptrace.c as seen below -
> 
> 	 ptrace (PT_DETACH, fpid, 1, 0)
> 
> The macro is taking care of all the type casts.
> ptrace64() accepts (long long) as the address parameter while
> ptrace() accepts int* or any integer pointer as the type for address.
> So, this type cast is necessary. Agree that pid cast is not needed.

No this will break other systems.  Instead, make sure PTRACE_TYPE_ARG3
gets defined to the proper type of the 3rd argument of ptrace64 when
you choose to use it.  I'd expect that if you add the autoconf check I
mentioned you could simply change the #ifdef PTRACE_TYPE_ARG5 in
gdb_ptrace.h to something like:

#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
#endif

and then everything should just work.