Re: [RFA] regcache.c (register_fetched) + related changes

[David Taylor <taylor at cygnus dot com>]

    Date: Thu, 01 Mar 2001 17:05:00 -0500
    From: Andrew Cagney <ac131313@cygnus.com>

    David Taylor wrote:

    >             blat the registers[] array
    >             set register_valid[] to 1
    > 
    > But, there also exists code that does not directly blat the
    > registers[] array, but nonetheless still needs to mark one or more
    > registers as cached -- i.e. has to set one or more register_valid[]
    > locations to 1!

    Can you give an example of where this is happening?  I'm currently aware
    of two cases:

	    [0 .. NUM_REGS)
		    where supply_register() should be used

Not always.  See below.

	    [NUM_REGS .. NUM_REGS + PSEUDO_NUM_REGS)
		    I suspect your case.

I *clear* in the [NUM_REGS, NUM_REGS+PSEUDO_NUM_REGS) range and *set*
in the [0, NUM_REGS) range.

I will probably eventually have to set in the range [NUM_REGS,
NUM_REGS + PSEUDO_NUM_REGS) as well -- if I want to manage 32 bytes of
non register state (though perhaps what I'm already doing will suffice
-- those 32 bytes are only an issue during when gdb makes inferior
function calls).

    > The STORE_PSEUDO_REGISTER routines do not get the register contents as
    > an argument.  But they potentially need to set and clear various
    > register_valid locations.

    Yes, agreed.

And the locations that they potentially need to clear are in the [0,
NUM_REGS) range.

    > To clear a location (i.e., mark it as "invalid") they can use the
    > existing register_changed function; but there is currently no
    > functional interface for them to set (i.e., mark as "cached" or
    > "valid") a location.

    > That is the point of my proposed function:

    Is:

	    [NUM_REGS .. NUM_REGS + PSEUDO_NUM_REGS)
		    I suspect your case.

    your case?

It is inside of my target's STORE_PSEUDO_REGISTER routine that I need
to do it.  So, yes, it is some registers in that range that create the
need to do it, but the registers for which it is done are in the
[0,NUM_REGS) range.

In STORE_PSEUDO_REGISTER, for some pseudo registers, I

. clear register_valid (by calling register_changed) for the pseudo
  register.
  (i.e., a register in the [NUM_REGS,  NUM_REGS+PSEUDO_NUM_REGS) range.)

. set register_valid (directly, since there is no functional
  interface) for two underlying real registers.
  (i.e., two registers in the [0, NUM_REGS) range.)

Example, for this processor, the frame pointer is a pseudo register.
The frame pointer is two random 8 bit registers -- 16 bits.  The
hardware does *NOT* have the ability to operate on those registers as
a pair.  It cannot load the frame pointer, it cannot store the frame
pointer, it can only load or store one of the 8 bit registers at a
time.

Since the compiler writer simply chose two adjacent 8 bit registers,
in the right order, there is no need, in REGISTER_BYTES to have a
special place for the fp and then copy things back and forth --
instead the value goes directly into the two underlying real
registers.

