* [PATCH v2 0/4] more Coverity-inspired tidying.
@ 2014-02-27 14:27 Tim Deegan
2014-02-27 14:27 ` [PATCH v2 1/4] common/vsprintf: Explicitly treat negative lengths as 'unlimited' Tim Deegan
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Tim Deegan @ 2014-02-27 14:27 UTC (permalink / raw)
To: xen-devel
These four patches are small cleanups of things that Coverity complains
about. AFAICT none of them fixes any bugs, but I do think that they make
the code more readable (i.e. I'm not just mangling the code to make
Coverity happy).
Reposting now that 4.4 has branched, with a v2 of patch 4/4 fixing
a missing argument evaluation.
Cheers,
Tim.
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v2 1/4] common/vsprintf: Explicitly treat negative lengths as 'unlimited' 2014-02-27 14:27 [PATCH v2 0/4] more Coverity-inspired tidying Tim Deegan @ 2014-02-27 14:27 ` Tim Deegan 2014-02-28 15:07 ` Keir Fraser 2014-02-27 14:27 ` [PATCH v2 2/4] x86/shadow: Drop shadow_mode_trap_reads() Tim Deegan ` (2 subsequent siblings) 3 siblings, 1 reply; 9+ messages in thread From: Tim Deegan @ 2014-02-27 14:27 UTC (permalink / raw) To: xen-devel The old code relied on implictly casting negative numbers to size_t making a very large limit, which was correct but non-obvious. Coverity CID 1128575 Signed-off-by: Tim Deegan <tim@xen.org> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/common/vsprintf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c index 1a6198e..0a6fa05 100644 --- a/xen/common/vsprintf.c +++ b/xen/common/vsprintf.c @@ -239,7 +239,7 @@ static char *number( static char *string(char *str, char *end, const char *s, int field_width, int precision, int flags) { - int i, len = strnlen(s, precision); + int i, len = (precision < 0) ? strlen(s) : strnlen(s, precision); if (!(flags & LEFT)) { while (len < field_width--) { -- 1.8.5.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/4] common/vsprintf: Explicitly treat negative lengths as 'unlimited' 2014-02-27 14:27 ` [PATCH v2 1/4] common/vsprintf: Explicitly treat negative lengths as 'unlimited' Tim Deegan @ 2014-02-28 15:07 ` Keir Fraser 0 siblings, 0 replies; 9+ messages in thread From: Keir Fraser @ 2014-02-28 15:07 UTC (permalink / raw) To: Tim Deegan; +Cc: Xen-devel [-- Attachment #1.1: Type: text/plain, Size: 1128 bytes --] On Thu, Feb 27, 2014 at 2:27 PM, Tim Deegan <tim@xen.org> wrote: > The old code relied on implictly casting negative numbers to size_t > making a very large limit, which was correct but non-obvious. > > Coverity CID 1128575 > > Signed-off-by: Tim Deegan <tim@xen.org> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > Acked-by: Keir Fraser <keir@xen.org> > --- > xen/common/vsprintf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c > index 1a6198e..0a6fa05 100644 > --- a/xen/common/vsprintf.c > +++ b/xen/common/vsprintf.c > @@ -239,7 +239,7 @@ static char *number( > static char *string(char *str, char *end, const char *s, > int field_width, int precision, int flags) > { > - int i, len = strnlen(s, precision); > + int i, len = (precision < 0) ? strlen(s) : strnlen(s, precision); > > if (!(flags & LEFT)) { > while (len < field_width--) { > -- > 1.8.5.2 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > [-- Attachment #1.2: Type: text/html, Size: 1968 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/4] x86/shadow: Drop shadow_mode_trap_reads() 2014-02-27 14:27 [PATCH v2 0/4] more Coverity-inspired tidying Tim Deegan 2014-02-27 14:27 ` [PATCH v2 1/4] common/vsprintf: Explicitly treat negative lengths as 'unlimited' Tim Deegan @ 2014-02-27 14:27 ` Tim Deegan 2014-02-27 14:27 ` [PATCH v2 3/4] x86/mem_sharing: drop unused variable Tim Deegan 2014-02-27 14:27 ` [PATCH v2 4/4] bitmaps/bitops: Clarify tests for small constant size Tim Deegan 3 siblings, 0 replies; 9+ messages in thread From: Tim Deegan @ 2014-02-27 14:27 UTC (permalink / raw) To: xen-devel This was never actually implemented, and is confusing coverity. Coverity CID 1090354 Signed-off-by: Tim Deegan <tim@xen.org> --- xen/arch/x86/mm/shadow/multi.c | 30 ++++-------------------------- xen/include/asm-x86/shadow.h | 4 ---- 2 files changed, 4 insertions(+), 30 deletions(-) diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c index 3d35537..5c7a7ac 100644 --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -692,21 +692,7 @@ _sh_propagate(struct vcpu *v, && (ft == ft_demand_write)) #endif /* OOS */ ) ) - { - if ( shadow_mode_trap_reads(d) ) - { - // if we are trapping both reads & writes, then mark this page - // as not present... - // - sflags &= ~_PAGE_PRESENT; - } - else - { - // otherwise, just prevent any writes... - // - sflags &= ~_PAGE_RW; - } - } + sflags &= ~_PAGE_RW; // PV guests in 64-bit mode use two different page tables for user vs // supervisor permissions, making the guest's _PAGE_USER bit irrelevant. @@ -3181,18 +3167,10 @@ static int sh_page_fault(struct vcpu *v, && !(mfn_is_out_of_sync(gmfn) && !(regs->error_code & PFEC_user_mode)) #endif - ) + && (ft == ft_demand_write) ) { - if ( ft == ft_demand_write ) - { - perfc_incr(shadow_fault_emulate_write); - goto emulate; - } - else if ( shadow_mode_trap_reads(d) && ft == ft_demand_read ) - { - perfc_incr(shadow_fault_emulate_read); - goto emulate; - } + perfc_incr(shadow_fault_emulate_write); + goto emulate; } /* Need to hand off device-model MMIO to the device model */ diff --git a/xen/include/asm-x86/shadow.h b/xen/include/asm-x86/shadow.h index 348915e..f40cab4 100644 --- a/xen/include/asm-x86/shadow.h +++ b/xen/include/asm-x86/shadow.h @@ -44,10 +44,6 @@ #define shadow_mode_external(_d) (paging_mode_shadow(_d) && \ paging_mode_external(_d)) -/* Xen traps & emulates all reads of all page table pages: - * not yet supported */ -#define shadow_mode_trap_reads(_d) ({ (void)(_d); 0; }) - /***************************************************************************** * Entry points into the shadow code */ -- 1.8.5.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 3/4] x86/mem_sharing: drop unused variable. 2014-02-27 14:27 [PATCH v2 0/4] more Coverity-inspired tidying Tim Deegan 2014-02-27 14:27 ` [PATCH v2 1/4] common/vsprintf: Explicitly treat negative lengths as 'unlimited' Tim Deegan 2014-02-27 14:27 ` [PATCH v2 2/4] x86/shadow: Drop shadow_mode_trap_reads() Tim Deegan @ 2014-02-27 14:27 ` Tim Deegan 2014-02-27 14:27 ` [PATCH v2 4/4] bitmaps/bitops: Clarify tests for small constant size Tim Deegan 3 siblings, 0 replies; 9+ messages in thread From: Tim Deegan @ 2014-02-27 14:27 UTC (permalink / raw) To: xen-devel Coverity CID 1087198 Signed-off-by: Tim Deegan <tim@xen.org> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org> --- xen/arch/x86/mm/mem_sharing.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c index 4a5d9e8..7ed6594 100644 --- a/xen/arch/x86/mm/mem_sharing.c +++ b/xen/arch/x86/mm/mem_sharing.c @@ -846,7 +846,6 @@ int mem_sharing_nominate_page(struct domain *d, mfn_t mfn; struct page_info *page = NULL; /* gcc... */ int ret; - struct gfn_info *gfn_info; *phandle = 0UL; @@ -905,7 +904,7 @@ int mem_sharing_nominate_page(struct domain *d, page->sharing->handle = get_next_handle(); /* Create the local gfn info */ - if ( (gfn_info = mem_sharing_gfn_alloc(page, d, gfn)) == NULL ) + if ( mem_sharing_gfn_alloc(page, d, gfn) == NULL ) { xfree(page->sharing); page->sharing = NULL; -- 1.8.5.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 4/4] bitmaps/bitops: Clarify tests for small constant size. 2014-02-27 14:27 [PATCH v2 0/4] more Coverity-inspired tidying Tim Deegan ` (2 preceding siblings ...) 2014-02-27 14:27 ` [PATCH v2 3/4] x86/mem_sharing: drop unused variable Tim Deegan @ 2014-02-27 14:27 ` Tim Deegan 2014-02-27 16:30 ` Jan Beulich 3 siblings, 1 reply; 9+ messages in thread From: Tim Deegan @ 2014-02-27 14:27 UTC (permalink / raw) To: xen-devel No semantic changes, just makes the control flow a bit clearer. I was looking at this bcause the (-!__builtin_constant_p(x) | x__) formula is too clever for Coverity, but in fact it always takes me a minute or two to understand it too. :) Signed-off-by: Tim Deegan <tim@xen.org> --- v2: fix find_next_bit macros to evaluate 'addr' exactly once. --- xen/include/asm-x86/bitops.h | 62 ++++++++++++++++++++------------------------ xen/include/xen/bitmap.h | 30 ++++++++++++--------- 2 files changed, 46 insertions(+), 46 deletions(-) diff --git a/xen/include/asm-x86/bitops.h b/xen/include/asm-x86/bitops.h index ab21d92..05ed2d7 100644 --- a/xen/include/asm-x86/bitops.h +++ b/xen/include/asm-x86/bitops.h @@ -335,23 +335,20 @@ static inline unsigned int __scanbit(unsigned long val, unsigned long max) * @offset: The bitnumber to start searching at * @size: The maximum size to search */ -#define find_next_bit(addr, size, off) ({ \ - unsigned int r__ = (size); \ - unsigned int o__ = (off); \ - switch ( -!__builtin_constant_p(size) | r__ ) \ - { \ - case 0: (void)(addr); break; \ - case 1 ... BITS_PER_LONG: \ - r__ = o__ + __scanbit(*(const unsigned long *)(addr) >> o__, r__); \ - break; \ - default: \ - if ( __builtin_constant_p(off) && !o__ ) \ - r__ = __find_first_bit(addr, r__); \ - else \ - r__ = __find_next_bit(addr, r__, o__); \ - break; \ - } \ - r__; \ +#define find_next_bit(addr, size, off) ({ \ + unsigned int r__; \ + const unsigned long *a__ = (addr); \ + unsigned int s__ = (size); \ + unsigned int o__ = (off); \ + if ( __builtin_constant_p(size) && s__ == 0 ) \ + r__ = s__; \ + else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG ) \ + r__ = o__ + __scanbit(*(const unsigned long *)(a__) >> o__, s__); \ + else if ( __builtin_constant_p(off) && !o__ ) \ + r__ = __find_first_bit(a__, s__); \ + else \ + r__ = __find_next_bit(a__, s__, o__); \ + r__; \ }) /** @@ -370,23 +367,20 @@ static inline unsigned int __scanbit(unsigned long val, unsigned long max) * @offset: The bitnumber to start searching at * @size: The maximum size to search */ -#define find_next_zero_bit(addr, size, off) ({ \ - unsigned int r__ = (size); \ - unsigned int o__ = (off); \ - switch ( -!__builtin_constant_p(size) | r__ ) \ - { \ - case 0: (void)(addr); break; \ - case 1 ... BITS_PER_LONG: \ - r__ = o__ + __scanbit(~*(const unsigned long *)(addr) >> o__, r__); \ - break; \ - default: \ - if ( __builtin_constant_p(off) && !o__ ) \ - r__ = __find_first_zero_bit(addr, r__); \ - else \ - r__ = __find_next_zero_bit(addr, r__, o__); \ - break; \ - } \ - r__; \ +#define find_next_zero_bit(addr, size, off) ({ \ + unsigned int r__; \ + const unsigned long *a__ = (addr); \ + unsigned int s__ = (size); \ + unsigned int o__ = (off); \ + if ( __builtin_constant_p(size) && s__ == 0 ) \ + r__ = s__; \ + else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG ) \ + r__ = o__ + __scanbit(~*(const unsigned long *)(a__) >> o__, s__); \ + else if ( __builtin_constant_p(off) && !o__ ) \ + r__ = __find_first_zero_bit(a__, s__); \ + else \ + r__ = __find_next_zero_bit(a__, s__, o__); \ + r__; \ }) /** diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h index b5ec455..166e1a0 100644 --- a/xen/include/xen/bitmap.h +++ b/xen/include/xen/bitmap.h @@ -110,13 +110,14 @@ extern int bitmap_allocate_region(unsigned long *bitmap, int pos, int order); #define bitmap_bytes(nbits) (BITS_TO_LONGS(nbits) * sizeof(unsigned long)) -#define bitmap_switch(nbits, zero_ret, small, large) \ - switch (-!__builtin_constant_p(nbits) | (nbits)) { \ - case 0: return zero_ret; \ - case 1 ... BITS_PER_LONG: \ - small; break; \ - default: \ - large; break; \ +#define bitmap_switch(nbits, zero, small, large) \ + unsigned int n__ = (nbits); \ + if (__builtin_constant_p(nbits) && n__ == 0) { \ + zero; \ + } else if (__builtin_constant_p(nbits) && n__ <= BITS_PER_LONG) { \ + small; \ + } else { \ + large; \ } static inline void bitmap_zero(unsigned long *dst, int nbits) @@ -191,7 +192,8 @@ static inline void bitmap_complement(unsigned long *dst, const unsigned long *sr static inline int bitmap_equal(const unsigned long *src1, const unsigned long *src2, int nbits) { - bitmap_switch(nbits, -1, + bitmap_switch(nbits, + return -1, return !((*src1 ^ *src2) & BITMAP_LAST_WORD_MASK(nbits)), return __bitmap_equal(src1, src2, nbits)); } @@ -199,7 +201,8 @@ static inline int bitmap_equal(const unsigned long *src1, static inline int bitmap_intersects(const unsigned long *src1, const unsigned long *src2, int nbits) { - bitmap_switch(nbits, -1, + bitmap_switch(nbits, + return -1, return ((*src1 & *src2) & BITMAP_LAST_WORD_MASK(nbits)) != 0, return __bitmap_intersects(src1, src2, nbits)); } @@ -207,21 +210,24 @@ static inline int bitmap_intersects(const unsigned long *src1, static inline int bitmap_subset(const unsigned long *src1, const unsigned long *src2, int nbits) { - bitmap_switch(nbits, -1, + bitmap_switch(nbits, + return -1, return !((*src1 & ~*src2) & BITMAP_LAST_WORD_MASK(nbits)), return __bitmap_subset(src1, src2, nbits)); } static inline int bitmap_empty(const unsigned long *src, int nbits) { - bitmap_switch(nbits, -1, + bitmap_switch(nbits, + return -1, return !(*src & BITMAP_LAST_WORD_MASK(nbits)), return __bitmap_empty(src, nbits)); } static inline int bitmap_full(const unsigned long *src, int nbits) { - bitmap_switch(nbits, -1, + bitmap_switch(nbits, + return -1, return !(~*src & BITMAP_LAST_WORD_MASK(nbits)), return __bitmap_full(src, nbits)); } -- 1.8.5.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 4/4] bitmaps/bitops: Clarify tests for small constant size. 2014-02-27 14:27 ` [PATCH v2 4/4] bitmaps/bitops: Clarify tests for small constant size Tim Deegan @ 2014-02-27 16:30 ` Jan Beulich 2014-02-27 16:38 ` Tim Deegan 0 siblings, 1 reply; 9+ messages in thread From: Jan Beulich @ 2014-02-27 16:30 UTC (permalink / raw) To: Tim Deegan; +Cc: xen-devel >>> On 27.02.14 at 15:27, Tim Deegan <tim@xen.org> wrote: > No semantic changes, just makes the control flow a bit clearer. > > I was looking at this bcause the (-!__builtin_constant_p(x) | x__) > formula is too clever for Coverity, but in fact it always takes me a > minute or two to understand it too. :) > > Signed-off-by: Tim Deegan <tim@xen.org> Well, I'm not really happy to see this changed into more text (even if fewer lines), because to me that's part of what makes such macros badly readable, but ... > xen/include/asm-x86/bitops.h | 62 ++++++++++++++++++++------------------------ > xen/include/xen/bitmap.h | 30 ++++++++++++--------- > 2 files changed, 46 insertions(+), 46 deletions(-) ... at least the overall change is not growing the number of lines. Nevertheless a small consistency nit: > +#define find_next_bit(addr, size, off) ({ \ > + unsigned int r__; \ > + const unsigned long *a__ = (addr); \ > + unsigned int s__ = (size); \ > + unsigned int o__ = (off); \ > + if ( __builtin_constant_p(size) && s__ == 0 ) \ Using == 0 here, ... > + r__ = s__; \ > + else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG ) \ > + r__ = o__ + __scanbit(*(const unsigned long *)(a__) >> o__, s__); \ > + else if ( __builtin_constant_p(off) && !o__ ) \ ... but using ! here. I'd prefer the latter everywhere, but I'd also be fine with you choosing the former consistently. (Same of course again further down.) Jan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 4/4] bitmaps/bitops: Clarify tests for small constant size. 2014-02-27 16:30 ` Jan Beulich @ 2014-02-27 16:38 ` Tim Deegan 2014-02-28 14:40 ` Jan Beulich 0 siblings, 1 reply; 9+ messages in thread From: Tim Deegan @ 2014-02-27 16:38 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel At 16:30 +0000 on 27 Feb (1393515036), Jan Beulich wrote: > Using == 0 here, ... > > > + r__ = s__; \ > > + else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG ) \ > > + r__ = o__ + __scanbit(*(const unsigned long *)(a__) >> o__, s__); \ > > + else if ( __builtin_constant_p(off) && !o__ ) \ > > ... but using ! here. I'd prefer the latter everywhere, but I'd also > be fine with you choosing the former consistently. (Same of course > again further down.) Good point. v3 is below Tim. >From dba941064825d723b79f866d16bb0f07585de320 Mon Sep 17 00:00:00 2001 From: Tim Deegan <tim@xen.org> Date: Thu, 28 Nov 2013 15:40:48 +0000 Subject: [PATCH] bitmaps/bitops: Clarify tests for small constant size. No semantic changes, just makes the control flow a bit clearer. I was looking at this bcause the (-!__builtin_constant_p(x) | x__) formula is too clever for Coverity, but in fact it always takes me a minute or two to understand it too. :) Signed-off-by: Tim Deegan <tim@xen.org> --- v3: Consistenly use '!foo' rather than 'foo == 0' v2: fix find_next_bit macros to evaluate 'addr' exactly once. --- xen/include/asm-x86/bitops.h | 62 ++++++++++++++++++++------------------------ xen/include/xen/bitmap.h | 30 ++++++++++++--------- 2 files changed, 46 insertions(+), 46 deletions(-) diff --git a/xen/include/asm-x86/bitops.h b/xen/include/asm-x86/bitops.h index ab21d92..82a08ee 100644 --- a/xen/include/asm-x86/bitops.h +++ b/xen/include/asm-x86/bitops.h @@ -335,23 +335,20 @@ static inline unsigned int __scanbit(unsigned long val, unsigned long max) * @offset: The bitnumber to start searching at * @size: The maximum size to search */ -#define find_next_bit(addr, size, off) ({ \ - unsigned int r__ = (size); \ - unsigned int o__ = (off); \ - switch ( -!__builtin_constant_p(size) | r__ ) \ - { \ - case 0: (void)(addr); break; \ - case 1 ... BITS_PER_LONG: \ - r__ = o__ + __scanbit(*(const unsigned long *)(addr) >> o__, r__); \ - break; \ - default: \ - if ( __builtin_constant_p(off) && !o__ ) \ - r__ = __find_first_bit(addr, r__); \ - else \ - r__ = __find_next_bit(addr, r__, o__); \ - break; \ - } \ - r__; \ +#define find_next_bit(addr, size, off) ({ \ + unsigned int r__; \ + const unsigned long *a__ = (addr); \ + unsigned int s__ = (size); \ + unsigned int o__ = (off); \ + if ( __builtin_constant_p(size) && !s__ ) \ + r__ = s__; \ + else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG ) \ + r__ = o__ + __scanbit(*(const unsigned long *)(a__) >> o__, s__); \ + else if ( __builtin_constant_p(off) && !o__ ) \ + r__ = __find_first_bit(a__, s__); \ + else \ + r__ = __find_next_bit(a__, s__, o__); \ + r__; \ }) /** @@ -370,23 +367,20 @@ static inline unsigned int __scanbit(unsigned long val, unsigned long max) * @offset: The bitnumber to start searching at * @size: The maximum size to search */ -#define find_next_zero_bit(addr, size, off) ({ \ - unsigned int r__ = (size); \ - unsigned int o__ = (off); \ - switch ( -!__builtin_constant_p(size) | r__ ) \ - { \ - case 0: (void)(addr); break; \ - case 1 ... BITS_PER_LONG: \ - r__ = o__ + __scanbit(~*(const unsigned long *)(addr) >> o__, r__); \ - break; \ - default: \ - if ( __builtin_constant_p(off) && !o__ ) \ - r__ = __find_first_zero_bit(addr, r__); \ - else \ - r__ = __find_next_zero_bit(addr, r__, o__); \ - break; \ - } \ - r__; \ +#define find_next_zero_bit(addr, size, off) ({ \ + unsigned int r__; \ + const unsigned long *a__ = (addr); \ + unsigned int s__ = (size); \ + unsigned int o__ = (off); \ + if ( __builtin_constant_p(size) && !s__ ) \ + r__ = s__; \ + else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG ) \ + r__ = o__ + __scanbit(~*(const unsigned long *)(a__) >> o__, s__); \ + else if ( __builtin_constant_p(off) && !o__ ) \ + r__ = __find_first_zero_bit(a__, s__); \ + else \ + r__ = __find_next_zero_bit(a__, s__, o__); \ + r__; \ }) /** diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h index b5ec455..e2a3686 100644 --- a/xen/include/xen/bitmap.h +++ b/xen/include/xen/bitmap.h @@ -110,13 +110,14 @@ extern int bitmap_allocate_region(unsigned long *bitmap, int pos, int order); #define bitmap_bytes(nbits) (BITS_TO_LONGS(nbits) * sizeof(unsigned long)) -#define bitmap_switch(nbits, zero_ret, small, large) \ - switch (-!__builtin_constant_p(nbits) | (nbits)) { \ - case 0: return zero_ret; \ - case 1 ... BITS_PER_LONG: \ - small; break; \ - default: \ - large; break; \ +#define bitmap_switch(nbits, zero, small, large) \ + unsigned int n__ = (nbits); \ + if (__builtin_constant_p(nbits) && !n__) { \ + zero; \ + } else if (__builtin_constant_p(nbits) && n__ <= BITS_PER_LONG) { \ + small; \ + } else { \ + large; \ } static inline void bitmap_zero(unsigned long *dst, int nbits) @@ -191,7 +192,8 @@ static inline void bitmap_complement(unsigned long *dst, const unsigned long *sr static inline int bitmap_equal(const unsigned long *src1, const unsigned long *src2, int nbits) { - bitmap_switch(nbits, -1, + bitmap_switch(nbits, + return -1, return !((*src1 ^ *src2) & BITMAP_LAST_WORD_MASK(nbits)), return __bitmap_equal(src1, src2, nbits)); } @@ -199,7 +201,8 @@ static inline int bitmap_equal(const unsigned long *src1, static inline int bitmap_intersects(const unsigned long *src1, const unsigned long *src2, int nbits) { - bitmap_switch(nbits, -1, + bitmap_switch(nbits, + return -1, return ((*src1 & *src2) & BITMAP_LAST_WORD_MASK(nbits)) != 0, return __bitmap_intersects(src1, src2, nbits)); } @@ -207,21 +210,24 @@ static inline int bitmap_intersects(const unsigned long *src1, static inline int bitmap_subset(const unsigned long *src1, const unsigned long *src2, int nbits) { - bitmap_switch(nbits, -1, + bitmap_switch(nbits, + return -1, return !((*src1 & ~*src2) & BITMAP_LAST_WORD_MASK(nbits)), return __bitmap_subset(src1, src2, nbits)); } static inline int bitmap_empty(const unsigned long *src, int nbits) { - bitmap_switch(nbits, -1, + bitmap_switch(nbits, + return -1, return !(*src & BITMAP_LAST_WORD_MASK(nbits)), return __bitmap_empty(src, nbits)); } static inline int bitmap_full(const unsigned long *src, int nbits) { - bitmap_switch(nbits, -1, + bitmap_switch(nbits, + return -1, return !(~*src & BITMAP_LAST_WORD_MASK(nbits)), return __bitmap_full(src, nbits)); } -- 1.8.5.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 4/4] bitmaps/bitops: Clarify tests for small constant size. 2014-02-27 16:38 ` Tim Deegan @ 2014-02-28 14:40 ` Jan Beulich 0 siblings, 0 replies; 9+ messages in thread From: Jan Beulich @ 2014-02-28 14:40 UTC (permalink / raw) To: Tim Deegan; +Cc: xen-devel >>> On 27.02.14 at 17:38, Tim Deegan <tim@xen.org> wrote: > From dba941064825d723b79f866d16bb0f07585de320 Mon Sep 17 00:00:00 2001 > From: Tim Deegan <tim@xen.org> > Date: Thu, 28 Nov 2013 15:40:48 +0000 > Subject: [PATCH] bitmaps/bitops: Clarify tests for small constant size. > > No semantic changes, just makes the control flow a bit clearer. > > I was looking at this bcause the (-!__builtin_constant_p(x) | x__) > formula is too clever for Coverity, but in fact it always takes me a > minute or two to understand it too. :) > > Signed-off-by: Tim Deegan <tim@xen.org> A little reluctantly Acked-by: Jan Beulich <jbeulich@suse.com> > --- > > v3: Consistenly use '!foo' rather than 'foo == 0' > v2: fix find_next_bit macros to evaluate 'addr' exactly once. > --- > xen/include/asm-x86/bitops.h | 62 ++++++++++++++++++++------------------------ > xen/include/xen/bitmap.h | 30 ++++++++++++--------- > 2 files changed, 46 insertions(+), 46 deletions(-) > > diff --git a/xen/include/asm-x86/bitops.h b/xen/include/asm-x86/bitops.h > index ab21d92..82a08ee 100644 > --- a/xen/include/asm-x86/bitops.h > +++ b/xen/include/asm-x86/bitops.h > @@ -335,23 +335,20 @@ static inline unsigned int __scanbit(unsigned long val, > unsigned long max) > * @offset: The bitnumber to start searching at > * @size: The maximum size to search > */ > -#define find_next_bit(addr, size, off) ({ \ > - unsigned int r__ = (size); \ > - unsigned int o__ = (off); \ > - switch ( -!__builtin_constant_p(size) | r__ ) \ > - { \ > - case 0: (void)(addr); break; \ > - case 1 ... BITS_PER_LONG: \ > - r__ = o__ + __scanbit(*(const unsigned long *)(addr) >> o__, r__); \ > - break; \ > - default: \ > - if ( __builtin_constant_p(off) && !o__ ) \ > - r__ = __find_first_bit(addr, r__); \ > - else \ > - r__ = __find_next_bit(addr, r__, o__); \ > - break; \ > - } \ > - r__; \ > +#define find_next_bit(addr, size, off) ({ > \ > + unsigned int r__; > \ > + const unsigned long *a__ = (addr); > \ > + unsigned int s__ = (size); > \ > + unsigned int o__ = (off); > \ > + if ( __builtin_constant_p(size) && !s__ ) \ > + r__ = s__; > \ > + else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG ) \ > + r__ = o__ + __scanbit(*(const unsigned long *)(a__) >> o__, s__); \ > + else if ( __builtin_constant_p(off) && !o__ ) \ > + r__ = __find_first_bit(a__, s__); > \ > + else > \ > + r__ = __find_next_bit(a__, s__, o__); > \ > + r__; > \ > }) > > /** > @@ -370,23 +367,20 @@ static inline unsigned int __scanbit(unsigned long val, > unsigned long max) > * @offset: The bitnumber to start searching at > * @size: The maximum size to search > */ > -#define find_next_zero_bit(addr, size, off) ({ \ > - unsigned int r__ = (size); \ > - unsigned int o__ = (off); \ > - switch ( -!__builtin_constant_p(size) | r__ ) \ > - { \ > - case 0: (void)(addr); break; \ > - case 1 ... BITS_PER_LONG: \ > - r__ = o__ + __scanbit(~*(const unsigned long *)(addr) >> o__, r__); \ > - break; \ > - default: \ > - if ( __builtin_constant_p(off) && !o__ ) \ > - r__ = __find_first_zero_bit(addr, r__); \ > - else \ > - r__ = __find_next_zero_bit(addr, r__, o__); \ > - break; \ > - } \ > - r__; \ > +#define find_next_zero_bit(addr, size, off) ({ > \ > + unsigned int r__; > \ > + const unsigned long *a__ = (addr); > \ > + unsigned int s__ = (size); > \ > + unsigned int o__ = (off); > \ > + if ( __builtin_constant_p(size) && !s__ ) \ > + r__ = s__; > \ > + else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG ) \ > + r__ = o__ + __scanbit(~*(const unsigned long *)(a__) >> o__, s__); \ > + else if ( __builtin_constant_p(off) && !o__ ) \ > + r__ = __find_first_zero_bit(a__, s__); > \ > + else > \ > + r__ = __find_next_zero_bit(a__, s__, o__); > \ > + r__; > \ > }) > > /** > diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h > index b5ec455..e2a3686 100644 > --- a/xen/include/xen/bitmap.h > +++ b/xen/include/xen/bitmap.h > @@ -110,13 +110,14 @@ extern int bitmap_allocate_region(unsigned long > *bitmap, int pos, int order); > > #define bitmap_bytes(nbits) (BITS_TO_LONGS(nbits) * sizeof(unsigned long)) > > -#define bitmap_switch(nbits, zero_ret, small, large) \ > - switch (-!__builtin_constant_p(nbits) | (nbits)) { \ > - case 0: return zero_ret; \ > - case 1 ... BITS_PER_LONG: \ > - small; break; \ > - default: \ > - large; break; \ > +#define bitmap_switch(nbits, zero, small, large) \ > + unsigned int n__ = (nbits); \ > + if (__builtin_constant_p(nbits) && !n__) { \ > + zero; \ > + } else if (__builtin_constant_p(nbits) && n__ <= BITS_PER_LONG) { \ > + small; \ > + } else { \ > + large; \ > } > > static inline void bitmap_zero(unsigned long *dst, int nbits) > @@ -191,7 +192,8 @@ static inline void bitmap_complement(unsigned long *dst, > const unsigned long *sr > static inline int bitmap_equal(const unsigned long *src1, > const unsigned long *src2, int nbits) > { > - bitmap_switch(nbits, -1, > + bitmap_switch(nbits, > + return -1, > return !((*src1 ^ *src2) & BITMAP_LAST_WORD_MASK(nbits)), > return __bitmap_equal(src1, src2, nbits)); > } > @@ -199,7 +201,8 @@ static inline int bitmap_equal(const unsigned long *src1, > static inline int bitmap_intersects(const unsigned long *src1, > const unsigned long *src2, int nbits) > { > - bitmap_switch(nbits, -1, > + bitmap_switch(nbits, > + return -1, > return ((*src1 & *src2) & BITMAP_LAST_WORD_MASK(nbits)) != 0, > return __bitmap_intersects(src1, src2, nbits)); > } > @@ -207,21 +210,24 @@ static inline int bitmap_intersects(const unsigned long > *src1, > static inline int bitmap_subset(const unsigned long *src1, > const unsigned long *src2, int nbits) > { > - bitmap_switch(nbits, -1, > + bitmap_switch(nbits, > + return -1, > return !((*src1 & ~*src2) & BITMAP_LAST_WORD_MASK(nbits)), > return __bitmap_subset(src1, src2, nbits)); > } > > static inline int bitmap_empty(const unsigned long *src, int nbits) > { > - bitmap_switch(nbits, -1, > + bitmap_switch(nbits, > + return -1, > return !(*src & BITMAP_LAST_WORD_MASK(nbits)), > return __bitmap_empty(src, nbits)); > } > > static inline int bitmap_full(const unsigned long *src, int nbits) > { > - bitmap_switch(nbits, -1, > + bitmap_switch(nbits, > + return -1, > return !(~*src & BITMAP_LAST_WORD_MASK(nbits)), > return __bitmap_full(src, nbits)); > } > -- > 1.8.5.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-02-28 15:07 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-02-27 14:27 [PATCH v2 0/4] more Coverity-inspired tidying Tim Deegan 2014-02-27 14:27 ` [PATCH v2 1/4] common/vsprintf: Explicitly treat negative lengths as 'unlimited' Tim Deegan 2014-02-28 15:07 ` Keir Fraser 2014-02-27 14:27 ` [PATCH v2 2/4] x86/shadow: Drop shadow_mode_trap_reads() Tim Deegan 2014-02-27 14:27 ` [PATCH v2 3/4] x86/mem_sharing: drop unused variable Tim Deegan 2014-02-27 14:27 ` [PATCH v2 4/4] bitmaps/bitops: Clarify tests for small constant size Tim Deegan 2014-02-27 16:30 ` Jan Beulich 2014-02-27 16:38 ` Tim Deegan 2014-02-28 14:40 ` Jan Beulich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).