[dpdk-dev,v6,5/8] rte_mbuf.h: avoid implicit demotion in 64-bit arith

  /projects/lagopus/src/dpdk/build/include/rte_mbuf.h:
In function 'rte_validate_tx_offload':
/projects/lagopus/src/dpdk/build/include/rte_mbuf.h:
2112:19: warning: conversion to 'uint64_t'
{aka 'long unsigned int'} from 'int' may change the
sign of the result [-Wsign-conversion]
  inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
                   ^~

  uint64_t inner_l3_offset...

  /* fields for TX offloading of tunnels */
  uint64_t outer_l3_len:9; /**< Outer L3 (IP) Hdr Length. */
  uint64_t outer_l2_len:7; /**< Outer L2 (MAC) Hdr Length. */

We want to do the arithmetic entirely in uint64_t
space, but there is an implicit demotion to int created by
the +=.  Remove the +=.

Fixes: 4fb7e803eb ("ethdev: add Tx preparation")
Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_mbuf/rte_mbuf.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
  

On Mon, May 21, 2018 at 10:01:18AM +0800, Andy Green wrote:
> /projects/lagopus/src/dpdk/build/include/rte_mbuf.h:
> In function 'rte_validate_tx_offload':
> /projects/lagopus/src/dpdk/build/include/rte_mbuf.h:
> 2112:19: warning: conversion to 'uint64_t'
> {aka 'long unsigned int'} from 'int' may change the
> sign of the result [-Wsign-conversion]
>   inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
>                    ^~
> 
>   uint64_t inner_l3_offset...
> 
>   /* fields for TX offloading of tunnels */
>   uint64_t outer_l3_len:9; /**< Outer L3 (IP) Hdr Length. */
>   uint64_t outer_l2_len:7; /**< Outer L2 (MAC) Hdr Length. */
> 
> We want to do the arithmetic entirely in uint64_t
> space, but there is an implicit demotion to int created by
> the +=.  Remove the +=.
> 
> Fixes: 4fb7e803eb ("ethdev: add Tx preparation")
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  lib/librte_mbuf/rte_mbuf.h |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>

Fix looks ok, but given that it's non-obvious why += doesn't work, I think
it would be good to put a comment in explaining it. Otherwise I could see
someone changing this back in a later patch, because the problem doesn't
arise with regular DPDK compiles 

Acked-by: Bruce Richardson <bruce.richardson@intel.com>
  

On 05/21/2018 08:27 PM, Bruce Richardson wrote:
> On Mon, May 21, 2018 at 10:01:18AM +0800, Andy Green wrote:
>> /projects/lagopus/src/dpdk/build/include/rte_mbuf.h:
>> In function 'rte_validate_tx_offload':
>> /projects/lagopus/src/dpdk/build/include/rte_mbuf.h:
>> 2112:19: warning: conversion to 'uint64_t'
>> {aka 'long unsigned int'} from 'int' may change the
>> sign of the result [-Wsign-conversion]
>>    inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
>>                     ^~
>>
>>    uint64_t inner_l3_offset...
>>
>>    /* fields for TX offloading of tunnels */
>>    uint64_t outer_l3_len:9; /**< Outer L3 (IP) Hdr Length. */
>>    uint64_t outer_l2_len:7; /**< Outer L2 (MAC) Hdr Length. */
>>
>> We want to do the arithmetic entirely in uint64_t
>> space, but there is an implicit demotion to int created by
>> the +=.  Remove the +=.
>>
>> Fixes: 4fb7e803eb ("ethdev: add Tx preparation")
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> ---
>>   lib/librte_mbuf/rte_mbuf.h |    3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
> 
> Fix looks ok, but given that it's non-obvious why += doesn't work, I think
> it would be good to put a comment in explaining it. Otherwise I could see

I commnented these and the related stanzas in the second patch you 
suggested the comment in v7.

> someone changing this back in a later patch, because the problem doesn't
> arise with regular DPDK compiles

Yes the header apis are not actually used just building dpdk, so you 
don't see any problem.  But the problem will come as soon as you try to 
actually build something else against dpdk + gcc8.1.

-Andy

> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>