[rfc] [00/14] Push REGCACHE into target_fetch/store_registers

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[rfc] [00/14] Push REGCACHE into target_fetch/store_registers

Ulrich Weigand
Hello,

one problem with using multiple gdbarch's at the same time is the notion
of "swapped" gdbarch data.  It really isn't safe to use any other gdbarch
but current_gdbarch as long as we still have those.

One particular important piece of such data is the global "current_regcache".
The following set of patches takes one step towards eliminating that global,
which would also make real per-thread register caches possible at some time.

Specifically, the patch adds a REGCACHE argument to the to_fetch_registers
and to_store_registers target operations, and uses that to eliminate use of
current_regcache in all implementation of those operations.

As the core file target implements to_fetch_registers, this means that the
core_read_registers core_fns callback also needs a REGCACHE parameter.

As core-regset.o implements that callback, this means the supply_gregset
etc. family of functions also needs a REGCACHE parameter.  (This is
probably a good idea anyway.)

In short, the patch set changes the following arguments:

extern void supply_gregset (struct regcache *regcache,
                            const gdb_gregset_t *gregs);
extern void supply_fpregset (struct regcache *regcache,
                             const gdb_fpregset_t *fpregs);

extern void fill_gregset (const struct regcache *regcache,
                          gdb_gregset_t *gregs, int regno);
extern void fill_fpregset (const struct regcache *regcache,
                           gdb_fpregset_t *fpregs, int regno);

extern void supply_fpxregset (struct regcache *regcache,
                              const gdb_fpxregset_t *fpxregs);
extern void fill_fpxregset (const struct regcache *regcache,
                            gdb_fpxregset_t *fpxregs, int regno);

    void (*core_read_registers) (struct regcache *regcache,
                                 char *core_reg_sect,
                                 unsigned core_reg_size,
                                 int which, CORE_ADDR reg_addr);

    void (*to_fetch_registers) (struct regcache *, int);
    void (*to_store_registers) (struct regcache *, int);


Note that I've attempted to make the supply_gregset etc. routines
const-correct.  This did expose two problems in current code:
the m32r-linux-nat.c supply_gregset routine actually wrote into
the register set it received, and the irix5-nat.c fill_ routines
used regcache_raw_read (which may change the regcache) instead
of regcache_raw_collect.  The patch set fixes both issues.


I've also tried to make the first argument to to_store_registers
const, but that would cause difficulties for various targets:

  Several monitor targets use _read routines instead of
  _collect.  That's probably just a bug.

  hppa-hpux and sparc use regcache_raw_read to read special
  register values ("flags" and sp, respectively) that are
  required to perform the store.  It is not clear that this
  can be safely replaced by regcache_raw_collect; what if the
  register was not previously read?

  sol-thread.c deliberately saves/restores regcache contents
  for some reason ...

  i386gnu-nat.c actually deliberately supplies a new register
  value during to_store_registers in some cases.  No idea
  what this is all about ...

Since these issues are somewhat non-obvious to fix, I'd suggest
to not bother making the argument const at this stage.


The patch set implementing the above change consists of the
following patches:

Fix the const-correctness problems:
 1. irix5-nat.c
 2. m32r-linux-nat.c

Prepare some (groups of) targets by pushing REGCACHE into
internal helper routines:
 3. aix-thread.c
 4. alpha targets
 5. arm targets
 6. cris target
 7. i387 targets
 8. mips targets
 9. sh targets
10. nto targets
11. monitor targets

Convert supply_gregset etc. routines
12. (one patch)

Convert core_fn core_read_registers callback
13. (one patch)

Actually convert to_store_registers / to_fetch_registers
14. (one patch)


Now, the testing issue :-)  This patch set touches just about every single
native target, and of course I cannot test even a significant fraction of
those.  I did full testing on:
  s390-ibm-linux
  s390x-ibm-linux
  i686-unknown-linux
  ia64-unknown-linux
  powerpc-ibm-aix5.3.0.0
  powerpc64-unknown-linux
  spu-elf


