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

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

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

agentzh at gmail dot com
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).

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug python/23841] cpychecker reference count checking issues

agentzh at gmail dot com
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.

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug python/23841] cpychecker reference count checking issues

agentzh at gmail dot com
In reply to this post by agentzh at gmail dot com
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)
...

--
You are receiving this mail because:
You are on the CC list for the bug.