[RFC] Tighten and tweak ptrace argument checks

[RFC] Tighten and tweak ptrace argument checks

[Daniel Jacobowitz]

Hi Mark,

I discovered recently that your rewrite of the ptrace argument type
checks had busted some (not-yet-contributed, sorry) changes to use long
long for ptrace calls on MIPS n32.

This made me take a closer look at the tests.  It turns out that
neither the return type checks nor argument type checks work on
GNU/Linux.  The return type check will work with oldish versions of
GCC, but not with 4.0 (or 3.4, I think).  The argument type checks
won't work at all.  Here's the problem:

  extern long int ptrace (enum __ptrace_request, ...);

() won't match ...; this is important because of different calling
conventions on some platforms.

On i386-pc-linux-gnu the defaults work so this is merely an annoyance.
When the real type is long long instead of long, well, it wasn't
pretty...

So here's a proposed patch.  It handles the GNU/Linux case.  It handles
long long.  It also is violently fatal if it fails, instead of making
up something sensible - this is because I wasted several days trying to
figure out what was wrong with GDB when it was casting all the ptrace
arguments to the wrong type.  I'm sure it'll break the build in a lot
of places but I think it's worth fixing if you want to use autoconf for
this at all!

Any comments on the patch?

I also noticed considerable PTRACE_ARG3_TYPE vs PTRACE_TYPE_ARG3
confusion in the sources.  I think that the last time I looked at this,
I convinced myself that it was possible to get them out of sync, and
the results would be pretty gruesome.  I didn't touch that for now.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

2005-09-09  Daniel Jacobowitz  <dan@codesourcery.com>

	* configure.ac: Improve tests for the return type and argument
	list of ptrace.  Exit if the tests fail.
	* configure: Regenerated.

Index: configure.ac
===================================================================
RCS file: /big/fsf/rsync/src/src/gdb/configure.ac,v
retrieving revision 1.24
diff -u -p -r1.24 configure.ac
--- configure.ac	25 Jul 2005 15:08:40 -0000	1.24
+++ configure.ac	9 Sep 2005 21:20:01 -0000
@@ -486,12 +486,28 @@ AC_CHECK_DECLS(ptrace, [], [
   : ${gdb_cv_func_ptrace_ret='int'}
   : ${gdb_cv_func_ptrace_args='int,int,long,long'}
 ], $gdb_ptrace_headers)
-# Check return type.
-AC_CACHE_CHECK([return type of ptrace], gdb_cv_func_ptrace_ret,
-  AC_TRY_COMPILE($gdb_ptrace_headers,
-    [extern int ptrace ();],
-    gdb_cv_func_ptrace_ret='int',
-    gdb_cv_func_ptrace_ret='long'))
+# Check return type.  First try without an argument list.  But
+# any argument list containing stdargs is incompatible with a missing
+# argument list, and recent (e.g. 4.x) versions of gcc will reject
+# them.  So list every known stdargs argument list too.
+AC_CACHE_CHECK([return type of ptrace], gdb_cv_func_ptrace_ret, [
+for gdb_args in '' 'unsigned int, ...'; do
+  for gdb_ret in 'int' 'long' 'long long'; do
+    AC_TRY_COMPILE($gdb_ptrace_headers,
+      [extern $gdb_ret ptrace ($gdb_args);],
+      [gdb_cv_func_ptrace_ret='int'; break 2])
+  done
+done
+if test "${gdb_cv_func_ptrace_ret+set}" != set; then
+  AC_MSG_ERROR(unknown)
+fi
+])
+
+# Currently the only systems known to use stdargs expect four arguments.
+if test x"$gdb_args" = x'unsigned int, ...'; then
+  gdb_cv_func_ptrace_args="unsigned int,int,$gdb_ret,$gdb_ret"
+fi
+
 AC_DEFINE_UNQUOTED(PTRACE_TYPE_RET, $gdb_cv_func_ptrace_ret,
   [Define as the return type of ptrace.])
 # Check argument types.
