[rfc/rft] [3/5] Untangle register_addr: CANNOT_FETCH/STORE_REGISTER

[rfc/rft] [3/5] Untangle register_addr: CANNOT_FETCH/STORE_REGISTER

[Ulrich Weigand]

Hello,

this patch removes the calls to CANNOT_FETCH_REGISTER / CANNOT_STORE_REGISTER
from within the inf_ptrace_trad_target routines.  This is enabled by two 
changes to the register_u_offset callback:
 - the routine is allowed to return (CORE_ADDR)-1 to indicate the 
   requested register cannot be accessed
 - the routine gains a new argument STORE_P that indicates whether
   the register is to be fetched or stored

There are three users of inf_ptrace_trad_target, vax-ultrix (which does
not have any CANNOT_FETCH_REGISTER / CANNOT_STORE_REGISTER anyway),
and alpha-linux and mips-linux via linux_trad_target.  For the latter,
this patch simply updates their register_u_offset routines to add
CANNOT_FETCH_REGISTER / CANNOT_STORE_REGISTER calls; this patch thus
should not cause any change in behaviour.  (Follow-on patches will
further clean up those two targets.)

Tested on i368-linux and powerpc-linux, and by verifying that alpha-linux,
mips-linux and vax-ultrix native targets still build.

Bye,
Ulrich


ChangeLog:

	* inf-ptrace.c (inf_ptrace_register_u_offset): Add STORE_P argument.
	(inf_ptrace_fetch_register): Update call.  Check for (CORE_ADDR)-1
	return value; replaces CANNOT_FETCH_REGISTER call.
	(inf_ptrace_store_register): Update call.  Check for (CORE_ADDR)-1
	return value; replaces CANNOT_STORE_REGISTER call.
	(inf_ptrace_trad_target): Update signature.
	* inf-ptrace.h (inf_ptrace_trad_target): Likewise.
	* linux-nat.c (linux_trad_target): Likewise.
	* linux-nat.h (linux_trad_target): Likewise.

	* alpha-linux-nat.c (alpha_linux_register_u_offset): Add STORE_P
	argument.  Call CANNOT_STORE_REGISTER / CANNOT_FETCH_REGISTER.
	* mips-linux-nat.c (mips_linux_register_u_offset): Likewise.
	* vax-nat.c (vax_register_u_addr): Add STORE_P argument.


diff -urNp gdb-orig/gdb/alpha-linux-nat.c gdb-head/gdb/alpha-linux-nat.c
--- gdb-orig/gdb/alpha-linux-nat.c	2007-04-14 00:46:45.843259816 +0200
+++ gdb-head/gdb/alpha-linux-nat.c	2007-04-14 00:46:32.987217952 +0200
@@ -24,8 +24,11 @@
 #include "gdbcore.h"
 
 static CORE_ADDR
