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

[Andrew Cagney <ac131313 at cygnus dot 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

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

> 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.

> 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?

>     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

	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.

> 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;

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

> 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;

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

be more applicable?

Why two functions?  The first may eventually be modified to report its
use: ``warning: this target uses the deprecated function ....''.  It may
simply be deleted.  The second may be re-implemented to use the a new
frame-register interface when that becomes available.

	Andrew