[KGDB][RFC] Send a fuller T packet

[KGDB][RFC] Send a fuller T packet

[Tom Rini]

Hello.  Since a 'T' packet is allowed to send back information on an
arbitrary number of registers, and on PPC32 we've always been including
information on the stack pointer and program counter, I was wondering
what people thought of the following patch:

diff -u linux-2.6.3/include/asm-x86_64/kgdb.h linux-2.6.3/include/asm-x86_64/kgdb.h
--- linux-2.6.3/include/asm-x86_64/kgdb.h	2004-02-27 11:30:37.445782703 -0700
+++ linux-2.6.3/include/asm-x86_64/kgdb.h	2004-03-02 14:42:47.854532793 -0700
@@ -48,6 +48,10 @@
 /* Number of bytes of registers.  */
 #define NUMREGBYTES (_LASTREG*8)
 
+#define PC_REGNUM	_PC	/* Program Counter */
+#define SP_REGNUM	_RSP	/* Stack Pointer */
+#define PTRACE_PC	rip	/* Program Counter, in ptrace regs. */
+
 #define BREAKPOINT() asm("   int $3");
 #define BREAK_INSTR_SIZE       1
 
diff -u linux-2.6.3/include/asm-i386/kgdb.h linux-2.6.3/include/asm-i386/kgdb.h
--- linux-2.6.3/include/asm-i386/kgdb.h	2004-02-26 13:14:41.769186750 -0700
+++ linux-2.6.3/include/asm-i386/kgdb.h	2004-03-02 14:42:03.232624041 -0700
@@ -43,6 +43,10 @@
 	_GS			/* 15 */
 };
 
+#define PC_REGNUM	_PC	/* Program Counter */
+#define SP_REGNUM	_ESP	/* Stack Pointer */
+#define PTRACE_PC	eip	/* Program Counter, in ptrace regs. */
+
 #define BREAKPOINT() asm("   int $3");
 #define BREAK_INSTR_SIZE       1
 
diff -u linux-2.6.3/kernel/kgdb.c linux-2.6.3/kernel/kgdb.c
--- linux-2.6.3/kernel/kgdb.c	2004-03-02 14:25:42.590401068 -0700
+++ linux-2.6.3/kernel/kgdb.c	2004-03-02 14:51:51.535627684 -0700
@@ -695,12 +695,24 @@
 	/* Master processor is completely in the debugger */
 	kgdb_post_master_code(linux_regs, exVector, err_code);
 
