Re: GDB patch

Re: GDB patch

[Nigel Stephens]

> On Tue, Dec 10, 2002 at 01:07:31PM +0100, Carsten Langgaard wrote:
> 
> > I've attached a patch for gdb-stub.c to make it work better with the
> > sde-gdb.
> > These changes should be backwards compatible with a standard gdb, so it
> > shouldn't break anything.
> > Ralf, could you please apply it.
> 
> 
> Strongly object.  While I didn't check the implementation, it's nice to
> see 'X' implemented.  And P.  But what the heck is this?
> 
> 
> > @@ -816,13 +839,64 @@
> >  		case 'k' :
> >  			break;		/* do nothing */
> >  
> > +		case 'R':
> > +			/* RNN[:SS],	Set the value of CPU register NN (size SS) */
> > +			/* FALL THROUGH */
> 
> 
> > -		/*
> > -		 * Reset the whole machine (FIXME: system dependent)
> > -		 */
> >  		case 'r':
> > -			break;
> > +			/* rNN[:SS]	Return the value of CPU register NN (size SS) */
> 
> 
> 
> We're not making up a protocol here, we're implementing one.  R and r
> don't have anything to do with setting registers.

Hi Dan

Actually Carsten *is* trying to implement a protocol, it's just that
it's an extension to the gdb remote debug protocol, as used in our
SDE-MIPS toolchain (viz sde-gdb).  Algorithmics (now MIPS Technologies
UK), always extended the gdb remote debug protocol to support reading
and writing of single registers, and to support variable register
sizes (to allow a 64-bit debug stub to inter-work with gdb debugging a
32-bit application).

When we first implemented these extensions we used the 'R' command to
write a single register, and 'r' to read one (they weren't then used
by gdb). Since then the remote protocol has gained the 'P' command to
write a single register, so we no longer use 'R' - and it would be
dangerous to do so since it can restart the target (so you can get rid
of the special 'R' case, Carsten).

But the standard gdb remote protocol still doesn't have the ability to
read a single register, so I believe that 'r' (or something like it)
is a useful addition, which speeds up the remote protocol
significantly when running over a serial line. And it won't break the
kernel to add support for this extension.

Regards

Nigel
-- 
                         Nigel Stephens         Mailto:nigel@mips.com
    _    _ ____  ___     MIPS Technologies (UK) Phone.: +44 1223 706200
    |\  /|||___)(___     The Fruit Farm         Direct: +44 1223 706207
    | \/ |||    ____)    Ely Road, Chittering   Fax...: +44 1223 706250
    TECHNOLOGIES (UK)    Cambridge CB5 9PH      Cell..: +44 7976 686470
 [formerly Algorithmics] England                http://www.algor.co.uk

Re: GDB patch

[Nigel Stephens]

> On Tue, Dec 10, 2002 at 01:07:31PM +0100, Carsten Langgaard wrote:
> 
> > I've attached a patch for gdb-stub.c to make it work better with the
> > sde-gdb.
> > These changes should be backwards compatible with a standard gdb, so it
> > shouldn't break anything.
> > Ralf, could you please apply it.
> 
> 
> Strongly object.  While I didn't check the implementation, it's nice to
> see 'X' implemented.  And P.  But what the heck is this?
> 
> 
> > @@ -816,13 +839,64 @@
> >  		case 'k' :
> >  			break;		/* do nothing */
> >  
> > +		case 'R':
> > +			/* RNN[:SS],	Set the value of CPU register NN (size SS) */
> > +			/* FALL THROUGH */
> 
> 
> > -		/*
> > -		 * Reset the whole machine (FIXME: system dependent)
> > -		 */
> >  		case 'r':
> > -			break;
> > +			/* rNN[:SS]	Return the value of CPU register NN (size SS) */
> 
> 
> 
> We're not making up a protocol here, we're implementing one.  R and r
> don't have anything to do with setting registers.

Hi Dan

Actually Carsten *is* trying to implement a protocol, it's just that
it's an extension to the gdb remote debug protocol, as used in our
SDE-MIPS toolchain (viz sde-gdb).  Algorithmics (now MIPS Technologies
UK), always extended the gdb remote debug protocol to support reading
and writing of single registers, and to support variable register
sizes (to allow a 64-bit debug stub to inter-work with gdb debugging a
32-bit application).

When we first implemented these extensions we used the 'R' command to
write a single register, and 'r' to read one (they weren't then used
by gdb). Since then the remote protocol has gained the 'P' command to
write a single register, so we no longer use 'R' - and it would be
dangerous to do so since it can restart the target (so you can get rid
of the special 'R' case, Carsten).

