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

[PATCH v2] 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.

Changes since version 1:
 - I've reworked the patch to not use a global variable.
 - Behaviour has not changed from version 1.

Built and regtested on RHEL 7.5 x86_64.

Ok to commit?

Thanks,
Gary

--
gdb/ChangeLog:

	PR gdb/13000:
	* gdb/main.c (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                                      |  7 ++
 gdb/NEWS                                           |  3 +
 gdb/main.c                                         | 78 +++++++++++++---------
 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, 129 insertions(+), 31 deletions(-)
 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..61644cd 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -486,6 +486,7 @@ captured_main_1 (struct captured_main_args *context)
   int i;
   int save_auto_load;
   struct objfile *objfile;
+  int ret = 1;
 
 #ifdef HAVE_USEFUL_SBRK
   /* Set this before constructing scoped_command_stats.  */
@@ -986,7 +987,7 @@ captured_main_1 (struct captured_main_args *context)
      processed; it sets global parameters, which are independent of
      what file you are debugging or what directory you are in.  */
   if (system_gdbinit && !inhibit_gdbinit)
-    catch_command_errors (source_script, system_gdbinit, 0);
+    ret = catch_command_errors (source_script, system_gdbinit, 0);
 
   /* Read and execute $HOME/.gdbinit file, if it exists.  This is done
      *before* all the command line arguments are processed; it sets
@@ -994,7 +995,7 @@ captured_main_1 (struct captured_main_args *context)
      debugging or what directory you are in.  */
 
   if (home_gdbinit && !inhibit_gdbinit && !inhibit_home_gdbinit)
-    catch_command_errors (source_script, home_gdbinit, 0);
+    ret = catch_command_errors (source_script, home_gdbinit, 0);
 
   /* Process '-ix' and '-iex' options early.  */
   for (i = 0; i < cmdarg_vec.size (); i++)
@@ -1004,12 +1005,12 @@ captured_main_1 (struct captured_main_args *context)
       switch (cmdarg_p.type)
 	{
 	case CMDARG_INIT_FILE:
-	  catch_command_errors (source_script, cmdarg_p.string,
-				!batch_flag);
+	  ret = catch_command_errors (source_script, cmdarg_p.string,
+				      !batch_flag);
 	  break;
 	case CMDARG_INIT_COMMAND:
-	  catch_command_errors (execute_command, cmdarg_p.string,
-				!batch_flag);
+	  ret = catch_command_errors (execute_command, cmdarg_p.string,
+				      !batch_flag);
 	  break;
 	}
     }
@@ -1017,11 +1018,11 @@ captured_main_1 (struct captured_main_args *context)
   /* Now perform all the actions indicated by the arguments.  */
   if (cdarg != NULL)
     {
-      catch_command_errors (cd_command, cdarg, 0);
+      ret = catch_command_errors (cd_command, cdarg, 0);
     }
 
   for (i = 0; i < dirarg.size (); i++)
-    catch_command_errors (directory_switch, dirarg[i], 0);
+    ret = catch_command_errors (directory_switch, dirarg[i], 0);
 
   /* Skip auto-loading section-specified scripts until we've sourced
      local_gdbinit (which is often used to augment the source search
@@ -1036,19 +1037,20 @@ captured_main_1 (struct captured_main_args *context)
       /* The exec file and the symbol-file are the same.  If we can't
          open it, better only print one error message.
          catch_command_errors returns non-zero on success!  */
-      if (catch_command_errors (exec_file_attach, execarg,
-				!batch_flag))
-	catch_command_errors (symbol_file_add_main_adapter, symarg,
-			      !batch_flag);
+      ret = catch_command_errors (exec_file_attach, execarg,
+				  !batch_flag);
+      if (ret != 0)
+	ret = catch_command_errors (symbol_file_add_main_adapter,
+				    symarg, !batch_flag);
     }
   else
     {
       if (execarg != NULL)
-	catch_command_errors (exec_file_attach, execarg,
-			      !batch_flag);
+	ret = catch_command_errors (exec_file_attach, execarg,
+				    !batch_flag);
       if (symarg != NULL)
-	catch_command_errors (symbol_file_add_main_adapter, symarg,
-			      !batch_flag);
+	ret = catch_command_errors (symbol_file_add_main_adapter,
+				    symarg, !batch_flag);
     }
 
   if (corearg && pidarg)
