[PATCH] Indicate batch mode failures by exiting with nonzero status

[PATCH] Indicate batch mode failures by exiting with nonzero status

[Gary Benson]

Hi all,

This patch causes GDB in batch mode to exit with nonzero status
if the last command to be executed fails.

Built and regtested on RHEL 7.5 x86_64.

Ok to commit?

Thanks,
Gary

--
gdb/ChangeLog:

	PR gdb/13000:
	* gdb/main.c (last_command_failed): New static global.
	(handle_command_errors): Set the above on error.
	(catch_command_errors): Clear the above before commands.
	(captured_main_1): Exit with nonzero status in batch mode
	if the last command to be executed failed.
	* NEWS: Mention the above.

gdb/testsuite/ChangeLog:

	PR gdb/13000:
	* gdb.base/batch-exit-status.exp: New file.
	* gdb.base/batch-exit-status.good-commands: Likewise.
	* gdb.base/batch-exit-status.bad-commands: Likewise.
---
 gdb/ChangeLog                                      | 10 ++++
 gdb/NEWS                                           |  3 ++
 gdb/main.c                                         | 13 ++++-
 gdb/testsuite/ChangeLog                            |  7 +++
 .../gdb.base/batch-exit-status.bad-commands        |  1 +
 gdb/testsuite/gdb.base/batch-exit-status.exp       | 63 ++++++++++++++++++++++
 .../gdb.base/batch-exit-status.good-commands       |  1 +
 7 files changed, 97 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.base/batch-exit-status.bad-commands
 create mode 100644 gdb/testsuite/gdb.base/batch-exit-status.exp
 create mode 100644 gdb/testsuite/gdb.base/batch-exit-status.good-commands

diff --git a/gdb/NEWS b/gdb/NEWS
index 16d3d72..6af712a 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -10,6 +10,9 @@
 * DWARF index cache: GDB can now automatically save indices of DWARF
   symbols on disk to speed up further loading of the same binaries.
 
+* GDB in batch mode now exits with status 1 if the last command to be
+  executed failed.
+
 * New commands
 
 frame apply [all | COUNT | -COUNT | level LEVEL...] [FLAG]... COMMAND
diff --git a/gdb/main.c b/gdb/main.c
index e925128..31a6ee0 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -343,6 +343,10 @@ captured_command_loop ()
   quit_command (NULL, ui->instream == ui->stdin_stream);
 }
 
+/* Did the last invocation of catch_command_errors throw an error?  */
+
+static bool last_command_failed = false;
+
 /* Handle command errors thrown from within catch_command_errors.  */
 
 static int
@@ -350,6 +354,8 @@ handle_command_errors (struct gdb_exception e)
 {
   if (e.reason < 0)
     {
+      last_command_failed = true;
+
       exception_print (gdb_stderr, e);
 
       /* If any exception escaped to here, we better enable stdin.
@@ -372,6 +378,8 @@ static int
 catch_command_errors (catch_command_errors_const_ftype command,
 		      const char *arg, int from_tty)
 {
+  last_command_failed = false;
+
   TRY
     {
       int was_sync = current_ui->prompt_state == PROMPT_BLOCKED;
@@ -1134,8 +1142,11 @@ captured_main_1 (struct captured_main_args *context)
 
   if (batch_flag)
     {
+      int error_status = EXIT_FAILURE;
+      int *exit_arg = last_command_failed ? &error_status : NULL;
+
       /* We have hit the end of the batch file.  */
-      quit_force (NULL, 0);
+      quit_force (exit_arg, 0);
     }
 }
 
