[Bug python/23841] New: cpychecker reference count checking issues

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23841

Bug ID: 23841
Summary: cpychecker reference count checking issues
Product: gdb
Version: HEAD
Status: NEW
Severity: normal
Priority: P2
Component: python
Assignee: unassigned at sourceware dot org
Reporter: vries at gcc dot gnu.org
Target Milestone: ---

Gdb supports a python interface, which is implemented using Python C
extensions.

The extensions are written in C, and interact with the Python interpreter.

Certain aspects of the Python runtime environment need to be taken into account
in the extensions, which is difficult to get right manually.

Using the static checker cpychecker, we try to find such problems in the
extension implementations. [ We can use any tool, but cpychecker seems the one
typically used, given that gdb source code contains cpychecker annotations. ]

One example of something the extensions need to take into account is the
reference counting aspect of objects, and cpychecker has a check for that.

The typical example is:
...
1 #include "Python.h"
2
3 PyObject *
4 test (PyObject *self, PyObject *args)
5 {
6 PyObject *list;
7 PyObject *item;
8
9 list = PyList_New (1);
10 if (list == NULL)
11 return NULL;
12
13 item = PyLong_FromLong (42);
14
15 /* This error handling is incorrect: it's missing an
16 invocation of Py_DECREF(list): */
17 if (!item)
18 return NULL;
19
20 /* This steals a reference to item; item is not leaked when we get
here: */
21 PyList_SetItem (list, 0, item);
22
23 return list;
24 }
...
where cpychecker produces an error indicating the missing Py_DECREF at line 18,
which can then be fixed using:
...
$ diff -u test.c.initial test.c.fixed
--- test.c.initial 2018-10-29 10:45:30.215131722 +0100
+++ test.c.fixed 2018-10-29 10:42:29.211132413 +0100
@@ -15,7 +15,10 @@
/* This error handling is incorrect: it's missing an
invocation of Py_DECREF(list): */
if (!item)
- return NULL;
+ {
+ Py_DECREF (list);
+ return NULL;
+ }

/* This steals a reference to item; item is not leaked when we get here: */
PyList_SetItem (list, 0, item);
...

However, gdb has a solution for easier writing of correct ref counting code:
gdbpy_ref, a smart pointer that calls Py_DECREF when the destructor is called:
...
PyObject *
test (PyObject *self, PyObject *args)
{
PyObject *item;

gdbpy_ref<> list (PyList_New (1));
if (list == NULL)
return NULL;

item = PyLong_FromLong (42);

if (!item)
return NULL;

/* This steals a reference to item; item is not leaked when we get here: */
PyList_SetItem (list.get (), 0, item);

return list.release ();
}
...
This means we don't have to add the Py_DECREF to the return NULL as before, but
OTOH we now need to manage more explicitly the regular execution path (here
using list.get and list.release).

However, cpychecker does not support interprocedural analysis (
https://github.com/davidmalcolm/gcc-python-plugin/issues/80 ), and consequently
generates false positives for the gdbpy_ref code (since it fails to recognize
that the list destructor calls Py_DECREF).

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23841

--- Comment #1 from Tom de Vries <vries at gcc dot gnu.org> ---
With a compile of master gdb with gcc-4.8 and master cpy-checker branch, I get:
...
$ grep '^/.*error:' build/MAKELOG | grep -v "Unhandled Python exception" |
egrep -c 'refcnt'
276
...

Typically, we want to drive error counts to zero, but with false positives
that's not doable without having a mechanism to register reviewed-and-accepted
false positives, f.i. by keeping a reviewed-and-accepted false positives
database file in the sources, but that may be cumbersome to maintain, and with
cpychecker being a gcc plugin with slightly different behaviour for each gcc
version, it may be even more cumbersome.

In absence of such a solution, we can still compare cpychecker error logs with
and without patch when testing changes to the extensions, and hope to detect
real problems, but of course we still need to filter false positives.

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23841

--- Comment #2 from Tom de Vries <vries at gcc dot gnu.org> ---
The cpychecker issue "provide a way to indicate that refcounting rules should
not apply" ( https://github.com/davidmalcolm/gcc-python-plugin/issues/58 )
identifies another false positive ref counting issue in gdb:
...
../../archer/gdb/python/py-block.c: In function ‘set_block’:
../../archer/gdb/python/py-block.c:265:1: error: ob_refcnt of '*obj' is 1 too
low [-Werror]
../../archer/gdb/python/py-block.c:265:1: note: was expecting final ob_refcnt
to be N + 1 (for some unknown N)
...