@@ -1056,9 +1058,14 @@ captured_main_1 (struct captured_main_args *context)
 	     "a core file at the same time."));
 
   if (corearg != NULL)
-    catch_command_errors (core_file_command, corearg, !batch_flag);
+    {
+      ret = catch_command_errors (core_file_command, corearg,
+				  !batch_flag);
+    }
   else if (pidarg != NULL)
-    catch_command_errors (attach_command, pidarg, !batch_flag);
+    {
+      ret = catch_command_errors (attach_command, pidarg, !batch_flag);
+    }
   else if (pid_or_core_arg)
     {
       /* The user specified 'gdb program pid' or gdb program core'.
@@ -1067,14 +1074,20 @@ captured_main_1 (struct captured_main_args *context)
 
       if (isdigit (pid_or_core_arg[0]))
 	{
-	  if (catch_command_errors (attach_command, pid_or_core_arg,
-				    !batch_flag) == 0)
-	    catch_command_errors (core_file_command, pid_or_core_arg,
-				  !batch_flag);
+	  ret = catch_command_errors (attach_command, pid_or_core_arg,
+				      !batch_flag);
+	  if (ret == 0)
+	    ret = catch_command_errors (core_file_command,
+					pid_or_core_arg,
+					!batch_flag);
+	}
+      else
+	{
+	  /* Can't be a pid, better be a corefile.  */
+	  ret = catch_command_errors (core_file_command,
+				      pid_or_core_arg,
+				      !batch_flag);
 	}
-      else /* Can't be a pid, better be a corefile.  */
-	catch_command_errors (core_file_command, pid_or_core_arg,
-			      !batch_flag);
     }
 
   if (ttyarg != NULL)
@@ -1098,7 +1111,7 @@ captured_main_1 (struct captured_main_args *context)
 	{
 	  auto_load_local_gdbinit_loaded = 1;
 
-	  catch_command_errors (source_script, local_gdbinit, 0);
+	  ret = catch_command_errors (source_script, local_gdbinit, 0);
 	}
     }
 
@@ -1118,12 +1131,12 @@ captured_main_1 (struct captured_main_args *context)
       switch (cmdarg_p.type)
 	{
 	case CMDARG_FILE:
-	  catch_command_errors (source_script, cmdarg_p.string,
-				!batch_flag);
+	  ret = catch_command_errors (source_script, cmdarg_p.string,
+				      !batch_flag);
 	  break;
 	case CMDARG_COMMAND:
-	  catch_command_errors (execute_command, cmdarg_p.string,
-				!batch_flag);
+	  ret = catch_command_errors (execute_command, cmdarg_p.string,
+				      !batch_flag);
 	  break;
 	}
     }
@@ -1134,8 +1147,11 @@ captured_main_1 (struct captured_main_args *context)
 
   if (batch_flag)
     {
+      int error_status = EXIT_FAILURE;
+      int *exit_arg = ret == 0 ? &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 v2] Indicate batch mode failures by exiting with nonzero status

[Eli Zaretskii]

> From: Gary Benson <gbenson@redhat.com>
> Cc: tom@tromey.com
> Date: Wed, 22 Aug 2018 11:46:53 +0100
> 
> 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.
> +

This part is OK, thanks.

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

[Gary Benson]

On 22 August 2018 at 15:36, Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Gary Benson <gbenson@redhat.com>
> > Cc: tom@tromey.com
> > Date: Wed, 22 Aug 2018 11:46:53 +0100
> >
> > 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.
> > +
>
> This part is OK, thanks.

Thanks Eli.

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

[Pedro Alves]

