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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.02.2024 16:55, Julien Grall wrote:
> 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.

It does, see e.g.

# Allows usercopy.c to include itself
$(obj)/usercopy.o: CFLAGS-y += -iquote .

in arch/x86/Makefile.

> AFAICT, the only advantage would be to avoid duplicating the pragmas. So 
> this is not a strong preference.

My other concern there are old gcc versions we still support. I haven't
checked (yet) when support for these pragma-s was introduced; I only
know they haven't been there forever. However, ...

>> --- 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?

... this may be relevant: Inline functions may have an issue with being
processed with the warning enabled. Otoh it may also be a problem if
the warning isn't enabled for them.

Jan