But the standard gdb remote protocol still doesn't have the ability to
read a single register, so I believe that 'r' (or something like it)
is a useful addition, which speeds up the remote protocol
significantly when running over a serial line. And it won't break the
kernel to add support for this extension.

Regards

Nigel
-- 
                         Nigel Stephens         Mailto:nigel@mips.com
    _    _ ____  ___     MIPS Technologies (UK) Phone.: +44 1223 706200
    |\  /|||___)(___     The Fruit Farm         Direct: +44 1223 706207
    | \/ |||    ____)    Ely Road, Chittering   Fax...: +44 1223 706250
    TECHNOLOGIES (UK)    Cambridge CB5 9PH      Cell..: +44 7976 686470
 [formerly Algorithmics] England                http://www.algor.co.uk

Re: GDB patch

[Daniel Jacobowitz]

On Tue, Dec 10, 2002 at 07:19:16PM +0000, Nigel Stephens wrote:
> > On Tue, Dec 10, 2002 at 01:07:31PM +0100, Carsten Langgaard wrote:
> > 
> > > I've attached a patch for gdb-stub.c to make it work better with the
> > > sde-gdb.
> > > These changes should be backwards compatible with a standard gdb, so it
> > > shouldn't break anything.
> > > Ralf, could you please apply it.
> > 
> > 
> > Strongly object.  While I didn't check the implementation, it's nice to
> > see 'X' implemented.  And P.  But what the heck is this?
> > 
> > 
> > > @@ -816,13 +839,64 @@
> > >  		case 'k' :
> > >  			break;		/* do nothing */
> > >  
> > > +		case 'R':
> > > +			/* RNN[:SS],	Set the value of CPU register NN (size SS) */
> > > +			/* FALL THROUGH */
> > 
> > 
> > > -		/*
> > > -		 * Reset the whole machine (FIXME: system dependent)
> > > -		 */
> > >  		case 'r':
> > > -			break;
> > > +			/* rNN[:SS]	Return the value of CPU register NN (size SS) */
> > 
> > 
> > 
> > We're not making up a protocol here, we're implementing one.  R and r
> > don't have anything to do with setting registers.
> 
> Hi Dan
> 
> Actually Carsten *is* trying to implement a protocol, it's just that
> it's an extension to the gdb remote debug protocol, as used in our
> SDE-MIPS toolchain (viz sde-gdb).  Algorithmics (now MIPS Technologies
> UK), always extended the gdb remote debug protocol to support reading
> and writing of single registers, and to support variable register
> sizes (to allow a 64-bit debug stub to inter-work with gdb debugging a
> 32-bit application).

My point is that we implement the GDB protocol, for use with GDB -
implementing random extensions to it is not a good idea.  I would
strongly prefer these extensions be discussed on the GDB list before
you try adding them to the CVS tree.  Also, I bet Andrew has a
different idea of how the 64/32 thing ought to work than you do.  He's
the remote protocol maintainer.

These things should be planned on the GDB side before making yet more
stubs use them.

> When we first implemented these extensions we used the 'R' command to
> write a single register, and 'r' to read one (they weren't then used
> by gdb). Since then the remote protocol has gained the 'P' command to

'R' was added in 1995 according to my records.  Really?

> write a single register, so we no longer use 'R' - and it would be
> dangerous to do so since it can restart the target (so you can get rid
> of the special 'R' case, Carsten).
> 
> But the standard gdb remote protocol still doesn't have the ability to
> read a single register, so I believe that 'r' (or something like it)
> is a useful addition, which speeds up the remote protocol
> significantly when running over a serial line. And it won't break the
> kernel to add support for this extension.

The protocol does, actually.  GDB doesn't _implement_ it, but the
extension is documented in the manual ('p') and I wouldn't be surprised
if Red Hat actually had an implementation somewhere.  I recommend the
documentation of the protocol, on the GDB web site.

Also note that `R' is extended restart process; the manual lists `r' as
"restart entire target system".  I don't know when that was used but
it's reason enough to stay away from using that letter to read a
register.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: GDB patch

[Nigel Stephens]

Daniel Jacobowitz wrote:

