Re: [Minios-devel] [UNIKRAFT PATCH 2/2] lib/uknetdev: Fix compliation warning about signed/unsigned comparision

[Julien Grall <julien.grall@xxxxxxx>]

On 11/10/2019 16:13, Justin He (Arm Technology China) wrote:
> Hi Julien
Hi,

> > -----Original Message-----
> > From: Julien Grall <julien.grall@xxxxxxx>
> > Sent: Friday, October 11, 2019 5:00 PM
> > To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; minios-
> > devel@xxxxxxxxxxxxxxxxxxxx; Simon Kuenzer <simon.kuenzer@xxxxxxxxx>;
> > Costin Lupu <costin.lupu@xxxxxxxxx>
> > Cc: Sharan.Santhanam@xxxxxxxxx; Felipe Huici <felipe.huici@xxxxxxxxx>;
> > Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>;
> > Santiago.Pagani@xxxxxxxxx; nd <nd@xxxxxxx>
> > Subject: Re: [UNIKRAFT PATCH 2/2] lib/uknetdev: Fix compliation warning
> > about signed/unsigned comparision
> >
> > Hi,
> >
> > On 11/10/2019 09:52, Jia He wrote:
> > > This fixes the compliation warning when compiling libuknetdev
> > Typo s/compliation/compilation/
> >
> > > /root/hj/UK/unikraft/lib/uknetdev/netbuf.c: In function
> > > 'uk_netbuf_alloc_buf':
> > > /root/hj/UK/unikraft/lib/uknetdev/netbuf.c:120:35: warning: comparison
> > > between signed and unsigned integer expressions [-Wsign-compare]
> > >     if (likely(UINT16_MAX - headroom > NETBUF_ADDR_ALIGNMENT)) {
> > >                                      ^
> > > /root/hj/UK/unikraft/include/uk/arch/lcpu.h:48:43: note: in definition
> > > of macro 'likely'
> > >    #define likely(x)   (__builtin_expect((!!(x)), 1))
> > >                                              ^
> > > Signed-off-by: Jia He <justin.he@xxxxxxx>
> > > ---
> > >    lib/uknetdev/netbuf.c | 2 +-
> > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/lib/uknetdev/netbuf.c b/lib/uknetdev/netbuf.c
> > > index 96a5f68..108bd36 100644
> > > --- a/lib/uknetdev/netbuf.c
> > > +++ b/lib/uknetdev/netbuf.c
> > > @@ -36,7 +36,7 @@
> > >    #include <uk/print.h>
> > >
> > >    /* Used to align netbuf's priv and data areas to `long long` data type */
> > > -#define NETBUF_ADDR_ALIGNMENT (sizeof(long long))
> > > +#define NETBUF_ADDR_ALIGNMENT (int)(sizeof(long long))
> > Maybe I am missing something but:
> >
> > UINT16_MAX - headrom should be unsigned, right?
> >
> > IIRC, sizeof() is also returning an unsigned. So how this is solving the
> > problem
> > here?
> Because UINT16_MAX and headroom are both unsigned short,
Looking at the definition, UINT16_MAX is an unsigned int. So I am a bit 
confused.

> Integer types smaller than int are promoted when an operation is
> performed on them. Thus, the left side of equation is "int", and
> the right side is size_t.
It would be nice if you can provide more information in your commit message. 
This helps the reviewer to figure out the problem more easily.
Although, I am still unsure to understand how this is related to the issue here. 
Wouldn't the problem be unsigned integer are promoted to long long?
Regarding the patch itself, this is only deferring the problem until someone 
come up with a check that will use unsigned value.
A better approach would be to fix it in the check directly. Maybe:

(size_t)(UINT16_MAX - headroom)?

Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel