Re: [PATCH] x86/bitmap: Compile with -Wsign-conversion

[Julien Grall <julien@xxxxxxx>]

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.cooper3@xxxxxxxxxx>
> ---
> CC: George Dunlap <George.Dunlap@xxxxxxxxxx>
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
>
> 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