@@ -517,8 +533,9 @@ gdb_cv_func_ptrace_args="$gdb_arg1,$gdb_
   done
  done
 done
-# Provide a safe default value.
-: ${gdb_cv_func_ptrace_args='int,int,long,long'}
+if test "${gdb_cv_func_ptrace_args+set}" != set; then
+  AC_MSG_ERROR(unknown)
+fi
 ])
 ac_save_IFS=$IFS; IFS=','
 set dummy `echo "$gdb_cv_func_ptrace_args" | sed 's/\*/\*/g'`

Re: [RFC] Tighten and tweak ptrace argument checks

[Mark Kettenis]

> Date: Fri, 9 Sep 2005 17:28:26 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> Hi Mark,
> 
> I discovered recently that your rewrite of the ptrace argument type
> checks had busted some (not-yet-contributed, sorry) changes to use long
> long for ptrace calls on MIPS n32.
> 
> This made me take a closer look at the tests.  It turns out that
> neither the return type checks nor argument type checks work on
> GNU/Linux.  The return type check will work with oldish versions of
> GCC, but not with 4.0 (or 3.4, I think).  The argument type checks
> won't work at all.  Here's the problem:
> 
>   extern long int ptrace (enum __ptrace_request, ...);
> 
> () won't match ...; this is important because of different calling
> conventions on some platforms.

It's always been my understanding that the the varargs stuff was used
because the number of arguments is actually dependent on the request.
It allows you to leave off any unused arguments instead of providing a
summy value like what's been done traditionally.  Bear in mind that
varargs functions only really work with word-sized arguments.
Otherwise you'll find yourself on a slippery slope very soon,
especially with system calls.

Anyway, the varargs stuff is a major nightmare, because different
versions of GCC treat the prototypes very differently.  That's why I
included a sane default.

> On i386-pc-linux-gnu the defaults work so this is merely an annoyance.
> When the real type is long long instead of long, well, it wasn't
> pretty...

But 'long long' as a return type or argument type really doesn't make
sense for ptrace(2).  But then of course MIPS doesn't make any sense
either, so that's perfectly fine ;-).

> So here's a proposed patch.  It handles the GNU/Linux case.  It handles
> long long.  It also is violently fatal if it fails, instead of making
> up something sensible - this is because I wasted several days trying to
> figure out what was wrong with GDB when it was casting all the ptrace
> arguments to the wrong type.  I'm sure it'll break the build in a lot
> of places but I think it's worth fixing if you want to use autoconf for
> this at all!

Sorry 'bout that, but you've created a ptrace variant that's
incompatible with everything else on the planet.  All other Linux
ports use long.  I presume you invented the 'long long' to be able to
return 64-bit register values with PTRACE_PEEKUSER.  That interface
should really die, and be replaced with PTRACE_GETREGS and friends.

> Any comments on the patch?

Sorry, yes, I'm almost certain this will break on OSF/1 or AIX or
HP-UX.  It'll probably break even for Linux if you're using an older
compiler.

I think it'd be better to explicitly check for the Linux
varargs-with-enum type prototype.  Give me a few days, and I'll try to
come up with something better.

> I also noticed considerable PTRACE_ARG3_TYPE vs PTRACE_TYPE_ARG3
> confusion in the sources.  I think that the last time I looked at this,
> I convinced myself that it was possible to get them out of sync, and
> the results would be pretty gruesome.  I didn't touch that for now.

This is because there are several ports that are basically
unmaintained; nobody feels responsible enough to keep them up to date.

Mark

Re: [RFC] Tighten and tweak ptrace argument checks

[Daniel Jacobowitz]

