From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cagney To: David Taylor Cc: gdb-patches@sources.redhat.com Subject: Re: [RFA] regcache.c (register_fetched) + related changes Date: Wed, 28 Feb 2001 15:17:00 -0000 Message-id: <3A9D8655.C57F3DB6@cygnus.com> References: <200102282113.QAA18390@texas.cygnus.com> X-SW-Source: 2001-02/msg00527.html 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 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