>>Actually Carsten *is* trying to implement a protocol, it's just that
>>it's an extension to the gdb remote debug protocol, as used in our
>>SDE-MIPS toolchain (viz sde-gdb).  Algorithmics (now MIPS Technologies
>>UK), always extended the gdb remote debug protocol to support reading
>>and writing of single registers, and to support variable register
>>sizes (to allow a 64-bit debug stub to inter-work with gdb debugging a
>>32-bit application).
>>    
>>
>
>My point is that we implement the GDB protocol, for use with GDB -
>implementing random extensions to it is not a good idea.  I would
>strongly prefer these extensions be discussed on the GDB list before
>you try adding them to the CVS tree.  Also, I bet Andrew has a
>different idea of how the 64/32 thing ought to work than you do.  He's
>the remote protocol maintainer.
>

>These things should be planned on the GDB side before making yet more
>stubs use them.
>  
>

I thought the Linux community prided itself on inventing new and 
"non-standard" extensions to the toolchain  ;-). But yes, we should try 
to avoid incompatible changes. As part of MIPS we will hopefully have 
the resources to interface with the rest of the GNU community, and argue 
for the inclusion of our patches in the CVS trees.

>>When we first implemented these extensions we used the 'R' command to
>>write a single register, and 'r' to read one (they weren't then used
>>by gdb). Since then the remote protocol has gained the 'P' command to
>>    
>>
>
>'R' was added in 1995 according to my records.  Really?
>  
>

Yup. SDE-MIPS 1.1 shipped in 1992. :-)

>The protocol does, actually.  GDB doesn't _implement_ it, but the
>extension is documented in the manual ('p') and I wouldn't be surprised
>if Red Hat actually had an implementation somewhere.  I recommend the
>documentation of the protocol, on the GDB web site.
>
>Also note that `R' is extended restart process; the manual lists `r' as
>"restart entire target system".  I don't know when that was used but
>it's reason enough to stay away from using that letter to read a
>register.
>  
>
Yeah, that's why we dropped 'R' in our more recent gdb ports, but I 
wasn't aware of the new use of 'r' - I'll check out that page. 
 Certainly 'p' is the logical inverse of 'P', so we'll change our gdb 
remote stub to use that. So how about accepting Carsten's change, with 
the 'R' case removed, and 'r' changed to 'p'?

Nigel

-- 
                         Nigel Stephens         Mailto:nigel@mips.com
    _    _ ____  ___     MIPS Technologies (UK) Phone.: +44 1223 706200
    |\  /|||___)(___     The Fruit Farm         Direct: +44 1223 706207
    | \/ |||    ____)    Ely Road, Chittering   Fax...: +44 1223 706250
    TECHNOLOGIES (UK)    Cambridge CB5 9PH      Cell..: +44 7976 686470
 [formerly Algorithmics] England                http://www.algor.co.uk

Re: GDB patch

[Daniel Jacobowitz]

On Tue, Dec 10, 2002 at 08:40:46PM +0000, Nigel Stephens wrote:
> I thought the Linux community prided itself on inventing new and 
> "non-standard" extensions to the toolchain  ;-). But yes, we should try 
> to avoid incompatible changes. As part of MIPS we will hopefully have 
> the resources to interface with the rest of the GNU community, and argue 
> for the inclusion of our patches in the CVS trees.

Actually, we've been trying to pride ourselves on not doing so :)

> Yup. SDE-MIPS 1.1 shipped in 1992. :-)

Wow...

> Yeah, that's why we dropped 'R' in our more recent gdb ports, but I 
> wasn't aware of the new use of 'r' - I'll check out that page. 

I think it's an old use, actually.  I'm not sure where it was used
though.

> Certainly 'p' is the logical inverse of 'P', so we'll change our gdb 
> remote stub to use that. So how about accepting Carsten's change, with 
> the 'R' case removed, and 'r' changed to 'p'?

Can't do it.  I strongly suspect that it will render the stub unusable
with current versions of FSF GDB.  Your tools add an explicit size to
the packet and the community tools do not; so when they probe for and
discover the P packet, they will probably try to use it and get
confused.  That's why I'd like to discuss this on the GDB list first.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: GDB patch

[Nigel Stephens]

Daniel Jacobowitz wrote:

>>Certainly 'p' is the logical inverse of 'P', so we'll change our gdb 
>>remote stub to use that. So how about accepting Carsten's change, with 
>>the 'R' case removed, and 'r' changed to 'p'?
>>    
>>
>
>Can't do it.  I strongly suspect that it will render the stub unusable
>with current versions of FSF GDB.  Your tools add an explicit size to
>the packet and the community tools do not; so when they probe for and
>discover the P packet, they will probably try to use it and get
>confused.  That's why I'd like to discuss this on the GDB list first.
>  
>

I don't see why it wouldn't work:

