xen/common/bitmap.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
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(-)
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;
--
2.30.2
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
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, -- Julien Grall
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
© 2016 - 2024 Red Hat, Inc.