On 08/22/2018 11:46 AM, Gary Benson wrote:
> Hi all,
> 
> This patch causes GDB in batch mode to exit with nonzero status
> if the last command to be executed fails.
> 
> Changes since version 1:
>  - I've reworked the patch to not use a global variable.
>  - Behaviour has not changed from version 1.
> 
> Built and regtested on RHEL 7.5 x86_64.
> 
> Ok to commit?
LGTM.

Thanks,
Pedro Alves

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

[Sergio Durigan Junior]

On Wednesday, August 22 2018, Gary Benson wrote:

> Hi all,
>
> This patch causes GDB in batch mode to exit with nonzero status
> if the last command to be executed fails.
>
> Changes since version 1:
>  - I've reworked the patch to not use a global variable.
>  - Behaviour has not changed from version 1.
>
> Built and regtested on RHEL 7.5 x86_64.

Hey, just to make sure this is reported here: I'm seeing failures when
running this test on my Fedora 28 x86_64:

  FAIL: gdb.base/batch-exit-status.exp: -batch -jslkflsdjlkfjlksdjf: $actual_status == $expect_status
  FAIL: gdb.base/batch-exit-status.exp: -batch -ex "set not-a-thing 4": $actual_status == $expect_status
  FAIL: gdb.base/batch-exit-status.exp: -batch -x /home/sergio/work/src/git/binutils-gdb/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base/batch-exit-status.bad-commands: $actua$
  _status == $expect_status
  FAIL: gdb.base/batch-exit-status.exp: -batch -x /home/sergio/work/src/git/binutils-gdb/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base/batch-exit-status.good-commands -x /ho$
  e/sergio/work/src/git/binutils-gdb/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base/batch-exit-status.bad-commands: $actual_status == $expect_status
  FAIL: gdb.base/batch-exit-status.exp: -batch -x /home/sergio/work/src/git/binutils-gdb/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base/batch-exit-status.good-commands -ex "s$
  t not-a-thing 4": $actual_status == $expect_status

This has also been caught by the BuildBot:

  https://sourceware.org/ml/gdb-testers/2018-q3/msg05129.html
  https://sourceware.org/ml/gdb-testers/2018-q3/msg05223.html

Let me know if you need more details.

Thanks,

