On Mon, 2012-02-27 at 23:30 +0000, Daniele Varrazzo wrote:
On Mon, Feb 27, 2012 at 7:43 PM, David Malcolm
<dmalcolm(a)redhat.com> wrote:
> This adds:
> __attribute__((cpychecker_negative_result_sets_exception))
Great, thank you for the quick roundtrip!
(various replies inline below)
> #define CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION() \
Note that you are proposing to use a macro with arguments: it looks
redundant, or at least not consistent with the other attribute macros.
This idiom is in the docs example too, as I can read from the patch.
I've removed the args from the docs in the latest commit.
> Does this give you want you need?
It is mostly working. The problem I'm finding now is when the
exception is set in another function, for instance, the typical
implementation of these -1-raising functions is something like:
int something() {
rv = -1
/* work work ... */
if (rv < 0) {
pq_complete_error(self, &pgres, &error);
}
return rv;
}
these functions get a "function is marked with
__attribute__((cpychecker_negative_result_sets_exception)) but can
return -1 without setting an exception". I have re-defined
pq_complete_error from returning void to returning int, and always
returning -1, but this doesn't help. OTOH, if I say:
if (rv < 0) {
rv = pq_complete_error(self, &pgres, &error);
}
no warning is generated. Unfortunately this pattern is not optimal, as
rv could be a different negative return value, to specify different
failure conditions, and this detail would be swallowed.
Likely the same shortcoming, a function:
PyObject *f() {
rv = NULL;
if (bad_stuff) {
psyco_set_error(ProgrammingError, self, "reason", NULL, NULL);
goto exit;
}
/* work work */
rv = some_object();
exit:
return rv;
}
this function is flagged as "returning (PyObject*)NULL without setting
an exception" even if psyco_set_error is flagged as setting an
exception on negative return, and constantly returns -1. I have tried
changing the psyco_set_error() signature as returning a PyObject* and
always returning NULL, but this is even worse as then it looks I get a
value I don't decref.
I've added a new attribute which marks a function as always setting an
exception:
__attribute__((cpychecker_sets_exception))
in 9e00c3a6648660c65415b9f2799f97da925da99f, which I hope will capture
the behavior of exception-setting helper functions like psyco_set_error.
Do you think it is something that can be fixed or I'm starting
asking
too much to the checker? Would it be handy a self-containing example
triggering the false positive to be added to the test suite?
A higher-level caution: please be careful not to place too much trust in
the checker! Remember that this is only alpha-quality software. I
expect there to be plenty of bugs in the checker.
If you find yourself having to make too many changes to the code to
satisfy the checker, it may be that the checker is getting it wrong, or
that the checker simply can't cope with the pattern of code that it's
seeing. The checker only analyzes each function individually: it has no
"whole program" knowledge, it merely tries to verify some heuristics
about the typical behaviors of individual functions within a CPython
extension.
It's possible to write perfectly correct CPython extensions that cause
the checker to emit numerous false warnings: a good example here would
be functions that return borrowed references, rather than new
references. In the automated analysis runs I've been doing, any time I
see a "perfect" result from the checker, it turns out to be a bug :(
(usually there was issue with the build flags, and the checker didn't
get run).
So I'd be wary about trying to achieve 100% perfection from the checker
at this point. Sorry if this comes across as negative, but when I read
about "worsening the code" to satisfy the checker, I have mixed
emotions: the checker is intended to serve you, not the other way
around. I don't want to create extra work for you: the idea is to make
it easier to identify genuine bugs, so that they can be fixed.
I'm sorry if the nuances here are unclear; email is a bad medium for
having this kind of discussion.
A separate, smallish issue, yet requiring "worsening the
code" in
order to silence a false positive is a function returning a specific
object - XidObject - a "subclass" of PyObject.
A caller to the function "XidObject *xid_ensure(PyObject *o)":
XidObject *xid = NULL;
PyObject *oxid; /* from the args */
if (NULL == (xid = xid_ensure(oxid))) {
goto exit;
}
is also flagged as returning NULL without setting an exception. The
above statement specifically is marked as "when treating unknown
struct XidObject * from ... as NULL
taking True path".
Fixing the warning by changing the xid_ensure signature as returning
PyObject *, and using it as "xid = (XidObject *)xid_ensure(oxid)"
works, but is not ideal.
Is there any way to propagate the PyObject semantics to its subtypes?
I'm investigating this.
Thank you very much for your active feedback.
You're welcome; likewise, thanks for yours!
Dave