| Message ID | 20200714110919.50724-3-imbrenda@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix some compilation issues on 32bit | expand |
On 14/07/2020 13.09, Claudio Imbrenda wrote: > The assert in lib/alloc_page is hardcoded to long, and size_t is just > an int on 32 bit architectures. > > Adding a cast makes the compiler happy. > > Fixes: 73f4b202beb39 ("lib/alloc_page: change some parameter types") > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > --- > lib/alloc_page.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/lib/alloc_page.c b/lib/alloc_page.c > index fa3c527..617b003 100644 > --- a/lib/alloc_page.c > +++ b/lib/alloc_page.c > @@ -29,11 +29,12 @@ void free_pages(void *mem, size_t size) > assert_msg((unsigned long) mem % PAGE_SIZE == 0, > "mem not page aligned: %p", mem); > > - assert_msg(size % PAGE_SIZE == 0, "size not page aligned: %#lx", size); > + assert_msg(size % PAGE_SIZE == 0, "size not page aligned: %#lx", > + (unsigned long)size); > > assert_msg(size == 0 || (uintptr_t)mem == -size || > (uintptr_t)mem + size > (uintptr_t)mem, > - "mem + size overflow: %p + %#lx", mem, size); > + "mem + size overflow: %p + %#lx", mem, (unsigned long)size); Looking at lib/printf.c, it seems like it also supports %z ... have you tried? Thomas
On Tue, 14 Jul 2020 13:20:16 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 14/07/2020 13.09, Claudio Imbrenda wrote: > > The assert in lib/alloc_page is hardcoded to long, and size_t is > > just an int on 32 bit architectures. > > > > Adding a cast makes the compiler happy. > > > > Fixes: 73f4b202beb39 ("lib/alloc_page: change some parameter types") > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > > --- > > lib/alloc_page.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/lib/alloc_page.c b/lib/alloc_page.c > > index fa3c527..617b003 100644 > > --- a/lib/alloc_page.c > > +++ b/lib/alloc_page.c > > @@ -29,11 +29,12 @@ void free_pages(void *mem, size_t size) > > assert_msg((unsigned long) mem % PAGE_SIZE == 0, > > "mem not page aligned: %p", mem); > > > > - assert_msg(size % PAGE_SIZE == 0, "size not page aligned: > > %#lx", size); > > + assert_msg(size % PAGE_SIZE == 0, "size not page aligned: > > %#lx", > > + (unsigned long)size); > > > > assert_msg(size == 0 || (uintptr_t)mem == -size || > > (uintptr_t)mem + size > (uintptr_t)mem, > > - "mem + size overflow: %p + %#lx", mem, size); > > + "mem + size overflow: %p + %#lx", mem, > > (unsigned long)size); > > Looking at lib/printf.c, it seems like it also supports %z ... have > you tried? no, but in hindsight I should have. It's probably a much cleaner solution. I'll try and respin.
On Tue, Jul 14, 2020 at 01:41:23PM +0200, Claudio Imbrenda wrote: > On Tue, 14 Jul 2020 13:20:16 +0200 > Thomas Huth <thuth@redhat.com> wrote: > > > On 14/07/2020 13.09, Claudio Imbrenda wrote: > > > The assert in lib/alloc_page is hardcoded to long, and size_t is > > > just an int on 32 bit architectures. > > > > > > Adding a cast makes the compiler happy. > > > > > > Fixes: 73f4b202beb39 ("lib/alloc_page: change some parameter types") > > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > > > --- > > > lib/alloc_page.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/lib/alloc_page.c b/lib/alloc_page.c > > > index fa3c527..617b003 100644 > > > --- a/lib/alloc_page.c > > > +++ b/lib/alloc_page.c > > > @@ -29,11 +29,12 @@ void free_pages(void *mem, size_t size) > > > assert_msg((unsigned long) mem % PAGE_SIZE == 0, > > > "mem not page aligned: %p", mem); > > > > > > - assert_msg(size % PAGE_SIZE == 0, "size not page aligned: > > > %#lx", size); > > > + assert_msg(size % PAGE_SIZE == 0, "size not page aligned: > > > %#lx", > > > + (unsigned long)size); > > > > > > assert_msg(size == 0 || (uintptr_t)mem == -size || > > > (uintptr_t)mem + size > (uintptr_t)mem, > > > - "mem + size overflow: %p + %#lx", mem, size); > > > + "mem + size overflow: %p + %#lx", mem, > > > (unsigned long)size); > > > > Looking at lib/printf.c, it seems like it also supports %z ... have > > you tried? > > no, but in hindsight I should have. It's probably a much cleaner > solution. I'll try and respin. I'm not opposed to using size_t, but if we go that route then the entirety of alloc_page.c should be converted to size_t. As is, there is code like: void free_pages_by_order(void *mem, unsigned int order) { free_pages(mem, 1ul << (order + PAGE_SHIFT)); } and void *alloc_pages(unsigned int order) { ... /* Looking for a run of length (1 << order). */ unsigned long run = 0; const unsigned long n = 1ul << order; const unsigned long align_mask = (n << PAGE_SHIFT) - 1; void *run_start = NULL; void *run_prev = NULL; unsigned long run_next_pa = 0; unsigned long pa; assert(order < sizeof(unsigned long) * 8); ... } that very explicitly uses 'unsigned long' for the size.
On Tue, 14 Jul 2020 07:05:34 -0700 Sean Christopherson <sean.j.christopherson@intel.com> wrote: > On Tue, Jul 14, 2020 at 01:41:23PM +0200, Claudio Imbrenda wrote: > > On Tue, 14 Jul 2020 13:20:16 +0200 > > Thomas Huth <thuth@redhat.com> wrote: > > > > > On 14/07/2020 13.09, Claudio Imbrenda wrote: > > > > The assert in lib/alloc_page is hardcoded to long, and size_t is > > > > just an int on 32 bit architectures. > > > > > > > > Adding a cast makes the compiler happy. > > > > > > > > Fixes: 73f4b202beb39 ("lib/alloc_page: change some parameter > > > > types") Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > > > > --- > > > > lib/alloc_page.c | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/lib/alloc_page.c b/lib/alloc_page.c > > > > index fa3c527..617b003 100644 > > > > --- a/lib/alloc_page.c > > > > +++ b/lib/alloc_page.c > > > > @@ -29,11 +29,12 @@ void free_pages(void *mem, size_t size) > > > > assert_msg((unsigned long) mem % PAGE_SIZE == 0, > > > > "mem not page aligned: %p", mem); > > > > > > > > - assert_msg(size % PAGE_SIZE == 0, "size not page > > > > aligned: %#lx", size); > > > > + assert_msg(size % PAGE_SIZE == 0, "size not page > > > > aligned: %#lx", > > > > + (unsigned long)size); > > > > > > > > assert_msg(size == 0 || (uintptr_t)mem == -size || > > > > (uintptr_t)mem + size > (uintptr_t)mem, > > > > - "mem + size overflow: %p + %#lx", mem, > > > > size); > > > > + "mem + size overflow: %p + %#lx", mem, > > > > (unsigned long)size); > > > > > > Looking at lib/printf.c, it seems like it also supports %z ... > > > have you tried? > > > > no, but in hindsight I should have. It's probably a much cleaner > > solution. I'll try and respin. > > I'm not opposed to using size_t, but if we go that route then the > entirety of alloc_page.c should be converted to size_t. As is, there > is code like: > > void free_pages_by_order(void *mem, unsigned int order) > { > free_pages(mem, 1ul << (order + PAGE_SHIFT)); > } > > and > > void *alloc_pages(unsigned int order) > { > ... > > /* Looking for a run of length (1 << order). */ > unsigned long run = 0; > const unsigned long n = 1ul << order; > const unsigned long align_mask = (n << PAGE_SHIFT) - > 1; void *run_start = NULL; > void *run_prev = NULL; > unsigned long run_next_pa = 0; > unsigned long pa; > > assert(order < sizeof(unsigned long) * 8); > > ... > } > > that very explicitly uses 'unsigned long' for the size. don't worry, those won't stay there for long :) once this patch series has stabilized, I'm going to send a more radical rewrite of the allocators
diff --git a/lib/alloc_page.c b/lib/alloc_page.c index fa3c527..617b003 100644 --- a/lib/alloc_page.c +++ b/lib/alloc_page.c @@ -29,11 +29,12 @@ void free_pages(void *mem, size_t size) assert_msg((unsigned long) mem % PAGE_SIZE == 0, "mem not page aligned: %p", mem); - assert_msg(size % PAGE_SIZE == 0, "size not page aligned: %#lx", size); + assert_msg(size % PAGE_SIZE == 0, "size not page aligned: %#lx", + (unsigned long)size); assert_msg(size == 0 || (uintptr_t)mem == -size || (uintptr_t)mem + size > (uintptr_t)mem, - "mem + size overflow: %p + %#lx", mem, size); + "mem + size overflow: %p + %#lx", mem, (unsigned long)size); if (size == 0) { freelist = NULL;
The assert in lib/alloc_page is hardcoded to long, and size_t is just an int on 32 bit architectures. Adding a cast makes the compiler happy. Fixes: 73f4b202beb39 ("lib/alloc_page: change some parameter types") Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> --- lib/alloc_page.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)