Re: [PATCH v2] Exit code of exited inferiors

[Pedro Alves <palves at redhat dot com>]

Hi Simon,

Thanks for adding a test.  Some comments below.

On 05/29/2013 05:00 PM, Simon Marchi wrote:

> Marc Khouzam made me realize that there is a specific ChangeLog in gdb/doc, so I guess any change in that folder should go in this ChangeLog, is that right? Same thing for gdb/testsuite.

Yeah.  Also, file path names in ChangeLog entries are relative to the
directory the ChangeLog is in.  So, "* inferior.c", not "* gdb/inferior.c".

> 2013-05-28  Simon Marchi  <simon.marchi@ericsson.com>
>
>     * gdb/NEWS: Announce new exit-code field.
>     * gdb/doc/gdb.texinfo: Document new exit-code field.
>     * gdb/inferior.c (exit_inferior_1): Remove exit code clear.
>     (inferior_appeared): Add exit code clear.

I'd suggest:

	* gdb/inferior.c (exit_inferior_1): Clear exit code.
	(inferior_appeared): Don't clear exit code.

>     * gdb/mi/mi-main.c (print_one_inferior): Add printing of the exit code.
>     * gdb/testsuite/gdb.mi/mi-list.exp: New file, added test case for
>         -list-thread-groups.

Watch out of tabs vs spaces.  This -list-thread-groups above looks
indented oddly.  Past tense is best avoided.  s/added/adding/.
You could just say "New file." though.


> +@item exit-code
> +The exit code of this thread group when it last exited. This field is

                                                   -------^
Double space after period.

> +only present for thread groups of type @samp{process} and only if the
> +process is not running.
> +
>  @item num_children
>  The number of children this thread group has.  This field may be
>  absent for an available thread group.
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index ed6b626..a145780 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -275,8 +275,6 @@ exit_inferior_1 (struct inferior *inftoex, int silent)
>        inf->vfork_child = NULL;
>      }
>
> -  inf->has_exit_code = 0;
> -  inf->exit_code = 0;
>    inf->pending_detach = 0;
>  }
>
> @@ -322,6 +320,8 @@ void
>  inferior_appeared (struct inferior *inf, int pid)
>  {
>    inf->pid = pid;
> +  inf->has_exit_code = 0;
> +  inf->exit_code = 0;
>
>    observer_notify_inferior_appeared (inf);
>  }
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index 9428e8c..6ce68ec 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -571,6 +571,9 @@ print_one_inferior (struct inferior *inferior, void *xdata)
>
>        ui_out_field_fmt (uiout, "id", "i%d", inferior->num);
>        ui_out_field_string (uiout, "type", "process");
> +      if (inferior->has_exit_code)
> +    ui_out_field_string (uiout, "exit-code",
> +      int_string (inferior->exit_code, 8, 0, 0, 1));

Indentation looks incorrect here too.

>        if (inferior->pid != 0)
>      ui_out_field_int (uiout, "pid", inferior->pid);
>
> diff --git a/gdb/testsuite/gdb.mi/mi-list.exp b/gdb/testsuite/gdb.mi/mi-list.exp
> new file mode 100644
> index 0000000..99e3e91
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-list.exp
> @@ -0,0 +1,74 @@
> +# Copyright 1999-2013 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/>.
> +
> +# Test Machine interface (MI) operations
> +#
> +# Verify various -list-* commands

Not sure bundling misc unrelated -list commands here would
be a good idea

> +standard_testfile "exit-code.c"

more so given even the .c file name isn't likewise generic.
I'd suggest going with mi-exit-code.c/mi-exit-code.exp,
and worry about generalizing when/if we later need it.  (It seems
more likely desirable to me to end up with other exit code
tests that don't use -list-thread-groups here, than tests for
unrelated -list-foo's.)

I didn't see exit-code.c in the patch though.  Did you forget to
add it?

> +
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
> +    untested mi-list.exp
> +    return -1
> +}

We use prepare_for_testing for new tests now.

> +
> +proc test_list_thread_groups { } {
> +    global hex
> +    global decimal
> +
> +    # Check before any run

"Check what?"  :-)  Please add full stops at end of sentences.

> +    mi_gdb_test "122-list-thread-groups" "122\\^done,groups=\\\[\{id=\"i1\",type=\"process\"\}\]" "test -list-thread-groups 1"
> +
> +    # Run to main
> +    mi_run_to_main

Comments like:

  /* Increment i.  */
  i++;

are practically useless :-)

> +
> +    # Check during the run
> +    mi_gdb_test "123-list-thread-groups" "123\\^done,groups=\\\[\{id=\"i1\",type=\"process\",pid=\"$decimal\",executable=\".*\",cores=\\\[\"$decimal\"\]\}\]" "test -list-thread-groups 2"
> +

Instead of "test -list-thread-groups 2", 3, 4, etc.  write
"-list-thread-groups before exit", "-list-thread-groups after exit",
etc.  No need to say "test" in the gdb.sum message.  It's all tests.

The "cores" attribute is optional.  It won't exist unless the target
supports reporting it.  As the main purpose of the test is unrelated,
better not assume it present.

> +    # Exit the inferior
> +    mi_send_resuming_command "exec-continue" "continuing to exit inferior"
> +    mi_expect_stop "exited-normally" "" "" "" "" "" "exiting inferior"
> +
> +    # Check after the run
> +    mi_gdb_test "124-list-thread-groups" "124\\^done,groups=\\\[\{id=\"i1\",type=\"process\",exit-code=\"0\",executable=\".*\"\}\]" "test -list-thread-groups 3"
> +
> +    # Start the program again to get an other exit code

"another".

> +    mi_run_to_main 10

What does this "10" do?

> +
> +    # Check during the second run (exit code should have disappeared)
> +    mi_gdb_test "125-list-thread-groups" "125\\^done,groups=\\\[\{id=\"i1\",type=\"process\",pid=\"$decimal\",executable=\".*\",cores=\\\[\"$decimal\"\]\}\]" "test -list-thread-groups 4"
> +
> +    # Exit the inferior
> +    mi_send_resuming_command "exec-continue" "continuing to exit inferior"
> +    mi_expect_stop "exited" "" "" "" "" "" "exiting inferior"

http://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

> +
> +    # Check after the second run
> +    mi_gdb_test "126-list-thread-groups" "126\\^done,groups=\\\[\{id=\"i1\",type=\"process\",exit-code=\"012\",executable=\".*\"\}\]" "test -list-thread-groups 5"
> +}
> +
> +test_list_thread_groups

Please also make sure the test works with gdbserver, with:

  make check RUNTESTFLAGS="--target_board=native-gdbserver"

Thanks,
-- 
Pedro Alves