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

David Taylor
2001-03-01T10:34:00Z

Date: Wed, 28 Feb 2001 18:14:29 -0500
From: Andrew Cagney <ac131313@cygnus.com>

David Taylor wrote:
>
> From: Andrew Cagney <ac131313@cygnus.com>
> Date: Tue, 27 Feb 2001 20:38:30 -0500
>
> David Taylor wrote:
>
> > I propose that we:
> >
> > . add register_fetched
>
> David,
>
> The functionality of register_fetched() overlaps with
> set_register_cached() and supply_register(). Rather than add a
> redundant method, could an existing interface be used or the current
> interfaces rationalized slightly?
>
> Andrew,
>
> Supply register does more than register_fetched; register_fetched only
> affects register_valid -- the register must have been supplied via
> some other method.

Yes, that is what I mean by their functionality overlapping. There is
much legacy code of the form:

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!

The accepted ``correct fix'' for that code is to replace it with
supply_register(). It is just an unfortunate reality that no one has
the time to do the work. I have a feeling that, right now GDB is
deleting this code faster than it is being fixed!

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.

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:

void
register_fetched (int regnum)
{
set_register_cached (regnum, 1);
}

And the rest of the changes (excluding the one line declaration in a
header file) are just to modify existing targets to use a functional
interface rather than referencing register_valid directly.

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.

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.

for (;;)
*deprecated_register_buffer(...) = ...;
deprecated_register_valid();

> set_register_cached is -- with one exception -- only used within
> regcache.c. The one exception is remote.c (remote_fetch_registers).
>
> I feel that a function should be created (register_unavailable?) and
> the call in remote.c to set_register_cached replaced with that call.
> Then set_register_cached should be made static.
>
> To call set_register_cached, you have to know what the meanings are of
> the various possible values of register_valid. This is knowledge that
> shouldn't really exist outside of regcache.c.

Yes. My suggestion, hinted at in the last e-mail, would be to combine
the two disjoint operations:

supply_register(....);
set_register_cache(..., -1)

into an atomic cache transaction:

supply_unavailable_register(....)

> Keep in mind that the long term goal is to tighten regcache's interface
> signficantly. That is, eliminate register_valid[], registers[] and
> possibly even set_register_cached() replacing them with a small set of
> functions such as:
> supply_register()
> supply_unavailable_register()
> If you are thinking of proposing further changes then you may want to
> keep that in mind.
>
> My change advances the goal of eliminating register_valid! It reduces
> significantly the number of files that even know that register_valid
> exists! Outside of regcache.c, only two files would reference it (for
> a total of three references). Those two files could also have their
> references to it removed with a little bit more work.

I agree, eliminating register_valid[] and registers[] are important
goals. The thing to be wary of is a change that just substitutes
something like registers[] and register_valid[] without actually
improving the overall quality of the code. I think it is important to
ensure that any proposed change to GDB moves its overall code quality
forward (and not just sideways).

Grubbing around for other cases.

--

Grubbing through the code, the other case where register_valid[] is
being blatted is:

/* These (for the most part) are pseudo registers and are obtained
by other means. Those that aren't are already handled by the
code above. */
for (regi = IA64_GR32_REGNUM; regi <= IA64_GR127_REGNUM; regi++)
register_valid[regi] = 1;
for (regi = IA64_PR0_REGNUM; regi <= IA64_PR63_REGNUM; regi++)
register_valid[regi] = 1;
for (regi = IA64_VFP_REGNUM; regi <= NUM_REGS; regi++)
register_valid[regi] = 1;

I'm guessing that code somewhere is looking at register_valid[] to
decide if a pseudo is available. The actual value (hopefully) being
supplied by the pseudo-register method.

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)

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.

Interface should be available. I think this also helps to differentiate
the more recent pseudo from legacy code.

--

The other case I'm aware of, is with registers[] and core-gdb poking
around in that instead of using the read/write register functions. I
think, there, the deprecated_register_buffer() method would be fine.
That code should be using the read_register*() functions and not pokeing
around in that buffer.

> With regard to regcache.h, yes the two clash. It both moves code around
> and changes the set_register_cached() interface. If anything regcache.h
> makes life easier because it is finally clear what the regcache
> interfaces really are.
>
> Andrew
>
> What change is this that you are referring to? The message with subject
>
> [rfc] Re-merged regcache.h patch

I also mentioned:

o The changes adding:
+ #define REGISTER_CACHE_UNAVAILABLE (1)
+ #define REGISTER_CACHE_VALID (0)
+ #define REGISTER_CACHE_INVALID (-1)
(or enum equivalents) were stripped out.
These changes badly clashed with Davids
proposed register_*() change so, I'll
leave them for later.

Which I saw; but, they aren't part of the patch. (And either the
un-included part changes the meanings of the register_valid slots, or
the above defines are wrong.)

I was actually wrong here. The changes were only in my local sandpit
and not in the original patch.

> +/* Character array containing the current state of each register
> + (unavailable<0, valid=0, invalid>0). */

See inferior.h:register_valid. I'll fix the comment. Thanks for
noticing this.

Yes, the comment in inferior.h is wrong.

I didn't delete the declaration as part of my proposed patch because,
as I said, there were still three locations (spread between two files)
that still referenced register_valid.

----

Any way, to summarize, I'm proposing:

replace
supply_register();
set_register_cached();
with:
supply_unavailable_register()

replace
blat registers[];
register_valid[] = 1;
with
blat deprecated_register_buffer(...);
depreciated_register_valid/fetched(...);
where this I/F would only work on raw registers.

replace
registers_valid[pseudo] = 1;
with
pseudo_register_valid/fetched(REGNUM)
where the register must be a pseudo.

replace remaining
... = registers[];
with
... = depreciated_register_buffer();

I think this is a reasonable balance between the immediate desire of
eliminate the globals registers_valid[] and registers[] and the long
term objective of eliminating code that blats arbitrary bytes in the
registers[] array.

Andrew

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