On Thu, 2012-03-01 at 03:19 +0000, Daniele Varrazzo wrote:
[...snip..]
The new flags have been really helpful, and have allowed to model
perfectly the exceptions flow into the library.
Thanks.
A small family of functions couldn't be modeled: stuff like
returning
a char * on success, NULL+exception on failure. In my case they were
not so many and are not exposed, so it's not been a problem to convert
"char *foo()" into "int foo(char **)" and flag them as raising on
negative value <
https://github.com/dvarrazzo/psycopg/commit/f2e4a8ed>.
I notice that two of the functions that couldn't be modeled are
"static". I've been wondering about making the checker a bit more
clever about interprocedural analysis. [I'm updating the Subject
accordingly].
Currently the checker works on one function at a time, and doesn't have
any "whole program" knowledge: when analyzing function foo() it assumes
that any functions that foo calls follow a particular set of
reference-counting rules (unless overridden by attributes):
But it would save a lot of attribute-marking if the checker was able to
take into account the actual behaviors of those functions, and if
they're in the same source file, then we're already analyzing them at
some point during the compilation.
So I'm wondering if we should do a two-pass analysis:
* pass 1: analyze all functions, capturing the traces for those for
which all function calls they make are to functions known to the checker
ahead of time (CPython API calls). For those functions that call other
functions (or call through function pointers), we can't capture the
transitions yet, but we can make a record of which functions they call.
* pass 2: sort the functions based on which functions call which other
functions (topological sort, I think), then use the information we've
already generated to redo the analysis for the functions for which
previously unknown functions were called.
My hope is that this would remove the need for manually adding
attributes to static helper functions, so if e.g. you have:
static void raise_some_err(const char *msg)
{
PyErr_SetString(SomeException, msg);
}
then raise_some_err would get analyzed in pass 1 and would be known to
have a single transition in which the exception state is set. Functions
that use it would get analyzed in pass 2, and the checker would know the
behavior, and avoid the false positive. This might also eliminate the
need for more complicate markings e.g. for the case where the effect a
function has on reference counts varies based on the result.
But this is considerably more complicated than what I've been doing up
until now. I wonder if we can perhaps defer the checker until gcc
itself has had a chance to do some inlining; perhaps that would solve a
lot of these problems for us? Or perhaps we could even use gcc's
inlining machinery to take our own copies of the insides of the
functions, and inline this special-purpose copy of the functions, and
inline them much more aggressively than gcc would normally do? (but,
again, this is much more complicated than what we've been doing so far).
This approach doesn't help for the case of cycles in the function
callgraph (e.g. recursion, or mutual recursion), or where a helper
function is defined in a different source file to the one being
compiled.
I'm not yet sure if I want to tackle the above.
Thinking aloud a bit more, we have several different ways of expressing
the behavior of a function:
* analyzing it by interpreting the gimple representation, within
absinterp.py
* the descriptions given in the various impl_ functions in
refcounts.py
* the attributes
I wonder if there's a way to unify these a bit more.
From a style PoV, I find the CPYCHECKER_* attribute macro too
verbose
to be practical: I've given them shorter aliases (BORROWED, STEALS(n),
RAISES, RAISES_NEG)
<
https://github.com/dvarrazzo/psycopg/commit/5bfb6cde>.
Unfortunately, changing the signature for the handler functions for
the "O&" format
<
https://github.com/dvarrazzo/psycopg/commit/4d15b973>
has't worked as expected: the traceback is still there. I also notice
that these functions probably cannot be modeled with the current set
of flags: in case of success they return 1 and steal a reference from
the 1st argument, in case of failure return 0 and don't steal. I think
the usage for this type of functions is too limited to be of concern
though, ok not to make it crash, but probably modeling them is not a
priority.
They are static, which means that they might be amenable to the approach
described above.
Last small thing to report: there is a funny false positive that has
required the patch in
<
https://github.com/dvarrazzo/psycopg/commit/94327872>: without it,
the function was reported as returning -1 without exception set. Is it
the expected behaviour (e.g. assuming that the value of
curs->conn->async_cursor could change in another thread)?
I'm not sure.
Please can you try to create a minimal C file that
demonstrates the false positive?
Will try to work around the source file that fails compiling
tomorrow,
and try to do something else for the plugin. I've noticed the
PyString_AsStringAndSize missing, but its semantic is peculiar
(returns a buffer in a char ** on success... don't know where to copy
that!), so I will really need to learn something to implement it.
I'd like an opinion from you: Ideally, I imagine running the checker
as a QA step, for instance before releasing. It could be run by
buildbot after every commit etc. There are a few issues that seem hard
to be solved, and OTOH is pretty clear that they are not a real
problem. Do you have in program a strategy to eventually silence them
somehow, so that a script may run the analysis and have a clean result
even if there are a few known false positives? Or do you think the
plugin will only remain as an interactive help?
I don't think we're going
to be able to eliminate all false positives.
We may be able to add some way of marking known false-positives within
the code (e.g. via pragmas).
Hope this is helpful
Dave