| Message ID | 20240205151454.1920291-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/bitmap: Compile with -Wsign-conversion | expand |
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@citrix.com> > --- > CC: George Dunlap <George.Dunlap@citrix.com> > CC: Jan Beulich <JBeulich@suse.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Wei Liu <wl@xen.org> > CC: Julien Grall <julien@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,
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@citrix.com> >> --- >> CC: George Dunlap <George.Dunlap@citrix.com> >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Stefano Stabellini <sstabellini@kernel.org> >> CC: Wei Liu <wl@xen.org> >> CC: Julien Grall <julien@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. 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
On 05.02.2024 16:14, Andrew Cooper wrote: > @@ -263,7 +266,7 @@ int bitmap_find_free_region(unsigned long *bitmap, unsigned int bits, unsigned i > unsigned int pages = 1 << order; If we mean to keep this function (see my reply to the other patch), shouldn't we also check "order" along with > unsigned int i; > > - if(pages > BITS_PER_LONG) > + if (pages > BITS_PER_LONG || bits >= INT_MAX) > return -EINVAL; ... these checks? > @@ -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; I agree with Julien that it looks like this change actually belongs in the other patch. Jan
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" + /* * 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; } } return -ENOMEM;
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@citrix.com> --- CC: George Dunlap <George.Dunlap@citrix.com> CC: Jan Beulich <JBeulich@suse.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Wei Liu <wl@xen.org> CC: Julien Grall <julien@xen.org> Slightly RFC. This is our first use of pragmas like this. --- xen/common/bitmap.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)