diff --git a/gdb/testsuite/gdb.base/batch-exit-status.bad-commands b/gdb/testsuite/gdb.base/batch-exit-status.bad-commands
new file mode 100644
index 0000000..4793acb
--- /dev/null
+++ b/gdb/testsuite/gdb.base/batch-exit-status.bad-commands
@@ -0,0 +1 @@
+bork
diff --git a/gdb/testsuite/gdb.base/batch-exit-status.exp b/gdb/testsuite/gdb.base/batch-exit-status.exp
new file mode 100644
index 0000000..bee4d72
--- /dev/null
+++ b/gdb/testsuite/gdb.base/batch-exit-status.exp
@@ -0,0 +1,63 @@
+# Copyright (C) 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Check that "gdb -batch" exits with appropriate status.
+
+standard_testfile
+
+set good_commands "$srcdir/$subdir/batch-exit-status.good-commands"
+set bad_commands "$srcdir/$subdir/batch-exit-status.bad-commands"
+
+proc _test_exit_status {expect_status cmdline_opts} {
+    global gdb_spawn_id
+
+    gdb_exit
+    if {[gdb_spawn_with_cmdline_opts $cmdline_opts] != 0} {
+	fail "spawn"
+	return
+    }
+
+    set result [wait -i $gdb_spawn_id]
+    verbose $result
+    gdb_assert { [lindex $result 2] == 0 }
+    set actual_status [lindex $result 3]
+    gdb_assert { $actual_status == $expect_status }
+}
+
+proc test_exit_status {expect_status cmdline_opts} {
+    with_test_prefix $cmdline_opts {
+	_test_exit_status $expect_status $cmdline_opts
+    }
+}
+
+# gdb -batch with nothing to do should exit 0.
+test_exit_status 0 "-batch"
+
+# Bad command-line options should cause exit 1.
+test_exit_status 1 "-batch -jslkflsdjlkfjlksdjf"
+
+# gdb -batch with good commands should exit 0.
+test_exit_status 0 "-batch -ex \"info source\""
+test_exit_status 0 "-batch -x $good_commands"
+
+# gdb -batch with bad commands should exit 1.
+test_exit_status 1 "-batch -ex \"set not-a-thing 4\""
+test_exit_status 1 "-batch -x $bad_commands"
+
+# Success or failure of the last thing determines the exit code.
+test_exit_status 0 "-batch -ex \"set not-a-thing 4\" -x $good_commands"
+test_exit_status 0 "-batch -x $bad_commands -ex \"info source\""
+test_exit_status 1 "-batch -x $good_commands -x $bad_commands"
+test_exit_status 1 "-batch -x $good_commands -ex \"set not-a-thing 4\""
diff --git a/gdb/testsuite/gdb.base/batch-exit-status.good-commands b/gdb/testsuite/gdb.base/batch-exit-status.good-commands
new file mode 100644
index 0000000..80708a9
--- /dev/null
+++ b/gdb/testsuite/gdb.base/batch-exit-status.good-commands
@@ -0,0 +1 @@
+info mem
-- 
1.8.3.1

Re: [PATCH] Indicate batch mode failures by exiting with nonzero status

[Tom Tromey]

>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:

Gary> +/* Did the last invocation of catch_command_errors throw an error?  */
Gary> +
Gary> +static bool last_command_failed = false;

catch_command_errors seems to return a boolean, so I think a global
shouldn't be necessary.

Also, why just the last command?  I mean, I guess I don't really know
what I would expect, but maybe if any command failed, gdb should exit?

Right now there seems to be a discrepancy where if you do
"gdb -ex fail -ex fail -ex fail", each one will be run; but if you
put the commands into a file and "gdb -x file", then only the first one
will be run.  I'm not sure if this is good or bad.

I suppose one nice thing about the "last command only" approach is that
you could ignore failures by tacking on one more -ex with some sort of no-op.

Tom

Re: [PATCH] Indicate batch mode failures by exiting with nonzero status

[Gary Benson]

Tom Tromey wrote:
> >>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
> 
> Gary> +/* Did the last invocation of catch_command_errors throw an error?  */
> Gary> +
> Gary> +static bool last_command_failed = false;
> 
> catch_command_errors seems to return a boolean, so I think a global
> shouldn't be necessary.

True.  It was a hang up from a previous version of the patch.
I can change it.

> Also, why just the last command?  I mean, I guess I don't really know
> what I would expect, but maybe if any command failed, gdb should exit?

That was my thinking, to have GDB exit 1 on the first error in batch
mode, but people objected; see the followups to:

  https://sourceware.org/ml/gdb/2018-07/msg00009.html