So, when the fp is fetched, there is a need to mark the underlying
real registers (i.e., registers in the range [0,NUM_REGS) as fetched.

    >     Given that reailty, if this code is going to be changed, I would prefer
    >     it changed in a way that made it very clear that this is not the correct
    >     way to do things.  Taking a lead from DEPRECATED_SERIAL_FD I think those
    >     blocks can be converted to something like:
    > 
    > I wasn't changing the accesses to the registers[] array.  Nor do I
    > currently have the time to spend to make such a change just to get the
    > above one line function accepted.  I've already spent more time on
    > this than I should have.

    Yes, I know.

    For my part, I've the following:

	    o       your patch to clean up register_valid[]

	    o       the very old regcache.h patch

	    o       Nick's proposal to change registers[]
		    to a function

Some of the problems that I've had during the port would not have been
present if Nick's proposed interface existed.

	    o       the proposal to make ``frame''
		    the center of the universe

	    o       the standing order that
		    registers[] should not be used
		    in all future code.

	    o       Steven's pseudo size patch

	    o       the request to eliminate
		    ARCH_NUM_REGS.

    to reconcile.

    It is just really unfortunate that your patch landed on my table at the
    exact same time that I was working on resolving the other issues listed
    above.

    You'll note that I've now resolved the regcache.h and pseudo size
    patches.  As for ARCH_NUM_REGS, the predicate - eliminate harris/88k -
    has been started.

    In case you're wondering, I'm not expecting you to solve all the above
    problems, nor submit a new patch :-)

    > I guess for now I'll just have to say register_valid[...] = 1; in my
    > tdep file.  Maybe when the revolution comes... but until then...  Very
    > big sigh.

    If you submited the target today, then yes, I would honestly recommend
    doing that!  It confines the changes to an existing well defined
    interface.

    However, if you're willing to help resolve this thread, then you'll
    hopefully find that your patch, or one very like it is integrated into
    GDB.

    > Unless a pseudo shares nothing with any real register, with the
    > current interfaces a tdep file should *NEVER* allow register_valid to
    > be 1 for that pseudo register.  Otherwise, when something updates one
    > of the contributing registers, the pseudo will still be marked as
    > valid when it might not be valid.
    > 
    >     While that particular hack could eventually be fixed in other ways
    >     (short term by having the ``register_cached()'' method ask gdbarch if a
    >     pseudo is valid; and long term by having the register_valid() method
    >     bound to a frame and always asking gdbarch if i is valid) that ain't
    >     going to solve the hear and now.
    > 
    >     Given that, I think this case (pseudo) is different/separate to the
    >     legacy code case, I don't think it makes sense to use an interface like
    >     deprecated_register_valid().  Instead, I think a separate:
    > 
    >             pseudo_register_valid(REGNUM)

      pseudo_register_valid(REGNUM) == register_fetched(REGNUM)

    when REGNUM in PSEUDOs. 

      set_pseudo_register_valid(REGNUM)
    or
      pseudo_register_fetched(REGNUM)

    would probably better names.

With the existing interfaces, nothing in the tdep file gets notified
when a register changes -- there is no mechanism in place to keep the
pseudo register valid bits accurate.

Given that lack of mechanism, if any real registers contribute to a
particular pseudo register, the ONLY safe value for that pseudo
register's valid field is 0 -- that way, the FETCH_PSEUDO_REGISTER
routine will be called and it can perform any necessary fetches and
updates.

    > Are you proposing this as setting the location?  Or are you proposing
    > this as a query function?  I would welcome a such a query function.
    > If you are proposing a "set" function, then until other changes are
    > made (namely, changes that allow the implementor to keep the pseudo
    > bits current), I would object to encouraging a false sense of security
    > in people.

    On existing implementations, it would have the effect of:

	    register_valid[REGNUM] = 1;

So, it would set the bit, not query it.

    I don't understand what you're refering to by ``false sense of
    security''.

Perhaps 'false sense of security' was a bad phrase.  But, if there is
a set_pseudo_register_valid function, then people will be potentially
lulled into thinking that they there is a mechanism for keeping the
state up to date when there is none.

Without additional changes to the register interface, the proposed
set_pseudo_register_valid function is only safe to call if the pseudo
register represents state that is not shared in any way shape or
fashion with any real register.

I would guess that most existing pseudo registers either are computed
from real registers or they share one or more bits with one or more
real registers.  For such registers you never want
register_valid[REGNUM] to be 1.

    > And, as I said, there are times when you need to get the effect of
    > register_valid[regno] = 1 when you have not blatted the registers[]
    > array.  The question, in my mind, is whether to continue to directly
    > set it or to instead have a functional interface.  I would prefer that
    > there be a functional interface -- then it would be possible to make
    > register_valid be static (i.e., private to the regcache.c file) [and
    > to eventually make it go away].

    I know.  I would like to identify an interface that lets that happen
    while at the same time ensuring that legacy code (namely registers[])
    doesn't get duplicated.

    Would functions with initial semantics:

	    deprecated_set_raw_register_fetched()
		    gdb_assert (regnum >= 0 && regnum < NUM_REGS);
		    register_valid[regnum] = 1;

This is the one that I would need to call in my tdep file.

    and     set_pseudo_register_fetched()
		    gdb_assert (regnum >= NUM_REGS && regnum < NUM_REGS +
    NUM_PSEUDO_REGS);
		    register_valid[regnum] = 1;

I would have no need of this one.

I would assert, having only looked briefly at the places where
register_valid is set that:

. most of them are calling it with a real register number, not a
  pseudo register number.

. most of the places where it is called with a pseudo register number
  are likely bugs.

    be more applicable?