Re: [patch] Compilation regression with python-2.6 [Re: [PATCH 24/28] introduce gdb_pymodule_addobject]

Pedro Alves
2013-05-21T16:48:28Z

On 05/21/2013 05:36 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
> Pedro> I notice that py_decref could actually be using Py_XDECREF instead
> Pedro> of doing the explicit 'if (py)' check.
>
> Pedro> - /* Note that we need the extra braces in this 'if' to avoid a
> Pedro> - warning from gcc. */
> Pedro> if (py)
> Pedro> - {
> Pedro> - Py_DECREF (py);
> Pedro> - }
> Pedro> + Py_DECREF (py);
>
> The X is missing.

Oh, I did not intend to do that change in the 7.6 workaround patch.
I was leaving that patch concerned only with the scoping issue.

>
> Also, as discussed on irc, if you do this you might as well zap
> make_cleanup_py_xdecref and py_xdecref.

Yeah. As discussed on irc, given the existence of py_xdecref, I
think py_decref calling Py_XDECREF would be odd. Given the only difference
between Py_DECREF and Py_XDECREF is that the latter allows passing
in a NULL object, while the former prohibits it, it's natural to expect
the same from py_decref vs py_xdecref.

> I think having one set of cleanup functions is plenty. The NULL check
> can't cost much, either in performance or clarity.

I removed the NULL check from py_decref, and ran the testsuite.
It caused a GDB crash in py-pp-maint.exp. I did that before seeing your
note... See below. The issue is of course that make_cleanup_py_decref
is installed on a NULL object. I could have used make_cleanup_py_xdecref,
but I actually went the other way... I didn't mean to get myself
into Python hacking. :-) If you think this is not good, then I'll
just file a PR.

> Really I would prefer to get rid of decref cleanups at all, since they
> mess with the checker. But this is a pain, and probably not worth the
> effort unless we think we could really get to 0 false reports.

---

gdb/python/py-prettyprint.c | 6 +++++-
gdb/python/py-utils.c | 3 +--
2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/gdb/python/py-prettyprint.c b/gdb/python/py-prettyprint.c
index 8c45cd6..8fa2f42 100644
--- a/gdb/python/py-prettyprint.c
+++ b/gdb/python/py-prettyprint.c
@@ -735,8 +735,12 @@ apply_val_pretty_printer (struct type *type, const gdb_byte *valaddr,
/* Find the constructor. */
printer = find_pretty_printer (val_obj);
Py_DECREF (val_obj);
+
+ if (printer == NULL)
+ goto done;
+
make_cleanup_py_decref (printer);
- if (! printer || printer == Py_None)
+ if (printer == Py_None)
goto done;

/* If we are printing a map, we want some special formatting. */
diff --git a/gdb/python/py-utils.c b/gdb/python/py-utils.c
index 80bacf7..7c7c5ca 100644
--- a/gdb/python/py-utils.c
+++ b/gdb/python/py-utils.c
@@ -31,8 +31,7 @@ py_decref (void *p)
{
PyObject *py = p;

- if (py)
- Py_DECREF (py);
+ Py_DECREF (py);
}

/* Return a new cleanup which will decrement the Python object's

--
Pedro Alves