> Ok to commit?
>
> Thanks,
> Gary
>
> --
> gdb/ChangeLog:
>
> 	PR gdb/13000:
> 	* gdb/main.c (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                                      |  7 ++
>  gdb/NEWS                                           |  3 +
>  gdb/main.c                                         | 78 +++++++++++++---------
>  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, 129 insertions(+), 31 deletions(-)
>  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..61644cd 100644
> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -486,6 +486,7 @@ captured_main_1 (struct captured_main_args *context)
>    int i;
>    int save_auto_load;
>    struct objfile *objfile;
> +  int ret = 1;
>  
>  #ifdef HAVE_USEFUL_SBRK
>    /* Set this before constructing scoped_command_stats.  */
> @@ -986,7 +987,7 @@ captured_main_1 (struct captured_main_args *context)
>       processed; it sets global parameters, which are independent of
>       what file you are debugging or what directory you are in.  */
>    if (system_gdbinit && !inhibit_gdbinit)
> -    catch_command_errors (source_script, system_gdbinit, 0);
> +    ret = catch_command_errors (source_script, system_gdbinit, 0);
>  
>    /* Read and execute $HOME/.gdbinit file, if it exists.  This is done
>       *before* all the command line arguments are processed; it sets
> @@ -994,7 +995,7 @@ captured_main_1 (struct captured_main_args *context)
>       debugging or what directory you are in.  */
>  
>    if (home_gdbinit && !inhibit_gdbinit && !inhibit_home_gdbinit)
> -    catch_command_errors (source_script, home_gdbinit, 0);
> +    ret = catch_command_errors (source_script, home_gdbinit, 0);
>  
>    /* Process '-ix' and '-iex' options early.  */
>    for (i = 0; i < cmdarg_vec.size (); i++)
> @@ -1004,12 +1005,12 @@ captured_main_1 (struct captured_main_args *context)
>        switch (cmdarg_p.type)
>  	{
>  	case CMDARG_INIT_FILE:
> -	  catch_command_errors (source_script, cmdarg_p.string,
> -				!batch_flag);
> +	  ret = catch_command_errors (source_script, cmdarg_p.string,
> +				      !batch_flag);
>  	  break;
>  	case CMDARG_INIT_COMMAND:
> -	  catch_command_errors (execute_command, cmdarg_p.string,
> -				!batch_flag);
> +	  ret = catch_command_errors (execute_command, cmdarg_p.string,
> +				      !batch_flag);
>  	  break;
>  	}
>      }
> @@ -1017,11 +1018,11 @@ captured_main_1 (struct captured_main_args *context)
>    /* Now perform all the actions indicated by the arguments.  */
>    if (cdarg != NULL)
>      {
> -      catch_command_errors (cd_command, cdarg, 0);
> +      ret = catch_command_errors (cd_command, cdarg, 0);
>      }
>  
>    for (i = 0; i < dirarg.size (); i++)
> -    catch_command_errors (directory_switch, dirarg[i], 0);
> +    ret = catch_command_errors (directory_switch, dirarg[i], 0);
>  
>    /* Skip auto-loading section-specified scripts until we've sourced
>       local_gdbinit (which is often used to augment the source search
> @@ -1036,19 +1037,20 @@ captured_main_1 (struct captured_main_args *context)
>        /* The exec file and the symbol-file are the same.  If we can't
>           open it, better only print one error message.
>           catch_command_errors returns non-zero on success!  */
> -      if (catch_command_errors (exec_file_attach, execarg,
> -				!batch_flag))
> -	catch_command_errors (symbol_file_add_main_adapter, symarg,
> -			      !batch_flag);
> +      ret = catch_command_errors (exec_file_attach, execarg,
> +				  !batch_flag);
> +      if (ret != 0)
> +	ret = catch_command_errors (symbol_file_add_main_adapter,
> +				    symarg, !batch_flag);
>      }
>    else
>      {
>        if (execarg != NULL)
> -	catch_command_errors (exec_file_attach, execarg,
> -			      !batch_flag);
> +	ret = catch_command_errors (exec_file_attach, execarg,
> +				    !batch_flag);
>        if (symarg != NULL)
> -	catch_command_errors (symbol_file_add_main_adapter, symarg,
> -			      !batch_flag);
> +	ret = catch_command_errors (symbol_file_add_main_adapter,
> +				    symarg, !batch_flag);
>      }
>  
>    if (corearg && pidarg)
> @@ -1056,9 +1058,14 @@ captured_main_1 (struct captured_main_args *context)
>  	     "a core file at the same time."));
>  
>    if (corearg != NULL)
> -    catch_command_errors (core_file_command, corearg, !batch_flag);
> +    {
> +      ret = catch_command_errors (core_file_command, corearg,
> +				  !batch_flag);
> +    }
>    else if (pidarg != NULL)
> -    catch_command_errors (attach_command, pidarg, !batch_flag);
> +    {
> +      ret = catch_command_errors (attach_command, pidarg, !batch_flag);
> +    }
>    else if (pid_or_core_arg)
>      {
>        /* The user specified 'gdb program pid' or gdb program core'.
> @@ -1067,14 +1074,20 @@ captured_main_1 (struct captured_main_args *context)
>  
>        if (isdigit (pid_or_core_arg[0]))
>  	{
> -	  if (catch_command_errors (attach_command, pid_or_core_arg,
> -				    !batch_flag) == 0)
> -	    catch_command_errors (core_file_command, pid_or_core_arg,
> -				  !batch_flag);
> +	  ret = catch_command_errors (attach_command, pid_or_core_arg,
> +				      !batch_flag);
> +	  if (ret == 0)
> +	    ret = catch_command_errors (core_file_command,
> +					pid_or_core_arg,
> +					!batch_flag);
> +	}
> +      else
> +	{
> +	  /* Can't be a pid, better be a corefile.  */
> +	  ret = catch_command_errors (core_file_command,
> +				      pid_or_core_arg,
> +				      !batch_flag);
>  	}
> -      else /* Can't be a pid, better be a corefile.  */
> -	catch_command_errors (core_file_command, pid_or_core_arg,
> -			      !batch_flag);
>      }
>  
>    if (ttyarg != NULL)
> @@ -1098,7 +1111,7 @@ captured_main_1 (struct captured_main_args *context)
>  	{
>  	  auto_load_local_gdbinit_loaded = 1;
>  
> -	  catch_command_errors (source_script, local_gdbinit, 0);
> +	  ret = catch_command_errors (source_script, local_gdbinit, 0);
>  	}
>      }
>  
> @@ -1118,12 +1131,12 @@ captured_main_1 (struct captured_main_args *context)
>        switch (cmdarg_p.type)
>  	{
>  	case CMDARG_FILE:
> -	  catch_command_errors (source_script, cmdarg_p.string,
> -				!batch_flag);
> +	  ret = catch_command_errors (source_script, cmdarg_p.string,
> +				      !batch_flag);
>  	  break;
>  	case CMDARG_COMMAND:
> -	  catch_command_errors (execute_command, cmdarg_p.string,
> -				!batch_flag);
> +	  ret = catch_command_errors (execute_command, cmdarg_p.string,
> +				      !batch_flag);
>  	  break;
>  	}
>      }
> @@ -1134,8 +1147,11 @@ captured_main_1 (struct captured_main_args *context)
>  
>    if (batch_flag)
>      {
> +      int error_status = EXIT_FAILURE;
> +      int *exit_arg = ret == 0 ? &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

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

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

[Gary Benson]

On 30 August 2018 at 21:26, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
> On Wednesday, August 22 2018, Gary Benson wrote:
>> This patch causes GDB in batch mode to exit with nonzero status
>> if the last command to be executed fails.
>>
>> Changes since version 1:
>>  - I've reworked the patch to not use a global variable.
>>  - Behaviour has not changed from version 1.
>>
>> Built and regtested on RHEL 7.5 x86_64.
>
> Hey, just to make sure this is reported here: I'm seeing failures when
> running this test on my Fedora 28 x86_64:
>
>   FAIL: gdb.base/batch-exit-status.exp: -batch -jslkflsdjlkfjlksdjf: $actual_status == $expect_status
>   FAIL: gdb.base/batch-exit-status.exp: -batch -ex "set not-a-thing 4": $actual_status == $expect_status
>   FAIL: gdb.base/batch-exit-status.exp: -batch -x /home/sergio/work/src/git/binutils-gdb/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base/batch-exit-status.bad-commands: $actua$
>   _status == $expect_status
>   FAIL: gdb.base/batch-exit-status.exp: -batch -x /home/sergio/work/src/git/binutils-gdb/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base/batch-exit-status.good-commands -x /ho$
>   e/sergio/work/src/git/binutils-gdb/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base/batch-exit-status.bad-commands: $actual_status == $expect_status
>   FAIL: gdb.base/batch-exit-status.exp: -batch -x /home/sergio/work/src/git/binutils-gdb/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base/batch-exit-status.good-commands -ex "s$
>   t not-a-thing 4": $actual_status == $expect_status
>
> This has also been caught by the BuildBot:
>
>   https://sourceware.org/ml/gdb-testers/2018-q3/msg05129.html
>   https://sourceware.org/ml/gdb-testers/2018-q3/msg05223.html
>
> Let me know if you need more details.

Yes please! Is it failing on *every* platform? I looked in gdb.log.xz
but there's no output from GDB, only the PASS/FAIL lines.  So I don't
really know what to do, I can't reproduce :/

Cheers,
Gary

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

[Tom Tromey]

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

Gary> Yes please! Is it failing on *every* platform? I looked in gdb.log.xz
Gary> but there's no output from GDB, only the PASS/FAIL lines.  So I don't
Gary> really know what to do, I can't reproduce :/

It fails for me too.  I ran with -v and saw this in the .log:

    RESULT = 11271 exp6 0 0
    PASS: gdb.base/batch-exit-status.exp: -batch: [lindex $result 2] == 0
    PASS: gdb.base/batch-exit-status.exp: -batch: $actual_status == $expect_status
    Quitting /home/tromey/gdb/build/gdb/testsuite/../../gdb/gdb -nw -nx -data-directory /home/tromey/gdb/build/gdb/testsuite/../data-directory 
    doing kill, pid is 11271
    Spawning /home/tromey/gdb/build/gdb/testsuite/../../gdb/gdb -nw -nx -data-directory /home/tromey/gdb/build/gdb/testsuite/../data-directory -batch -jslkflsdjlkfjlksdjf
    RESULT = 11271 exp6 0 0
    PASS: gdb.base/batch-exit-status.exp: -batch -jslkflsdjlkfjlksdjf: [lindex $result 2] == 0
    FAIL: gdb.base/batch-exit-status.exp: -batch -jslkflsdjlkfjlksdjf: $actual_status == $expect_status
    Quitting /home/tromey/gdb/build/gdb/testsuite/../../gdb/gdb -nw -nx -data-directory /home/tromey/gdb/build/gdb/testsuite/../data-directory 
    doing kill, pid is 11271


Note how the PID doesn't change.

I suspect that what is happening is that gdb_spawn_id isn't being reset,
and so we're hitting this code in default_gdb_spawn:

    if [info exists gdb_spawn_id] {
	return 0
    }

Something has to unset this after each invocation.
Maybe calling clear_gdb_spawn_id, but I don't know if that's best or if
there should be something else.

Tom

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

[Pedro Alves]

On 08/31/2018 03:32 PM, Tom Tromey wrote:

> Maybe calling clear_gdb_spawn_id, but I don't know if that's best or if
> there should be something else.

Yeah, if we compare with gdb.base/quit.exp, that is missing
as well as the "remote_close host" call.  For the latter, when I last
touched quit.exp, I remember fretting a bit about whether the remote_close
was really necessary, but in the end decided to keep it, I don't recall
exactly if I kept it because I found it's necessary (probably for
remote hosts), or because it doesn't hurt.

