Re: [PATCH v2] Exit code of exited inferiors

Re: [PATCH v2] Exit code of exited inferiors

[Eli Zaretskii]

> Date: Tue, 28 May 2013 16:39:41 -0400
> From: Simon Marchi <simon.marchi@ericsson.com>
> CC: Marc Khouzam <marc.khouzam@ericsson.com>
> 
> This is the second version of my patch to add the exit-code in the 
> -list-thread-groups output. New in v2: NEWS file, documentation and a 
> test case. I didn't find any test about -list-thread-groups so I created it.
> 
> As far as I know, this does not break any other test.
> 
> Changelog:
> 
> 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.
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
You should state the name of the node in which you made the change, as
if it were a function (i.e. in parentheses).

>      * gdb/inferior.c (exit_inferior_1): Remove exit code clear.
>      (inferior_appeared): Add exit code clear.
>      * 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.

OK for the documentation part, after you have fixed the above gotcha.

Thanks.

Re: [PATCH v2] Exit code of exited inferiors

[Simon Marchi]

On 13-05-29 11:57 AM, Eli Zaretskii wrote:
>> Date: Tue, 28 May 2013 16:39:41 -0400
>> From: Simon Marchi <simon.marchi@ericsson.com>
>> CC: Marc Khouzam <marc.khouzam@ericsson.com>
>>
>> This is the second version of my patch to add the exit-code in the
>> -list-thread-groups output. New in v2: NEWS file, documentation and a
>> test case. I didn't find any test about -list-thread-groups so I created it.
>>
>> As far as I know, this does not break any other test.
>>
>> Changelog:
>>
>> 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.
>           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> You should state the name of the node in which you made the change, as
> if it were a function (i.e. in parentheses).
>
>>       * gdb/inferior.c (exit_inferior_1): Remove exit code clear.
>>       (inferior_appeared): Add exit code clear.
>>       * 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.
> OK for the documentation part, after you have fixed the above gotcha.
>
> Thanks.
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.

Simon

Re: [PATCH v2] Exit code of exited inferiors

[Pedro Alves]

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

Re: [PATCH v2] Exit code of exited inferiors

[Tom Tromey]

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

Pedro> We use prepare_for_testing for new tests now.

You can't use that for MI tests, unfortunately.

Tom

Re: [PATCH v2] Exit code of exited inferiors

[Simon Marchi]

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

Oops, probably.

>> +    # 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?

I thought I did, but apparently I didn't mention that this patch depends 
on this one, which adds the ability to pass arguments to mi_run_to_main 
that are passed to the test executable:
http://sourceware.org/ml/gdb-patches/2013-05/msg00971.html

That reminds me, I still need to produce an updated version of this 
other patch, following the comments I got.

Basically, exit-code.c (which, as you noted, I forgot to add to the 
patch) simply returns what you ask it to return (10 in this case).

> Thanks,

Ok for the rest of the comments, they are all relevant and useful.

Thanks,

Simon

Re: [PATCH v2] Exit code of exited inferiors

[Simon Marchi]

On 13-06-06 12:50 PM, Pedro Alves wrote:
> Please also make sure the test works with gdbserver, with:
>
>    make check RUNTESTFLAGS="--target_board=native-gdbserver"
>
> Thanks,

Hmm, it seems that testing with gdbserver doesn't handle arguments 
passing, so it doesn't work. How could it be done?

Re: [PATCH v2] Exit code of exited inferiors

[Pedro Alves]

On 06/17/2013 05:04 PM, Simon Marchi wrote:
> On 13-06-06 12:50 PM, Pedro Alves wrote:
>> Please also make sure the test works with gdbserver, with:
>>
>>    make check RUNTESTFLAGS="--target_board=native-gdbserver"
>>
>> Thanks,
> 
> Hmm, it seems that testing with gdbserver doesn't handle arguments passing, so it doesn't work. How could it be done?

We haven't seen exit-code.c yet :-) but I imagine that it just
returns a different error code depending on the argument?
You could instead make gdb run to main, and then set a global in the
program with the desired return code.  Or build two programs, and pick
the return code depending on a macro.  You can pass additional_flags=-Dfoo=bar
to prepare_for_testing/gdb_compile to define macros/symbols.  Several
examples on that in the testsuite.

Do note:
http://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_executables_are_unique

-- 
Pedro Alves

Re: [PATCH v2] Exit code of exited inferiors

[Simon Marchi]

On 13-06-17 12:37 PM, Pedro Alves wrote:
> On 06/17/2013 05:04 PM, Simon Marchi wrote:
>> On 13-06-06 12:50 PM, Pedro Alves wrote:
>>> Please also make sure the test works with gdbserver, with:
>>>
>>>     make check RUNTESTFLAGS="--target_board=native-gdbserver"
>>>
>>> Thanks,
>>
>> Hmm, it seems that testing with gdbserver doesn't handle arguments passing, so it doesn't work. How could it be done?
>
> We haven't seen exit-code.c yet :-) but I imagine that it just
> returns a different error code depending on the argument?
> You could instead make gdb run to main, and then set a global in the
> program with the desired return code.  Or build two programs, and pick
> the return code depending on a macro.  You can pass additional_flags=-Dfoo=bar
> to prepare_for_testing/gdb_compile to define macros/symbols.  Several
> examples on that in the testsuite.
>
> Do note:
> http://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_executables_are_unique
>

Thanks, I chose the global variable solution.