+	/* If kgdb is connected, then an exception has occured, and
+	 * we need to pass something back to GDB. */
 	if (kgdb_connected) {
-		/* reply to host that an exception has occurred */
 		ptr = remcom_out_buffer;
 		*ptr++ = 'T';
 		*ptr++ = hexchars[(signo >> 4) % 16];
 		*ptr++ = hexchars[signo % 16];
+		*ptr++ = hexchars[(PC_REGNUM >> 4) % 16];
+		*ptr++ = hexchars[PC_REGNUM % 16];
+		*ptr++ = ':';
+		ptr = kgdb_mem2hex((char *)&linux_regs->PTRACE_PC, ptr, 4);
+		*ptr++ = ';';
+		*ptr++ = hexchars[SP_REGNUM >> 4];
+		*ptr++ = hexchars[SP_REGNUM & 0xf];
+		*ptr++ = ':';
+		ptr = kgdb_mem2hex(((char *)linux_regs) + SP_REGNUM * 4, ptr,
+				4);
+		*ptr++ = ';';
 		ptr += strlen(strcpy(ptr, "thread:"));
 		int_to_threadref(&thref, shadow_pid(current->pid));
 		ptr = pack_threadid(ptr, &thref);
@@ -728,11 +740,28 @@
 			 * we clear out our breakpoints now incase
 			 * GDB is reconnecting. */
 			remove_all_break();
-			remcom_out_buffer[0] = 'S';
-			remcom_out_buffer[1] = hexchars[signo >> 4];
-			remcom_out_buffer[2] = hexchars[signo % 16];
+			/* reply to host that an exception has occurred */
+			ptr = remcom_out_buffer;
+			*ptr++ = 'T';
+			*ptr++ = hexchars[(signo >> 4) % 16];
+			*ptr++ = hexchars[signo % 16];
+			*ptr++ = hexchars[(PC_REGNUM >> 4) % 16];
+			*ptr++ = hexchars[PC_REGNUM % 16];
+			*ptr++ = ':';
+			ptr = kgdb_mem2hex((char *)&linux_regs->PTRACE_PC, ptr,
+					4);
+			*ptr++ = ';';
+			*ptr++ = hexchars[SP_REGNUM >> 4];
+			*ptr++ = hexchars[SP_REGNUM & 0xf];
+			*ptr++ = ':';
+			ptr = kgdb_mem2hex(((char *)linux_regs) + SP_REGNUM * 4,
+					ptr, 4);
+			*ptr++ = ';';
+			ptr += strlen(strcpy(ptr, "thread:"));
+			int_to_threadref(&thref, shadow_pid(current->pid));
+			ptr = pack_threadid(ptr, &thref);
+			*ptr++ = ';';
 			break;
-
 		case 'g':	/* return the value of the CPU registers */
 			thread = kgdb_usethread;
 

Is this too much extra stuff to bother with, since that information can
be gotten elsewhere?

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [Kgdb-bugreport] [KGDB][RFC] Send a fuller T packet

[George Anzinger]

Tom Rini wrote:
> Hello.  Since a 'T' packet is allowed to send back information on an
> arbitrary number of registers, and on PPC32 we've always been including
> information on the stack pointer and program counter, I was wondering
> what people thought of the following patch:
> 
> diff -u linux-2.6.3/include/asm-x86_64/kgdb.h linux-2.6.3/include/asm-x86_64/kgdb.h
> --- linux-2.6.3/include/asm-x86_64/kgdb.h	2004-02-27 11:30:37.445782703 -0700
> +++ linux-2.6.3/include/asm-x86_64/kgdb.h	2004-03-02 14:42:47.854532793 -0700
> @@ -48,6 +48,10 @@
>  /* Number of bytes of registers.  */
>  #define NUMREGBYTES (_LASTREG*8)
>  
> +#define PC_REGNUM	_PC	/* Program Counter */
> +#define SP_REGNUM	_RSP	/* Stack Pointer */
> +#define PTRACE_PC	rip	/* Program Counter, in ptrace regs. */

I would really like to keep this stuff out of kgdb.h since it may be included by 
the user to pick up the BREAKPOINT() (which, by the way we should standardize as 
I note that here it has () while not on the current x86).

Isn't there a kgdb_local.h which is used only by kdgd and friends?  We really do 
want to keep the name space as clean as possible to prevent possible conflicts.
> +
>  #define BREAKPOINT() asm("   int $3");
>  #define BREAK_INSTR_SIZE       1
>  
> diff -u linux-2.6.3/include/asm-i386/kgdb.h linux-2.6.3/include/asm-i386/kgdb.h
> --- linux-2.6.3/include/asm-i386/kgdb.h	2004-02-26 13:14:41.769186750 -0700
> +++ linux-2.6.3/include/asm-i386/kgdb.h	2004-03-02 14:42:03.232624041 -0700
> @@ -43,6 +43,10 @@
>  	_GS			/* 15 */
>  };
>  
> +#define PC_REGNUM	_PC	/* Program Counter */
> +#define SP_REGNUM	_ESP	/* Stack Pointer */
> +#define PTRACE_PC	eip	/* Program Counter, in ptrace regs. */
> +
>  #define BREAKPOINT() asm("   int $3");
>  #define BREAK_INSTR_SIZE       1
>  
> diff -u linux-2.6.3/kernel/kgdb.c linux-2.6.3/kernel/kgdb.c
> --- linux-2.6.3/kernel/kgdb.c	2004-03-02 14:25:42.590401068 -0700
> +++ linux-2.6.3/kernel/kgdb.c	2004-03-02 14:51:51.535627684 -0700
> @@ -695,12 +695,24 @@
>  	/* Master processor is completely in the debugger */
>  	kgdb_post_master_code(linux_regs, exVector, err_code);
>  
> +	/* If kgdb is connected, then an exception has occured, and
> +	 * we need to pass something back to GDB. */
>  	if (kgdb_connected) {
> -		/* reply to host that an exception has occurred */
>  		ptr = remcom_out_buffer;
>  		*ptr++ = 'T';
>  		*ptr++ = hexchars[(signo >> 4) % 16];
>  		*ptr++ = hexchars[signo % 16];
> +		*ptr++ = hexchars[(PC_REGNUM >> 4) % 16];
> +		*ptr++ = hexchars[PC_REGNUM % 16];
> +		*ptr++ = ':';
> +		ptr = kgdb_mem2hex((char *)&linux_regs->PTRACE_PC, ptr, 4);
> +		*ptr++ = ';';
> +		*ptr++ = hexchars[SP_REGNUM >> 4];
> +		*ptr++ = hexchars[SP_REGNUM & 0xf];
> +		*ptr++ = ':';
> +		ptr = kgdb_mem2hex(((char *)linux_regs) + SP_REGNUM * 4, ptr,
> +				4);
> +		*ptr++ = ';';
>  		ptr += strlen(strcpy(ptr, "thread:"));
>  		int_to_threadref(&thref, shadow_pid(current->pid));
>  		ptr = pack_threadid(ptr, &thref);
> @@ -728,11 +740,28 @@
>  			 * we clear out our breakpoints now incase
>  			 * GDB is reconnecting. */
>  			remove_all_break();
> -			remcom_out_buffer[0] = 'S';
> -			remcom_out_buffer[1] = hexchars[signo >> 4];
> -			remcom_out_buffer[2] = hexchars[signo % 16];
> +			/* reply to host that an exception has occurred */
> +			ptr = remcom_out_buffer;
> +			*ptr++ = 'T';
> +			*ptr++ = hexchars[(signo >> 4) % 16];
> +			*ptr++ = hexchars[signo % 16];
> +			*ptr++ = hexchars[(PC_REGNUM >> 4) % 16];
> +			*ptr++ = hexchars[PC_REGNUM % 16];
> +			*ptr++ = ':';
> +			ptr = kgdb_mem2hex((char *)&linux_regs->PTRACE_PC, ptr,
> +					4);
> +			*ptr++ = ';';
> +			*ptr++ = hexchars[SP_REGNUM >> 4];
> +			*ptr++ = hexchars[SP_REGNUM & 0xf];
> +			*ptr++ = ':';
> +			ptr = kgdb_mem2hex(((char *)linux_regs) + SP_REGNUM * 4,
> +					ptr, 4);
> +			*ptr++ = ';';
> +			ptr += strlen(strcpy(ptr, "thread:"));
> +			int_to_threadref(&thref, shadow_pid(current->pid));
> +			ptr = pack_threadid(ptr, &thref);
> +			*ptr++ = ';';
>  			break;
> -
>  		case 'g':	/* return the value of the CPU registers */
>  			thread = kgdb_usethread;
>  
> 
> Is this too much extra stuff to bother with, since that information can
> be gotten elsewhere?
> 

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [Kgdb-bugreport] [KGDB][RFC] Send a fuller T packet

[Tom Rini]

On Tue, Mar 02, 2004 at 03:28:45PM -0800, George Anzinger wrote:

> Tom Rini wrote:
> >Hello.  Since a 'T' packet is allowed to send back information on an
> >arbitrary number of registers, and on PPC32 we've always been including
> >information on the stack pointer and program counter, I was wondering
> >what people thought of the following patch:
> >
> >diff -u linux-2.6.3/include/asm-x86_64/kgdb.h 
> >linux-2.6.3/include/asm-x86_64/kgdb.h
> >--- linux-2.6.3/include/asm-x86_64/kgdb.h	2004-02-27 
> >11:30:37.445782703 -0700
> >+++ linux-2.6.3/include/asm-x86_64/kgdb.h	2004-03-02 
> >14:42:47.854532793 -0700
> >@@ -48,6 +48,10 @@
> > /* Number of bytes of registers.  */
> > #define NUMREGBYTES (_LASTREG*8)
> > 
> >+#define PC_REGNUM	_PC	/* Program Counter */
> >+#define SP_REGNUM	_RSP	/* Stack Pointer */
> >+#define PTRACE_PC	rip	/* Program Counter, in ptrace regs. */
> 
> I would really like to keep this stuff out of kgdb.h since it may be 
> included by the user to pick up the BREAKPOINT() (which, by the way we 
> should standardize as I note that here it has () while not on the current 
> x86).

It's BREAKPOINT() everywhere:
$ grep BREAKPOINT include/asm-*/kgdb.h
include/asm-i386/kgdb.h:#define BREAKPOINT() asm("   int $3");
include/asm-ppc/kgdb.h:#define BREAKPOINT()             asm(".long 0x7d821008") /* twge r2, r2 */
include/asm-x86_64/kgdb.h:#define BREAKPOINT() asm("   int $3");

> Isn't there a kgdb_local.h which is used only by kdgd and friends?  We 
> really do want to keep the name space as clean as possible to prevent 
> possible conflicts.

The simple answer is you don't call BREAKPOINT() in your code anywhere.
You call breakpoint() or kgdb_schedule_breakpoint().
The split here is different in that <linux/kgdb.h> should be standalone
(it's not, _yet_).

But this is all an aside to my question. :)

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [Kgdb-bugreport] [KGDB][RFC] Send a fuller T packet

[George Anzinger]

Tom Rini wrote:
> On Tue, Mar 02, 2004 at 03:28:45PM -0800, George Anzinger wrote:
> 
> 
>>Tom Rini wrote:
>>
>>>Hello.  Since a 'T' packet is allowed to send back information on an
>>>arbitrary number of registers, and on PPC32 we've always been including
>>>information on the stack pointer and program counter, I was wondering
>>>what people thought of the following patch:
>>>
>>>diff -u linux-2.6.3/include/asm-x86_64/kgdb.h 
>>>linux-2.6.3/include/asm-x86_64/kgdb.h
>>>--- linux-2.6.3/include/asm-x86_64/kgdb.h	2004-02-27 
>>>11:30:37.445782703 -0700
>>>+++ linux-2.6.3/include/asm-x86_64/kgdb.h	2004-03-02 
>>>14:42:47.854532793 -0700
>>>@@ -48,6 +48,10 @@
>>>/* Number of bytes of registers.  */
>>>#define NUMREGBYTES (_LASTREG*8)
>>>
>>>+#define PC_REGNUM	_PC	/* Program Counter */
>>>+#define SP_REGNUM	_RSP	/* Stack Pointer */
>>>+#define PTRACE_PC	rip	/* Program Counter, in ptrace regs. */
>>
>>I would really like to keep this stuff out of kgdb.h since it may be 
>>included by the user to pick up the BREAKPOINT() (which, by the way we 
>>should standardize as I note that here it has () while not on the current 
>>x86).
> 
> 
> It's BREAKPOINT() everywhere:
Yeah, something you changed?  Oh well, I will just have to learn to put the "()" 
in :)
> $ grep BREAKPOINT include/asm-*/kgdb.h
> include/asm-i386/kgdb.h:#define BREAKPOINT() asm("   int $3");
> include/asm-ppc/kgdb.h:#define BREAKPOINT()             asm(".long 0x7d821008") /* twge r2, r2 */
> include/asm-x86_64/kgdb.h:#define BREAKPOINT() asm("   int $3");
> 
> 
>>Isn't there a kgdb_local.h which is used only by kdgd and friends?  We 
>>really do want to keep the name space as clean as possible to prevent 
>>possible conflicts.
> 
> 
> The simple answer is you don't call BREAKPOINT() in your code anywhere.
> You call breakpoint() or kgdb_schedule_breakpoint().