Now that I look at gdb.base/quit.exp again, I see that gdb.base/batch-exit-status.exp
can hang forever in the same way that 15763a09d4ae fixed, isn't it?
Gary, any reason you didn't do the "eof" thing here too?

Thanks,
Pedro Alves

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

[Sergio Durigan Junior]

On Friday, August 31 2018, Gary Benson wrote:

> On 30 August 2018 at 21:26, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>> On Wednesday, August 22 2018, Gary Benson wrote:
>>> This patch causes GDB in batch mode to exit with nonzero status
>>> if the last command to be executed fails.
>>>
>>> Changes since version 1:
>>>  - I've reworked the patch to not use a global variable.
>>>  - Behaviour has not changed from version 1.
>>>
>>> Built and regtested on RHEL 7.5 x86_64.
>>
>> Hey, just to make sure this is reported here: I'm seeing failures when
>> running this test on my Fedora 28 x86_64:
>>
>>   FAIL: gdb.base/batch-exit-status.exp: -batch -jslkflsdjlkfjlksdjf: $actual_status == $expect_status
>>   FAIL: gdb.base/batch-exit-status.exp: -batch -ex "set not-a-thing 4": $actual_status == $expect_status
>>   FAIL: gdb.base/batch-exit-status.exp: -batch -x
>> /home/sergio/work/src/git/binutils-gdb/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base/batch-exit-status.bad-commands:
>> $actua$
>>   _status == $expect_status
>>   FAIL: gdb.base/batch-exit-status.exp: -batch -x
>> /home/sergio/work/src/git/binutils-gdb/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base/batch-exit-status.good-commands
>> -x /ho$
>>   e/sergio/work/src/git/binutils-gdb/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base/batch-exit-status.bad-commands: $actual_status == $expect_status
>>   FAIL: gdb.base/batch-exit-status.exp: -batch -x
>> /home/sergio/work/src/git/binutils-gdb/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base/batch-exit-status.good-commands
>> -ex "s$
>>   t not-a-thing 4": $actual_status == $expect_status
>>
>> This has also been caught by the BuildBot:
>>
>>   https://sourceware.org/ml/gdb-testers/2018-q3/msg05129.html
>>   https://sourceware.org/ml/gdb-testers/2018-q3/msg05223.html
>>
>> Let me know if you need more details.
>
> Yes please! Is it failing on *every* platform? I looked in gdb.log.xz
> but there's no output from GDB, only the PASS/FAIL lines.  So I don't
> really know what to do, I can't reproduce :/

