From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Taylor To: Andrew Cagney Cc: gdb-patches@sources.redhat.com Subject: Re: [RFA] regcache.c (register_fetched) + related changes Date: Thu, 01 Mar 2001 10:34:00 -0000 Message-id: <200103011834.NAA18546@texas.cygnus.com> X-SW-Source: 2001-03/msg00016.html Date: Wed, 28 Feb 2001 18:14:29 -0500 From: Andrew Cagney David Taylor wrote: > > From: Andrew Cagney > 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].