Uh, why?  Last I knew that was a real function.  Most of the time I just want a 
simple breakpoint.  I surly don't want the register dumps and such that a 
function call causes, not to mention that it may do something else that is not 
friendly.


> The split here is different in that <linux/kgdb.h> should be standalone
> (it's not, _yet_).

Yeah, but it will most likely include asm/kgdb.h....
> 
> But this is all an aside to my question. :)

Right, my answer on that is if it reduces the line traffic yes, if not, no. 
Because then it is just bloat.
> 

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

Re: [Kgdb-bugreport] [KGDB][RFC] Send a fuller T packet

[Amit S. Kale]

Hi,

Polution of kgdb.h is definitely bad.

I tried this code from Tom's bitkeeper tree some time back. It was incorrect 
in two aspects hence I took only the minimal 'T' packet.
1. It assumes 32 bit pc and sp.
2. sp is not equal to ((char *)linux_regs) + SP_REGNUM * 4 on powerpc.

A full 'T' packet is still a good idea because it saves a 'g' packet in 
following cases:
1. gdb internal breakpoints, like module_event.
2. conditional breakpoints.
3. tracepoints.

In general we can report an arbitrary number of registers in a 'T' packet.  
Reporting registers other than PC and SP is effectively making 'T' packet 
into a 'g' packet.

