From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29194 invoked by alias); 26 Sep 2009 03:43:58 -0000 Received: (qmail 29176 invoked by uid 22791); 26 Sep 2009 03:43:57 -0000 X-SWARE-Spam-Status: No, hits=-2.2 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail-px0-f202.google.com (HELO mail-px0-f202.google.com) (209.85.216.202) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 26 Sep 2009 03:43:50 +0000 Received: by pxi40 with SMTP id 40so4032880pxi.24 for ; Fri, 25 Sep 2009 20:43:48 -0700 (PDT) Received: by 10.114.162.20 with SMTP id k20mr904962wae.135.1253936628507; Fri, 25 Sep 2009 20:43:48 -0700 (PDT) Received: from xpjpn (pool-71-111-147-240.ptldor.dsl-w.verizon.net [71.111.147.240]) by mx.google.com with ESMTPS id 20sm853795pxi.4.2009.09.25.20.43.46 (version=SSLv3 cipher=RC4-MD5); Fri, 25 Sep 2009 20:43:46 -0700 (PDT) From: Caz Yokoyama To: "'Joel Brobecker'" Cc: "'Pedro Alves'" , References: <409D09C1E1964C5EAFF5EFBAD6E936ED@xpjpn> <200905152223.58241.pedro@codesourcery.com> <20090515213410.GA10064@caradoc.them.org> <8AA4B846934A4A9081F778449B96F416@xpjpn> <4A0DE914.1050800@vmware.com> <20090923004802.GA20859@adacore.com> <9ECED0F0DCF04CC185B027503876430D@xpjpn> <20090925160627.GB5077@adacore.com> Subject: RE: symbolic debug of loadable modules with kgdb light Date: Sat, 26 Sep 2009 03:43:00 -0000 Message-ID: <66E35EA6599040F894D040E4F50389D0@xpjpn> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----=_NextPart_000_03A6_01CA3E21.0194C3D0" In-Reply-To: <20090925160627.GB5077@adacore.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-09/txt/msg00830.txt.bz2 This is a multi-part message in MIME format. ------=_NextPart_000_03A6_01CA3E21.0194C3D0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-length: 5281 Hello Joel, I revised my patch. As I wrote in ChangeLog, I expanded interrupt_sequence to arbitrary characters include ^C and BREAK instead of 3 choices. -caz -----Original Message----- From: Joel Brobecker [mailto:brobecker@adacore.com] Sent: Friday, September 25, 2009 9:06 AM To: Caz Yokoyama Cc: 'Pedro Alves'; gdb-patches@sourceware.org Subject: Re: symbolic debug of loadable modules with kgdb light Caz, A couple of opening remarks: Is there any way you can quote emails using the standard '>' or '|' instead of using using '------------------'. This convention makes it harder to separate my text from your replies. The second remark is that the other Global Maintainers that I talked to about this, felt that sending (or not) the interrupt sequence when connecting to the target should be done independently of the actual interruption sequence. We all really like Daniel's (drow) suggestion: set/show remote interrupt-sequence set/show remote interrupt-after-connection [on|off] If "set remote interrupt-sequence interrupt" sounds weird, then perhaps we should go back to your initial suggestion of using "control-c". The last remark is that some of us felt that it was unusual to introduce some Linux-specific code in remote.c, and that this could be easily handled in a netcat-like wrapper. You'd then do: (gdb) target remote | kernel-wrapper /foo/device What do you think? We're not opposed to your patch, but the wrapper does have the advantage of allowing you to work with any debugger, including older versions of it :). > BREAK is usually all capitalized in chip manual because it is high level of > serial line for certain time. But I don't care weather capitalized or not. OK, let's use BREAK then. > +2009-09-23 Kazuyoshi Caz Yokoyama > + > + * remote.c: Allow the user to select one of ^C, a break or > + Magic SysRq g as the sequence to the remote in order to > + interrupt the execution. You'll have to be a little more detailed in the ChangeLog entry. Have a look at the ChangeLog file for tons of example on how we are expected to write them. > +/* Allow the user to specify what sequence to send to the remote > + when he requests a program interruption: Although ^C is usually > + what remote systems expect (this is the default, here), it is > + sometimes preferable to send a break. On other systems such > + as the Linux kernel, a break followed by g, which is Magic SysRq g > + is required in order to interrupt the execution. */ > +const char remote_break_interrupt[] = "interrutpt"; [etc] Can you change the name of this option and add the second option as per above? Also, still in line with having these two new commands, this makes the old "set remotedebug" command deprecated. To help users transition to the new set of commands, can you do the following: If the user tries to change the behavior using "set remotebreak", then issue a warning that this command is deprecated in favor of "set remote interrupt-...", and then change the set remote interrupt-sequence setting accordingly. If the user types "show remotebreak", then just emit the deprecated warning, but then either print nothing, or print the output that "show remote interrupt-sequence" would print. To achieve this, you will need: deprecate_cmd, and you'll need to use the set_fun/show_func arguments of the add_..._cmd function. > + fprintf_filtered (file, > + ("You are sending %s to the remote target " > + "which is not expected."), remote_break_mode); Let's replace that with an internal_error. > + /* Send the Magic SysRg g sequence in order to interrupt ^^^^^ typo: SysRq > + the execution of Linux kernel. */ > + if (remote_break_mode == remote_break_sysrq_g) > + { > + serial_send_break (remote_desc); > + serial_write (remote_desc, "g", 1); This block needs to be conditionalized on the second option. > + /* Send ^C, a break or a break g, depending on user preference. */ > + if (remote_break_mode == remote_break_interrupt) > + { > + serial_write (remote_desc, "\003", 1); > + } > + else if (remote_break_mode == remote_break_break) > + { > + serial_send_break (remote_desc); > + } If the if/while/etc block only contains one statement, then we prefer that the curly braces NOT be used. Can you remove them, please? > =================================================================== > RCS file: /cvs/src/src/gdb/remote.h,v > retrieving revision 1.17 > diff -u -r1.17 remote.h > --- gdb/remote.h 3 Jan 2009 05:57:53 -0000 1.17 > +++ gdb/remote.h 24 Sep 2009 22:39:33 -0000 > @@ -77,4 +77,7 @@ > > int remote_filename_p (const char *filename); > > +extern const char remote_break_sysrq_g[]; > +extern const char *remote_break_mode; > + > #endif I don't think you meant to make these changes (and they are not described in the ChangeLog). Just make sure you undo them in your local sandbox, to make sure you don't accidently check them in. > Index: gdb/doc/gdb.texinfo The documentation patch is reviewed by Eli. Can you also add an entry in the NEWS file describing the new commands, and explaining that the old "set/show remotebreak" commands are now deprecated? -- Joel ------=_NextPart_000_03A6_01CA3E21.0194C3D0 Content-Type: application/octet-stream; name="remotebreak.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="remotebreak.patch" Content-length: 8601 Index: gdb/ChangeLog=0A= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=0A= RCS file: /cvs/src/src/gdb/ChangeLog,v=0A= retrieving revision 1.10906=0A= diff -u -r1.10906 ChangeLog=0A= --- gdb/ChangeLog 25 Sep 2009 21:39:52 -0000 1.10906=0A= +++ gdb/ChangeLog 26 Sep 2009 03:38:17 -0000=0A= @@ -1,3 +1,24 @@=0A= +2009-09-25 Kazuyoshi Caz Yokoyama =0A= +=0A= + * remote.c: Add a new remote variable, interrupt_sequence and=0A= + interrupt-on-start. interrupt_sequence provides arbitrary number of=0A= + characters include Ctrl-C to the remote target to stop the execution=0A= + of the program on the target. Not only characters but it also send=0A= + BREAK signal to the target. For example,=0A= + ^C: ASCII ETX character (Ctrl-c, 0x03)=0A= + BREAK: BREAK signal=0A= + BREAK-g: BREAK signal and 'g' which is Magic SysRq which interrupt=0A= + the execution of Linux kernel=20=0A= + abcdef: ASCII character, abcdef=0A= + abc-xyz: ASCII character, abcxyz=0A= + remotebreak is deprecated even though it is still available. It switches= =0A= + ^C and BREAK.=0A= + interrupt-on-start is a boolean variable and controls whether=0A= + interrupt_sequence is sent when gdb starts. When you debug Linux kernel,= =0A= + you probably do=0A= + set remote interrupt-sequence BREAK-g=0A= + set remote interrupt-on-start on=0A= +=09=0A= 2009-09-25 Tom Tromey =0A= =20=0A= PR python/10664:=0A= Index: gdb/remote.c=0A= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=0A= RCS file: /cvs/src/src/gdb/remote.c,v=0A= retrieving revision 1.372=0A= diff -u -r1.372 remote.c=0A= --- gdb/remote.c 10 Sep 2009 22:47:56 -0000 1.372=0A= +++ gdb/remote.c 26 Sep 2009 03:38:20 -0000=0A= @@ -546,13 +546,66 @@=0A= this can go away. */=0A= static int wait_forever_enabled_p =3D 1;=0A= =20=0A= +/* Interrupt sequence to the remote target to interrupt the program=0A= + or OS. */=0A= +=0A= +#define CTRL_C "^C" /* default of interrupt_sequence */=0A= +#define BREAK "BREAK"=0A= +static char *interrupt_sequence =3D NULL;=0A= +=0A= +/* This boolean variable specifies whether interrupt_sequence is sent=0A= + to remote target when gdb starts. This is mostly needed when you debug= =0A= + Linux kernel. Linux kernel expects BREAK g which is Magic SysRq for=0A= + connecting gdb.=0A= + */=0A= +static int interrupt_on_start =3D 0;=0A= =20=0A= /* This variable chooses whether to send a ^C or a break when the user=0A= requests program interruption. Although ^C is usually what remote=0A= systems expect, and that is the default here, sometimes a break is=0A= preferable instead. */=0A= =20=0A= -static int remote_break;=0A= +static int remote_break =3D 0;=0A= +=0A= +static void=0A= +set_remote_break (char *args, int from_tty, struct cmd_list_element *c)=0A= +{=0A= + if (strcmp(interrupt_sequence, CTRL_C) =3D=3D 0)=0A= + {=0A= + interrupt_sequence =3D xmalloc(sizeof(BREAK) + 1);=0A= + strcpy(interrupt_sequence, BREAK);=0A= + remote_break =3D 1;=0A= + }=0A= + else if (strcmp(interrupt_sequence, BREAK) =3D=3D 0)=0A= + {=0A= + interrupt_sequence =3D xmalloc(sizeof(CTRL_C) + 1);=0A= + strcpy(interrupt_sequence, CTRL_C);=0A= + remote_break =3D 0;=0A= + }=0A= +#if 0=0A= + fprintf_filtered (file, _("\=0A= +Deprecated. Use interrupt_sequence instead."));=0A= +#endif=0A= +}=0A= +=0A= +static void=0A= +show_remote_break (struct ui_file *file, int from_tty,=0A= + struct cmd_list_element *c,=0A= + const char *value)=0A= +{=0A= + if (strcmp(interrupt_sequence, CTRL_C) =3D=3D 0)=0A= + fprintf_filtered (file, _("\=0A= +Whether to send break if interrupted is off.\n\=0A= +Deprecated. Use set remote interrupt_sequence instead.\n"));=0A= + else if (strcmp(interrupt_sequence, "BREAK") =3D=3D 0)=0A= + fprintf_filtered (file, _("\=0A= +Whether to send break if interrupted is on.\n\=0A= +Deprecated. Use set remote interrupt_sequence instead.\n"));=0A= + else=0A= + fprintf_filtered (file, _("\=0A= +Sending %s as interrupt_sequence.\n\=0A= +Deprecated. Use interrupt_sequence instead.\n"), interrupt_sequence);=0A= +}=0A= =20=0A= /* Descriptor for I/O to remote machine. Initialize it to NULL so that=0A= remote_open knows that we don't have a file open when the program=0A= @@ -2585,6 +2638,38 @@=0A= int extended_p;=0A= };=0A= =20=0A= +/*=0A= + Send interrupt_sequence to remote target.=0A= +*/=0A= +static void=0A= +send_interrupt_sequence ()=0A= +{=0A= + char *b, *p, ch;=0A= +=0A= + b =3D xmalloc (strlen(interrupt_sequence) + 1);=0A= + if (b =3D=3D NULL)=0A= + internal_error (__FILE__, __LINE__,=0A= + _("can't allocate b in send_interrupt_sequence()"));=0A= + strcpy (b, interrupt_sequence);=0A= + for (p =3D strtok (b, "-");=0A= + p; p =3D strtok(NULL, "-"))=0A= + {=0A= + if (strcmp(p, CTRL_C) =3D=3D 0)=0A= + serial_write (remote_desc, "\x03", 1);=0A= + else if (strcmp(p, "BREAK") =3D=3D 0)=0A= + serial_send_break (remote_desc);=0A= + else if (*p =3D=3D '\\')=0A= + {=0A= + sscanf (p, "%c", &ch);=0A= + serial_write (remote_desc, &ch, 1);=0A= + }=0A= + else=0A= + serial_write (remote_desc, p, strlen(p));=0A= + }=0A= +=0A= + xfree (b);=0A= +}=0A= +=0A= static void=0A= remote_start_remote (struct ui_out *uiout, void *opaque)=0A= {=0A= @@ -2598,6 +2683,12 @@=0A= /* Ack any packet which the remote side has already sent. */=0A= serial_write (remote_desc, "+", 1);=0A= =20=0A= + /* Interrupt_sequence is sent to remote target when gdb starts.=0A= + This is mostly needed when you debug Linux kernel.=0A= + Linux kernel expects BREAK g which is Magic SysRq for connecting gdb.= */=0A= + if (interrupt_on_start)=0A= + send_interrupt_sequence ();=0A= +=0A= /* The first packet we send to the target is the optional "supported=0A= packets" request. If the target can answer this, it will tell us=0A= which later probes to skip. */=0A= @@ -4021,12 +4112,8 @@=0A= if (rs->cached_wait_status)=0A= return;=0A= =20=0A= - /* Send a break or a ^C, depending on user preference. */=0A= -=0A= - if (remote_break)=0A= - serial_send_break (remote_desc);=0A= - else=0A= - serial_write (remote_desc, "\003", 1);=0A= + /* Send interrupt_sequence to remote target. */=0A= + send_interrupt_sequence();=0A= }=0A= =20=0A= /* This is the generic stop called via the target vector. When a target=0A= @@ -9056,13 +9143,33 @@=0A= terminating `#' character and checksum."),=0A= &maintenancelist);=0A= =20=0A= - add_setshow_boolean_cmd ("remotebreak", no_class, &remote_break, _("\=0A= + add_setshow_boolean_cmd ("remotebreak", class_obscure, &remote_break, _(= "\=0A= Set whether to send break if interrupted."), _("\=0A= Show whether to send break if interrupted."), _("\=0A= If set, a break, instead of a cntrl-c, is sent to the remote target."),=0A= - NULL, NULL, /* FIXME: i18n: Whether to send break if interrupted is = %s. */=0A= + set_remote_break, show_remote_break,=0A= &setlist, &showlist);=0A= =20=0A= + /* When set command is called for the item which is defined by=0A= + add_setshow_string_cmd, a string is freed when the string is not NULL= =0A= + even it is set by initialization. */=0A= + interrupt_sequence =3D xmalloc(sizeof(CTRL_C) + 1);=0A= + strcpy(interrupt_sequence, CTRL_C);=0A= + add_setshow_string_noescape_cmd ("interrupt-sequence", class_support,=0A= + &interrupt_sequence, _("\=0A= +Set interrupt sequence to remote target."), _("\=0A= +Show interrupt sequence to remote target."), _("\=0A= +\\xNN, ^C and BREAK are interpreted as appropriately."),=0A= + NULL, NULL,=0A= + &remote_set_cmdlist, &remote_show_cmdlist);=0A= +=0A= + add_setshow_boolean_cmd ("interrupt-on-start", no_class, &interrupt_on_s= tart, _("\=0A= +Set whether to send interrupt sequence when gdb starts."), _(" \=0A= +Show whether to send interrupt sequence when gdb starts."), _(" \=0A= +If set, interrupt sequence is sent to the remote target."),=0A= + NULL, NULL,=0A= + &remote_set_cmdlist, &remote_show_cmdlist);=0A= +=0A= /* Install commands for configuring memory read/write packets. */=0A= =20=0A= add_cmd ("remotewritesize", no_class, set_memory_write_packet_size, _("\= =0A= ------=_NextPart_000_03A6_01CA3E21.0194C3D0--