From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13082 invoked by alias); 1 Oct 2009 17:18:35 -0000 Received: (qmail 13070 invoked by uid 22791); 1 Oct 2009 17:18:34 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail-fx0-f225.google.com (HELO mail-fx0-f225.google.com) (209.85.220.225) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 01 Oct 2009 17:18:28 +0000 Received: by fxm25 with SMTP id 25so355377fxm.0 for ; Thu, 01 Oct 2009 10:18:26 -0700 (PDT) Received: by 10.86.220.11 with SMTP id s11mr1417977fgg.47.1254417506224; Thu, 01 Oct 2009 10:18:26 -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 e20sm63495fga.5.2009.10.01.10.18.22 (version=SSLv3 cipher=RC4-MD5); Thu, 01 Oct 2009 10:18:24 -0700 (PDT) From: Caz Yokoyama To: "'Joel Brobecker'" Cc: "'Eli Zaretskii'" , , References: <1724490614004CEB9EE1A091A151E05B@xpjpn> <20090929042226.GK9003@adacore.com> <2C14068798BA41219F3603CDD24C8BC0@xpjpn> <20090929051929.GL9003@adacore.com> <7063C3E99BE344B2B98EDC0318ED852A@xpjpn> <20090929163910.GO9003@adacore.com> <93F096FEF7ED4579B52B23D69DA91195@xpjpn> <8363b0qm0n.fsf@gnu.org> <20090930201204.GH10338@adacore.com> <5650DA603A804427AA3B3F8F91164548@xpjpn> <20091001163330.GL10338@adacore.com> Subject: RE: symbolic debug of loadable modules with kgdb light Date: Thu, 01 Oct 2009 17:18:00 -0000 Message-ID: <0C6ED71F84CD48A9A37E2161011EF64B@xpjpn> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit In-Reply-To: <20091001163330.GL10338@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-10/txt/msg00017.txt.bz2 Hello Joel, >> +set remote interrupt-sequence [Ctrl-C | BREAK | SysRq-g] >You changed the user interface again, this is really frustrating. Sorry for frustrating you. The reason I changed is the following words by Eli. >Please use the same conventions for these keys as in the portion you are replacing. >For example, BREAK should be in caps and control-c should be spelled Ctrl-C. When interrupt sequence is controlled by remotebreak, the user does not need to enter strings. On the other hand, s/he has to specify sequence name on "set remote interrupt-sequence". In addition, the sequence name has to be same on document and actual command. Therefore, I changed according to Eli. Let me know what you want. -caz -----Original Message----- From: Joel Brobecker [mailto:brobecker@adacore.com] Sent: Thursday, October 01, 2009 9:34 AM To: Caz Yokoyama Cc: 'Eli Zaretskii'; pedro@codesourcery.com; gdb-patches@sourceware.org Subject: Re: symbolic debug of loadable modules with kgdb light Regarding testing, since you are modifying remote.c, you'll need to run the testsuite using gdbserver. The testsuite needs to be run before and after the patch, in order to compare the results. Normally, there should be no regression. Directions on how to do that are explained at http://sourceware.org/gdb/wiki/TestingGDB (see Testing gdbserver in a native configuration). Please indicate when you submit patches that you ran the testsuite, and that you found no regression. > + * remote.c (interrupt_sequence_control_c) > + (interrupt_sequence_break, interrupt_sequence_sysrq_g) > + (interrupt_sequence_modes): New constants. > + (interrupt_sequence_mode, interrupt_on_connect): New variable. > + (show_interrupt_sequence): New function. > + (set_remotebreak, show_remotebreak): New function. > + (send_interrupt_sequence: New function. > + (remote_start_remote): Call send_interrupt_sequence if interrupt_on_connect. > + (remote_stop_as): Call send_interrupt_sequence. > + (_initialize_remote): Add interrupt-sequence and interrupt-on-connect, > + modify remotebreak to call set_remotebreak and show_remotebreak. Just a few formatting nits left: The ChangeLog entries needs to be indented using tabs. You're mixing spaces and tabs. You're missing a closing parenthesis after send_interrupt_sequence and the line just after is too long (maximum is 79 or 80 characters). > +set remote interrupt-sequence [Ctrl-C | BREAK | SysRq-g] You changed the user interface again, this is really fustrating. Personally, I don't care anymore what names we use for these options and I can see why you prefer them, but since we're changing them again, please ask Daniel Jacobowitz and Pedro Alves, who are the major maintainers and regular users of this code, whether they are OK with your choices. In the future, I would really appreciate if we agreed on the user interface first, without considering code while doing that, and then stay with what we've agreed on. Otherwise, things keep changing every time I look at a patch, and we both waste valuable time. That being said, we're getting there. > +/* This boolean variable specifies whether interrupt_sequence is sent > + to remote target when gdb starts. This is mostly needed when you debug > + Linux kernel. Linux kernel expects BREAK g which is Magic SysRq for > + connecting gdb. */ Formatting: 2 spaces after each of the 2 periods. > @@ -8993,6 +9073,10 @@ > _initialize_remote (void) > { > struct remote_state *rs; > + struct cmd_list_element *cmd; > + /* I can't use the same string for lookup_cmd(). Cause segment fault. */ > + static char *_set_remotebreak_ = "remotebreak"; > + static char *_show_remotebreak_ = "remotebreak"; Try this instead: char *cmd_name; cmd_name = "remotebreak"; cmd = lookup_cmd (&cmd_name, setlist, "", -1, 1); deprecate_cmd (cmd, "set remote interrupt-sequence"); cmd_name = "remotebreak"; cmd = lookup_cmd (&cmd_name, showlist, "", -1, 1); deprecate_cmd (cmd, "show remote interrupt-sequence"); The reason why you're getting the SEGV is because lookup_cmd updates the pointer your passing to point after the command name it has matched. > - add_setshow_boolean_cmd ("remotebreak", no_class, &remote_break, _("\ > -Set whether to send break if interrupted."), _("\ > -Show whether to send break if interrupted."), _("\ > + add_setshow_boolean_cmd ("remotebreak", class_obscure, &remote_break, _("\ > +Deprecated. Use \"set remote interrupt-sequence [control-c|break]\" instead."), _("\ > +Deprecated. Use \"show remote interrupt-sequence\" instead."), _("\ Please undo this part of the change. You do NOT need to update the command documentation for "set/show remotebreak", since this is already taken care -- Joel