In addition, I've compiled every file touched by the patch set.  Of course,
this results in compiler errors due to missing native header files.  However,
I've made sure that after the patch set is applied, there is no *additional*
error compared to before.

As the patch set is pretty much mechanical, and shouldn't introduce any
functional change at all, the fact that there are no further compiler
errors should be a pretty good indication that it'll work ...


I'd appreciate any feedback!  Do you think this would be OK to commit?

Bye,
Ulrich



--
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [rfc] [00/14] Push REGCACHE into target_fetch/store_registers

Mark Kettenis
> Date: Tue, 1 May 2007 03:26:04 +0200 (CEST)
> From: "Ulrich Weigand" <[hidden email]>
>
> I'd appreciate any feedback!  Do you think this would be OK to commit?

Some general notes:

1. core_fns/core_read_registers should really die (it'd already
   deprecated) and replaced by gdbarch_regset_from_core_section().

2. supply_gregset, supply_fpregset, fill_gregset or fill_fpregset
   should really be replaced by something like
   gdbarch_regset_from_core_section().

That said, this diff is great progress, and I think it should go in
ASAP.  It's probably polite to give other responsable maintainers the
opportunity to react to the diff.  So consider this an explicit ok for
the i387 targets.

Mark
Reply | Threaded
Open this post in threaded view
|

Re: [rfc] [00/14] Push REGCACHE into target_fetch/store_registers

Daniel Jacobowitz-2
On Tue, May 01, 2007 at 10:39:34PM +0200, Mark Kettenis wrote:
> 2. supply_gregset, supply_fpregset, fill_gregset or fill_fpregset
>    should really be replaced by something like
>    gdbarch_regset_from_core_section().

Well, maybe - the other main use is for Linux thread-db, where they
don't actually deal with a core file.  But it should still die somehow.

> That said, this diff is great progress, and I think it should go in
> ASAP.  It's probably polite to give other responsable maintainers the
> opportunity to react to the diff.  So consider this an explicit ok for
> the i387 targets.

Everything I looked at looked right; OK by me too.

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

Re: [rfc] [00/14] Push REGCACHE into target_fetch/store_registers

Ulrich Weigand
In reply to this post by Mark Kettenis
Mark Kettenis wrote:

> 1. core_fns/core_read_registers should really die (it'd already
>    deprecated) and replaced by gdbarch_regset_from_core_section().
>
> 2. supply_gregset, supply_fpregset, fill_gregset or fill_fpregset
>    should really be replaced by something like
>    gdbarch_regset_from_core_section().

I agree, but those changes are difficult to make without having
the target to test on.  In particular making sure the osabi sniffer
detects the core file correctly is something that really needs
to be tested ...   That's why I left those things as-is for now.

> That said, this diff is great progress, and I think it should go in
> ASAP.  It's probably polite to give other responsable maintainers the
> opportunity to react to the diff.  So consider this an explicit ok for
> the i387 targets.

Thanks!   I'll wait for a couple of days for further comments before
checking it in  (but not too long -- I really don't want mainline to
change so much I'll have to re-do the patch set ...).

Bye,
Ulrich

--
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [rfc] [00/14] Push REGCACHE into target_fetch/store_registers

Ulrich Weigand
In reply to this post by Ulrich Weigand
> The patch set implementing the above change consists of the
> following patches:
>
> Fix the const-correctness problems:
>  1. irix5-nat.c
>  2. m32r-linux-nat.c
>
> Prepare some (groups of) targets by pushing REGCACHE into
> internal helper routines:
>  3. aix-thread.c
>  4. alpha targets
>  5. arm targets
>  6. cris target
>  7. i387 targets
>  8. mips targets
>  9. sh targets
> 10. nto targets
> 11. monitor targets
>
> Convert supply_gregset etc. routines
> 12. (one patch)
>
> Convert core_fn core_read_registers callback
> 13. (one patch)
>
> Actually convert to_store_registers / to_fetch_registers
> 14. (one patch)

I've committed those patches now.

Bye,
Ulrich

--
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  [hidden email]