read_register_byte can't work with pseudo-reg model

read_register_byte can't work with pseudo-reg model

[Richard Earnshaw]

Given the following code in read_register_byte:

     reg_start = REGISTER_BYTE (regnum);
      reg_len = REGISTER_RAW_SIZE (regnum);
      reg_end = reg_start + reg_len;

      if (reg_end <= in_start || in_end <= reg_start)
	/* The range the user wants to read doesn't overlap with regnum.  */
	continue;

      if (REGISTER_NAME (regnum) != NULL && *REGISTER_NAME (regnum) != 
'\0')
	/* Force the cache to fetch the entire register.  */
	read_register_gen (regnum, reg_buf);
      else
	/* Legacy note: even though this register is ``invalid'' we
           still need to return something.  It would appear that some
           code relies on apparent gaps in the register array also
           being returned.  */
	/* FIXME: cagney/2001-08-18: This is just silly.  It defeats
           the entire register read/write flow of control.  Must
           resist temptation to return 0xdeadbeef.  */
	memcpy (reg_buf, registers + reg_start, reg_len);

Then the new model of having all named registers be pseudos will never 
re-read the registers, because all registers with an entry in registers[] 
will not have a name.

Shouldn't the "REGISTER_NAME" check be a direct check for 
register_cached(regno) == 0

That would mean that we could change the above to be something like

     reg_start = REGISTER_BYTE (regnum);
      reg_len = REGISTER_RAW_SIZE (regnum);
      reg_end = reg_start + reg_len;

      if (reg_end <= in_start || in_end <= reg_start)
	/* The range the user wants to read doesn't overlap with regnum.  */
	continue;

      if (register_cached (regnum) == 0)
	/* Force the cache to fetch the entire register.  */
	legacy_read_register_gen (regnum, reg_buf);
      else
	/* Legacy note: even though this register is ``invalid'' we
           still need to return something.  It would appear that some
           code relies on apparent gaps in the register array also
           being returned.  */
	/* FIXME: cagney/2001-08-18: This is just silly.  It defeats
           the entire register read/write flow of control.  Must
           resist temptation to return 0xdeadbeef.  */
	memcpy (reg_buf, registers + reg_start, reg_len);

Though I'm still not sure what we should do for a pseudo with no entry in 
the cache.

R.

Re: read_register_byte can't work with pseudo-reg model

[Andrew Cagney]

> Given the following code in read_register_byte:

[read_register_bytes]

>      reg_start = REGISTER_BYTE (regnum);
>       reg_len = REGISTER_RAW_SIZE (regnum);
>       reg_end = reg_start + reg_len;
> 
>       if (reg_end <= in_start || in_end <= reg_start)
> 	/* The range the user wants to read doesn't overlap with regnum.  */
> 	continue;
> 
>       if (REGISTER_NAME (regnum) != NULL && *REGISTER_NAME (regnum) != 
> '\0')
> 	/* Force the cache to fetch the entire register.  */
> 	read_register_gen (regnum, reg_buf);
>       else
> 	/* Legacy note: even though this register is ``invalid'' we
>            still need to return something.  It would appear that some
>            code relies on apparent gaps in the register array also
>            being returned.  */
> 	/* FIXME: cagney/2001-08-18: This is just silly.  It defeats
>            the entire register read/write flow of control.  Must
>            resist temptation to return 0xdeadbeef.  */
> 	memcpy (reg_buf, registers + reg_start, reg_len);

I'm guessing. Try:

	if (REGISTER_READ_P ())
	  {
	    do something fairly sane;
	  }
	else
	  {
	    all the legacy cruft including the call to
	    legacy_read_register_gen() and that test.
	  }

Thing is that there is only one target in the FSF using 
READ_REGISTER_P() so there is this dividing line - something using 
read_register_p() can be given far stronger requirements than for the 
older code.

``do something sane'' would be go straight through to the raw cache 
(regcache_read) for [0..REG_NUM) and go via READ_REGISTER() for anything 
else.

Elena, how would sh5 cope with this change?

