Re: terminate-on-error patch

[Daniel Jacobowitz <drow at false dot org>]

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 <stdio.h>
> +
> + 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