Hi Andrew,
Title: You seem to change common code. So s/x86//
On 05/02/2024 15:14, Andrew Cooper wrote:
Use pragmas to able the warning in this file only. All supported versions of
Clang understand this, while older GCCs simply ignore it.
bitmap_find_free_region() is the only function which isn't sign-convert
clean. This highlights a latent bug in that it can't return successfully for
a bitmap larger than 2G.
Add an extra check, and explicit cast to silence the warning.
Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
---
CC: George Dunlap <george.dun...@citrix.com>
CC: Jan Beulich <jbeul...@suse.com>
CC: Stefano Stabellini <sstabell...@kernel.org>
CC: Wei Liu <w...@xen.org>
CC: Julien Grall <jul...@xen.org>
Slightly RFC. This is our first use of pragmas like this.
The only other approach I can think of is specifying the CFLAGS per file
like Linux did. I don't know if our build system supports that though.
AFAICT, the only advantage would be to avoid duplicating the pragmas. So
this is not a strong preference.
---
xen/common/bitmap.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/xen/common/bitmap.c b/xen/common/bitmap.c
index 18e23e2f0e18..b14f8a3b3030 100644
--- a/xen/common/bitmap.c
+++ b/xen/common/bitmap.c
@@ -14,6 +14,9 @@
#include <xen/lib.h>
#include <asm/byteorder.h>
+#pragma GCC diagnostic warning "-Wsign-conversion"
+#pragma clang diagnostic warning "-Wsign-conversion"
+
OOI, any reason why wasn't added at the right at the top of the file?
/*
* bitmaps provide an array of bits, implemented using an an
* array of unsigned longs. The number of valid bits in a
@@ -263,7 +266,7 @@ int bitmap_find_free_region(unsigned long *bitmap, unsigned
int bits, unsigned i
unsigned int pages = 1 << order;
unsigned int i;
- if(pages > BITS_PER_LONG)
+ if (pages > BITS_PER_LONG || bits >= INT_MAX)
return -EINVAL;
/* make a mask of the order */
@@ -278,7 +281,7 @@ int bitmap_find_free_region(unsigned long *bitmap, unsigned
int bits, unsigned i
if((bitmap[index] & (mask << offset)) == 0) {
/* set region in bimap */
bitmap[index] |= (mask << offset);
- return i;
+ return (int)i;
It took me a while to realize that this patch should be reviewed after
"x86/bitmap: Even more signed-ness fixes".
Before hand, 'i' is a signed int and we would return -ENOMEM if 'bits'
is negative. So I wonder whether the change in common/bitmap.c should
belong to the other patch?
Cheers,
--
Julien Grall