1) Existing FSF gdb doesn't use 'p' yet anyway - it will continue to 
work as before, using the 'g' request to fetch all the registers.

2) If and when gdb does use 'p', then there's still no problem - if the 
kernel gdb stub sees a 'p' request without the ":SIZE" extension, it can 
just treat it like the FSF protocol and use the "default" register size.

Nigel

Re: GDB patch

[Daniel Jacobowitz]

On Wed, Dec 11, 2002 at 05:24:44PM +0000, Nigel Stephens wrote:
> Daniel Jacobowitz wrote:
> 
> >>Certainly 'p' is the logical inverse of 'P', so we'll change our gdb 
> >>remote stub to use that. So how about accepting Carsten's change, with 
> >>the 'R' case removed, and 'r' changed to 'p'?
> >>   
> >>
> >
> >Can't do it.  I strongly suspect that it will render the stub unusable
> >with current versions of FSF GDB.  Your tools add an explicit size to
> >the packet and the community tools do not; so when they probe for and
> >discover the P packet, they will probably try to use it and get
> >confused.  That's why I'd like to discuss this on the GDB list first.
> > 
> >
> 
> I don't see why it wouldn't work:
> 
> 1) Existing FSF gdb doesn't use 'p' yet anyway - it will continue to 
> work as before, using the 'g' request to fetch all the registers.
> 
> 2) If and when gdb does use 'p', then there's still no problem - if the 
> kernel gdb stub sees a 'p' request without the ":SIZE" extension, it can 
> just treat it like the FSF protocol and use the "default" register size.

3) Existing FSF gdb does use 'P' when it is available.  This does not
work with Carsten's patch.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

GDB patch

[Carsten Langgaard]

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

I've attached a patch for gdb-stub.c to make it work better with the
sde-gdb.
These changes should be backwards compatible with a standard gdb, so it
shouldn't break anything.
Ralf, could you please apply it.

/Carsten

--
_    _ ____  ___   Carsten Langgaard   Mailto:carstenl@mips.com
|\  /|||___)(___   MIPS Denmark        Direct: +45 4486 5527
| \/ |||    ____)  Lautrupvang 4B      Switch: +45 4486 5555
  TECHNOLOGIES     2750 Ballerup       Fax...: +45 4486 5556
                   Denmark             http://www.mips.com



