This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]