Tim Deegan
2013-Sep-12 13:47 UTC
[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
Jan Beulich
2013-Sep-12 14:06 UTC
Re: [PATCH] RFC xen: suppress Coverity warnings about atomic_read and atomic_set.
>>> 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
Ian Campbell
2013-Sep-12 14:14 UTC
Re: [PATCH] RFC xen: suppress Coverity warnings about atomic_read and atomic_set.
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.
Andrew Cooper
2013-Sep-12 14:17 UTC
Re: [PATCH] RFC xen: suppress Coverity warnings about atomic_read and atomic_set.
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
Tim Deegan
2013-Sep-12 14:23 UTC
Re: [PATCH] RFC xen: suppress Coverity warnings about atomic_read and atomic_set.
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.
Matthew Daley
2013-Sep-12 14:32 UTC
Re: [PATCH] RFC xen: suppress Coverity warnings about atomic_read and atomic_set.
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);
Tim Deegan
2013-Sep-13 07:27 UTC
Re: [PATCH] RFC xen: suppress Coverity warnings about atomic_read and atomic_set.
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: 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.