From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier MATZ Subject: Re: [PATCH] ixgbe: fix clang compile - remove truncation errors Date: Mon, 01 Dec 2014 22:55:29 +0100 Message-ID: <547CE3D1.50104@6wind.com> References: <20141130010514.GA19479@localhost.localdomain> <547C3052.4080106@6wind.com> <20141201111817.GA15135@hmsreliant.think-freely.org> <20141201112458.GD4856@bricha3-MOBL3> <20141201142544.GB15135@hmsreliant.think-freely.org> <20141201143646.GE4856@bricha3-MOBL3> <20141201151806.GC15135@hmsreliant.think-freely.org> <20141201152432.GF4856@bricha3-MOBL3> <20141201163528.GD15135@hmsreliant.think-freely.org> <20141201164439.GG4856@bricha3-MOBL3> <20141201171623.GE15135@hmsreliant.think-freely.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: dev-VfR2kkLFssw@public.gmane.org To: Neil Horman , Bruce Richardson Return-path: In-Reply-To: <20141201171623.GE15135-B26myB8xz7F8NnZeBjwnZQMhkBWG/bsMQH7oEaQurus@public.gmane.org> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" 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