From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29844 invoked by alias); 13 Dec 2007 13:53:15 -0000 Received: (qmail 29835 invoked by uid 22791); 13 Dec 2007 13:53:14 -0000 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 13 Dec 2007 13:53:03 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id C31F92A96C8; Thu, 13 Dec 2007 08:53:01 -0500 (EST) 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 uNuczXXWT1f1; Thu, 13 Dec 2007 08:53:01 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 636A22A96C4; Thu, 13 Dec 2007 08:53:01 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 8595FE7ACA; Thu, 13 Dec 2007 14:52:59 +0100 (CET) Date: Thu, 13 Dec 2007 14:44:00 -0000 From: Joel Brobecker To: Vladimir Prus Cc: gdb-patches@sources.redhat.com Subject: Re: Pending breakpoints in MI Message-ID: <20071213135259.GA10040@adacore.com> References: <200711081412.14452.ghost@cs.msu.su> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200711081412.14452.ghost@cs.msu.su> User-Agent: Mutt/1.4.2.2i 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: 2007-12/txt/msg00171.txt.bz2 Volodya, You forgot to provide a ChangeLog... Here are my comments on your patch itself. It's mostly fine so we should be able to check it in soon :). > gdb_breakpoint (char *address, char *condition, > int hardwareflag, int tempflag, > int thread, int ignore_count, > + int pending, > char **error_message) I can't believe I'm saying this, since I absolutely hate tabs, but these are the coding rules: You need to use a tab instead of 8 spaces in the line above. Compare for instance how the "char **error_message" line is indented. > - AUTO_BOOLEAN_FALSE /* no pending. */, > + pending > + ? AUTO_BOOLEAN_TRUE : AUTO_BOOLEAN_FALSE, Same here. > int thread, int ignore_count, > + int pending, > char **error_message); Likewise in the .h prototype. > --- gdb/testsuite/gdb.mi/mi-pending.exp (/patches/pending_mi_2_code_duplication) (revision 45) > +++ gdb/testsuite/gdb.mi/mi-pending.exp (/patches/pending_mi_3_breakpoints) (revision 45) > @@ -0,0 +1,75 @@ > +# Copyright 2003, 2004, 2005, 2007 Free Software Foundation, Inc. Shouldn't the copyright year be only 2007? > --- gdb/testsuite/gdb.mi/mi-pending.c (/patches/pending_mi_2_code_duplication) (revision 45) > +++ gdb/testsuite/gdb.mi/mi-pending.c (/patches/pending_mi_3_breakpoints) (revision 45) > @@ -0,0 +1,34 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2004, 2007 Free Software Foundation, Inc. Same question here. > --- gdb/testsuite/gdb.mi/mi-pendshr.c (/patches/pending_mi_2_code_duplication) (revision 45) > +++ gdb/testsuite/gdb.mi/mi-pendshr.c (/patches/pending_mi_3_breakpoints) (revision 45) > @@ -0,0 +1,32 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2004, 2007 Free Software Foundation, Inc. Likewise here... > --- gdb/mi/mi-cmd-break.c (/patches/pending_mi_2_code_duplication) (revision 45) > +++ gdb/mi/mi-cmd-break.c (/patches/pending_mi_3_breakpoints) (revision 45) > @@ -63,7 +63,10 @@ enum bp_type > breakpoint. -break-insert -t -h --> insert a temporary > hw bp. > -break-insert -r --> insert a bp at functions matching > - */ > + > + > + The -f flag makes GDB create a pending breakpoint if no > + locations for breakpoint are found now. */ Actually, I think that the entire comment needs to be rewritten in a way that the syntax is not described there at all. Just say that this function implements the -break-insert command which, as its name suggests, inserts a breakpoint. The syntax needs to be documented in the GDB manual. See http://www.sourceware.org/gdb/current/onlinedocs/gdb_25.html#SEC259 for what is currently documented. Let's use this opportunity to make sure that every option (and only the supported options) are documented. > @@ -122,6 +127,9 @@ mi_cmd_break_insert (char *command, char > case THREAD_OPT: > thread = atol (optarg); > break; > + case PENDING_OPT: > + pending = 1; > + break; The indentation is strange, could you double-check it? > @@ -139,12 +147,14 @@ mi_cmd_break_insert (char *command, char > rc = gdb_breakpoint (address, condition, > 0 /*hardwareflag */ , temp_p, > thread, ignore_count, > + pending, > &mi_error_message); > break; > case HW_BP: > rc = gdb_breakpoint (address, condition, > 1 /*hardwareflag */ , temp_p, > thread, ignore_count, > + pending, I think you need to replace some spaces with tabs here too... Thanks, -- Joel