diff mbox

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

Message ID 1534934813-10188-1-git-send-email-gbenson@redhat.com
State New
Headers show

Commit Message

Gary Benson Aug. 22, 2018, 10:46 a.m. UTC
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

Comments

Eli Zaretskii Aug. 22, 2018, 2:36 p.m. UTC | #1
> 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.
Gary Benson Aug. 23, 2018, 8 a.m. UTC | #2
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.
Pedro Alves Aug. 23, 2018, 4:37 p.m. UTC | #3
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
Sergio Durigan Junior Aug. 30, 2018, 8:26 p.m. UTC | #4
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
Gary Benson Aug. 31, 2018, 1:30 p.m. UTC | #5
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
Tom Tromey Aug. 31, 2018, 2:32 p.m. UTC | #6
>>>>> "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
Pedro Alves Aug. 31, 2018, 2:42 p.m. UTC | #7
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
Sergio Durigan Junior Aug. 31, 2018, 3:38 p.m. UTC | #8
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,
diff mbox

Patch

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