From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32257 invoked by alias); 1 Oct 2009 23:32:56 -0000 Received: (qmail 32247 invoked by uid 22791); 1 Oct 2009 23:32:55 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 01 Oct 2009 23:32:51 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 5F4682BABA2; Thu, 1 Oct 2009 19:32:49 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id aLyUnZgmuvLB; Thu, 1 Oct 2009 19:32:49 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 1E05F2BAB5A; Thu, 1 Oct 2009 19:32:48 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id E66B6F593D; Thu, 1 Oct 2009 16:32:45 -0700 (PDT) Date: Thu, 01 Oct 2009 23:32:00 -0000 From: Joel Brobecker To: Caz Yokoyama Cc: 'Eli Zaretskii' , gdb-patches@sourceware.org, 'Daniel Jacobowitz' , 'Pedro Alves' Subject: Re: symbolic debug of loadable modules with kgdb light Message-ID: <20091001233245.GU10338@adacore.com> References: <20090929163910.GO9003@adacore.com> <20091001204624.GP10338@adacore.com> <20091001211015.GA17752@caradoc.them.org> <200910012313.43368.pedro@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.18 (2008-05-17) 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/msg00041.txt.bz2 > +2009-10-01 Kazuyoshi Caz Yokoyama > + > + * 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 is true. > + (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. Very nice and clean! Just a few nits as shown below, but they are minor and can be mechanically fixed. So the code part of the patch is pre-approved, after you address the last comments below (it means that you have permission to commit the patch, but please make sure to send a copy of the patch that you end up checking in). I think Eli already approved the latest version of the documentation part, but he'll probably want to have a last quick look. And of course, now that I talk about checking in your patch, I am now only realizing that you do not seem to have an assignment filed with the FSF. This is a requirement for us to accept your patch. Perhaps you have an assignment on file through your employer? I couldn't find you in their records using either your name or email domain. Let me know if you'd like me to get you started with the paperwork. It takes a few weeks, so start ASAP if needed. > +const char interrupt_sequence_sysrq_g[] = "BREAK-g"; You forgot to rename this constant to interrupt_sequence_break_g, to match the actual litteral value. > +/* This boolean variable specifies whether interrupt_sequence is sent > + to remote target when gdb connect to it. > + This is mostly needed when you debug Linux kernel. > + Linux kernel expects BREAK g which is Magic SysRq g for connecting gdb. */ I've noticed a few English mistakes. I started pointed them out, but in the end, it's probably simpler if I give you the correct version: /* This boolean variable specifies whether interrupt_sequence is sent to the remote target when gdb connects to it. This is mostly needed when you debug theLinux kernel: The Linux kernel expects BREAK g which is Magic SysRq g for connecting gdb. */ > + struct cmd_list_element *cmd; > + static char *cmd_name; This should not be static. That way, it gets allocated on the stack and then released after the function returns. -- Joel