powerpc remote target registers

powerpc remote target registers

[Jonathan Larmour]

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

I've been having a problem where (via the MI interface, but that's 
irrelevant), GDB (6.0) would report e.g. "register vr8 not available", 
where "8" may be any number, changing each time the program is stepped or 
restarted. Register vr8 is an altivec register and my remote target does 
not support altivec.

What appears to be the problem is that the binaries built by 
powerpc-eabi-gcc are marked as being for architecture powerpc, machine 
"common" (by BFD in the ELF header). This tells GDB that it supports 
certain register sets, including altivec, i.e. vr* registers.

In fact the target doesn't support altivec, and the target sends a short 
packet compared to GDB's expectations, but that shouldn't matter as by 
rights GDB should just ignore registers the target doesn't supply, right? 
Certainly code appears to handle that case without a warning.

However, at the end of remote_fetch_registers(), it does a check for if 
any of the registers start with an "x" character. If it does, it is marked 
as invalid. However the buffer containing the register set is never 
initialised, and so contains random junk. The size of the altivec register 
set makes it more likely to find an "x" somewhere (although curiously it 
does happen more frequently than I'd expect). Which also explains it being 
a different register most times it's run.

Now, arguably this is a problem with GCC/GAS/GLD: there is a machine type 
specifically for mpc860 with the correct register definitions, instead of 
"common" with the correct register definitions. However GCC/GAS don't 
allow you to set that when compiling objects, and LD appears to ignore my 
request to set it explicitly when linking (using -A powerpc:mpc860). That 
shouldn't really be needed anyway, as it should be implied by a compile 
with -mcpu=860, but GCC doesn't pass anything to GAS/the linker to reflect 
that. Sigh.

However I would say that GDB is also mistaken for not initializing the 
packet buffer in remote_fetch_registers() to 0 first, so that registers 
that aren't supplied by the remote target don't have uninitialised data, 
which may include an "x" in them.

Another possibility is for the remote stub to send the much longer packets 
(nearly 2kb!) including the altivec registers, however this may cause 
backward compatibility problems with older GDBs which is very bad indeed 
for us. Instead GDB should be able to cope with stubs not returning full 
register sets despite expectations.

So, for that reason I suggest the attached patch. Sorry for the long 
e-mail for a one-liner :-).

Thanks,

Jifl

2003-11-29  Jonathan Larmour  <jifl@eCosCentric.com>

	* remote.c (remote_fetch_registers): Clear buf to avoid treating
	junk as register data.



-- 
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
--["No sense being pessimistic, it wouldn't work anyway"]-- Opinions==mine

[-- Attachment #2: remoteclearbuf.patch --]
[-- Type: text/plain, Size: 570 bytes --]

--- remote.c~	2003-06-30 16:51:49.000000000 +0100
+++ remote.c	2003-11-29 01:50:05.000000000 +0000
@@ -3440,10 +3440,11 @@ remote_fetch_registers (int regnum)
 	internal_error (__FILE__, __LINE__,
 			"Attempt to fetch a non G-packet register when this "
 			"remote.c does not support the p-packet.");
     }
 