On Mon, Sep 12, 2005 at 11:47:56PM +0200, Mark Kettenis wrote:
> > On i386-pc-linux-gnu the defaults work so this is merely an annoyance.
> > When the real type is long long instead of long, well, it wasn't
> > pretty...
> 
> But 'long long' as a return type or argument type really doesn't make
> sense for ptrace(2).  But then of course MIPS doesn't make any sense
> either, so that's perfectly fine ;-).
> 
> > So here's a proposed patch.  It handles the GNU/Linux case.  It handles
> > long long.  It also is violently fatal if it fails, instead of making
> > up something sensible - this is because I wasted several days trying to
> > figure out what was wrong with GDB when it was casting all the ptrace
> > arguments to the wrong type.  I'm sure it'll break the build in a lot
> > of places but I think it's worth fixing if you want to use autoconf for
> > this at all!
> 
> Sorry 'bout that, but you've created a ptrace variant that's
> incompatible with everything else on the planet.  All other Linux
> ports use long.  I presume you invented the 'long long' to be able to
> return 64-bit register values with PTRACE_PEEKUSER.  That interface
> should really die, and be replaced with PTRACE_GETREGS and friends.

I "invented" the long long in order to:
  - be compatible with the n64 ptrace interface; the design of n32
    Linux is to be compatible with o32 wherever possible, be compatible
    with n64 where it isn't possible, and discourage n32-specific ABIs.
  - use PTRACE_PEEKDATA to read n64 data spaces, much as Richard is
    doing now for ppc64.  This is important on multiple-architecture
    installations.

_Constructive_ criticism is welcome.  I did this work a long time ago,
but never got it merged to either gdb or glibc (just the kernel bits).
So if you have an alternate suggestion, I don't feel too bad about
making an incompatible change to the kernel ABI here.  Ralf probably
won't object either.

PTRACE_PEEKUSER is not a big deal for this interface; you can return
32-bit chunks of the 64-bit registers.  It just requires an
n32-specific ABI for ptrace.

> > Any comments on the patch?
> 
> Sorry, yes, I'm almost certain this will break on OSF/1 or AIX or
> HP-UX.  It'll probably break even for Linux if you're using an older
> compiler.

Hmm, you're right - I got the logic backwards.

> > I also noticed considerable PTRACE_ARG3_TYPE vs PTRACE_TYPE_ARG3
> > confusion in the sources.  I think that the last time I looked at this,
> > I convinced myself that it was possible to get them out of sync, and
> > the results would be pretty gruesome.  I didn't touch that for now.
> 
> This is because there are several ports that are basically
> unmaintained; nobody feels responsible enough to keep them up to date.

I think that you bear some responsibility here, for introducing one
without getting rid of the other.  It's impossible to keep them
straight and there's no obvious difference between them.  This sort of
unfinished transition is exactly what I remember arguing against when
we last discussed "deprecated_*".

There's all of five definitions under config/, the default definition
in inferior.h, and a couple of uses.  I don't see any reason why any of
the existing definitions are necessary, so why not just remove them and
sed the uses?  If there's some particular platform from the bunch that
you think would be non-obvious, you could always ask specifically for
someone to try it.  I can't help with (and don't care about) lynx, but
I have access to all the others.

The above is for any value of "you".  I don't think you, Mark, have an
obligation to fix it at this date.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFC] Tighten and tweak ptrace argument checks

[Mark Kettenis]