Hey Gary,

At least according to the BuildBot, the test is failing on Fedora x86_64
(F-28).  I don't see the failures on s390x, but maybe that's because
they're running old systems...

I guess you might be able to reproduce this if you run the test on a
Fedora 28 machine/VM.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

[PATCH] Fix batch exit status test failure on Fedora 28

[Gary Benson]

Pedro Alves <palves@redhat.com> wrote:
> Yeah, if we compare with gdb.base/quit.exp, that is missing
> as well as the "remote_close host" call.  For the latter, when I last
> touched quit.exp, I remember fretting a bit about whether the remote_close
> was really necessary, but in the end decided to keep it, I don't recall
> exactly if I kept it because I found it's necessary (probably for
> remote hosts), or because it doesn't hurt.
>
> Now that I look at gdb.base/quit.exp again, I see that
> gdb.base/batch-exit-status.exp can hang forever in the same way that
> 15763a09d4ae fixed, isn't it?  Gary, any reason you didn't do the
> "eof" thing here too?

I didn't realize what it did; I thought it was something specific to
that test.  And I thought the gdb_exit at the start of the test would
clear $gdb_spawn_id but apparently not! :)  So I made a fix, basically
copied and pasted from the commit you referenced:

This commit changes the batch exit status test to detect GDB's exit
using gdb_test_multiple expecting 'eof', instead of using 'wait'.
This fixes failures reported by buildbot on Fedora 28, and also means
the testcase won't hang forever on failure as fixed in
gdb.base/quit.exp by commit 15763a09d4aea85fc3153d6746c040dd48565637.