+  memset (buf, 0, rs->remote_packet_size);
   sprintf (buf, "g");
   remote_send (buf, (rs->remote_packet_size));
 
   /* Save the size of the packet sent to us by the target.  Its used
      as a heuristic when determining the max size of packets that the

Re: powerpc remote target registers

[Andrew Cagney]

> Now, arguably this is a problem with GCC/GAS/GLD: there is a machine type specifically for mpc860 with the correct register definitions, instead of "common" with the correct register definitions. However GCC/GAS don't allow you to set that when compiling objects, and LD appears to ignore my request to set it explicitly when linking (using -A powerpc:mpc860). That shouldn't really be needed anyway, as it should be implied by a compile with -mcpu=860, but GCC doesn't pass anything to GAS/the linker to reflect that. Sigh.
> 
> However I would say that GDB is also mistaken for not initializing the packet buffer in remote_fetch_registers() to 0 first, so that registers that aren't supplied by the remote target don't have uninitialised data, which may include an "x" in them. 

I don't think this fixes the bug.  It will fill each nibble of the 
altivec registers with (0 - '0') instead of zero (you should see this 
with "maint print raw-registers").

Suggest instead changing the for loop filling in regs[] so that it 
doesn't run off the end of the NUL terminated buf[] (I think this is the 
real bug).

Andrew

Re: powerpc remote target registers

[Jonathan Larmour]

Andrew Cagney wrote:
>> Now, arguably this is a problem with GCC/GAS/GLD: there is a machine 
>> type specifically for mpc860 with the correct register definitions, 
>> instead of "common" with the correct register definitions. However 
>> GCC/GAS don't allow you to set that when compiling objects, and LD 
>> appears to ignore my request to set it explicitly when linking (using 
>> -A powerpc:mpc860). That shouldn't really be needed anyway, as it 
>> should be implied by a compile with -mcpu=860, but GCC doesn't pass 
>> anything to GAS/the linker to reflect that. Sigh.
>>
>> However I would say that GDB is also mistaken for not initializing the 
>> packet buffer in remote_fetch_registers() to 0 first, so that 
>> registers that aren't supplied by the remote target don't have 
>> uninitialised data, which may include an "x" in them. 
> 
> 
> I don't think this fixes the bug.  It will fill each nibble of the 
> altivec registers with (0 - '0') instead of zero (you should see this 
> with "maint print raw-registers").

Ah, it still came up with 0 until I did that so I thought that was it.

> Suggest instead changing the for loop filling in regs[] so that it 
> doesn't run off the end of the NUL terminated buf[] (I think this is the 
> real bug).

But then the registers aren't marked as cached at all, so they're now 
requested from the target each time you do "info all-registers", even 
though they come up with 0s. Should I pretend the registers not supplied 
by the target were 0, or should I mark them as unavailable (i.e. the same 
as what having an "x" does) so at least it's consistent?

Jifl
-- 
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
--["No sense being pessimistic, it wouldn't work anyway"]-- Opinions==mine

Re: powerpc remote target registers

[Andrew Cagney]

> But then the registers aren't marked as cached at all, so they're now requested from the target each time you do "info all-registers", even though they come up with 0s. Should I pretend the registers not supplied by the target were 0, or should I mark them as unavailable (i.e. the same as what having an "x" does) so at least it's consistent?

Ah, they should be supplied but with a value of zero.  The protocol (for 
  historic reasons) specifies that a short G packet should have the 
missing entries treated as zero (like you intended).

Andrew

Re: powerpc remote target registers

[Jonathan Larmour]

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

Andrew Cagney wrote:
> 
>> But then the registers aren't marked as cached at all, so they're now 
>> requested from the target each time you do "info all-registers", even 
>> though they come up with 0s. Should I pretend the registers not 
>> supplied by the target were 0, or should I mark them as unavailable 
>> (i.e. the same as what having an "x" does) so at least it's consistent?
> 
> 
> Ah, they should be supplied but with a value of zero.  The protocol (for 
>  historic reasons) specifies that a short G packet should have the 
> missing entries treated as zero (like you intended).

Good, in which case the attached patch (against 6.0) should do it. Mostly 
indent changes, boringly enough.


2003-12-04  Jonathan Larmour  <jifl@eCosCentric.com>

	* remote.c (remote_fetch_registers): If target doesn't supply
	registers, set them to zero.

Thanks,

Jifl
-- 
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
--["No sense being pessimistic, it wouldn't work anyway"]-- Opinions==mine

[-- Attachment #2: remoteregzero.patch --]
[-- Type: text/plain, Size: 975 bytes --]

--- remote.c.old	2003-12-02 03:05:46.000000000 +0000
+++ remote.c	2003-12-04 07:19:38.000000000 +0000
@@ -3498,19 +3498,31 @@ remote_fetch_registers (int regnum)
 	warning ("Remote reply is too short: %s", buf);
     }
 
  supply_them:
   {
-    int i;
+    int i, end_targ_regs=0;
     for (i = 0; i < NUM_REGS + NUM_PSEUDO_REGS; i++)
       {
 	struct packet_reg *r = &rs->regs[i];
+
+	if (buf[r->offset * 2] == 0)
+	  end_targ_regs = 1;  /* end of registers supplied by target */
 	if (r->in_g_packet)
 	  {
-	    supply_register (r->regnum, regs + r->offset);
-	    if (buf[r->offset * 2] == 'x')
-	      set_register_cached (i, -1);
+	    if (end_targ_regs)
+	      {
+		/* If the target hasn't sent enough registers, set
+		   the remainder to 0. */
+		supply_register (r->regnum, 0);
+	      }
+	    else
+	      {
+		supply_register (r->regnum, regs + r->offset);
+		if (buf[r->offset * 2] == 'x')
+		  set_register_cached (i, -1);
+	      }
 	  }
       }
   }
 }
 

Re: powerpc remote target registers

[Andrew Cagney]

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

> Andrew Cagney wrote:
> 
> But then the registers aren't marked as cached at all, so they're now requested from the target each time you do "info all-registers", even though they come up with 0s. Should I pretend the registers not supplied by the target were 0, or should I mark them as unavailable (i.e. the same as what having an "x" does) so at least it's consistent?
> 
> 
> Ah, they should be supplied but with a value of zero.  The protocol (for  historic reasons) specifies that a short G packet should have the missing entries treated as zero (like you intended).
> 
> Good, in which case the attached patch (against 6.0) should do it. Mostly indent changes, boringly enough.
> 
> 
> 2003-12-04  Jonathan Larmour  <jifl@eCosCentric.com>
> 
>     * remote.c (remote_fetch_registers): If target doesn't supply
>     registers, set them to zero.
> 
> Thanks,

