From patchwork Tue Oct 9 09:51:11 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Phil Muldoon X-Patchwork-Id: 29676 Received: (qmail 117561 invoked by alias); 9 Oct 2018 09:51:18 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 117545 invoked by uid 89); 9 Oct 2018 09:51:17 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=oo, slip, phil, H*RU:2a02 X-HELO: mail-wr1-f41.google.com Received: from mail-wr1-f41.google.com (HELO mail-wr1-f41.google.com) (209.85.221.41) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 09 Oct 2018 09:51:15 +0000 Received: by mail-wr1-f41.google.com with SMTP id n11-v6so981865wru.13 for ; Tue, 09 Oct 2018 02:51:15 -0700 (PDT) Return-Path: Received: from ?IPv6:2a02:c7f:ae6a:ed00:4685:ff:fe66:9f4? ([2a02:c7f:ae6a:ed00:4685:ff:fe66:9f4]) by smtp.gmail.com with ESMTPSA id y12-v6sm12990959wrq.33.2018.10.09.02.51.12 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 09 Oct 2018 02:51:12 -0700 (PDT) Subject: Re: [RFC] Release the GIL while running a gdb command or expression To: gdb-patches@sourceware.org, Tom Tromey References: <20180915040732.6718-1-tom@tromey.com> From: Phil Muldoon Message-ID: <807f284a-e227-37ed-c197-170a7f2abe40@redhat.com> Date: Tue, 9 Oct 2018 10:51:11 +0100 MIME-Version: 1.0 In-Reply-To: <20180915040732.6718-1-tom@tromey.com> X-IsSubscribed: yes 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: Cheers, Phil --- 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 +#include + +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 . + +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