Is this ok to commit?

Thanks,
Gary

--
gdb/testsuite/ChangeLog:

	* gdb.base/batch-exit-status.exp: Use gdb_test_multiple and expect
	'eof' before 'wait -i'.  Use remote_close and clear_gdb_spawn_id.
---
 gdb/testsuite/ChangeLog                      |  5 +++++
 gdb/testsuite/gdb.base/batch-exit-status.exp | 17 ++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/gdb.base/batch-exit-status.exp b/gdb/testsuite/gdb.base/batch-exit-status.exp
index bee4d72..3b5de59 100644
--- a/gdb/testsuite/gdb.base/batch-exit-status.exp
+++ b/gdb/testsuite/gdb.base/batch-exit-status.exp
@@ -29,11 +29,18 @@ proc _test_exit_status {expect_status cmdline_opts} {
 	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 }
+    gdb_test_multiple "" "run til exit" {
+	eof {
+	    set result [wait -i $gdb_spawn_id]
+	    verbose $result
+
+	    gdb_assert { [lindex $result 2] == 0 }
+	    gdb_assert { [lindex $result 3] == $expect_status }
+
+	    remote_close host
+	    clear_gdb_spawn_id
+	}
+    }
 }
 
 proc test_exit_status {expect_status cmdline_opts} {
-- 
1.8.3.1

Re: [PATCH] Fix batch exit status test failure on Fedora 28

[Pedro Alves]

On 09/03/2018 05:12 PM, Gary Benson wrote:
> Pedro Alves <palves@redhat.com> wrote:
>> Yeah, if we compare with gdb.base/quit.exp, that is missing
>> as well as the "remote_close host" call.  For the latter, when I last
>> touched quit.exp, I remember fretting a bit about whether the remote_close
>> was really necessary, but in the end decided to keep it, I don't recall
>> exactly if I kept it because I found it's necessary (probably for
>> remote hosts), or because it doesn't hurt.
>>
>> Now that I look at gdb.base/quit.exp again, I see that
>> gdb.base/batch-exit-status.exp can hang forever in the same way that
>> 15763a09d4ae fixed, isn't it?  Gary, any reason you didn't do the
>> "eof" thing here too?
> 
> I didn't realize what it did; I thought it was something specific to
> that test.  And I thought the gdb_exit at the start of the test would
> clear $gdb_spawn_id but apparently not! :)  So I made a fix, basically
> copied and pasted from the commit you referenced:
> 
> This commit changes the batch exit status test to detect GDB's exit
> using gdb_test_multiple expecting 'eof', instead of using 'wait'.

