Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect /detach

From: George Anzinger
Date: Thu Feb 26 2004 - 18:39:22 EST

Amit S. Kale wrote:
On Thursday 26 Feb 2004 3:23 am, Tom Rini wrote:

The following patch fixes a number of little issues here and there, and
ends up making things more robust.
- We don't need kgdb_might_be_resumed or kgdb_killed_or_detached.
GDB attaching is GDB attaching, we haven't preserved any of the
previous context anyhow.


If gdb is restarted, kgdb has to remove all breakpoints. Present kgdb does that in the code this patch removes:

- if (remcom_in_buffer[0] == 'H' && remcom_in_buffer[1] == 'c') {
- remove_all_break();
- atomic_set(&kgdb_killed_or_detached, 0);
- ok_packet(remcom_out_buffer);

If we don't remove breakpoints, they stay in kgdb without gdb not knowing it and causes consistency problems.

I wonder if this is worth the trouble. Does kgdb need to know about breakpoints at all? Is there some other reason it needs to track them?


- Don't try and look for a connection in put_packet, after we've tried
to put a packet. Instead, when we receive a packet, GDB has
connected.


We have to check for gdb connection in putpacket or else following problem occurs.

1. kgdb console messages are to be put.
2. gdb dies
3. putpacket writes the packet and waits for a '+'

Oops! Tom, this '+' will be sent under interrupt and while kgdb is not connected. Looks like it needs to be passed through without causing a breakpoint. Possible salvation if we disable interrupts while waiting for the '+' but I don't think that is a good idea.

-g

4. new gdb sends a protocol initialization packet
5. putpacket reads characters in that packet hoping for an incoming '+' sending out console message packet on each incoming character
6. gdb receives and rejects each console message packet


- Remove ok_packet(), excessive, IMHO.


ok_packet is better than littering "OK" all over the place.

Other lindent changes are good.

-Amit


# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or
higher. # This patch includes the following deltas:
# ChangeSet 1.1664 -> 1.1665
# arch/ppc/8260_io/uart.c 1.31 -> 1.32
# kernel/kgdb.c 1.4 -> 1.5
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/02/25 trini@xxxxxxxxxxxxxxxxxxx 1.1665
# - Kill kgdb_might_be_resumed and kgdb_killed_or_detached.
# - Spacing and comments.
# - put_packet doesn't try and check for a connect now.
# - Kill ok_packet().
# - Only send an initial packet in kgdb_handle_exception if
# kgdb has connected already.
# --------------------------------------------
#
diff -Nru a/arch/ppc/8260_io/uart.c b/arch/ppc/8260_io/uart.c
--- a/arch/ppc/8260_io/uart.c Wed Feb 25 14:21:38 2004
+++ b/arch/ppc/8260_io/uart.c Wed Feb 25 14:21:38 2004
@@ -396,14 +396,8 @@
#ifdef CONFIG_KGDB
if (info->state->smc_scc_num == KGDB_SER_IDX) {
while (i-- > 0) {
- if (*cp == 0x03) {
+ if (*cp == 0x03 || *cp == '$') {
breakpoint();
- return;
- }
- if (*cp == '$') {
- atomic_set(&kgdb_might_be_resumed, 1);
- breakpoint();
- atomic_set(&kgdb_might_be_resumed, 0);
return;
}
cp++;
diff -Nru a/kernel/kgdb.c b/kernel/kgdb.c
--- a/kernel/kgdb.c Wed Feb 25 14:21:38 2004
+++ b/kernel/kgdb.c Wed Feb 25 14:21:38 2004
@@ -15,6 +15,7 @@
* Copyright (C) 2002-2004 Timesys Corporation
* Copyright (C) 2003-2004 Amit S. Kale
* Copyright (C) 2004 Pavel Machek <pavel@xxxxxxx>
+ * Copyright (C) 2004 Tom Rini <trini@xxxxxxxxxxxxxxxxxxx>
*
* Restructured KGDB for 2.6 kernels.
* thread support, support for multiple processors,support for ia-32(x86)
@@ -87,8 +88,6 @@
static spinlock_t slavecpulocks[KGDB_MAX_NO_CPUS];
static volatile int procindebug[KGDB_MAX_NO_CPUS];
atomic_t kgdb_setting_breakpoint;
-atomic_t kgdb_killed_or_detached;
-atomic_t kgdb_might_be_resumed;
struct task_struct *kgdb_usethread, *kgdb_contthread;

int debugger_step;
@@ -212,8 +211,10 @@
char ch;

do {
- /* wait around for the start character, ignore all other characters */
- while ((ch = (kgdb_serial->read_char() & 0x7f)) != '$') ;
+ /* wait around for the start character, ignore all other
+ * characters */
+ while ((ch = (kgdb_serial->read_char() & 0x7f)) != '$')
+ ; /* Spin. */
kgdb_connected = 1;
checksum = 0;
xmitcsum = -1;
@@ -249,27 +250,22 @@
* Send the packet in buffer.
* Check for gdb connection if asked for.
*/
-static void put_packet(char *buffer, int checkconnect)
+static void put_packet(char *buffer)
{
unsigned char checksum;
int count;
char ch;
- static char gdbseq[] = "$Hc-1#09";
- int i;
- int send_count;

/* $<packet info>#<checksum>. */
do {
kgdb_serial->write_char('$');
checksum = 0;
count = 0;
- send_count = 0;

while ((ch = buffer[count])) {
kgdb_serial->write_char(ch);
checksum += ch;
count++;
- send_count++;
}

kgdb_serial->write_char('#');
@@ -277,30 +273,7 @@
kgdb_serial->write_char(hexchars[checksum % 16]);
if (kgdb_serial->flush)
kgdb_serial->flush();
-
- i = 0;
- while ((ch = kgdb_serial->read_char()) == gdbseq[i++] &&
- checkconnect) {
- if (!gdbseq[i]) {
- kgdb_serial->write_char('+');
- if (kgdb_serial->flush)
- kgdb_serial->flush();
- breakpoint();
-
- /*
- * GDB is available now.
- * Retransmit this packet.
- */
- break;
- }
- }
- if (checkconnect && ch == 3) {
- kgdb_serial->write_char('+');
- if (kgdb_serial->flush)
- kgdb_serial->flush();
- breakpoint();
- }
- } while ((ch & 0x7f) != '+');
+ } while ((kgdb_serial->read_char() & 0x7f) != '+');

}

@@ -427,11 +400,6 @@
pkt[3] = '\0';
}

-static void ok_packet(char *pkt)
-{
- strcpy(pkt, "OK");
-}
-
static char *pack_threadid(char *pkt, threadref * id)
{
char *limit;
@@ -502,7 +470,8 @@
procindebug[processor] = 1;
current->thread.debuggerinfo = regs;

- /* Wait till master processor goes completely into the debugger. FIXME:
this looks racy */ + /* Wait till master processor goes completely into the
debugger. + * FIXME: this looks racy */
while (!procindebug[atomic_read(&debugger_active) - 1]) {
int i = 10; /* an arbitrary number */

@@ -701,17 +670,7 @@
/* Master processor is completely in the debugger */
kgdb_post_master_code(linux_regs, exVector, err_code);

- if (atomic_read(&kgdb_killed_or_detached) &&
- atomic_read(&kgdb_might_be_resumed)) {
- get_packet(remcom_in_buffer);
- if (remcom_in_buffer[0] == 'H' && remcom_in_buffer[1] == 'c') {
- remove_all_break();
- atomic_set(&kgdb_killed_or_detached, 0);
- ok_packet(remcom_out_buffer);
- } else
- return 1;
- } else {
-
+ if (kgdb_connected) {
/* reply to host that an exception has occurred */
ptr = remcom_out_buffer;
*ptr++ = 'T';
@@ -721,11 +680,9 @@
int_to_threadref(&thref, shadow_pid(current->pid));
ptr = pack_threadid(ptr, &thref);
*ptr++ = ';';
- *ptr = '\0';
- }

- put_packet(remcom_out_buffer, 0);
- kgdb_connected = 1;
+ put_packet(remcom_out_buffer);
+ }

kgdb_usethread = current;
kgdb_usethreadid = shadow_pid(current->pid);
@@ -798,7 +755,7 @@
else {
gdb_regs_to_regs(gdb_regs, (struct pt_regs *)
current->thread.debuggerinfo);
- ok_packet(remcom_out_buffer);
+ strcpy(remcom_out_buffer, "OK");
}

break;
@@ -838,10 +795,10 @@
if ((error = remove_all_break()) < 0) {
error_packet(remcom_out_buffer, error);
} else {
- ok_packet(remcom_out_buffer);
+ strcpy(remcom_out_buffer, "OK");
kgdb_connected = 0;
}
- put_packet(remcom_out_buffer, 0);
+ put_packet(remcom_out_buffer);
goto default_handle;

case 'k':
@@ -947,11 +904,10 @@
}
kgdb_usethread = thread;
kgdb_usethreadid = threadid;
- ok_packet(remcom_out_buffer);
+ strcpy(remcom_out_buffer, "OK");
break;

case 'c':
- atomic_set(&kgdb_killed_or_detached, 0);
ptr = &remcom_in_buffer[2];
kgdb_hex2long(&ptr, &threadid);
if (!threadid) {
@@ -966,7 +922,7 @@
}
kgdb_contthread = thread;
}
- ok_packet(remcom_out_buffer);
+ strcpy(remcom_out_buffer, "OK");
break;
}
break;
@@ -977,7 +933,7 @@
kgdb_hex2long(&ptr, &threadid);
thread = getthread(linux_regs, threadid);
if (thread)
- ok_packet(remcom_out_buffer);
+ strcpy(remcom_out_buffer, "OK");
else
error_packet(remcom_out_buffer, -EINVAL);
break;
@@ -1018,7 +974,7 @@
}

if (error == 0)
- ok_packet(remcom_out_buffer);
+ strcpy(remcom_out_buffer, "OK");
else
error_packet(remcom_out_buffer, error);

@@ -1039,7 +995,7 @@
} /* switch */

/* reply to the request */
- put_packet(remcom_out_buffer, 0);
+ put_packet(remcom_out_buffer);
}

kgdb_exit:
@@ -1063,7 +1019,6 @@
}

/* Free debugger_active */
- atomic_set(&kgdb_killed_or_detached, 1);
atomic_set(&debugger_active, 0);
local_irq_restore(flags);

@@ -1132,12 +1087,6 @@
/* Free debugger_active */
atomic_set(&debugger_active, 0);

- /* This flag is used, if gdb has detached and wants to start
- * another session
- */
- atomic_set(&kgdb_killed_or_detached, 1);
- atomic_set(&kgdb_might_be_resumed, 0);
-
for (i = 0; i < MAX_BREAKPOINTS; i++)
kgdb_break[i].state = bp_disabled;

@@ -1222,7 +1171,7 @@
*bufptr = '\0';
s += wcount;

- put_packet(kgdbconbuf, 1);
+ put_packet(kgdbconbuf);

}
local_irq_restore(flags);


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/