> Then the new model of having all named registers be pseudos will never 
> re-read the registers, because all registers with an entry in registers[] 
> will not have a name.
> 
> Shouldn't the "REGISTER_NAME" check be a direct check for 
> register_cached(regno) == 0
> 
> That would mean that we could change the above to be something like
> 
>      reg_start = REGISTER_BYTE (regnum);
>       reg_len = REGISTER_RAW_SIZE (regnum);
>       reg_end = reg_start + reg_len;
> 
>       if (reg_end <= in_start || in_end <= reg_start)
> 	/* The range the user wants to read doesn't overlap with regnum.  */
> 	continue;
> 
>       if (register_cached (regnum) == 0)
> 	/* Force the cache to fetch the entire register.  */
> 	legacy_read_register_gen (regnum, reg_buf);

Thinking about it, my sugestion isn't sufficent :-(  One of the problems 
with read_register_bytes is that it is used to both read the raw 
register cache (a rawreg thing) and read sequences of pseudo registers 
(a cookedreg thing).

>       else
> 	/* Legacy note: even though this register is ``invalid'' we
>            still need to return something.  It would appear that some
>            code relies on apparent gaps in the register array also
>            being returned.  */
> 	/* FIXME: cagney/2001-08-18: This is just silly.  It defeats
>            the entire register read/write flow of control.  Must
>            resist temptation to return 0xdeadbeef.  */
> 	memcpy (reg_buf, registers + reg_start, reg_len);
> 
> Though I'm still not sure what we should do for a pseudo with no entry in 
> the cache.

Andrew

Re: read_register_byte can't work with pseudo-reg model

[Richard Earnshaw]

> I'm guessing. Try:
> 
> 	if (REGISTER_READ_P ())
> 	  {
> 	    do something fairly sane;
> 	  }
> 	else
> 	  {
> 	    all the legacy cruft including the call to
> 	    legacy_read_register_gen() and that test.
> 	  }
> 
> Thing is that there is only one target in the FSF using 
> READ_REGISTER_P() so there is this dividing line - something using 
> read_register_p() can be given far stronger requirements than for the 
> older code.

Which target is that, and where is READ_REGISTER_P ?  I can't find 
anything in the either the source or the mailing lists.  Mind you, the 
web-based mailing list search even fails to find your message when I 
search for READ_REGISTER_P.

R.

Re: read_register_byte can't work with pseudo-reg model

[Andrew Cagney]

> I'm guessing. Try:
>> 
>> 	if (REGISTER_READ_P ())
>> 	  {
>> 	    do something fairly sane;
>> 	  }
>> 	else
>> 	  {
>> 	    all the legacy cruft including the call to
>> 	    legacy_read_register_gen() and that test.
>> 	  }
>> 
>> Thing is that there is only one target in the FSF using 
>> READ_REGISTER_P() so there is this dividing line - something using 
>> read_register_p() can be given far stronger requirements than for the 
>> older code.
> 
> 
> Which target is that, and where is READ_REGISTER_P ?  I can't find 
> anything in the either the source or the mailing lists.  Mind you, the 
> web-based mailing list search even fails to find your message when I 
> search for READ_REGISTER_P.

Sorry, register_read_p() -> sh5.
(something else on my todo list is stomp on some of these redundant 
interfaces :-)

Andrew

Re: read_register_byte can't work with pseudo-reg model

[Richard Earnshaw]

> (something else on my todo list is stomp on some of these redundant 
> interfaces :-)
> 


Hmm, how about doing some of this stomping in a more aggressive manner on 
the regbuf branch; causing temporary breakage for some targets should be 
less of an issue there.

R.

Re: read_register_byte can't work with pseudo-reg model

[Richard Earnshaw]

