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

["Justin He (Arm Technology China)" <Justin.He@xxxxxxx>]

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: Friday, October 11, 2019 11:30 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
> 
> 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.

I tracked the header file including process, I thought UINT16_MAX is int here

build/libnewlibc/origin/newlib-2.5.0.20170922/newlib/libc/include/stdint.h:176:#define
 UINT16_MAX (__UINT16_MAX__)

Then (__UINT16_MAX__) is a builtin of gcc.
I wrote a simple example to indicate this:
int main()
{ 
        unsigned short a;
        size_t b;

        {__UINT16_MAX__ - a < b;}
}

It is 0xffff instead of 0xffffU after pre-compilation

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

Sure

> 
> 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)?
Yes, agree with that


--
Cheers,
Justin (Jia He)


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