> Date: Wed, 14 Sep 2005 10:43:03 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Mon, Sep 12, 2005 at 11:47:56PM +0200, Mark Kettenis wrote:
> > > On i386-pc-linux-gnu the defaults work so this is merely an annoyance.
> > > When the real type is long long instead of long, well, it wasn't
> > > pretty...
> > 
> > But 'long long' as a return type or argument type really doesn't make
> > sense for ptrace(2).  But then of course MIPS doesn't make any sense
> > either, so that's perfectly fine ;-).
> > 
> > > So here's a proposed patch.  It handles the GNU/Linux case.  It handles
> > > long long.  It also is violently fatal if it fails, instead of making
> > > up something sensible - this is because I wasted several days trying to
> > > figure out what was wrong with GDB when it was casting all the ptrace
> > > arguments to the wrong type.  I'm sure it'll break the build in a lot
> > > of places but I think it's worth fixing if you want to use autoconf for
> > > this at all!
> > 
> > Sorry 'bout that, but you've created a ptrace variant that's
> > incompatible with everything else on the planet.  All other Linux
> > ports use long.  I presume you invented the 'long long' to be able to
> > return 64-bit register values with PTRACE_PEEKUSER.  That interface
> > should really die, and be replaced with PTRACE_GETREGS and friends.
> 
> I "invented" the long long in order to:
>   - be compatible with the n64 ptrace interface; the design of n32
>     Linux is to be compatible with o32 wherever possible, be compatible
>     with n64 where it isn't possible, and discourage n32-specific ABIs.

But that means you now have an n32-specific API.  I'd say that's much
worse than an n32-specific ABI.  As a programmer I don't really care
about the ABI, as long as it is stable.

>   - use PTRACE_PEEKDATA to read n64 data spaces, much as Richard is
>     doing now for ppc64.  This is important on multiple-architecture
>     installations.