[-- Attachment #1: Type: text/plain, Size: 1526 bytes --]

> > I'm guessing. Try:
> >> 
> >> 	if (REGISTER_READ_P ())
> >> 	  {
> >> 	    do something fairly sane;
> >> 	  }
> >> 	else
> >> 	  {
> >> 	    all the legacy cruft including the call to
> >> 	    legacy_read_register_gen() and that test.
> >> 	  }
> >> 
> >> Thing is that there is only one target in the FSF using 
> >> READ_REGISTER_P() so there is this dividing line - something using 
> >> read_register_p() can be given far stronger requirements than for the 
> >> older code.
> > 
> > 
> > Which target is that, and where is READ_REGISTER_P ?  I can't find 
> > anything in the either the source or the mailing lists.  Mind you, the 
> > web-based mailing list search even fails to find your message when I 
> > search for READ_REGISTER_P.

Ok, the following solves my problem; it also makes saving/restoring the 
regcache far more efficient on machines that have that have 
READ_REGISTER_P.

The only assumption is that if this is defined, then pseudos do not need 
unique entries in the regcache (ie they always map onto physical 
registers), so we can copy the regcache simply by iterating over 
0..NUM_REGS.

I need to re-baseline my testsuite runs, but the results look pretty 
encouraging compared to previous runs.

Elena, would this be compatible with the SH model?

R.

	* regcache.c (read_register_bytes): If read_register_p is defined
	and we are saving the entire register set, then short-cut the
	save operation.
	(write_register_bytes): Similarly for write_register_p and restoring
	the register set.



[-- Attachment #2: gdb-regbytes.patch --]
[-- Type: text/x-patch , Size: 1971 bytes --]

Index: regcache.c
===================================================================
RCS file: /cvs/src/src/gdb/regcache.c,v
retrieving revision 1.34
diff -p -r1.34 regcache.c
*** regcache.c	6 Apr 2002 00:02:50 -0000	1.34
--- regcache.c	16 May 2002 15:31:56 -0000
*************** read_register_bytes (int in_start, char 
*** 228,233 ****
--- 228,248 ----
    int regnum;
    char *reg_buf = alloca (MAX_REGISTER_RAW_SIZE);
  
+   if (gdbarch_register_read_p (current_gdbarch)
+       && in_start == 0 && in_len == REGISTER_BYTES) /* OK Use.  */
+     {
+       /* We assume that if the target has set register_read_p() then
+ 	 all the pseuos will be correctly mapped onto raw registers.
+ 	 Therefore, the regcache describes only the registers in the
+ 	 range [0..NUM_REGS) and the code for reading the entire
+ 	 regset becomes much simpler.  */
+ 
+       for (regnum = 0; regnum < NUM_REGS; regnum++)
+ 	regcache_read (regnum, in_buf + REGISTER_BYTE (regnum));
+ 
+       return;
+     }
+ 
    /* See if we are trying to read bytes from out-of-date registers.  If so,
       update just those registers.  */
  
*************** write_register_bytes (int myregstart, ch
*** 397,402 ****
--- 412,432 ----
    int regnum;
  
    target_prepare_to_store ();
+ 
+   if (gdbarch_register_write_p (current_gdbarch)
+       && myregstart == 0 && inlen == REGISTER_BYTES) /* OK Use.  */
+     {
+       /* We assume that if the target has set register_write_p() then
+ 	 all the pseuos will be correctly mapped onto raw registers.
+ 	 Therefore, the regcache describes only the registers in the
+ 	 range [0..NUM_REGS) and the code for restoring things becomes
+ 	 much simpler.  */
+ 
+       for (regnum = 0; regnum < NUM_REGS; regnum++)
+ 	regcache_write (regnum, myaddr + REGISTER_BYTE (regnum));
+ 
+       return;
+     }
  
    /* Scan through the registers updating any that are covered by the
       range myregstart<=>myregend using write_register_gen, which does

Re: read_register_byte can't work with pseudo-reg model

[Andrew Cagney]

> (something else on my todo list is stomp on some of these redundant 
>> interfaces :-)
>> 
> 
> 
> 
> Hmm, how about doing some of this stomping in a more aggressive manner on 
> the regbuf branch; causing temporary breakage for some targets should be 
> less of an issue there.

I prefer the technique of adding legacy_ prefixes and then deleting 
``legacy'' targets :-^

More seriously, I've re-done the reg stuff to have a regcache (like you 
suggested) (yes we both fixed the same param bug) instead of a regbuf - 
regbuf was overkill for the problem immediatly to hand.

Part of this change will involve replacing two interfaces.  Getting just 
that right will be fun enough.

enjoy,
Andrew

Re: read_register_byte can't work with pseudo-reg model

[Andrew Cagney]

> The only assumption is that if this is defined, then pseudos do not need 
> unique entries in the regcache (ie they always map onto physical 
> registers), so we can copy the regcache simply by iterating over 
> 0..NUM_REGS.

So the [0..NUM_REGS) space is mapped 1:1, sounds good.

Just resist the temptation to look under the following rock: 
0..NUM_REGS will include hardware registers save/restoring that probably 
isn't a good idea.  Per other e-mail, it will eventually need to check 
if the register should be saved/restored.

I think I'll tweak that branch to, when restoring the cache and 
register_write_p, not call write_register_bytes().

> I need to re-baseline my testsuite runs, but the results look pretty 
> encouraging compared to previous runs.

Andrew

Re: read_register_byte can't work with pseudo-reg model

[Richard Earnshaw]

> > The only assumption is that if this is defined, then pseudos do not need 
> > unique entries in the regcache (ie they always map onto physical 
> > registers), so we can copy the regcache simply by iterating over 
> > 0..NUM_REGS.
> 
> So the [0..NUM_REGS) space is mapped 1:1, sounds good.

Yep, that's the idea.

> 
> Just resist the temptation to look under the following rock: 
> 0..NUM_REGS will include hardware registers save/restoring that probably 
> isn't a good idea.  Per other e-mail, it will eventually need to check 
> if the register should be saved/restored.

I think regcache_read() could be made to handle that easily enough.

> 
> I think I'll tweak that branch to, when restoring the cache and 
> register_write_p, not call write_register_bytes().
> 
> > I need to re-baseline my testsuite runs, but the results look pretty 
> > encouraging compared to previous runs.
> 

I ran into another problem last night.  The code to update a register value, say from

(gdb) set $f0 = 1.0

causes write_register_bytes to iterate over the entire
[0..NUM_REGS+NUM_PSEUDO_REGS), even though a start regno is given.  Since
this then calls through the write_register_gen() interface whenever
REGISTER_BYTE indicates an overlap we end up calling REGISTER_WRITE() with
a non-pseudo regno.  The ARM implementation of this code is designed to
fault when this occurs (since it shouldn't -- gdb-core should only be
accessing the pseudo view of the register).  Would the following tweak be
acceptable?  That is, only do the update if the register has a name.

  else if (REGISTER_NAME (regnum) != NULL && *REGISTER_NAME (regnum) != '\0')
    {
      /* Is this register completely within the range the user is writing?  */
      if (myregstart <= regstart && regend <= myregend)
        write_register_gen (regnum, myaddr + (regstart - myregstart));

      /* The register partially overlaps the range being written.  */
      else
        {
          char *regbuf = (char*) alloca (MAX_REGISTER_RAW_SIZE);
          /* What's the overlap between this register's bytes and
             those the caller wants to write?  */
          int overlapstart = max (regstart, myregstart);
          int overlapend   = min (regend,   myregend);

          /* We may be doing a partial update of an invalid register.
             Update it from the target before scribbling on it.  */
          read_register_gen (regnum, regbuf);

          memcpy (registers + overlapstart,
                  myaddr + (overlapstart - myregstart),
                  overlapend - overlapstart);

          store_register (regnum);
        }
    }

Note that even with this change some weird multi-writing effects are going
to happen.  Consider a frag:

	+------------+------------+
	| PHYS S0 "" | PHYS S1 "" |  Physical regs in regbank
	+------------+------------+
	|    "S0"    |    "S1"    |  First pseudo view (single precision)
	+------------+------------+
        |          "D0"           |  Second pseudo view (double)
	+------------+------------+

Then with the existing code

(gdb) set $s0 = 1.0

will call write_register_gen for PHYS S0 (anonymous), then for "S0" the
pseudo view of S0, and finally it will read the whole of "D0", update part
of it and write all of "D0" back again.

My change would eliminate the update of PHYS S0, but not simplify the
remaining multi-updates.

R.

Re: read_register_byte can't work with pseudo-reg model

[Richard Earnshaw]

rearnsha@arm.com said:
> Would the following tweak be acceptable?  That is, only do the update
> if the register has a name.

>   else if (REGISTER_NAME (regnum) != NULL && *REGISTER_NAME (regnum)
> != '\0')
>     {
>       /* Is this register completely within the range the user is
> writing?  */
>       if (myregstart <= regstart && regend <= myregend)
>         write_register_gen (regnum, myaddr + (regstart - myregstart));

>       /* The register partially overlaps the range being written.  */
>       else
>         { 



I've just noticed that read_register_bytes is doing almost exactly this.  
Why do the two differ?

R.