| Message ID | 1455294689-29249-2-git-send-email-martin.peres@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 12, 2016 at 06:31:23PM +0200, Martin Peres wrote: > This is not a big issue to return -1 since the only codepath that uses > it is for display purposes. Impossible. -Chris
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@linux.intel.com> > --- > 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. .Dave. > - return dev->fd; > + if (!dev) > + return -1; > + else > + return dev->fd; > } > > int intel_has_render_node(struct intel_device *dev)
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@linux.intel.com> >> --- >> 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...
On Mon, Feb 15, 2016 at 12:24:04PM +0000, 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@linux.intel.com> > >--- > > 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. My guess is that klockwork recognises that assert() can be a no-op if NDEBUG is defined and in such case won't generate code. In such a case neither of those two checks are performed. Kind regards, David
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@linux.intel.com> >>> --- >>> 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); ? .Dave.
On 15/02/16 13:43, David Weinehall wrote: > On Mon, Feb 15, 2016 at 12:24:04PM +0000, 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@linux.intel.com> >>> --- >>> 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. > > My guess is that klockwork recognises that assert() can be a no-op > if NDEBUG is defined and in such case won't generate code. > In such a case neither of those two checks are performed. > > Kind regards, David Klocwork is a static analysis tool, it doesn't generate code at all. It's supposed to recognise assert() and similar macros as constraints on what values particular expressions may have; in particular, it knows that if a code path ends with a call to abort(), that path *should* never be taken and it will assume (for static analysis purposes) that it *will* not be taken. Thus any combination of values that leads to an abort() is considered "impossible" and need not be further checked. Then assert() is typically defined as: #define assert(x) do { if(!(x)) abort(); } while (0) and then int foo(char *s) { assert(s); return *s; } should not cause Klocwork to complain, whereas without the assert() it should say that 's' might be NULL when dereferenced. If it's getting false positives it may be that Klocwork hasn't been told the proper (conditional) definition for assert()? .Dave.
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@linux.intel.com> >>>> --- >>>> 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
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@linux.intel.com> >>>>> --- >>>>> 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 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. .Dave.
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@linux.intel.com> >>>>>> --- >>>>>> 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
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); - return dev->fd; + if (!dev) + return -1; + else + return dev->fd; } int intel_has_render_node(struct intel_device *dev)
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@linux.intel.com> --- src/intel_device.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)