Message ID | 20170525162612.GA10119@turing |
---|---|
State | New, archived |
Headers |
Received: (qmail 42439 invoked by alias); 25 May 2017 16:26:25 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 42338 invoked by uid 89); 25 May 2017 16:26:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 spammy=shutting, HContent-Transfer-Encoding:8bit X-HELO: esther.thepaul.org Received: from thepaul.xen.prgmr.com (HELO esther.thepaul.org) (71.19.158.106) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 25 May 2017 16:26:17 +0000 Received: from aus0.edge.epochlabs.com ([38.67.3.216] helo=turing) by esther.thepaul.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2) (envelope-from <paul@thepaul.org>) id 1dDvad-0004RU-Av for gdb-patches@sourceware.org; Thu, 25 May 2017 11:26:19 -0500 Date: Thu, 25 May 2017 11:26:12 -0500 From: paul cannon <paul@thepaul.org> To: gdb-patches@sourceware.org Subject: [PATCH][PR python/21460] Avoid segfault during Python cleanup Message-ID: <20170525162612.GA10119@turing> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit User-Agent: Mutt/1.5.24 (2015-08-30) |
Commit Message
paul cannon
May 25, 2017, 4:26 p.m. UTC
Rationale for the patch and repro instructions are explained in the bug. I don't have any copyright assignment on file but this really should be trivial enough to avoid that, I think. gdb/Changelog: 2017-05-25 paul cannon <paul@thepaul.org> python/21460 * python.c (gdbpy_set_quit_flag) Check Py_IsInitialized() before calling PyErr_SetInterrupt(), as Python may be shutting down already. --- gdb/python/python.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) -- 2.7.4
Comments
Hi Paul, On 2017-05-25 18:26, paul cannon wrote: > Rationale for the patch and repro instructions are explained in the > bug. Thanks for the patch, and sorry for the little delay in processing it. I think the code in itself is good. I'll just point out a few things to fix. > I don't have any copyright assignment on file but this really should > be trivial enough to avoid that, I think. That's what I think too. Do you intend to contribute to GDB regularly? If so we can get you write access to the repo. If not, I can push the patch on your behalf (when the issues I mentioned are fixed). > gdb/Changelog: > 2017-05-25 paul cannon <paul@thepaul.org> Should your name have capital letters :) ? > > python/21460 > * python.c (gdbpy_set_quit_flag) Check Py_IsInitialized() before > calling PyErr_SetInterrupt(), as Python may be shutting down already. The ChangeLog should only contain "what" changed and not "why". So just the first part: Check Py_IsInitialized() before calling PyErr_SetInterrupt(). is sufficient. However, the why should be contained in the commit message. You did a good job at explaining the problem in the bug you filed, so I think you could just take that and put it in the commit log (and massage it a bit if needed). > > --- > gdb/python/python.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/gdb/python/python.c b/gdb/python/python.c > index be92f36..c6a8c17 100644 > --- a/gdb/python/python.c > +++ b/gdb/python/python.c > @@ -247,7 +247,10 @@ gdbpy_enter::~gdbpy_enter () > static void > gdbpy_set_quit_flag (const struct extension_language_defn *extlang) > { > - PyErr_SetInterrupt (); > + if (Py_IsInitialized ()) > + { > + PyErr_SetInterrupt (); > + } Drop the braces for a single line body: if (Py_IsInitialized ()) PyErr_SetInterrupt (); > } > > /* Return true if the quit flag has been set, false otherwise. */ > -- > 2.7.4 Thanks! Simon
On Sunday, June 11 2017, Simon Marchi wrote: >> >> python/21460 >> * python.c (gdbpy_set_quit_flag) Check Py_IsInitialized() before >> calling PyErr_SetInterrupt(), as Python may be shutting down already. > > The ChangeLog should only contain "what" changed and not "why". So > just the first part: > > Check Py_IsInitialized() before calling PyErr_SetInterrupt(). > > is sufficient. However, the why should be contained in the commit > message. You did a good job at explaining the problem in the bug you > filed, so I think you could just take that and put it in the commit > log (and massage it a bit if needed). Also, I think it's worth mentioning that the ChangeLog lines shouldn't exceed 76 chars (soft limit). And the file 'python.c' is inside the python/ directory. So a good example would be: yyyy-mm-dd Name <email> PR python/21460 * python/python.c: Check Py_IsInitialized() before calling PyErr_SetInterrupt(). Cheers,
diff --git a/gdb/python/python.c b/gdb/python/python.c index be92f36..c6a8c17 100644 --- a/gdb/python/python.c +++ b/gdb/python/python.c @@ -247,7 +247,10 @@ gdbpy_enter::~gdbpy_enter ()  static void  gdbpy_set_quit_flag (const struct extension_language_defn *extlang)  { -  PyErr_SetInterrupt (); +  if (Py_IsInitialized ()) +   { +    PyErr_SetInterrupt (); +   }  }   /* Return true if the quit flag has been set, false otherwise.  */