Re: Dominator Analysis

Adding new methods and attributes to the classes requires editing the appropriate generate-*.py script to wire up the new entrypoint.  For very simple attributes you can embed the C code directly there, but anything that's more than a one-liner should have its implementation in the relevant C file. So you'd edit:  * generate-cfg-c.py   to add the new methods and attributes to the relevant tables of callbacks  * gcc-python-wrappers.h   to add declarations of the new C functions  * gcc-python-cfg.c   to add the implementations of the new C functions

Going through the patch itself: +static PyObject * +gcc_python_dominates(PyGccBasicBlock *n, PyObject *args) +{ +    static int calc = 0; +    PyGccBasicBlock *a, *b; + +    if(calc == 0) { +        calculate_dominance_info (CDI_DOMINATORS); +        calc = 1; +    } I'm a little wary about the global state that "calc" introduces here: it looks like you're lazily calculating the dominance info.  Where does GCC normally calculate the information?  Could the CFG have changed since the last call to this function?

since PyArg_ParseTuple will have set the current thread's exception for us, thus printing a Python traceback with a good error message, rather than halting GCC with an inscrutable Internal Compiler Error.

(b) The "O" flag only checks for an object; it doesn't do any type checking, so if someone passes in something that isn't a gcc.BasicBlock, there's an implicit cast to the wrong C struct, and we'll likely have a segfault.  The easy way to avoid this is to use the "O!" format code, which takes a PyTypeObject and gives us type checking against it, so something like this:  if (!PyArg_ParseTuple(args, "O!O!",                      &gcc_BasicBlock_Type, &a,                      &gcc_BasicBlock_Type, &b)) {     return NULL;  } so that if the args aren't of the correct type, a meaningful exception will be set for us.

+    {"dominates", +     (PyCFunction)gcc_python_dominates, +     (METH_VARARGS | METH_KEYWORDS), +     "Checks if a BB dominates another."}, ^^^^ if you use METH_KEYWORDS, the callback function takes 3 arguments (and you need to use PyArg_ParseTupleAndKeywords() instead): http://docs.python.org/c-api/structures.html#METH_KEYWORDS despite needing a cast to (PyCFunction). This *is* confusing, but note that the plugin's own gcc-with-cpychecker script actually checks for all of these issues (which may be why I'm being pedantic about them - sorry!).   If you want to try doing so, it's possible to build the plugin once, then compile it with itself (using CC=gcc-with-cpychecker as a Makefile variable:  make CC=/path/to/a/clean/build/of/the/plugin/gcc-with-cpychecker though unfortunately it doesn't quite compile itself cleanly right now :(

 static PyObject * +gcc_python_calculate_dominance_info(PyObject *n, PyObject *args) +{ +    calculate_dominance_info (CDI_DOMINATORS); +    Py_RETURN_NONE; +} + +static PyObject * +gcc_python_free_dominance_info(PyObject *n, PyObject *args) +{ +    free_dominance_info (CDI_DOMINATORS); +    Py_RETURN_NONE; +} + +static PyObject *  gcc_python_dominates(PyGccBasicBlock *n, PyObject *args)  { -    static int calc = 0;     PyGccBasicBlock *a, *b; - -    if(calc == 0) { -        calculate_dominance_info (CDI_DOMINATORS); -        calc = 1; -    } - Looks like you fixed the issue I mentioned above.  It may make it easier to review if you generate a diff combining all of the changes (not sure).

+        py_son = gcc_python_make_wrapper_basic_block(son); This could fail (e.g. under low memory conditions); you need something like this:         if (!py_son) {              goto error;         }

at this point, either py_son "owns" a reference on a gcc.BasicBlock: every path onwards needs to DECREF it, or the object will leak. +        if (-1 == PyList_Append(result, py_son)) {                Py_DECREF(py_son); /* need one here */ +               goto error; +           }                Py_DECREF(py_son);  /* need another one here */ +    }

@@ -386,6 +421,11 @@ static PyMethodDef GccMethods[] = {      (METH_VARARGS | METH_KEYWORDS),      "Frees dominance info."}, +    {"dominator_sons", +     (PyCFunction)gcc_python_dominator_sons, +     (METH_VARARGS | METH_KEYWORDS), +     "Returns the dominated sons of a bb."}, Is this recursive?

 static PyObject * +gcc_python_loop_contains(PyObject *n, PyObject *args) +{ +    int result; +    PyGccBasicBlock *a, *b; +    struct loop *A, *B, *step; +    gcc_assert(PyArg_ParseTuple(args, "OO", &a, &b)); Again, sorry for my lack of knowledge of GCC at this point, but would it be possible to expose the "loop" type in python form?  (would it be a good idea?  I don't know, but it might make it possible to implement this kind of analysis in Python rather than in C, and it then becomes much easier to hack on, try new algorithms, etc.  But wrapping a new type does take some effort).

+static PyObject * +gcc_python_loop_depth(PyObject *n, PyObject *args) +{ +    PyGccBasicBlock *py_bb; +    gcc_assert(PyArg_ParseTuple(args, "O", &py_bb)); + +    return Py_BuildValue("i", py_bb->bb->loop_depth);  } Probably should be a readonly attribute of the gcc.BasicBlock (as a getter within the PyGetSetDef table), rather than a function of the "gcc" module, so that you're dealing with "self". Sorry if the above came across as negative, thanks again for your work on this, and I hope the above makes sense.