Re: [PATCH] bitmap: fix n__ signess

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 29 Sep 2023, Julien Grall wrote:
> On 29/09/2023 00:19, Stefano Stabellini wrote:
> > All callers of the bitmap_switch macro (which are all within bitmap.h)
> > pass an int as first parameter. Do not assign it to an unsigned int
> > potentially introducing errors.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> > ---
> > 
> > We could also have gone the other way and change all the callers and the
> > callers' callers to use an unsigned int instead, but I went for the path
> > of least resistance.
> 
> I understand this will solve the issue right now because the callers are all
> passing 'int'. However, all the callers will need to switch to 'unsigned int'
> in order to solve violations in their callers.
> 
> That unless we decide to use 'int' everywhere, but I think this is a bad idea
> because 'n__' is not supposed to be negative.
> 
> Overall, this may be an easy win right now, but this will need to be reverted.
> So, I am not happy to ack it and would in fact be leaning towards Nacking it.

I understand this point and I was undecided myself about the approach.
The issue for me is the overwhelming amount of gcc warnings (thankfully
Luca's script helps a lot with it). With so many warning, it is
difficult to draw the line where to stop fixing things to generate a
digestable patch and not having the feeling of unraveling an infinite
ball of yarn. So, worried about having to change hundreds of lines of
code, I submitted the minimal change instead.

In this case though unsigned int is obviously the right type and the
patch below works as well. So I think that's better.

---
[PATCH v2] bitmap: fix nbits signess

To avoid potentially dangerous sign conversions in bitmap_switch, all
the callers of the bitmap_switch macro (which are all within bitmap.h)
should pass an unsigned int as first parameter.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
---
Changes in v2:
- change nbits instead

diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h
index 3caf92c76d..657390e32e 100644
--- a/xen/include/xen/bitmap.h
+++ b/xen/include/xen/bitmap.h
@@ -110,7 +110,7 @@ extern int bitmap_allocate_region(unsigned long *bitmap, 
int pos, int order);
                large;                                                    \
        }
 
-static inline void bitmap_zero(unsigned long *dst, int nbits)
+static inline void bitmap_zero(unsigned long *dst, unsigned int nbits)
 {
        bitmap_switch(nbits,,
                *dst = 0UL,
@@ -134,7 +134,7 @@ static inline void bitmap_fill(unsigned long *dst, int 
nbits)
 }
 
 static inline void bitmap_copy(unsigned long *dst, const unsigned long *src,
-                       int nbits)
+                       unsigned int nbits)
 {
        bitmap_switch(nbits,,
                *dst = *src,
@@ -142,7 +142,7 @@ static inline void bitmap_copy(unsigned long *dst, const 
unsigned long *src,
 }
 
 static inline void bitmap_and(unsigned long *dst, const unsigned long *src1,
-                       const unsigned long *src2, int nbits)
+                       const unsigned long *src2, unsigned int nbits)
 {
        bitmap_switch(nbits,,
                *dst = *src1 & *src2,
@@ -150,7 +150,7 @@ static inline void bitmap_and(unsigned long *dst, const 
unsigned long *src1,
 }
 
 static inline void bitmap_or(unsigned long *dst, const unsigned long *src1,
-                       const unsigned long *src2, int nbits)
+                       const unsigned long *src2, unsigned int nbits)
 {
        bitmap_switch(nbits,,
                *dst = *src1 | *src2,
@@ -158,7 +158,7 @@ static inline void bitmap_or(unsigned long *dst, const 
unsigned long *src1,
 }
 
 static inline void bitmap_xor(unsigned long *dst, const unsigned long *src1,
-                       const unsigned long *src2, int nbits)
+                       const unsigned long *src2, unsigned int nbits)
 {
        bitmap_switch(nbits,,
                *dst = *src1 ^ *src2,
@@ -166,7 +166,7 @@ static inline void bitmap_xor(unsigned long *dst, const 
unsigned long *src1,
 }
 
 static inline void bitmap_andnot(unsigned long *dst, const unsigned long *src1,
-                       const unsigned long *src2, int nbits)
+                       const unsigned long *src2, unsigned int nbits)
 {
        bitmap_switch(nbits,,
                *dst = *src1 & ~*src2,
@@ -174,7 +174,7 @@ static inline void bitmap_andnot(unsigned long *dst, const 
unsigned long *src1,
 }
 
 static inline void bitmap_complement(unsigned long *dst, const unsigned long 
*src,
-                       int nbits)
+                       unsigned int nbits)
 {
        bitmap_switch(nbits,,
                *dst = ~*src & BITMAP_LAST_WORD_MASK(nbits),
@@ -182,7 +182,7 @@ 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)
+                       const unsigned long *src2, unsigned int nbits)
 {
        bitmap_switch(nbits,
                return -1,
@@ -191,7 +191,7 @@ static inline int bitmap_equal(const unsigned long *src1,
 }
 
 static inline int bitmap_intersects(const unsigned long *src1,
-                       const unsigned long *src2, int nbits)
+                       const unsigned long *src2, unsigned int nbits)
 {
        bitmap_switch(nbits,
                return -1,
@@ -200,7 +200,7 @@ static inline int bitmap_intersects(const unsigned long 
*src1,
 }
 
 static inline int bitmap_subset(const unsigned long *src1,
-                       const unsigned long *src2, int nbits)
+                       const unsigned long *src2, unsigned int nbits)
 {
        bitmap_switch(nbits,
                return -1,
@@ -208,7 +208,7 @@ static inline int bitmap_subset(const unsigned long *src1,
                return __bitmap_subset(src1, src2, nbits));
 }
 
-static inline int bitmap_empty(const unsigned long *src, int nbits)
+static inline int bitmap_empty(const unsigned long *src, unsigned int nbits)
 {
        bitmap_switch(nbits,
                return -1,
@@ -216,7 +216,7 @@ static inline int bitmap_empty(const unsigned long *src, 
int nbits)
                return __bitmap_empty(src, nbits));
 }
 
-static inline int bitmap_full(const unsigned long *src, int nbits)
+static inline int bitmap_full(const unsigned long *src, unsigned int nbits)
 {
        bitmap_switch(nbits,
                return -1,