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

From: Amit S. Kale
Date: Mon Mar 01 2004 - 03:38:10 EST

Next message: Nick Piggin: "Re: MM VM patches was: 2.6.3-mm4"
Previous message: Amit S. Kale: "Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint"
In reply to: Amit S. Kale: "Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach"
Next in thread: Tom Rini: "Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Friday 27 Feb 2004 5:00 am, George Anzinger wrote:
> 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?

kgdb tracks breakpoints in the cvs version.

gdb many a times has a shorter life compared to kgdb (unfortunate but true).
If gdb dies when breakpoints are present in a kernel, the kernel too has to
be killed as there is no way to correct the kernel code modified by gdb.

>
> >>- 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.

Yes. That's why I added a paramter to putpacket to skip causing a breakpoint
when not required.

-Amit

>
> -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/

-
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/

Next message: Nick Piggin: "Re: MM VM patches was: 2.6.3-mm4"
Previous message: Amit S. Kale: "Re: [Kgdb-bugreport] [PATCH][2/3] Update CVS KGDB's have kgdb_{schedule,process}_breakpoint"
In reply to: Amit S. Kale: "Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach"
Next in thread: Tom Rini: "Re: [Kgdb-bugreport] [PATCH][3/3] Update CVS KGDB's wrt connect / detach"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]