But this would work (almost) just as well if ptrace(2) would return a
`long' as you indicate below.

> _Constructive_ criticism is welcome.  I did this work a long time ago,
> but never got it merged to either gdb or glibc (just the kernel bits).
> So if you have an alternate suggestion, I don't feel too bad about
> making an incompatible change to the kernel ABI here.  Ralf probably
> won't object either.

I think it would be better to change things such that there is a
consistent API across all the MIPS ABI's.

> 
> PTRACE_PEEKUSER is not a big deal for this interface; you can return
> 32-bit chunks of the 64-bit registers.  It just requires an
> n32-specific ABI for ptrace.

The way I view ptrace, this would be the right API.  Think about the
PTRACE_PEEKXXXX (or PT_READ_X) requests as "return the memory at this
particular address in area XXXX as a word-sized chunk".  Then realize
that USER is nothing but a funky area that stores some interesting
information about the process, such as the registers.  On traditional
UNIX systems, the user area did contain quite a bit of interesting
stuff, but on Linux I think the registers are the only stuff left
there.  In fact the user area doesn't really exists anaymore and is
synthesised by ptrace, but that doesn't really change the way the
interface should be viewed.  Please take a look at
inf-ptrace.c:inf_ptrace_fetch_register(); the code to read
multiple-word sized registers is already there!

The actual code could still be almost identical to the n64 ptrace;
just return things in chucks based on the ABI's (n32 or n64) wordsize.

Now this of course sucks performance-wise, but than these interfaces
already suck performance-wise.  The BSD's have a nice PT_IO request,
that lets you transfer chunks of memory with a single request:

http://www.openbsd.org/cgi-bin/man.cgi?query=ptrace

it'd be nice if Linux has a similar thing.

> 
> > > Any comments on the patch?
> > 
> > Sorry, yes, I'm almost certain this will break on OSF/1 or AIX or
> > HP-UX.  It'll probably break even for Linux if you're using an older
> > compiler.
> 
> Hmm, you're right - I got the logic backwards.
> 
> > > I also noticed considerable PTRACE_ARG3_TYPE vs PTRACE_TYPE_ARG3
> > > confusion in the sources.  I think that the last time I looked at this,
> > > I convinced myself that it was possible to get them out of sync, and
> > > the results would be pretty gruesome.  I didn't touch that for now.
> > 
> > This is because there are several ports that are basically
> > unmaintained; nobody feels responsible enough to keep them up to date.
> 
> I think that you bear some responsibility here, for introducing one
> without getting rid of the other.  It's impossible to keep them
> straight and there's no obvious difference between them.  This sort of
> unfinished transition is exactly what I remember arguing against when
> we last discussed "deprecated_*".

Yup, I'm getting really frustrated with this.  We have a lot of ports
where no-one cares enough about the port to periodically go over the
code and replace the deprecated_ stuff.  I suspect the fact that with
the commercialization of Linux a lot of people are no longer allowed
to do "turd-shining" and their stupid short-term shareholder value
focussed bosses don't realize that a certain amount of turd-shining is
essential to keep the code they depend on maintainable on a longer
time-scale.

It's exactly this frustration that made me send my initial, rather
unconstructive reply; sorry 'bout that.

However, I really do think that an attempt to unify the ptrace API on
the different Linux platforms will actually help to remidy the
situation.  It'll allow us to share more code between them and
therefore less likely that they'll get out of sync.

> There's all of five definitions under config/, the default definition
> in inferior.h, and a couple of uses.  I don't see any reason why any of
> the existing definitions are necessary, so why not just remove them and
> sed the uses?  If there's some particular platform from the bunch that
> you think would be non-obvious, you could always ask specifically for
> someone to try it.  I can't help with (and don't care about) lynx, but
> I have access to all the others.
> 
> The above is for any value of "you".  I don't think you, Mark, have an
> obligation to fix it at this date.

I'll be happy to simply weed out the obsolete PTRACE_XXX defs from
config/ for the ports that I can't test.  Of course that means risking
potentially breaking those ports.  Thus far we have been rather
conservative about doing such things.  

We (the core GDB maintainers) should probably become a bit more active
in prodding people to clean up the ports they're looking after.  We
probably could hack up a patch that we think should work, and ask
people to test it for us.

So, yes, I think it'd be good if you could still change things such
that the n32 ptrace API would be identical to the o32 and n64 APIs.
If not, I think the patch below is a low-risk way to check for the
right prototype on n32 MIPS Linux.

Cheers,

Mark

Index: configure.ac
===================================================================
RCS file: /cvs/src/src/gdb/configure.ac,v
retrieving revision 1.24
diff -u -p -r1.24 configure.ac
--- configure.ac 25 Jul 2005 15:08:40 -0000 1.24
+++ configure.ac 18 Sep 2005 10:30:23 -0000
@@ -489,9 +489,14 @@ AC_CHECK_DECLS(ptrace, [], [
 # Check return type.
 AC_CACHE_CHECK([return type of ptrace], gdb_cv_func_ptrace_ret,
   AC_TRY_COMPILE($gdb_ptrace_headers,
+    [extern long long ptrace(enum __ptrace_request, ...);],
+    [gdb_cv_func_ptrace_ret='long long';
+     gdb_cv_func_ptrace_args=['enum __ptrace_request,...,long long,...']])
+  AC_TRY_COMPILE($gdb_ptrace_headers,
     [extern int ptrace ();],
-    gdb_cv_func_ptrace_ret='int',
-    gdb_cv_func_ptrace_ret='long'))
+    gdb_cv_func_ptrace_ret='int')
+  # Provide a safe default value.
+  : ${gdb_cv_func_ptrace_ret='long'})
 AC_DEFINE_UNQUOTED(PTRACE_TYPE_RET, $gdb_cv_func_ptrace_ret,
   [Define as the return type of ptrace.])
 # Check argument types.

Re: [RFC] Tighten and tweak ptrace argument checks

[Daniel Jacobowitz]

[Eliding most of this, sorry...]

On Sun, Sep 18, 2005 at 12:35:45PM +0200, Mark Kettenis wrote:
> I'll be happy to simply weed out the obsolete PTRACE_XXX defs from
> config/ for the ports that I can't test.  Of course that means risking
> potentially breaking those ports.  Thus far we have been rather
> conservative about doing such things.  

I'd rather do it than not...

> So, yes, I think it'd be good if you could still change things such
> that the n32 ptrace API would be identical to the o32 and n64 APIs.
> If not, I think the patch below is a low-risk way to check for the
> right prototype on n32 MIPS Linux.

I'm working on the API/ABI changes.

-- 
Daniel Jacobowitz
CodeSourcery, LLC