Architecture dependent code is the right place to compose the PC and SP part 
of a 'T' packet. Given a pt_regs pointer, an architecture dependent function 
can compose PC number, PC value, SP number and SP value, all of which are 
arch dependent. How about architecture dependent function:
int make_pcsp_packet(struct pt_regs *, char *buffer)

-Amit

On Wednesday 03 Mar 2004 5:52 am, George Anzinger wrote:
> Tom Rini wrote:
> > On Tue, Mar 02, 2004 at 03:28:45PM -0800, George Anzinger wrote:
> >>Tom Rini wrote:
> >>>Hello.  Since a 'T' packet is allowed to send back information on an
> >>>arbitrary number of registers, and on PPC32 we've always been including
> >>>information on the stack pointer and program counter, I was wondering
> >>>what people thought of the following patch:
> >>>
> >>>diff -u linux-2.6.3/include/asm-x86_64/kgdb.h
> >>>linux-2.6.3/include/asm-x86_64/kgdb.h
> >>>--- linux-2.6.3/include/asm-x86_64/kgdb.h	2004-02-27
> >>>11:30:37.445782703 -0700
> >>>+++ linux-2.6.3/include/asm-x86_64/kgdb.h	2004-03-02
> >>>14:42:47.854532793 -0700
> >>>@@ -48,6 +48,10 @@
> >>>/* Number of bytes of registers.  */
> >>>#define NUMREGBYTES (_LASTREG*8)
> >>>
> >>>+#define PC_REGNUM	_PC	/* Program Counter */
> >>>+#define SP_REGNUM	_RSP	/* Stack Pointer */
> >>>+#define PTRACE_PC	rip	/* Program Counter, in ptrace regs. */
> >>
> >>I would really like to keep this stuff out of kgdb.h since it may be
> >>included by the user to pick up the BREAKPOINT() (which, by the way we
> >>should standardize as I note that here it has () while not on the current
> >>x86).
> >
> > It's BREAKPOINT() everywhere:
>
> Yeah, something you changed?  Oh well, I will just have to learn to put the
> "()" in :)
>
> > $ grep BREAKPOINT include/asm-*/kgdb.h
> > include/asm-i386/kgdb.h:#define BREAKPOINT() asm("   int $3");
> > include/asm-ppc/kgdb.h:#define BREAKPOINT()             asm(".long
> > 0x7d821008") /* twge r2, r2 */ include/asm-x86_64/kgdb.h:#define
> > BREAKPOINT() asm("   int $3");
> >
> >>Isn't there a kgdb_local.h which is used only by kdgd and friends?  We
> >>really do want to keep the name space as clean as possible to prevent
> >>possible conflicts.
> >
> > The simple answer is you don't call BREAKPOINT() in your code anywhere.
> > You call breakpoint() or kgdb_schedule_breakpoint().
>
> Uh, why?  Last I knew that was a real function.  Most of the time I just
> want a simple breakpoint.  I surly don't want the register dumps and such
> that a function call causes, not to mention that it may do something else
> that is not friendly.
>
> > The split here is different in that <linux/kgdb.h> should be standalone
> > (it's not, _yet_).
>
> Yeah, but it will most likely include asm/kgdb.h....
>
> > But this is all an aside to my question. :)
>
> Right, my answer on that is if it reduces the line traffic yes, if not, no.
> Because then it is just bloat.

