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

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

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