-alpha_linux_register_u_offset (int regno)
+alpha_linux_register_u_offset (int regno, int store_p)
 {
+  if (store_p? CANNOT_STORE_REGISTER (regno) : CANNOT_FETCH_REGISTER (regno))
+    return (CORE_ADDR)-1;
+
   /* FIXME drow/2005-09-04: The hardcoded use of register_addr should go
      away.  This requires disentangling the various definitions of it
      (particularly alpha-nat.c's).  */
diff -urNp gdb-orig/gdb/inf-ptrace.c gdb-head/gdb/inf-ptrace.c
--- gdb-orig/gdb/inf-ptrace.c	2007-04-12 16:12:54.000000000 +0200
+++ gdb-head/gdb/inf-ptrace.c	2007-04-14 00:46:32.992217192 +0200
@@ -607,7 +607,7 @@ inf_ptrace_target (void)
 
 /* Pointer to a function that returns the offset within the user area
    where a particular register is stored.  */
-static CORE_ADDR (*inf_ptrace_register_u_offset)(int);
+static CORE_ADDR (*inf_ptrace_register_u_offset)(int, int);
 
 /* Fetch register REGNUM from the inferior.  */
 
@@ -619,7 +619,9 @@ inf_ptrace_fetch_register (int regnum)
   PTRACE_TYPE_RET *buf;
   int pid, i;
 
-  if (CANNOT_FETCH_REGISTER (regnum))
+  /* This isn't really an address, but ptrace thinks of it as one.  */
+  addr = inf_ptrace_register_u_offset (regnum, 0);
+  if (addr == (CORE_ADDR)-1)
     {
       regcache_raw_supply (current_regcache, regnum, NULL);
       return;
@@ -631,10 +633,7 @@ inf_ptrace_fetch_register (int regnum)
   if (pid == 0)
     pid = ptid_get_pid (inferior_ptid);
 
-  /* This isn't really an address, but ptrace thinks of it as one.  */
-  addr = inf_ptrace_register_u_offset (regnum);
   size = register_size (current_gdbarch, regnum);
-
   gdb_assert ((size % sizeof (PTRACE_TYPE_RET)) == 0);
   buf = alloca (size);
 
@@ -675,7 +674,9 @@ inf_ptrace_store_register (int regnum)
   PTRACE_TYPE_RET *buf;
   int pid, i;
 
-  if (CANNOT_STORE_REGISTER (regnum))
+  /* This isn't really an address, but ptrace thinks of it as one.  */
+  addr = inf_ptrace_register_u_offset (regnum, 1);
+  if (addr == (CORE_ADDR)-1)
     return;
 
   /* Cater for systems like GNU/Linux, that implement threads as
@@ -684,10 +685,7 @@ inf_ptrace_store_register (int regnum)
   if (pid == 0)
     pid = ptid_get_pid (inferior_ptid);
 
-  /* This isn't really an address, but ptrace thinks of it as one.  */
-  addr = inf_ptrace_register_u_offset (regnum);
   size = register_size (current_gdbarch, regnum);
-
   gdb_assert ((size % sizeof (PTRACE_TYPE_RET)) == 0);
   buf = alloca (size);
 
@@ -723,7 +721,7 @@ inf_ptrace_store_registers (int regnum)
    particular register is stored.  */
 
 struct target_ops *
-inf_ptrace_trad_target (CORE_ADDR (*register_u_offset)(int))
+inf_ptrace_trad_target (CORE_ADDR (*register_u_offset)(int, int))
 {
   struct target_ops *t = inf_ptrace_target();
 
diff -urNp gdb-orig/gdb/inf-ptrace.h gdb-head/gdb/inf-ptrace.h
--- gdb-orig/gdb/inf-ptrace.h	2007-04-12 16:12:54.000000000 +0200
+++ gdb-head/gdb/inf-ptrace.h	2007-04-14 00:46:32.996216584 +0200
@@ -32,6 +32,6 @@ extern struct target_ops *inf_ptrace_tar
    particular register is stored.  */
 
 extern struct target_ops *
-  inf_ptrace_trad_target (CORE_ADDR (*register_u_offset)(int));
+  inf_ptrace_trad_target (CORE_ADDR (*register_u_offset)(int, int));
 
 #endif
diff -urNp gdb-orig/gdb/linux-nat.c gdb-head/gdb/linux-nat.c
--- gdb-orig/gdb/linux-nat.c	2007-04-14 00:46:45.941244920 +0200
+++ gdb-head/gdb/linux-nat.c	2007-04-14 00:46:33.005215216 +0200
@@ -3196,7 +3196,7 @@ linux_target (void)
 }
 
 struct target_ops *
-linux_trad_target (CORE_ADDR (*register_u_offset)(int))
+linux_trad_target (CORE_ADDR (*register_u_offset)(int, int))
 {
   struct target_ops *t;
 
diff -urNp gdb-orig/gdb/linux-nat.h gdb-head/gdb/linux-nat.h
--- gdb-orig/gdb/linux-nat.h	2007-04-14 00:46:45.946244160 +0200
+++ gdb-head/gdb/linux-nat.h	2007-04-14 00:46:33.046208984 +0200
@@ -91,7 +91,7 @@ struct target_ops * linux_target (void);
 /* Create a generic GNU/Linux target using traditional 
    ptrace register access.  */
 struct target_ops *
-linux_trad_target (CORE_ADDR (*register_u_offset)(int));
+linux_trad_target (CORE_ADDR (*register_u_offset)(int, int));
 
 /* Register the customized GNU/Linux target.  This should be used
    instead of calling add_target directly.  */
diff -urNp gdb-orig/gdb/mips-linux-nat.c gdb-head/gdb/mips-linux-nat.c
--- gdb-orig/gdb/mips-linux-nat.c	2007-04-14 00:46:45.960242032 +0200
+++ gdb-head/gdb/mips-linux-nat.c	2007-04-14 00:46:33.062206552 +0200
@@ -251,8 +251,11 @@ mips64_linux_store_registers (int regnum
    REGNO.  */
 
 static CORE_ADDR
-mips_linux_register_u_offset (int regno)
+mips_linux_register_u_offset (int regno, int store_p)
 {
+  if (store_p? CANNOT_STORE_REGISTER (regno) : CANNOT_FETCH_REGISTER (regno))
+    return (CORE_ADDR)-1;
+
   /* FIXME drow/2005-09-04: The hardcoded use of register_addr should go
      away.  This requires disentangling the various definitions of it
      (particularly alpha-nat.c's).  */
diff -urNp gdb-orig/gdb/vax-nat.c gdb-head/gdb/vax-nat.c
--- gdb-orig/gdb/vax-nat.c	2007-04-13 20:40:05.608266416 +0200
+++ gdb-head/gdb/vax-nat.c	2007-04-14 00:46:33.066205944 +0200
@@ -69,7 +69,7 @@ vax_register_u_addr (CORE_ADDR u_ar0, in
 }
 
 static CORE_ADDR
-vax_register_u_offset (int regnum)
+vax_register_u_offset (int regnum, int store_p)
 {
   size_t u_ar0_offset = offsetof (struct user, u_ar0);
   CORE_ADDR u_ar0;
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc/rft] [3/5] Untangle register_addr: CANNOT_FETCH/STORE_REGISTER

[Mark Kettenis]

> Date: Sat, 14 Apr 2007 23:10:28 +0200 (CEST)
> From: "Ulrich Weigand" <uweigand@de.ibm.com>
> 
> Hello,
> 
> this patch removes the calls to CANNOT_FETCH_REGISTER / CANNOT_STORE_REGISTER
> from within the inf_ptrace_trad_target routines.  This is enabled by two 
> changes to the register_u_offset callback:
>  - the routine is allowed to return (CORE_ADDR)-1 to indicate the 
>    requested register cannot be accessed
>  - the routine gains a new argument STORE_P that indicates whether
>    the register is to be fetched or stored

So effectively, this turns CANNOT_FETCH_REGISTER/CANNOT_STORE_REGISTER
from being overloaded as both a native and target property into a
target-architecture-only property instead.  That's probably a good
idea.

However, I think you removing the calls from the calls from the
inf_ptrace_trad_target routines is wrong.  We should never fetch/store
the registers for which the target architecture says that they cannot
be fetched/stored.

If you leave them in, do any targets remain that have any registers
that can be fetched but cannot be stored by ptrace(2), that are not
defined as such by the target architecture.  I'd be surprised if there
were any such registers.

In that cas you wouldn't need the extra store_p argument for the
register_u_addr() callback.


> ChangeLog:
> 
> 	* inf-ptrace.c (inf_ptrace_register_u_offset): Add STORE_P argument.
> 	(inf_ptrace_fetch_register): Update call.  Check for (CORE_ADDR)-1
> 	return value; replaces CANNOT_FETCH_REGISTER call.
> 	(inf_ptrace_store_register): Update call.  Check for (CORE_ADDR)-1
> 	return value; replaces CANNOT_STORE_REGISTER call.
> 	(inf_ptrace_trad_target): Update signature.
> 	* inf-ptrace.h (inf_ptrace_trad_target): Likewise.
> 	* linux-nat.c (linux_trad_target): Likewise.
> 	* linux-nat.h (linux_trad_target): Likewise.
> 
> 	* alpha-linux-nat.c (alpha_linux_register_u_offset): Add STORE_P
> 	argument.  Call CANNOT_STORE_REGISTER / CANNOT_FETCH_REGISTER.
> 	* mips-linux-nat.c (mips_linux_register_u_offset): Likewise.
> 	* vax-nat.c (vax_register_u_addr): Add STORE_P argument.

Re: [rfc/rft] [3/5] Untangle register_addr: CANNOT_FETCH/STORE_REGISTER

[Ulrich Weigand]

Mark Kettenis wrote:

> So effectively, this turns CANNOT_FETCH_REGISTER/CANNOT_STORE_REGISTER
> from being overloaded as both a native and target property into a
> target-architecture-only property instead.  That's probably a good
> idea.

Yes, that's certainly a main point of this.  Having an NM file provide
an override definition of a gdbarch routine is quite broken and prevents
further multi-arch conversion -- fortunately the mips-linux definition
of CANNOT_FETCH_REGISTER/CANNOT_STORE_REGISTER is the last remaining
instance of this.

> However, I think you removing the calls from the calls from the
> inf_ptrace_trad_target routines is wrong.  We should never fetch/store
> the registers for which the target architecture says that they cannot
> be fetched/stored.

Hmmm.  The reason why I did it this way was that I found it odd
for a native target fetch_registers routine to have to rely on
a gdbarch routine to make that determination.

Say, suppose on a target some register cannot be fetched (or stored)
by ptrace due to some limitation, but that same register may very well
be accessible via a remote stub.  In that case, it is not really a
gdbarch property that the register isn't accessible, but should be
handled solely within the native target code.

Now, a native target that simply provides its own fetch_registers
/ store_registers *is* of course free to implement such rules on
its own.  My patch simply makes that same set of choices available
to native targets that avail themselves of the inf_trad_ptrace
helpers.


Longer term, I would even think that the gdbarch cannot_store_register
and cannot_fetch_register routines could be completely obsoleted --
the per-target aspects should be handled by each target itself,
and the remaining per-architecture aspects could be covered by the
reggroup mechanism (in particular fetch_reggroup / store_reggroup).

But maybe this (if it should be the right direction anyway) should
be left to another patch, and not intermixed with the other set
of changes ...

> If you leave them in, do any targets remain that have any registers
> that can be fetched but cannot be stored by ptrace(2), that are not
> defined as such by the target architecture.  I'd be surprised if there
> were any such registers.

Well, there is the problem on mips-linux, because cannot_fetch is
different from cannot_store there.

However, as an intermediate step I could install the mips-linux
functions as Linux-specific gdbarch callbacks instead of NM file
overrides.  That would affect cross-debugging too, though ...

Bye,
Ulrich


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com