[rfc] [02/05] Get rid of current_gdbarch in hppa-linux-nat.c

[rfc] [02/05] Get rid of current_gdbarch in hppa-linux-nat.c

[Markus Deuling]

[-- Attachment #1: Type: text/plain, Size: 395 bytes --]

Hi,

this patch adds gdbarch as a parameter to hppa_linux_register_addr. 

Tested with gdb_mbuild. Ok to commit ?

ChangeLog:

	* hppa-linux-nat.c (hppa_linux_register_addr): Add gdbarch as parameter.
	Replace current_gdbarch by gdbarch.
	(fetch_register, store_register): Update caller of
	hppa_linux_register_addr.


-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com




[-- Attachment #2: diff-hppa-linux-nat --]
[-- Type: text/plain, Size: 1494 bytes --]

diff -urpN src/gdb/hppa-linux-nat.c dev/gdb/hppa-linux-nat.c
--- src/gdb/hppa-linux-nat.c	2007-10-08 14:50:56.000000000 +0200
+++ dev/gdb/hppa-linux-nat.c	2007-11-07 09:00:34.000000000 +0100
@@ -152,11 +152,12 @@ static const int u_offsets[] =
   };
 
 static CORE_ADDR
-hppa_linux_register_addr (int regno, CORE_ADDR blockend)
+hppa_linux_register_addr (struct gdbarch *gdbarch, int regno,
+			  CORE_ADDR blockend)
 {
   CORE_ADDR addr;
 
-  if ((unsigned) regno >= gdbarch_num_regs (current_gdbarch))
+  if ((unsigned) regno >= gdbarch_num_regs (gdbarch))
     error (_("Invalid register number %d."), regno);
 
   if (u_offsets[regno] == -1)
@@ -232,7 +233,8 @@ fetch_register (struct regcache *regcach
     tid = PIDGET (inferior_ptid); /* Not a threaded program.  */
 
   errno = 0;
-  val = ptrace (PTRACE_PEEKUSER, tid, hppa_linux_register_addr (regno, 0), 0);
+  val = ptrace (PTRACE_PEEKUSER, tid,
+		hppa_linux_register_addr (gdbarch, regno, 0), 0);
   if (errno != 0)
     error (_("Couldn't read register %s (#%d): %s."), 
 	   gdbarch_register_name (gdbarch, regno),
@@ -260,7 +262,8 @@ store_register (const struct regcache *r
 
   errno = 0;
   regcache_raw_collect (regcache, regno, &val);
-  ptrace (PTRACE_POKEUSER, tid, hppa_linux_register_addr (regno, 0), val);
+  ptrace (PTRACE_POKEUSER, tid,
+	  hppa_linux_register_addr (gdbarch, regno, 0), val);
   if (errno != 0)
     error (_("Couldn't write register %s (#%d): %s."),
 	   gdbarch_register_name (gdbarch, regno),

Re: [rfc] [02/05] Get rid of current_gdbarch in hppa-linux-nat.c

[Mark Kettenis]

> Date: Wed, 07 Nov 2007 12:11:02 +0100
> From: Markus Deuling <deuling@de.ibm.com>
> 
> Hi,
> 
> this patch adds gdbarch as a parameter to hppa_linux_register_addr. 
> 
> Tested with gdb_mbuild. Ok to commit ?

Actually, this is getting a bit silly.  That gdbarch is only needed
for a sanaity check, and obviously the gdbarch_num_regs call can just
be replaced with an appropriate bounds check on the u_offsets array.

   if (regno < 0 || regno >= ARRAY_SIZE(u_offsets))

should do the trick.

> ChangeLog:
> 
> 	* hppa-linux-nat.c (hppa_linux_register_addr): Add gdbarch as parameter.
> 	Replace current_gdbarch by gdbarch.
> 	(fetch_register, store_register): Update caller of
> 	hppa_linux_register_addr.

Re: [rfc] [02/05] Get rid of current_gdbarch in hppa-linux-nat.c

[Markus Deuling]

Mark Kettenis schrieb:
>> Date: Wed, 07 Nov 2007 12:11:02 +0100
>> From: Markus Deuling <deuling@de.ibm.com>
>>
>> Hi,
>>
>> this patch adds gdbarch as a parameter to hppa_linux_register_addr. 
>>
>> Tested with gdb_mbuild. Ok to commit ?
> 
> Actually, this is getting a bit silly.  That gdbarch is only needed
> for a sanaity check, and obviously the gdbarch_num_regs call can just
> be replaced with an appropriate bounds check on the u_offsets array.
> 
>    if (regno < 0 || regno >= ARRAY_SIZE(u_offsets))
> 
> should do the trick.

Sure it would. But what for do we have gdbarch_num_regs? I dont think its a good idea
to either use gdbarch_num_regs or ARRAY_SIZE(whatever) at will. This is redundant and error-prone.
Btw, there are two further uses of gdbarch_num_regs in that file.

For my opinion gdbarch should be used to describe an architecture.

 
-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com

Re: [rfc] [02/05] Get rid of current_gdbarch in hppa-linux-nat.c

[Daniel Jacobowitz]

On Wed, Nov 07, 2007 at 12:49:22PM +0100, Markus Deuling wrote:
> Sure it would. But what for do we have gdbarch_num_regs? I dont think its a 
> good idea
> to either use gdbarch_num_regs or ARRAY_SIZE(whatever) at will. This is 
> redundant and error-prone.
> Btw, there are two further uses of gdbarch_num_regs in that file.
> 
> For my opinion gdbarch should be used to describe an architecture.

That doesn't make any sense.  The error is there to make sure we don't
access memory outside the array.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] [02/05] Get rid of current_gdbarch in hppa-linux-nat.c

[Markus Deuling]

Daniel Jacobowitz schrieb:

> That doesn't make any sense.  The error is there to make sure we don't
> access memory outside the array.
> 

What this patch does is transform

  if ((unsigned) regno >= gdbarch_num_regs (current_gdbarch))
    error (_("Invalid register number %d."), regno);


into

  if ((unsigned) regno >= gdbarch_num_regs (gdbarch))
    error (_("Invalid register number %d."), regno);


The gdbarch comes from regcache in {fetch,store}_register. Why is that wrong ?


-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com

Re: [rfc] [02/05] Get rid of current_gdbarch in hppa-linux-nat.c

[Daniel Jacobowitz]

On Wed, Nov 07, 2007 at 03:50:33PM +0100, Markus Deuling wrote:
> What this patch does is transform
> 
>  if ((unsigned) regno >= gdbarch_num_regs (current_gdbarch))
>    error (_("Invalid register number %d."), regno);
> 
> 
> into
> 
>  if ((unsigned) regno >= gdbarch_num_regs (gdbarch))
>    error (_("Invalid register number %d."), regno);
> 
> 
> The gdbarch comes from regcache in {fetch,store}_register. Why is that wrong ?

Again, it is not necessary to add a gdbarch parameter here.  Why are
we checking regno?  We're checking it because we're about to use it to
index into the u_offsets array, and we don't want to go out of bounds.
-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] [02/05] Get rid of current_gdbarch in hppa-linux-nat.c

[Ulrich Weigand]

Markus Deuling wrote:
> Daniel Jacobowitz schrieb:
> > That doesn't make any sense.  The error is there to make sure we don't
> > access memory outside the array.
> 
> What this patch does is transform
> 
>   if ((unsigned) regno >= gdbarch_num_regs (current_gdbarch))
>     error (_("Invalid register number %d."), regno);
> 
> into
> 
>   if ((unsigned) regno >= gdbarch_num_regs (gdbarch))
>     error (_("Invalid register number %d."), regno);
> 
> The gdbarch comes from regcache in {fetch,store}_register. Why is that wrong ?

It is not that this is *wrong* (this is, the code will continue to
work correctly after this change), it just doesn't make any sense;
I fully agree with Mark and Dan on this.

Maybe it helps to think of how that code originally looked,
before all the multi-arch related changes.  A couple of years
ago, platform back-end header files used to define a number of
target macros, including NUM_REGS, to constants in their tm.h
header files.

These macros would be used throughout the debugger, both in
the back end and in common code; any particular GDB build
could only support one architecture variant.

This changed when the gdbarch mechanism was introduced; it
initially allowed back-end to support multiple variants of
an architecture at the same time.  To provide for some
backwards compatibility, the code would continue to use
macros like NUM_REGS, but those would now be defined by
the gdbarch header file to resolve to the proper values
dynamically, depending on the current architecture.

For uses of those macros in common code that supports many
different architectures, this flexibility was actually a
good thing.  However, for some back-end code that by its
fundamental design only supports one single architecture
variant anyway, this doesn't really make sense, it just
adds unnecessary complexity.  However, as the code still
used the old macros, this wasn't really very obvious.

Now, as we've completed the transition away from target
macros, and all code uses gdbarch calls directly, it has
become obvious that in some places, we should not have 
continued to use the generic macro, but gone back to
whatever constant is appropriate for that file.

To take the function your patch changes as example:

static CORE_ADDR
hppa_linux_register_addr (int regno, CORE_ADDR blockend)
{
  CORE_ADDR addr;

  if ((unsigned) regno >= gdbarch_num_regs (current_gdbarch))
    error (_("Invalid register number %d."), regno);

  if (u_offsets[regno] == -1)
    addr = 0;
  else
    {
      addr = (CORE_ADDR) u_offsets[regno];
    }

  return addr;
}

This function is was clearly written with the expectation
that the value of NUM_REGS (now replaced) is a constant;
in fact, the expectation is that this constant is identical
to the size of the u_offsets array.

If -in the current code- gdbarch_num_regs (current_gdbarch)
were to actually return anything *else* but that constant,
the code would simply break.  (Of course, it never does.)

Thus, the correct change to this file would be to go back
and replace the misleading "flexibility" of a gdbarch call
by the value that's actually needed here; this could be
written as ARRAY_SIZE (u_offsets).  (Even better might
be to move hppa32_num_regs as a macro to hppa-tdep.h,
and declare that array with specified size to begin with.)

Your patch goes into the completely opposite direction,
by adding a parameter to the routine to pass in a variable
gdbarch -- not only does this add unnecessary runtime
overhead, but even worse, it reinforces the impression
that there is really something variable here, when in
fact that value really still must be constant (128) here.

Bye,
Ulrich

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

Re: [rfc] [02/05] Get rid of current_gdbarch in hppa-linux-nat.c

[Mark Kettenis]

> Date: Wed, 07 Nov 2007 12:49:22 +0100
> From: Markus Deuling <deuling@de.ibm.com>
> CC: gdb-patches@sourceware.org, uweigand@de.ibm.com
> X-XS4ALL-DNSBL-Checked: mxdrop125.xs4all.nl checked 195.212.29.157 against DNS blacklists
> X-Virus-Scanned: by XS4ALL Virus Scanner
> X-XS4ALL-Spam-Score: -0.0 () SPF_PASS
> X-XS4ALL-Spam: NO
> X-CNFS-Analysis: v=1.0 c=1 a=oLMpFZsLkpsA:15 a=zG//nGEBJd+KC+fclfqWqQ==:17 a=qtUXupW5FscZJ7vd4pkA:9 a=BxgqAi6GPNXszpZsWqxtEzE25FcA:4 a=vNGxQsTWjH8A:10
> Envelope-To: mark.kettenis@xs4all.nl
> X-UIDL: 1194436282._smtp.mxdrop125.51362,S=3200
> 
> Mark Kettenis schrieb:
> >> Date: Wed, 07 Nov 2007 12:11:02 +0100
> >> From: Markus Deuling <deuling@de.ibm.com>
> >>
> >> Hi,
> >>
> >> this patch adds gdbarch as a parameter to hppa_linux_register_addr. 
> >>
> >> Tested with gdb_mbuild. Ok to commit ?
> > 
> > Actually, this is getting a bit silly.  That gdbarch is only needed
> > for a sanaity check, and obviously the gdbarch_num_regs call can just
> > be replaced with an appropriate bounds check on the u_offsets array.
> > 
> >    if (regno < 0 || regno >= ARRAY_SIZE(u_offsets))
> > 
> > should do the trick.
> 
> Sure it would. But what for do we have gdbarch_num_regs? I dont think its a good idea
> to either use gdbarch_num_regs or ARRAY_SIZE(whatever) at will. This is redundant and error-prone.
> Btw, there are two further uses of gdbarch_num_regs in that file.
> 
> For my opinion gdbarch should be used to describe an architecture.

Indeed, and the check in hppa_linux_register_addr is not checking the
architecture.  It's there to check whether that function is being
called for a register that's outside the range covered by u_offsets.
Using gdb_num_regs for that purpose is error-prone, since someone
might increase the number of registers for the hppa architecture, but
forget to update the u_offsets array.  The whole check should probably
be converted into a gdb_assert(), since this function should not be
called with a register number not covered by u_offsets in the first
place.

The other uses of gdbarch_num_regs() in this file are fine, since
there they are used as the number of registers defined by the
architecture.

Mark