[-- Attachment #2: gdb-stub.patch --]
[-- Type: text/plain, Size: 3896 bytes --]

Index: arch/mips/kernel/gdb-stub.c
===================================================================
RCS file: /home/cvs/linux/arch/mips/kernel/gdb-stub.c,v
retrieving revision 1.15.2.3
diff -u -r1.15.2.3 gdb-stub.c
--- arch/mips/kernel/gdb-stub.c	7 Nov 2002 01:47:45 -0000	1.15.2.3
+++ arch/mips/kernel/gdb-stub.c	10 Dec 2002 11:50:35 -0000
@@ -166,7 +166,7 @@
  * BUFMAX defines the maximum number of characters in inbound/outbound buffers
  * at least NUMREGBYTES*2 are needed for register packets
  */
-#define BUFMAX 2048
+#define BUFMAX 8192
 
 static char input_buffer[BUFMAX];
 static char output_buffer[BUFMAX];
@@ -218,7 +218,7 @@
 		 * now, read until a # or end of buffer is found
 		 */
 		while (count < BUFMAX) {
-			ch = getDebugChar() & 0x7f;
+			ch = getDebugChar();
 			if (ch == '#')
 				break;
 			checksum = checksum + ch;
@@ -324,19 +324,33 @@
  * may_fault is non-zero if we are reading from arbitrary memory, but is currently
  * not used.
  */
-static char *hex2mem(char *buf, char *mem, int count, int may_fault)
+static char *hex2mem(char *buf, char *mem, int count, int binary, int may_fault)
 {
 	int i;
 	unsigned char ch;
+	char *startadr = mem;
 
 	for (i=0; i<count; i++)
 	{
-		ch = hex(*buf++) << 4;
-		ch |= hex(*buf++);
+		if (binary) {
+			ch = *buf++;
+			if (ch == 0x7d)
+				ch = 0x20 ^ *buf++;
+		}
+		else {
+			ch = hex(*buf++) << 4;
+			ch |= hex(*buf++);
+		}
 		if (kgdb_write_byte(ch, mem++) != 0)
 			return 0;
 	}
 
+	/* 
+	 * Since we may have written to instruction space via 
+	 * the data path, the icache needs to be flushed here.
+	 */
+	flush_icache_range(startadr, count);
+
 	return mem;
 }
 
@@ -594,6 +608,7 @@
 	int length;
 	char *ptr;
 	unsigned long *stack;
+	int bflag;
 
 #if 0
 	printk("in handle_exception()\n");
@@ -695,6 +710,7 @@
 	 * Wait for input from remote GDB
 	 */
 	while (1) {
+		bflag = 0;
 		output_buffer[0] = 0;
 		getpacket(input_buffer);
 
@@ -767,6 +783,13 @@
 			break;
 
 		/*
+		 * XAA..AA,LLLL: Write LLLL escaped binary bytes at address AA.AA
+		 */
+		case 'X':
+			bflag = 1;
+			/* fall through */
+
+		/*
 		 * MAA..AA,LLLL: Write LLLL bytes at address AA.AA return OK
 		 */
 		case 'M':
@@ -776,7 +799,7 @@
 				&& *ptr++ == ','
 				&& hexToInt(&ptr, &length)
 				&& *ptr++ == ':') {
-				if (hex2mem(ptr, (char *)addr, length, 1))
+				if (hex2mem(ptr, (char *)addr, length, bflag, 1))
 					strcpy(output_buffer, "OK");
 				else
 					strcpy(output_buffer, "E03");
@@ -816,13 +839,64 @@
 		case 'k' :
 			break;		/* do nothing */
 
+		case 'R':
+			/* RNN[:SS],	Set the value of CPU register NN (size SS) */
+			/* FALL THROUGH */
+
+		case 'P':
+			/* PNN[:SS]=	Set the value of CPU register NN (size SS) */
+		{
+			int regno, regsize = 4, regval;
+			ptr = &input_buffer[1];
+
+			if (!hexToInt (&ptr, &regno))
+				goto error;
+
+			ptr++;
+			if (!hexToInt (&ptr, &regsize))
+				goto error;
+
+			if (regsize != 4)
+				goto error;
+			if (*ptr != ((input_buffer[0] == 'P') ? '=' : ','))
+				goto error;
+			ptr++;
+			if (!hex2mem (ptr, (char *)&regval, 4, 0, 0))
+				goto error;
+
+			memcpy ((char *)&regs->reg0+regno*4, &regval, 4);
+			strcpy (output_buffer, "OK");
+		}
+		break;
 
-		/*
-		 * Reset the whole machine (FIXME: system dependent)
-		 */
 		case 'r':
-			break;
+			/* rNN[:SS]	Return the value of CPU register NN (size SS) */
+		{
+			int regno, regsize = 4;
+			ptr = &input_buffer[1];
+			if (hexToInt (&ptr, &regno)) {
+				if (*ptr == ':') {
+					ptr++;
+					if (!hexToInt (&ptr, &regsize))
+						goto error;
+				}
+
+				if (regsize != 4)
+					goto error;
+				(void) mem2hex((char *)&regs->reg0+regno*4, 
+					       output_buffer, 4, 0);
+			}
+			else {
+			error:
+				strcpy(output_buffer,"E22 invalid arguments");
+			}
+		}
+		break;
 
+		case 'D':
+			putpacket("OK");
+			return;
+			/* NOTREACHED */
 
 		/*
 		 * Step to next instruction

Re: GDB patch

[Daniel Jacobowitz]

On Tue, Dec 10, 2002 at 01:07:31PM +0100, Carsten Langgaard wrote:
> I've attached a patch for gdb-stub.c to make it work better with the
> sde-gdb.
> These changes should be backwards compatible with a standard gdb, so it
> shouldn't break anything.
> Ralf, could you please apply it.

Strongly object.  While I didn't check the implementation, it's nice to
see 'X' implemented.  And P.  But what the heck is this?

> @@ -816,13 +839,64 @@
>  		case 'k' :
>  			break;		/* do nothing */
>  
> +		case 'R':
> +			/* RNN[:SS],	Set the value of CPU register NN (size SS) */
> +			/* FALL THROUGH */

> -		/*
> -		 * Reset the whole machine (FIXME: system dependent)
> -		 */
>  		case 'r':
> -			break;
> +			/* rNN[:SS]	Return the value of CPU register NN (size SS) */


We're not making up a protocol here, we're implementing one.  R and r
don't have anything to do with setting registers.

> +		case 'D':
> +			putpacket("OK");
> +			return;
> +			/* NOTREACHED */
>  
>  		/*
>  		 * Step to next instruction

'D' should generally resume the machine, by the way.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer