[RFC] [gdb/python] Make sure python sys.exit makes gdb exit

[RFC] [gdb/python] Make sure python sys.exit makes gdb exit

[Tom de Vries]

With gdb 15.1, python sys.exit no longer makes gdb exit:
...
$ gdb -q -batch -ex "python sys.exit(2)" -ex "print 123"; echo $?
Python Exception <class 'SystemExit'>: 2
Error occurred in Python: 2
$1 = 123
0
...

This is a change in behaviour since commit a207f6b3a38 ("Rewrite "python"
command exception handling"), first available in gdb 15.1.

The question is whether this is a regression.

The semantics of sys.exit / exception SystemExit allow intercepting the thrown
exception [1].

This patch reverts to the old behaviour by handling PyExc_SystemExit in
gdbpy_handle_exception, such what we have instead:
...
$ gdb -q -batch -ex "python sys.exit(2)" -ex "print 123"; echo $?
2
...

But alternatively, we could decide that this is the behaviour we want for gdb.

Note that os._exit works as expected, and could be used instead.

Tested on x86_64-linux.

PR python/31946
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31946

[1] https://docs.python.org/3/library/sys.html#sys.exit
---
 gdb/python/py-utils.c                 |  7 +++++
 gdb/testsuite/gdb.python/sys-exit.exp | 37 +++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)
 create mode 100644 gdb/testsuite/gdb.python/sys-exit.exp

diff --git a/gdb/python/py-utils.c b/gdb/python/py-utils.c
index 47f65f4fd44..ef70d937f9c 100644
--- a/gdb/python/py-utils.c
+++ b/gdb/python/py-utils.c
@@ -390,6 +390,13 @@ gdbpy_handle_exception ()
 
   if (fetched_error.type_matches (PyExc_KeyboardInterrupt))
     throw_quit ("Quit");