The last command thing was something that Pedro wanted.  The patch
would originally exit 1 if _any_ error occurred, but Pedro reasoned
that the last operation is the thing you wanted to happen and earlier
failures might be spurious, e.g. bad commands in gdbinit files.
Though you have situations like:

  bash$ gdb -batch -p 12345 -ex "info sharedlibrary"
  ptrace: No such process.
  No shared libraries loaded at this time.
  bash$ echo $?
  0

> Right now there seems to be a discrepancy where if you do "gdb -ex
> fail -ex fail -ex fail", each one will be run; but if you put the
> commands into a file and "gdb -x file", then only the first one will
> be run.  I'm not sure if this is good or bad.

Yeah, it's inconsistent and I hate it :/

I wonder if people would accept me changing the default behaviour to
exit 1 on the first error, with an option e.g. -batch-ignore-errors
to revert to the current behaviour.  I might reply to the original
thread suggesting that.

> I suppose one nice thing about the "last command only" approach is
> that you could ignore failures by tacking on one more -ex with some
> sort of no-op.

Every cloud has a silver lining :)

Cheers,
Gary

Re: [PATCH] Indicate batch mode failures by exiting with nonzero status

[Pedro Alves]

On 08/17/2018 11:59 AM, Gary Benson wrote:
> Tom Tromey wrote:
>>>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
>>
>> Gary> +/* Did the last invocation of catch_command_errors throw an error?  */
>> Gary> +
>> Gary> +static bool last_command_failed = false;
>>
>> catch_command_errors seems to return a boolean, so I think a global
>> shouldn't be necessary.
> 
> True.  It was a hang up from a previous version of the patch.
> I can change it.
> 
>> Also, why just the last command?  I mean, I guess I don't really know
>> what I would expect, but maybe if any command failed, gdb should exit?
> 
> That was my thinking, to have GDB exit 1 on the first error in batch
> mode, but people objected; see the followups to:
> 
>   https://sourceware.org/ml/gdb/2018-07/msg00009.html
> 
> The last command thing was something that Pedro wanted.

s/wanted/suggested investigating/.

> The patch
> would originally exit 1 if _any_ error occurred, but Pedro reasoned
> that the last operation is the thing you wanted to happen and earlier
> failures might be spurious, e.g. bad commands in gdbinit files.

Right, and given that, I recalled how TCL handles the return code of
procedures that don't have an explicit return, which is that the return
code of the last command counts.  So I thought that approach was
worth investigating.

> Though you have situations like:
> 
>   bash$ gdb -batch -p 12345 -ex "info sharedlibrary"
>   ptrace: No such process.
>   No shared libraries loaded at this time.
>   bash$ echo $?
>   0

As long as GDB runs _both_ commands, I think the "exit status is
determined by success/failure of last command run" rule is better.

Making GDB exit immediately on first error (and thus not run
"info sharedlibrary" in that case) might be worth pursuing as well,
maybe with a new command line option, but it seems orthogonal to
the exit code rules.

> 
>> Right now there seems to be a discrepancy where if you do "gdb -ex
>> fail -ex fail -ex fail", each one will be run; but if you put the
>> commands into a file and "gdb -x file", then only the first one will
>> be run.  I'm not sure if this is good or bad.
> 
> Yeah, it's inconsistent and I hate it :/

I think it depends on perspective.  If you put "fail" in a file, and then
do "gdb -x file -x file -x file", then "fail" is tried 3 times as well:

$ gdb -x file -x file -x file
[...]
file:1: Error in sourced command file:
Undefined command: "fail".  Try "help".
file:1: Error in sourced command file:
Undefined command: "fail".  Try "help".
file:1: Error in sourced command file:
Undefined command: "fail".  Try "help".
(gdb) 

> 
> I wonder if people would accept me changing the default behaviour to
> exit 1 on the first error, with an option e.g. -batch-ignore-errors
> to revert to the current behaviour.  I might reply to the original
> thread suggesting that.

If we went that route, I'd think that could be a separate orthogonal
option.  I.e., a "stop/continue processing commands on first error"
option.  That could be used even without -batch.

