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