+  else if (fetched_error.type_matches (PyExc_SystemExit))
+    {
+      int exit_arg = 0;
+      if (msg != nullptr && *msg != '\0')
+	exit_arg = atoi (msg.get ());
+      quit_force (&exit_arg, 0);
+    }
   else if (! fetched_error.type_matches (gdbpy_gdberror_exc)
 	   || msg == NULL || *msg == '\0')
     {
diff --git a/gdb/testsuite/gdb.python/sys-exit.exp b/gdb/testsuite/gdb.python/sys-exit.exp
new file mode 100644
index 00000000000..9dd914d46f9
--- /dev/null
+++ b/gdb/testsuite/gdb.python/sys-exit.exp
@@ -0,0 +1,37 @@
+# Copyright 2024 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 python sys.exit makes gdb exit, with the correct exit status.
+
+require allow_python_tests
+
+foreach_with_prefix n { 0 1 2 } {
+    clean_restart
+
+    # Regression test for PR python/31946.
+    gdb_test_multiple "python sys.exit ($n)" "python sys.exit" {
+	eof {
+	    set wait_status [wait -i $gdb_spawn_id]
+	    unset gdb_spawn_id
+
+	    verbose -log "GDB process exited with wait status $wait_status"
+
+	    set os_error [lindex $wait_status 2]
+	    set exit_status [lindex $wait_status 3]
+
+	    gdb_assert { $os_error == 0 && $exit_status == $n } $gdb_test_name
+	}
+    }
+}

base-commit: 8d40cbfde5988ac6d7dad2fc2796d13c9ddb210b
-- 
2.35.3

Re: [RFC] [gdb/python] Make sure python sys.exit makes gdb exit

[Guinevere Larsen]

On 7/27/24 6:48 AM, Tom de Vries wrote:
> With gdb 15.1, python sys.exit no longer makes gdb exit:
> ...
> $ gdb -q -batch -ex "python sys.exit(2)" -ex "print 123"; echo $?
> Python Exception <class 'SystemExit'>: 2
> Error occurred in Python: 2
> $1 = 123
> 0
> ...
>
> This is a change in behaviour since commit a207f6b3a38 ("Rewrite "python"
> command exception handling"), first available in gdb 15.1.
>
> The question is whether this is a regression.
>
> The semantics of sys.exit / exception SystemExit allow intercepting the thrown
> exception [1].
>
> This patch reverts to the old behaviour by handling PyExc_SystemExit in
> gdbpy_handle_exception, such what we have instead:
> ...
> $ gdb -q -batch -ex "python sys.exit(2)" -ex "print 123"; echo $?
> 2
> ...
>
> But alternatively, we could decide that this is the behaviour we want for gdb.

For what it's worth, I think we should honor sys.exit requests from 
users, and exit GDB. Throwing an exception to exit seems (to me) to be 
intended for what this patch does: perform cleanups as required so we 
leave the system in a reasonable state when quitting, but still quit 
nonetheless.

>
> Note that os._exit works as expected, and could be used instead.

python's docs for os._exit[1] say the following:

 > The standard way to exit is sys.exit(n). _exit() should normally only 
be used in the child process after a fork().

so just eating the exception and not exiting feels wrong.


The code and test look fine, but I don't really understand this area of 
GDB, but I have tested and verified that it fixes the issue so feel free 
to add my tb tag: Tested-By: Guinevere Larsen <blarsen@redhat.com>

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

[1] https://docs.python.org/3/library/os.html#os._exit

>
> Tested on x86_64-linux.
>
> PR python/31946
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31946
>
> [1] https://docs.python.org/3/library/sys.html#sys.exit
> ---
>   gdb/python/py-utils.c                 |  7 +++++
>   gdb/testsuite/gdb.python/sys-exit.exp | 37 +++++++++++++++++++++++++++
>   2 files changed, 44 insertions(+)
>   create mode 100644 gdb/testsuite/gdb.python/sys-exit.exp
>
> diff --git a/gdb/python/py-utils.c b/gdb/python/py-utils.c
> index 47f65f4fd44..ef70d937f9c 100644
> --- a/gdb/python/py-utils.c
> +++ b/gdb/python/py-utils.c
> @@ -390,6 +390,13 @@ gdbpy_handle_exception ()
>   
>     if (fetched_error.type_matches (PyExc_KeyboardInterrupt))
>       throw_quit ("Quit");
> +  else if (fetched_error.type_matches (PyExc_SystemExit))
> +    {
> +      int exit_arg = 0;
> +      if (msg != nullptr && *msg != '\0')
> +	exit_arg = atoi (msg.get ());
> +      quit_force (&exit_arg, 0);
> +    }
>     else if (! fetched_error.type_matches (gdbpy_gdberror_exc)
>   	   || msg == NULL || *msg == '\0')
>       {
> diff --git a/gdb/testsuite/gdb.python/sys-exit.exp b/gdb/testsuite/gdb.python/sys-exit.exp
> new file mode 100644
> index 00000000000..9dd914d46f9
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/sys-exit.exp
> @@ -0,0 +1,37 @@
> +# Copyright 2024 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 python sys.exit makes gdb exit, with the correct exit status.
> +
> +require allow_python_tests
> +
> +foreach_with_prefix n { 0 1 2 } {
> +    clean_restart
> +
> +    # Regression test for PR python/31946.
> +    gdb_test_multiple "python sys.exit ($n)" "python sys.exit" {
> +	eof {
> +	    set wait_status [wait -i $gdb_spawn_id]
> +	    unset gdb_spawn_id
> +
> +	    verbose -log "GDB process exited with wait status $wait_status"
> +
> +	    set os_error [lindex $wait_status 2]
> +	    set exit_status [lindex $wait_status 3]
> +
> +	    gdb_assert { $os_error == 0 && $exit_status == $n } $gdb_test_name
> +	}
> +    }
> +}
>
> base-commit: 8d40cbfde5988ac6d7dad2fc2796d13c9ddb210b

Re: [RFC] [gdb/python] Make sure python sys.exit makes gdb exit

[Joel Brobecker]

> > With gdb 15.1, python sys.exit no longer makes gdb exit:
> > ...
> > $ gdb -q -batch -ex "python sys.exit(2)" -ex "print 123"; echo $?
> > Python Exception <class 'SystemExit'>: 2
> > Error occurred in Python: 2
> > $1 = 123
> > 0
> > ...
> > 
> > This is a change in behaviour since commit a207f6b3a38 ("Rewrite "python"
> > command exception handling"), first available in gdb 15.1.
> > 
> > The question is whether this is a regression.

I had a bit of a knee-jerk reaction at first, and thought perhaps
we should perhaps provide an API for exiting GDB. Looking at
the semantics of what sys.exit does, I eventually reached the same
conclusion as Tom and Guinevere. It makes more sense to restore
the previous behavior, and have GDB exit when a SystemExit exception
is raised in the main thread, and unhandled.

-- 
Joel

Re: [RFC] [gdb/python] Make sure python sys.exit makes gdb exit

[Tom de Vries]

On 8/12/24 01:22, Joel Brobecker wrote:
>>> With gdb 15.1, python sys.exit no longer makes gdb exit:
>>> ...
>>> $ gdb -q -batch -ex "python sys.exit(2)" -ex "print 123"; echo $?
>>> Python Exception <class 'SystemExit'>: 2
>>> Error occurred in Python: 2
>>> $1 = 123
>>> 0
>>> ...
>>>
>>> This is a change in behaviour since commit a207f6b3a38 ("Rewrite "python"
>>> command exception handling"), first available in gdb 15.1.
>>>
>>> The question is whether this is a regression.
> 
> I had a bit of a knee-jerk reaction at first, and thought perhaps
> we should perhaps provide an API for exiting GDB. Looking at
> the semantics of what sys.exit does, I eventually reached the same
> conclusion as Tom and Guinevere. It makes more sense to restore
> the previous behavior, and have GDB exit when a SystemExit exception
> is raised in the main thread, and unhandled.
> 

Hi,

thank you both for your reactions.

With two votes pointing in the same direction, I've dropped the RCF bit 
from the commit log and resubmitted here ( 
https://sourceware.org/pipermail/gdb-patches/2024-August/211022.html ).

Thanks,
- Tom