From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19094 invoked by alias); 30 Aug 2005 02:35:01 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 18979 invoked by uid 22791); 30 Aug 2005 02:34:55 -0000 Received: from nevyn.them.org (HELO nevyn.them.org) (66.93.172.17) by sourceware.org (qpsmtpd/0.30-dev) with ESMTP; Tue, 30 Aug 2005 02:34:55 +0000 Received: from drow by nevyn.them.org with local (Exim 4.52) id 1E9vxe-0004IZ-4Q; Mon, 29 Aug 2005 22:34:54 -0400 Date: Tue, 30 Aug 2005 02:37:00 -0000 From: Daniel Jacobowitz To: Sean Callanan Cc: gdb-patches@sources.redhat.com Subject: Re: terminate-on-error patch Message-ID: <20050830023454.GA16189@nevyn.them.org> Mail-Followup-To: Sean Callanan , gdb-patches@sources.redhat.com References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.8i X-SW-Source: 2005-08/txt/msg00248.txt.bz2 On Thu, Aug 11, 2005 at 12:49:23PM -0700, Sean Callanan wrote: > In general, it is desirable to have at least optional handling of > errors by scripts rather than having the scripts simply terminate in > all cases. I have a simple patch that adds this functionality to the > GDB command line. > > When the test-and-set variable "terminate-on-error" is on (default), > the behavior is the same as unmodified GDB. When it is off, a failed > command does not terminate a script; rather, the $_error convenience > variable is set to 1. Upon a successful command execution, the > variable is set to 0. I'm not thrilled with the approach, but GDB's CLI is generally un-thrilling at present, and Mark and Michael seem in favor. So let's do it. Thanks for the documentation and tests. The minimum additional we'd need to merge the patch would be a ChangeLog entry and a pass with the GNU coding standards in mind; I'll pick out the highlights. If you could arrange to not have your patches mauled, butchered, and devastated by the Apple mail client and its bizarre approach to line wrapping, that'd be good too. > + /* Longjmp-safe wrapper for "execute_command". */ > + extern struct gdb_exception safe_execute_command (struct ui_out *, > char *, int); > + Mark noted that you're over the 80-column limit here, and possibly elsewhere. > + #include "wrapper.h" If you add includes, you need to update the dependencies in the Makefile. > + /* Terminate scripts on errors, rather than setting _error = 1 */ Comments end with a period and two spaces, please. > ! > ! if(terminate_on_error) > ! { > ! execute_command (new_line, 0); > ! } > ! else > ! { > ! struct gdb_exception rv; > ! struct value* rv_val; > ! > ! rv = safe_execute_command (uiout, new_line, 0); > ! rv_val = value_from_longest (builtin_type_int, (rv.error ! > = NO_ERROR)); > ! set_internalvar (lookup_internalvar ("_error"), rv_val); > ! } > ! > ret = cmd->control_type; > break; Needless braces (the first set) should generally be omitted. Space before parenthesis in the if (). > + void > + _initialize_cli_script (void) > + { > + add_setshow_boolean_cmd("terminate-on-error", class_support, > &terminate_on_error, > + "Set termination of scripts on command > errors.", > + "Show script termination on command > errors.", > + "Controls how scripts respond to command > errors: by terminating or setting $_error to 1.", > + NULL, NULL, &setlist, &showlist); > + } Overlong line I think? Also, I believe these are supposed to be translated strings nowadays. There's plenty of examples lying around. And space before parenthesis again. You probably need a prototype for this function, or GCC will warn. > + Some @value{GDBN} commands cannot complete successfully, and must > terminate. I found this a little unclear; maybe an example? > + #include > + > + int main(int argc, char** argv) > + { > + printf("Hello world!\n"); > + } We're trying to put copyright notices on all new tests. > + # Copyright 2001, 2003 Free Software Foundation, Inc. That's not right... > + # Please email any bugs, comments, and/or additions to this file to: > + # bug-gdb@prep.ai.mit.edu > + # use this to debug: > + # > + # log_user 1 And neither is this bit; please omit. > + if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" > executable {debug}] != "" } { > + gdb_suppress_entire_file "Testcase compile failed, so all > tests in this file will automatically fail." > + } Please don't add new uses of gdb_suppress_entire_file. IIRC just "return -1" will do fine. -- Daniel Jacobowitz CodeSourcery, LLC