Re: [Kgdb-bugreport] [KGDB][RFC] Send a fuller T packet

[Pavel Machek]

Hi!


> > I would really like to keep this stuff out of kgdb.h since it may be 
> > included by the user to pick up the BREAKPOINT() (which, by the way we 
> > should standardize as I note that here it has () while not on the current 
> > x86).
> 
> It's BREAKPOINT() everywhere:
> $ grep BREAKPOINT include/asm-*/kgdb.h
> include/asm-i386/kgdb.h:#define BREAKPOINT() asm("   int $3");
> include/asm-ppc/kgdb.h:#define BREAKPOINT()             asm(".long 0x7d821008") /* twge r2, r2 */
> include/asm-x86_64/kgdb.h:#define BREAKPOINT() asm("   int $3");

Notice how it ends with ';' on everything but ppc. Perhaps it needs do
{ } while (0) wrapping?
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [Kgdb-bugreport] [KGDB][RFC] Send a fuller T packet

[Tom Rini]

On Wed, Mar 03, 2004 at 11:52:47AM +0100, Pavel Machek wrote:
> Hi!
> 
> 
> > > I would really like to keep this stuff out of kgdb.h since it may be 
> > > included by the user to pick up the BREAKPOINT() (which, by the way we 
> > > should standardize as I note that here it has () while not on the current 
> > > x86).
> > 
> > It's BREAKPOINT() everywhere:
> > $ grep BREAKPOINT include/asm-*/kgdb.h
> > include/asm-i386/kgdb.h:#define BREAKPOINT() asm("   int $3");
> > include/asm-ppc/kgdb.h:#define BREAKPOINT()             asm(".long 0x7d821008") /* twge r2, r2 */
> > include/asm-x86_64/kgdb.h:#define BREAKPOINT() asm("   int $3");
> 
> Notice how it ends with ';' on everything but ppc. Perhaps it needs do
> { } while (0) wrapping?

... not that PPC works right now :)  But yes, you're right.

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [Kgdb-bugreport] [KGDB][RFC] Send a fuller T packet

[George Anzinger]

Tom Rini wrote:
> On Wed, Mar 03, 2004 at 11:52:47AM +0100, Pavel Machek wrote:
> 
>>Hi!
>>
>>
>>
>>>>I would really like to keep this stuff out of kgdb.h since it may be 
>>>>included by the user to pick up the BREAKPOINT() (which, by the way we 
>>>>should standardize as I note that here it has () while not on the current 
>>>>x86).
>>>
>>>It's BREAKPOINT() everywhere:
>>>$ grep BREAKPOINT include/asm-*/kgdb.h
>>>include/asm-i386/kgdb.h:#define BREAKPOINT() asm("   int $3");
>>>include/asm-ppc/kgdb.h:#define BREAKPOINT()             asm(".long 0x7d821008") /* twge r2, r2 */
>>>include/asm-x86_64/kgdb.h:#define BREAKPOINT() asm("   int $3");
>>
>>Notice how it ends with ';' on everything but ppc. Perhaps it needs do
>>{ } while (0) wrapping?
> 
> 
> ... not that PPC works right now :)  But yes, you're right.
> 
I agree, lets force the ";".
-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml