Xen devel - Sep 2013 - [PATCH] RFC xen: suppress Coverity warnings about atomic_read and atomic_set.

Coverity generates false positives when read_atomic() and
write_atomic() are called with pointers to objects smaller than 64
bits (because it can''t see that the 64-bit access in the switych
statement is dead code).

I don''t want to automatically suppress all ofthose, because
read_atomic() and write_atomic() could still be called with mis-cast
pointers, but for atomic_t accessors it''s pretty clealry always safe.

RFC because I''m not sure what people think about scattering coverity
annotations in the code.

Signed-off-by: Tim Deegan <tim@xen.org>
---
 xen/include/asm-x86/atomic.h |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h
index e476ab5..cfa3f66 100644
--- a/xen/include/asm-x86/atomic.h
+++ b/xen/include/asm-x86/atomic.h
@@ -70,7 +70,11 @@ typedef struct { int counter; } atomic_t;
  * Atomically reads the value of @v.
  */
 #define _atomic_read(v)  ((v).counter)
-#define atomic_read(v)   read_atomic(&((v)->counter))
+static inline int atomic_read(atomic_t *v)
+{
+    /* coverity[incompatible_cast : FALSE] */
+    return read_atomic(&v->counter);
+}
 
 /**
  * atomic_set - set atomic variable
@@ -80,7 +84,11 @@ typedef struct { int counter; } atomic_t;
  * Atomically sets the value of @v to @i.
  */ 
 #define _atomic_set(v,i) (((v).counter) = (i))
-#define atomic_set(v,i)  write_atomic(&((v)->counter), (i))
+static inline void atomic_set(atomic_t *v, int i)
+{
+    /* coverity[incompatible_cast : FALSE] */
+    write_atomic(&v->counter, i);
+}
 
 /**
  * atomic_add - add integer to atomic variable
-- 
1.7.10.4

>>> On 12.09.13 at 15:47, Tim Deegan <tim@xen.org> wrote:
> RFC because I''m not sure what people think about scattering
coverity
> annotations in the code.
I personally dislike such tool specific annotations. What if someone
suggests a second tool to pass our code through? If there was
some standardization, that''d be a different thing...

Jan

On Thu, 2013-09-12 at 15:06 +0100, Jan Beulich wrote:
> >>> On 12.09.13 at 15:47, Tim Deegan <tim@xen.org> wrote:
> > RFC because I''m not sure what people think about scattering
coverity
> > annotations in the code.
> 
> I personally dislike such tool specific annotations. What if someone
> suggests a second tool to pass our code through? If there was
> some standardization, that''d be a different thing...
Could we handle this how we do different compilers:

#ifdef COVERITY
#define __false_cast_thing THE ANNOTATION
#else...

static inline void atomic_set(atomic_t *v, int i)
{
    __false_cast_thing
    write_atomic(&v->counter, i);

Although if the tools are not consistent about placement etc this won''t
help at all.

On 12/09/13 15:14, Ian Campbell wrote:
> On Thu, 2013-09-12 at 15:06 +0100, Jan Beulich wrote:
>>>>> On 12.09.13 at 15:47, Tim Deegan <tim@xen.org> wrote:
>>> RFC because I''m not sure what people think about
scattering coverity
>>> annotations in the code.
>> I personally dislike such tool specific annotations. What if someone
>> suggests a second tool to pass our code through? If there was
>> some standardization, that''d be a different thing...
> Could we handle this how we do different compilers:
>
> #ifdef COVERITY
> #define __false_cast_thing THE ANNOTATION
> #else...
>
> static inline void atomic_set(atomic_t *v, int i)
> {
>     __false_cast_thing
>     write_atomic(&v->counter, i);
>
> Although if the tools are not consistent about placement etc this
won''t
> help at all.
The annotations are just comments, and as there is an implication of
being able to annotate next to macro declarations, I doubt this would
have the intended effect.

I was wondering whether we can use the "model" file to specify this
annotation next to a modelled version of atomic_set() ?  That way, all
Coverity gubbins would be in a single file unreferenced by anything else.

~Andrew

At 15:14 +0100 on 12 Sep (1378998843), Ian Campbell
wrote:
> On Thu, 2013-09-12 at 15:06 +0100, Jan Beulich wrote:
> > >>> On 12.09.13 at 15:47, Tim Deegan <tim@xen.org>
wrote:
> > > RFC because I''m not sure what people think about
scattering coverity
> > > annotations in the code.
> > 
> > I personally dislike such tool specific annotations. What if someone
> > suggests a second tool to pass our code through? If there was
> > some standardization, that''d be a different thing...
> 
> Could we handle this how we do different compilers:
> 
> #ifdef COVERITY
> #define __false_cast_thing THE ANNOTATION
> #else...
> 
> static inline void atomic_set(atomic_t *v, int i)
> {
>     __false_cast_thing
>     write_atomic(&v->counter, i);
> 
> Although if the tools are not consistent about placement etc this
won''t
> help at all.
The coverity system seems to be to add a comment immediately above the
code in question, which seems like it wouldn''t play well with macro
trickery.

I would prefer, if we can, to either fix this in configuration or get
coverity fixed to understand the switch(sizeof) trick.  And in the
meantime we can carry on flagging these false positives by hand. 

Tim.

On Fri, Sep 13, 2013 at 1:47 AM, Tim Deegan <tim@xen.org>
wrote:
> RFC because I''m not sure what people think about scattering
coverity
> annotations in the code.
This patch of mine (which I''m posting here as part of the RFC
discussion only) adds a Coverity annotation as well. Doing this might
also make Coverity start complaining that the watch list is mutated
without holding the watch_mutex, but that would only be a single
defect instead of many, and easy to triage as False Positive.

Another possible annotation-based fix would be to make a wrapper
around cleanup_push(pthread_mutex_unlock, <mutex>) that was annotated
as unlocking <mutex>, which I guess makes more sense.

Then again, the CIDs could also probably be silenced by modifying the
unlock to be done directly.


Author: Matthew Daley <mattjd@gmail.com>
Date:   Thu Sep 12 15:33:02 2013 +1200

    xenstore: prevent Coverity from being confused in read_message

    Coverity cannot see the cleanup_push''d unlock of watch_mutex, so
    annotate the lock so that it is ignored. This will silence many CIDs
    scattered around the project (eg. wherever xs_{read,write,...} are used)

    Signed-off-by: Matthew Daley <mattjd@gmail.com>

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index 86ef6c7..d12e42f 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -1154,6 +1154,7 @@ static int read_message(struct xs_handle *h, int
nonblocking)
        body[msg->hdr.len] = ''\0'';

        if (msg->hdr.type == XS_WATCH_EVENT) {
+               /* coverity[lock] */
                mutex_lock(&h->watch_mutex);
                cleanup_push(pthread_mutex_unlock, &h->watch_mutex);

