* [RFC] Release the GIL while running a gdb command or expression @ 2018-09-15 4:07 Tom Tromey 2018-10-09 9:51 ` Phil Muldoon 0 siblings, 1 reply; 6+ messages in thread From: Tom Tromey @ 2018-09-15 4:07 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey PR python/23615 points out that gdb.execute_gdb_command does not release the Python GIL. This means that, while the gdb command is running, other Python threads do not run. This patch solves the problem by introducing a new RAII class that can be used to temporarily release and then re-acquire the GIL, then puts this into the appropriate places in execute_gdb_command and gdbpy_parse_and_eval. The main issue with this patch is that I could not think of a non-racy way to test it. Any ideas? gdb/ChangeLog 2018-09-07 Tom Tromey <tom@tromey.com> PR python/23615: * python/python.c (execute_gdb_command): Use gdbpy_allow_threads. (gdbpy_parse_and_eval): Likewise. * python/python-internal.h (gdbpy_allow_threads): New class. --- gdb/ChangeLog | 7 +++++++ gdb/python/python-internal.h | 25 +++++++++++++++++++++++++ gdb/python/python.c | 3 +++ 3 files changed, 35 insertions(+) diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h index 785ad171511..9041fbeab3b 100644 --- a/gdb/python/python-internal.h +++ b/gdb/python/python-internal.h @@ -647,6 +647,31 @@ class gdbpy_enter_varobj : public gdbpy_enter }; +/* The opposite of gdb_enter: this releases the GIL around a region, + allowing other Python threads to run. No Python APIs may be used + while this is active. */ +class gdbpy_allow_threads +{ +public: + + gdbpy_allow_threads () + : m_save (PyEval_SaveThread ()) + { + gdb_assert (m_save != nullptr); + } + + ~gdbpy_allow_threads () + { + PyEval_RestoreThread (m_save); + } + + DISABLE_COPY_AND_ASSIGN (gdbpy_allow_threads); + +private: + + PyThreadState *m_save; +}; + extern struct gdbarch *python_gdbarch; extern const struct language_defn *python_language; diff --git a/gdb/python/python.c b/gdb/python/python.c index e89c90f8d9f..ca0c7478e91 100644 --- a/gdb/python/python.c +++ b/gdb/python/python.c @@ -586,6 +586,8 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw) TRY { + gdbpy_allow_threads allow_threads; + struct interp *interp; std::string arg_copy = arg; @@ -934,6 +936,7 @@ gdbpy_parse_and_eval (PyObject *self, PyObject *args) TRY { + gdbpy_allow_threads allow_threads; result = parse_and_eval (expr_str); } CATCH (except, RETURN_MASK_ALL) -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] Release the GIL while running a gdb command or expression 2018-09-15 4:07 [RFC] Release the GIL while running a gdb command or expression Tom Tromey @ 2018-10-09 9:51 ` Phil Muldoon 2018-10-09 19:42 ` Tom Tromey 0 siblings, 1 reply; 6+ messages in thread From: Phil Muldoon @ 2018-10-09 9:51 UTC (permalink / raw) To: gdb-patches, Tom Tromey On 15/09/2018 05:07, Tom Tromey wrote: > PR python/23615 points out that gdb.execute_gdb_command does not > release the Python GIL. This means that, while the gdb command is > running, other Python threads do not run. > > This patch solves the problem by introducing a new RAII class that can > be used to temporarily release and then re-acquire the GIL, then puts > this into the appropriate places in execute_gdb_command and > gdbpy_parse_and_eval. > > The main issue with this patch is that I could not think of a non-racy > way to test it. Any ideas? We've had a similar patch in the Fedora RPM for a while. It's been on my list to upstream for a bit. Initially I was a bit reluctant because I hadn't audited all the Python reentry points in GDB to make sure we reacquired the GIL before interacting with Python. I realize now this was not a real concern (reviews would catch this and if one did slip by it would be fixed as a bug.) The only real difference in the patch approach was to make the GIL release an option over a default. I've included the relevant patch snippet here: --- a/gdb/python/python.c +++ b/gdb/python/python.c @@ -554,12 +554,16 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw) { const char *arg; PyObject *from_tty_obj = NULL, *to_string_obj = NULL; - int from_tty, to_string; - static const char *keywords[] = { "command", "from_tty", "to_string", NULL }; + int from_tty, to_string, release_gil; + static const char *keywords[] = {"command", "from_tty", "to_string", "release_gil", NULL }; + PyObject *release_gil_obj = NULL; - if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s|O!O!", keywords, &arg, + if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s|O!O!O!", keywords, &arg, &PyBool_Type, &from_tty_obj, - &PyBool_Type, &to_string_obj)) + &PyBool_Type, &to_string_obj, + &PyBool_Type, &release_gil_obj)) return NULL; from_tty = 0; @@ -580,12 +584,28 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw) to_string = cmp; } + release_gil = 0; + if (release_gil_obj) + { + int cmp = PyObject_IsTrue (release_gil_obj); + if (cmp < 0) + return NULL; + release_gil = cmp; + } + std::string to_string_res; TRY { struct interp *interp; + /* In the case of long running GDB commands, allow the user to + release the Python GIL. Restore the GIL + after the command has completed before handing back to + Python. */ + if (release_gil) As for a test, we also have a test included. It does not appear to be racy for our purposes. I also include it for consideration. This snippet is just for initial consideration and will make any changes needed to include it in the patch. diff --git a/gdb/testsuite/gdb.python/py-gil-mthread.c b/gdb/testsuite/gdb.python/py-gil-mthread.c new file mode 100644 --- /dev/null +++ b/gdb/testsuite/gdb.python/py-gil-mthread.c @@ -0,0 +1,13 @@ +#include <stdio.h> +#include <unistd.h> + +int +main (void) +{ + int i; + for (i = 0; i < 10; i++) + { + sleep (1); /* break-here */ + printf ("Sleeping %d\n", i); + } +} diff --git a/gdb/testsuite/gdb.python/py-gil-mthread.exp b/gdb/testsuite/gdb.python/py-gil-mthread.exp new file mode 100644 --- /dev/null +++ b/gdb/testsuite/gdb.python/py-gil-mthread.exp @@ -0,0 +1,69 @@ +# Copyright (C) 2014 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/>. + +standard_testfile .c .py +set executable $testfile + +if { [prepare_for_testing $testfile.exp $executable $srcfile] } { + return -1 +} + +# Skip all tests if Python scripting is not enabled. +if { [skip_python_tests] } { continue } + +if ![runto_main] { + return -1 +} + +gdb_breakpoint $srcfile:[gdb_get_line_number "break-here"] temporary +gdb_continue_to_breakpoint "break-here" ".* break-here .*" + +set test "response" +set timeout 60 +set sleeping_last -1 +set hello_last 0 +set minimal 5 +gdb_test_multiple "python exec (open ('$srcdir/$subdir/$srcfile2').read ())" $test { + -re "Error: unable to start thread\r\n" { + fail $test + } + -re "Sleeping (\[0-9\]+)\r\n" { + set n $expect_out(1, string) + if { $sleeping_last + 1 != $n } { + fail $test + } else { + set sleeping_last $n + if { $sleeping_last >= $minimal && $hello_last >= $minimal } { + pass $test + } else { + exp_continue + } + } + } + -re "Hello \\( (\[0-9\]+) \\)\r\n" { + set n $expect_out(1,string) + if { $hello_last + 1 != $n } { + fail $test + } else { + set hello_last $n + if { $sleeping_last >= $minimal && $hello_last >= $minimal } { + pass $test + } else { + exp_continue + } + } + } +} diff --git a/gdb/testsuite/gdb.python/py-gil-mthread.py b/gdb/testsuite/gdb.python/py-gil-mthread.py new file mode 100644 --- /dev/null +++ b/gdb/testsuite/gdb.python/py-gil-mthread.py @@ -0,0 +1,28 @@ +try: + import thread +except: + import _thread +import time +import gdb + +# Define a function for the thread +def print_thread_hello(): + count = 0 + while count < 10: + time.sleep(1) + count += 1 + print ("Hello ( %d )" % count) + +# Create a thread and continue +try: + thread.start_new_thread (print_thread_hello, ()) + gdb.execute ("continue", release_gil=True) +except: + try: + _thread.start_new_thread (print_thread_hello, ()) + gdb.execute ("continue", release_gil=True) + except: + print ("Error: unable to start thread") + +while 1: + pass Cheers, Phil ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] Release the GIL while running a gdb command or expression 2018-10-09 9:51 ` Phil Muldoon @ 2018-10-09 19:42 ` Tom Tromey 2018-10-10 8:33 ` Phil Muldoon 0 siblings, 1 reply; 6+ messages in thread From: Tom Tromey @ 2018-10-09 19:42 UTC (permalink / raw) To: Phil Muldoon; +Cc: gdb-patches, Tom Tromey >>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes: Phil> We've had a similar patch in the Fedora RPM for a while. It's been on Phil> my list to upstream for a bit. Initially I was a bit reluctant Phil> because I hadn't audited all the Python reentry points in GDB to make Phil> sure we reacquired the GIL before interacting with Python. I realize Phil> now this was not a real concern (reviews would catch this and if one Phil> did slip by it would be fixed as a bug.) Yes, I think it would result in a crash. Phil> The only real difference in the patch approach was to make the GIL Phil> release an option over a default. I don't think this is necessary, mostly because I can't think of when it would be desirable not to release the GIL; but also because when writing Python one doesn't normally have to worry about the GIL -- it's not really a Python-visible feature, nor should it be, since implementations like PyPY don't have it. Phil> As for a test, we also have a test included. It does not appear to be Phil> racy for our purposes. I also include it for consideration. This Phil> snippet is just for initial consideration and will make any changes Phil> needed to include it in the patch. I can try it. If it works, whose name should I put on the ChangeLog? Tom ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] Release the GIL while running a gdb command or expression 2018-10-09 19:42 ` Tom Tromey @ 2018-10-10 8:33 ` Phil Muldoon 2018-10-10 14:07 ` Tom Tromey 0 siblings, 1 reply; 6+ messages in thread From: Phil Muldoon @ 2018-10-10 8:33 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 09/10/2018 20:42, Tom Tromey wrote: >>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes: > > Phil> We've had a similar patch in the Fedora RPM for a while. It's been on > Phil> my list to upstream for a bit. Initially I was a bit reluctant > Phil> because I hadn't audited all the Python reentry points in GDB to make > Phil> sure we reacquired the GIL before interacting with Python. I realize > Phil> now this was not a real concern (reviews would catch this and if one > Phil> did slip by it would be fixed as a bug.) > > Yes, I think it would result in a crash. > > Phil> The only real difference in the patch approach was to make the GIL > Phil> release an option over a default. > > I don't think this is necessary, mostly because I can't think of when it > would be desirable not to release the GIL; but also because when writing > Python one doesn't normally have to worry about the GIL -- it's not > really a Python-visible feature, nor should it be, since implementations > like PyPY don't have it. It's not so much an implementation detail that should be exposed to the user but rather a change in behavior around gdb.execute. Given that now, with this patch, we always release the Python GIL during the execution of a GDB command via gdb.execute, any other Python thread that was previously blocked by the GIL is now unblocked, and it may appear to those threads that the Python thread that initiated the gdb.execute has returned from that command when it may not have (this is especially so in cases where a GDB command takes seconds to complete a command). Also, any other Python threads that wish to interact with GDB will have to wait until the GDB event loop returns to a state where it is accepting input (at least I think this is true). This may break some scripts out there. Are these scripts relying on what we now classify as a bug or is there is a reasonable expectation, on the users' behalf, that a script could rely on GDB's previous GIL blocking behavior? I'm not advocating we should have a release_gil parameter, I'm just unsure of the expectations of users and scripts out there, and that if we don't provide a mechanism to optionally block the GIL, it will cause disruption to any established scripts out there. I suppose the solution is to either provide a GIL blocking parameter or to thoroughly document this new behavior in the manual. What do you think? > Phil> As for a test, we also have a test included. It does not appear to be > Phil> racy for our purposes. I also include it for consideration. This > Phil> snippet is just for initial consideration and will make any changes > Phil> needed to include it in the patch. > > I can try it. If it works, whose name should I put on the ChangeLog? I wrote the GIL patch for Fedora so I suppose mine. The test itself may need some modification in that it can be optimized and it may also need some format tweaks. Cheers Phil ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] Release the GIL while running a gdb command or expression 2018-10-10 8:33 ` Phil Muldoon @ 2018-10-10 14:07 ` Tom Tromey 2018-10-10 14:38 ` Phil Muldoon 0 siblings, 1 reply; 6+ messages in thread From: Tom Tromey @ 2018-10-10 14:07 UTC (permalink / raw) To: Phil Muldoon; +Cc: Tom Tromey, gdb-patches >>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes: >> I don't think this is necessary, mostly because I can't think of when it >> would be desirable not to release the GIL; but also because when writing >> Python one doesn't normally have to worry about the GIL -- it's not >> really a Python-visible feature, nor should it be, since implementations >> like PyPY don't have it. Phil> It's not so much an implementation detail that should be exposed to Phil> the user but rather a change in behavior around gdb.execute. Given Phil> that now, with this patch, we always release the Python GIL during the Phil> execution of a GDB command via gdb.execute, any other Python thread Phil> that was previously blocked by the GIL is now unblocked, and it may Phil> appear to those threads that the Python thread that initiated the Phil> gdb.execute has returned from that command when it may not have (this Phil> is especially so in cases where a GDB command takes seconds to Phil> complete a command). Also, any other Python threads that wish to Phil> interact with GDB will have to wait until the GDB event loop returns Phil> to a state where it is accepting input (at least I think this is Phil> true). Actually we forbid using gdb APIs from threads other than the gdb thread. From python.texi: @value{GDBN} is not thread-safe. If your Python program uses multiple threads, you must be careful to only call @value{GDBN}-specific functions in the @value{GDBN} thread. @code{post_event} ensures this. Phil> This may break some scripts out there. Are these scripts Phil> relying on what we now classify as a bug or is there is a reasonable Phil> expectation, on the users' behalf, that a script could rely on GDB's Phil> previous GIL blocking behavior? I'm not advocating we should have a Phil> release_gil parameter, I'm just unsure of the expectations of users Phil> and scripts out there, and that if we don't provide a mechanism to Phil> optionally block the GIL, it will cause disruption to any established Phil> scripts out there. Phil> I suppose the solution is to either provide a GIL blocking parameter Phil> or to thoroughly document this new behavior in the manual. What do Phil> you think? I think there's little risk of this breaking anything. It seems like just an ordinary bug fix to me. Tom ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] Release the GIL while running a gdb command or expression 2018-10-10 14:07 ` Tom Tromey @ 2018-10-10 14:38 ` Phil Muldoon 0 siblings, 0 replies; 6+ messages in thread From: Phil Muldoon @ 2018-10-10 14:38 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 10/10/2018 15:07, Tom Tromey wrote: >>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes: > >>> I don't think this is necessary, mostly because I can't think of when it >>> would be desirable not to release the GIL; but also because when writing >>> Python one doesn't normally have to worry about the GIL -- it's not >>> really a Python-visible feature, nor should it be, since implementations >>> like PyPY don't have it. > > Phil> It's not so much an implementation detail that should be exposed to > Phil> the user but rather a change in behavior around gdb.execute. Given > Phil> that now, with this patch, we always release the Python GIL during the > Phil> execution of a GDB command via gdb.execute, any other Python thread > Phil> that was previously blocked by the GIL is now unblocked, and it may > Phil> appear to those threads that the Python thread that initiated the > Phil> gdb.execute has returned from that command when it may not have (this > Phil> is especially so in cases where a GDB command takes seconds to > Phil> complete a command). Also, any other Python threads that wish to > Phil> interact with GDB will have to wait until the GDB event loop returns > Phil> to a state where it is accepting input (at least I think this is > Phil> true). > > Actually we forbid using gdb APIs from threads other than the gdb > thread. From python.texi: > > @value{GDBN} is not thread-safe. If your Python program uses multiple > threads, you must be careful to only call @value{GDBN}-specific > functions in the @value{GDBN} thread. @code{post_event} ensures > this. Aha, well I learned something new today! > Phil> This may break some scripts out there. Are these scripts > Phil> relying on what we now classify as a bug or is there is a reasonable > Phil> expectation, on the users' behalf, that a script could rely on GDB's > Phil> previous GIL blocking behavior? I'm not advocating we should have a > Phil> release_gil parameter, I'm just unsure of the expectations of users > Phil> and scripts out there, and that if we don't provide a mechanism to > Phil> optionally block the GIL, it will cause disruption to any established > Phil> scripts out there. > > Phil> I suppose the solution is to either provide a GIL blocking parameter > Phil> or to thoroughly document this new behavior in the manual. What do > Phil> you think? > > I think there's little risk of this breaking anything. It seems like > just an ordinary bug fix to me. Ok. I looked at the patch and, from my point of view, it looks fine to me. I'll leave any testing decisions in your hands. Cheers Phil ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-10 14:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-15 4:07 [RFC] Release the GIL while running a gdb command or expression Tom Tromey 2018-10-09 9:51 ` Phil Muldoon 2018-10-09 19:42 ` Tom Tromey 2018-10-10 8:33 ` Phil Muldoon 2018-10-10 14:07 ` Tom Tromey 2018-10-10 14:38 ` Phil Muldoon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).