On 16/02/16 10:54, Dave Gordon wrote:
On 15/02/16 15:56, Martin Peres wrote:
On 15/02/16 15:47, Dave Gordon wrote:
On 15/02/16 13:40, Martin Peres wrote:
On 15/02/16 14:24, Dave Gordon wrote:
On 12/02/16 16:31, Martin Peres wrote:
This is not a big issue to return -1 since the only codepath that
uses
it is for display purposes.
Caught by Klockwork.
Signed-off-by: Martin Peres <martin.peres@xxxxxxxxxxxxxxx>
---
src/intel_device.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/intel_device.c b/src/intel_device.c
index 54c1443..35e652a 100644
--- a/src/intel_device.c
+++ b/src/intel_device.c
@@ -650,7 +650,10 @@ int __intel_peek_fd(ScrnInfoPtr scrn)
dev = intel_device(scrn);
assert(dev && dev->fd != -1);
Doesn't Klocwork recognise the assert() above?
I thought that would tell it that dev can't be NULL.
It does not, I had to close many false positives related to this...
Hmmm .. elsewhere (e.g. [4/7]) you have /added/ an assert, which I
thought must be so that Klocwork stops complaining that something might
be NULL ... maybe it can't handle the composite assertion? Does it
silence the complaint if you change:
assert(dev && dev->fd != -1);
into:
assert(dev);
assert(dev->fd != -1);
?
Sure, I added an assert, but not to silence patchwork, just to make sure
we have no problem. I cannot run klokwork myself and my goal was not to
silence but instead to check the reported issues.
David is right, I think Klokwork only cares about runtime checks and
wants to make sure that we never de-reference a NULL pointer.
Martin
Klocwork is trying (by static analysis) to find all reachable code, with
all possible parameter values at each point. It's configured with
various checkers that examine each expression reached for things such as
dereferencing a possibly-NULL pointer, or indexing beyond the bounds of
an array, or integer overflow, or many other things ...
The standard definition of assert() is something like:
#define assert(x) do { if(!(x)) abort(); } while (0)
and Klocwork knows that abort() doesn't return, so in the block
dev = intel_device(scrn);
assert(dev);
return dev->fd;
it can deduce that the 'return' is reached only if the abort() was not,
hence only if 'dev' is non-NULL. Therefore, this doesn't produce a
complaint about a possibly-NULL pointer, because Klocwork knows it isn't
because of the assert().
Of course there are potentially multiple definitions of assert(),
typically including a null one, for production code, and a debug version
that gives more detail. So the usual thing is to ensure that there's a
Klocwork-specific version that allows KW to do the analysis above, even
if that version isn't something you would ever run:
#if defined(__KLOCWORK__)
#define assert(x) do { if(!(x)) abort(); } while (0)
#elif defined(NO_DEBUG)
#define assert(x) do { /* nothing */ ; } while (0)
#elif defined(EXTRA_DEBUG)
#define assert(x) do { my_assert(x, #x, __LINE__, __FILE__); } while (0)
#else
// ... etc ...
#endif
That sounds like a good idea, yes.
Here is the current definition:
https://cgit.freedesktop.org/xorg/driver/xf86-video-intel/tree/src/sna/xassert.h
If we don't have something like this, Klocwork may not be able to make
effective deductions about the possible values of variables at specific
points, so it would be worth checking that we're using macros that it
understands.
We don't, because there is a test on NDEBUG which Klokwork cannot make
assumptions on, as David said.
I will hold on to this idea a little as there are talks internally on to
which static analysis tool needs to be used.
Thanks for your feedback,
Martin
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