> This fixes failures reported by buildbot on Fedora 28, and also means

Please expand here what the failure was, for the logs.  Something concise
mentioning gdb_spawn_id, doesn't have to be long.  It just should be more
self contained so that someone reading back the logs later on understands
what it was.

> the testcase won't hang forever on failure as fixed in
> gdb.base/quit.exp by commit 15763a09d4aea85fc3153d6746c040dd48565637.

Please write the commit in Linux-commit-style:

commit 15763a09d4ae ("Fix 'gdb.base/quit.exp hangs forever' if
the test fails").

to help the human reading the logs.

> Is this ok to commit?

OK.

Thanks,
Pedro Alves

Re: [PATCH] Fix batch exit status test failure on Fedora 28

[Gary Benson]

Pedro Alves wrote:
> On 09/03/2018 05:12 PM, Gary Benson wrote:
> > Pedro Alves <palves@redhat.com> wrote:
> > > Yeah, if we compare with gdb.base/quit.exp, that is missing
> > > as well as the "remote_close host" call.  For the latter, when I last
> > > touched quit.exp, I remember fretting a bit about whether the remote_close
> > > was really necessary, but in the end decided to keep it, I don't recall
> > > exactly if I kept it because I found it's necessary (probably for
> > > remote hosts), or because it doesn't hurt.
> > >
> > > Now that I look at gdb.base/quit.exp again, I see that
> > > gdb.base/batch-exit-status.exp can hang forever in the same way that
> > > 15763a09d4ae fixed, isn't it?  Gary, any reason you didn't do the
> > > "eof" thing here too?
> > 
> > I didn't realize what it did; I thought it was something specific to
> > that test.  And I thought the gdb_exit at the start of the test would
> > clear $gdb_spawn_id but apparently not! :)  So I made a fix, basically
> > copied and pasted from the commit you referenced:
> > 
> > This commit changes the batch exit status test to detect GDB's exit
> > using gdb_test_multiple expecting 'eof', instead of using 'wait'.
> 
> > This fixes failures reported by buildbot on Fedora 28, and also means
> 
> Please expand here what the failure was, for the logs.  Something concise
> mentioning gdb_spawn_id, doesn't have to be long.  It just should be more
> self contained so that someone reading back the logs later on understands
> what it was.
> 
> > the testcase won't hang forever on failure as fixed in
> > gdb.base/quit.exp by commit 15763a09d4aea85fc3153d6746c040dd48565637.
> 
> Please write the commit in Linux-commit-style:
> 
> commit 15763a09d4ae ("Fix 'gdb.base/quit.exp hangs forever' if
> the test fails").
> 
> to help the human reading the logs.

I've pushed the patch with the following message:

  Fix batch exit status test failure on Fedora 28
  
  This commit adds calls to remote_close and clear_gdb_spawn_id to
  gdb.base/batch-exit-status.exp, fixing failures reported by buildbot
  on Fedora 28 where gdb_spawn_id not being reset by the previous test
  caused default_gdb_spawn to return without spawning.
  
  This commit also changes the test to use detect GDB's exit using
  gdb_test_multiple expecting 'eof', rather than using 'wait -i' alone.
  This means the testcase won't hang forever on failure as fixed in
  gdb.base/quit.exp by commit 15763a09d4ae ("Fix 'gdb.base/quit.exp
  hangs forever' if the test fails").
  
  gdb/testsuite/ChangeLog:

        * gdb.base/batch-exit-status.exp: Use gdb_test_multiple and expect
        'eof' before 'wait -i'.  Use remote_close and clear_gdb_spawn_id.

Thanks,
Gary