At 15:23 +0100 on 12 Sep (1378999422), Tim Deegan wrote:
> At 15:14 +0100 on 12 Sep (1378998843), Ian Campbell wrote:
> > On Thu, 2013-09-12 at 15:06 +0100, Jan Beulich wrote:
> > > >>> On 12.09.13 at 15:47, Tim Deegan <tim@xen.org>
wrote:
> > > > RFC because I''m not sure what people think about
scattering coverity
> > > > annotations in the code.
> > > 
> > > I personally dislike such tool specific annotations. What if
someone
> > > suggests a second tool to pass our code through? If there was
> > > some standardization, that''d be a different thing...
> > 
> > Could we handle this how we do different compilers:
> > 
> > #ifdef COVERITY
> > #define __false_cast_thing THE ANNOTATION
> > #else...
> > 
> > static inline void atomic_set(atomic_t *v, int i)
> > {
> >     __false_cast_thing
> >     write_atomic(&v->counter, i);
> > 
> > Although if the tools are not consistent about placement etc this
won''t
> > help at all.
> 
> The coverity system seems to be to add a comment immediately above the
> code in question, which seems like it wouldn''t play well with
macro
> trickery.
> 
> I would prefer, if we can, to either fix this in configuration or get
> coverity fixed to understand the switch(sizeof) trick.
A Coverity engineer has kindly pointed me to the modelling interface,
which I knew we''d have to get into some time.  In particular, this is
python''s model:
http://hg.python.org/cpython/file/-1/Misc/coverity_model.c

So we can probably sort this out using a model that defines the atomic
r/w ops as straight pointer dereferences.  I''ll look at that it I get a
chance.

Tim.