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

Andrew Cagney
2001-02-28T15:17:00Z

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

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!

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:

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.

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)

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.

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.

----

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