Try the attached, its basicly the same but with a few not very obvious 
tweaks: supply_register is actually deprecated (but you couldn't tell 
:-); ->offset is really only valid when ->in_g_packet; avoids an 
assuption about the total size of the buffer and the behavior of get packet.

I think I got the logic right.

Andrew

(PS: paperwork sent)

> --- remote.c.old	2003-12-02 03:05:46.000000000 +0000
> +++ remote.c	2003-12-04 07:19:38.000000000 +0000
> @@ -3498,19 +3498,31 @@ remote_fetch_registers (int regnum)
>  	warning ("Remote reply is too short: %s", buf);
>      }
>  
>   supply_them:
>    {
> -    int i;
> +    int i, end_targ_regs=0;
>      for (i = 0; i < NUM_REGS + NUM_PSEUDO_REGS; i++)
>        {
>  	struct packet_reg *r = &rs->regs[i];
> +
> +	if (buf[r->offset * 2] == 0)
> +	  end_targ_regs = 1;  /* end of registers supplied by target */
>  	if (r->in_g_packet)
>  	  {
> -	    supply_register (r->regnum, regs + r->offset);
> -	    if (buf[r->offset * 2] == 'x')
> -	      set_register_cached (i, -1);
> +	    if (end_targ_regs)
> +	      {
> +		/* If the target hasn't sent enough registers, set
> +		   the remainder to 0. */
> +		supply_register (r->regnum, 0);
> +	      }
> +	    else
> +	      {
> +		supply_register (r->regnum, regs + r->offset);
> +		if (buf[r->offset * 2] == 'x')
> +		  set_register_cached (i, -1);
> +	      }
>  	  }
>        }
>    }
>  }
>  



[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 1455 bytes --]

2003-12-04  Andrew Cagney  <cagney@redhat.com>

	* remote.c (remote_fetch_registers): For short packets, explicitly
	supply a zero value.  Use regcache_raw_supply.  Fix suggested by
	Jonathan Larmour.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.122
diff -u -r1.122 remote.c
--- remote.c	10 Nov 2003 21:20:44 -0000	1.122
+++ remote.c	4 Dec 2003 15:05:49 -0000
@@ -3558,9 +3558,23 @@
 	struct packet_reg *r = &rs->regs[i];
 	if (r->in_g_packet)
 	  {
-	    supply_register (r->regnum, regs + r->offset);
-	    if (buf[r->offset * 2] == 'x')
-	      set_register_cached (i, -1);
+	    if (r->offset * 2 >= strlen (buf))
+	      /* A short packet that didn't include the register's
+                 value, this implies that the register is zero (and
+                 not that the register is unavailable).  Supply that
+                 zero value.  */
+	      regcache_raw_supply (current_regcache, r->regnum, NULL);
+	    else if (buf[r->offset * 2] == 'x')
+	      {
+		gdb_assert (r->offset * 2 < strlen (buf));
+		/* The register isn't available, mark it as such (at
+                   the same time setting the value to zero).  */
+		regcache_raw_supply (current_regcache, r->regnum, NULL);
+		set_register_cached (i, -1);
+	      }
+	    else
+	      regcache_raw_supply (current_regcache, r->regnum,
+				   regs + r->offset);
 	  }
       }
   }

Re: powerpc remote target registers

[Jonathan Larmour]

Andrew Cagney wrote:
> 
> Try the attached, its basicly the same but with a few not very obvious 
> tweaks: supply_register is actually deprecated (but you couldn't tell 
> :-); ->offset is really only valid when ->in_g_packet; avoids an 
> assuption about the total size of the buffer and the behavior of get 
> packet.
> 
> I think I got the logic right.

Just FAOD in case you're waiting for me to confirm, the patch does indeed 
appear to work fine.

Jifl
-- 
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
--["No sense being pessimistic, it wouldn't work anyway"]-- Opinions==mine

Re: powerpc remote target registers

[Andrew Cagney]

> Andrew Cagney wrote:
> 
> Try the attached, its basicly the same but with a few not very obvious tweaks: supply_register is actually deprecated (but you couldn't tell :-); ->offset is really only valid when ->in_g_packet; avoids an assuption about the total size of the buffer and the behavior of get packet.
> 
> I think I got the logic right.
> 
> Just FAOD in case you're waiting for me to confirm, the patch does indeed appear to work fine.

Thanks!  Committed.

Andrew