From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 18325DE0 for ; Mon, 1 Dec 2014 22:55:44 +0100 (CET) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:128) (Exim 4.80) (envelope-from ) id 1XvYza-0006o3-35; Mon, 01 Dec 2014 22:58:53 +0100 Message-ID: <547CE3D1.50104@6wind.com> Date: Mon, 01 Dec 2014 22:55:29 +0100 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Icedove/24.5.0 MIME-Version: 1.0 To: Neil Horman , Bruce Richardson 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> In-Reply-To: <20141201171623.GE15135@hmsreliant.think-freely.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH] ixgbe: fix clang compile - remove truncation errors X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 01 Dec 2014 21:55:44 -0000 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