Re: [dpdk-dev] [PATCH] ixgbe: fix clang compile - remove truncation errors

[Neil Horman <nhorman@tuxdriver.com>]

On Mon, Dec 01, 2014 at 10:55:29PM +0100, Olivier MATZ wrote:
> Hi Neil,
> 
> On 12/01/2014 06:16 PM, Neil Horman wrote:
> >>> [...]
> >>>
> >>> Whats the advantage to keeping this warning?
> >>>
> >> The advantage is that it does exactly what it's meant to do. If someone goes to
> >> assign l2_len = 128; somewhere, it will throw a warning. If someone goes to change
> >> the lpm library and set [lpm_table_entry].depth = 64 somewhere, it will throw
> >> a warning. The reason the warning is a problem here is because we are in the
> >> usual position of wanting to initialize all values to 1's. It's causing problems
> >> nowhere else.
> >>
> >> However, let me query the scope of the disabling the warning you are talking about.
> >> If we just disable the warning for the one problematic function, it's probably
> >> reasonable enough because of the all-1's initialization, but disabling it globally
> >> is not something I would agree with.
> >>
> > No, this actually makes some sense as far as the warning goes, though it seems
> > like we can't rely on it, since clang is the only thing that throws the warning.
> > 
> > That said, I was just looking at this code, and I think the use of these
> > bitfields is problematic anyway (or at least potentially so).  The bitfields
> > exist as a set in a union that also contains a data field, and the bitfields and
> > data are type puned (both in the ixgbe implementation and I think in the
> > rte_mbuf implementation).  GCC (nor any C compiler that I'm aware of) provides
> > any guarantee regarding the bit endianess of any given field, nor any padding in
> > between bitfields, nor any ordering among bitfields.  The take away from that
> > is, while I can't currently find any use of the data member of the referenced
> > unions, if theres any expectation as to the value of said data member of that
> > union, theres no guarantee it will be correct between platforms.  It seems like
> > we should be using a single typed integer value here and some bit shifting
> > values to set fields within it, rather than bitfields.
> 
> The padding and endianess of bitfields is maybe not defined, but I think
> many people at least suppose the way it works, since we have the
> following code in standard headers (netinet/ip.h):
> 
>   #if __BYTE_ORDER == __LITTLE_ENDIAN
>     unsigned int flags:4;
>     unsigned int overflow:4;
>   #elif __BYTE_ORDER == __BIG_ENDIAN
>     unsigned int overflow:4;
>     unsigned int flags:4;
> 
Thats true, but the Linux kernel has the luxury of assuming that gcc is the only
compiler that will be building it, and so developers are free to make
assumptions about its behavior.  Thats not true of the DPDK.  DPDK allows at
least 3 compilers to build it in a supported fashion (clan/llvm, icc and gcc).
The behavior of the above may be completely different on each compiler.

> That said, the .data field of the union is only used to do faster
> assignment and comparison in ixgbe or mbuf, so I don't think there is
> no issue with that.
> 
Thats exactly the problem I'm referring to.  If ixgbe preforms assignment on the
.data value (of anything other than all 1's or all 0's), this will break.  Its
also problematic in a larger scope because rte_mbuf uses this same pattern, and
that interface is exposed to applications, where we have no visibility over what
is being accessed and how.

> About removing the warning, I agree with Bruce: it is maybe useful in
> other cases and I think we should keep it. However, if there is no
> consensus on the "|=" solution, I can provide another patch that solves
> the issue in a different way, maybe using a static const variable.
> 
> Regards,
> Olivier
>