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

[Olivier MATZ <olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

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;

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.

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