Bug 23841 - cpychecker reference count checking issues
Summary: cpychecker reference count checking issues
Status: NEW
Alias: None
Product: gdb
Classification: Unclassified
Component: python (show other bugs)
Version: HEAD
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-29 12:40 UTC by Tom de Vries
Modified: 2022-04-15 18:10 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tom de Vries 2018-10-29 12:40:09 UTC
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).
Comment 1 Tom de Vries 2018-10-29 12:43:32 UTC
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.
Comment 2 Tom de Vries 2018-10-29 12:50:32 UTC
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)
...
Comment 3 Tom Tromey 2022-04-15 18:10:17 UTC
I suspect we should just remove the cpychecker annotations.
My sense is that it isn't supported all that much and especially
not for C++.