That would be something in the same spirit of the new
"thread apply" / "frame apply" flags:

  FLAG arguments are -q (quiet), -c (continue), -s (silent).

> 
>> I suppose one nice thing about the "last command only" approach is
>> that you could ignore failures by tacking on one more -ex with some
>> sort of no-op.
> 
> Every cloud has a silver lining :)
> 
Thanks,
Pedro Alves

Re: [PATCH] Indicate batch mode failures by exiting with nonzero status

[Gary Benson]

Pedro Alves wrote:
> If we went that route, I'd think that could be a separate orthogonal
> option.  I.e., a "stop/continue processing commands on first error"
> option.  That could be used even without -batch.
>
> That would be something in the same spirit of the new
> "thread apply" / "frame apply" flags:
> 
>   FLAG arguments are -q (quiet), -c (continue), -s (silent).

I think, if we're going to add the behaviour that GDB should stop on
error, then that behaviour should be the default, at least for batch
mode.  Stopping at the first error is what most languages do isn't it?
Apart from shell scripts :)  Having to have an option to enable the
intuitive behaviour (what *I* think is intuitive anyway) just
maintains GDB's reputation as cryptic and hard to use.

Cheers,
Gary

Re: [PATCH] Indicate batch mode failures by exiting with nonzero status

[Tom Tromey]

>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:

>> Also, why just the last command?  I mean, I guess I don't really know
>> what I would expect, but maybe if any command failed, gdb should exit?

Gary> That was my thinking, to have GDB exit 1 on the first error in batch
Gary> mode, but people objected; see the followups to:

Oh, ok, sorry for wading into an already open issue without the
background.

On reflection perhaps I'm overthinking it.  It seems to me that it is
fine for these command line options to be defined based on whatever
seems convenient.

For "real" scripting, a script file could be used.  Maybe we could
finally land that try/catch patch, or the ignore-errors command.
Or, longer term, finally make it possible to really script with Python.

Tom

Re: [PATCH] Indicate batch mode failures by exiting with nonzero status

[Gary Benson]

On 19 August 2018 at 16:35, Tom Tromey <tom@tromey.com> wrote:

> >>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
>
> >> Also, why just the last command?  I mean, I guess I don't really know
> >> what I would expect, but maybe if any command failed, gdb should exit?
>
> Gary> That was my thinking, to have GDB exit 1 on the first error in batch
> Gary> mode, but people objected; see the followups to:
>
> Oh, ok, sorry for wading into an already open issue without the
> background.
>

Ah, no worries :)


> On reflection perhaps I'm overthinking it.  It seems to me that it is
> fine for these command line options to be defined based on whatever
> seems convenient.
>

I'm probably overthinking this stuff too. I'll rework the patch without the
global and resubmit.

Cheers,
Gary

Re: [PATCH] Indicate batch mode failures by exiting with nonzero status

[Philippe Waroquiers]

On Sun, 2018-08-19 at 09:35 -0600, Tom Tromey wrote:
> > > > > > "Gary" == Gary Benson <gbenson@redhat.com> writes:
> > > Also, why just the last command?  I mean, I guess I don't really know
> > > what I would expect, but maybe if any command failed, gdb should exit?
> 
> Gary> That was my thinking, to have GDB exit 1 on the first error in batch
> Gary> mode, but people objected; see the followups to:
> 
> Oh, ok, sorry for wading into an already open issue without the
> background.
> 
> On reflection perhaps I'm overthinking it.  It seems to me that it is
> fine for these command line options to be defined based on whatever
> seems convenient.
> 
> For "real" scripting, a script file could be used.  Maybe we could
> finally land that try/catch patch, or the ignore-errors command.
> Or, longer term, finally make it possible to really script with Python.

Note that I still have on my list of things to do to rework
the 'block of commands' patch that I started some (long) time ago.

One of the objectives was to allow to group commands in blocks such as:

{ command1 ; command2 ; command3 }

And have (optional) flags -c and -s to follow the {
to control continuing (silently or not) the execution
of a sequence of commands when there is a failure.

Another objective is to allow something like:
  thread apply all { command1; command 2}
and similar for frame apply,

Philippe