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

[Neil Horman <nhorman@tuxdriver.com>]

On Mon, Dec 01, 2014 at 04:44:39PM +0000, Bruce Richardson wrote:
> On Mon, Dec 01, 2014 at 11:35:28AM -0500, Neil Horman wrote:
> > On Mon, Dec 01, 2014 at 03:24:32PM +0000, Bruce Richardson wrote:
> > > On Mon, Dec 01, 2014 at 10:18:06AM -0500, Neil Horman wrote:
> > > > On Mon, Dec 01, 2014 at 02:36:46PM +0000, Bruce Richardson wrote:
> > > > > On Mon, Dec 01, 2014 at 09:25:44AM -0500, Neil Horman wrote:
> > > > > > On Mon, Dec 01, 2014 at 11:24:58AM +0000, Bruce Richardson wrote:
> > > > > > > On Mon, Dec 01, 2014 at 06:18:17AM -0500, Neil Horman wrote:
> > > > > > > > On Mon, Dec 01, 2014 at 10:09:38AM +0100, Olivier MATZ wrote:
> > > > > > > > > Hi Bruce, Hi Neil,
> > > > > > > > > 
> > > > > > > > > On 11/30/2014 02:05 AM, Neil Horman wrote:
> > > > > > > > > > On Fri, Nov 28, 2014 at 03:31:00PM +0000, Bruce Richardson wrote:
> > > > > > > > > >> When compiling with clang, errors were being emitted due to truncation
> > > > > > > > > >> of values when assigning to the tx_offload_mask bit fields.
> > > > > > > > > >>
> > > > > > > > > >> dpdk.org/lib/librte_pmd_ixgbe/ixgbe_rxtx.c:404:27: fatal error: implicit truncation from 'int' to bitfield changes value from -1 to 127 [-Wbitfield-constant-conversion]
> > > > > > > > > >> 		    tx_offload_mask.l2_len = ~0;
> > > > > > > > > >>
> > > > > > > > > >> The fix proposed here is to define a static const value of the same type
> > > > > > > > > >> with all fields set to 1s, and use that instead of constants for assigning to.
> > > > > > > > > >>
> > > > > > > > > >> Other options would be to explicitily define the suitable constants that
> > > > > > > > > >> would not truncate for each individual field e.g. 0x7f for l2_len, 0x1FF
> > > > > > > > > >> for l3_len, etc., but this solution here has the advantage that it works
> > > > > > > > > >> without any changes to values if the field sizes are ever modified.
> > > > > > > > > >>
> > > > > > > > > >> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > > > > > > > >> ---
> > > > > > > > > >>  lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 29 +++++++++++++++--------------
> > > > > > > > > >>  1 file changed, 15 insertions(+), 14 deletions(-)
> > > > > > > > > >>
> > > > > > > > > >> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> > > > > > > > > >> index 8559ef6..4f71194 100644
> > > > > > > > > >> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> > > > > > > > > >> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> > > > > > > > > >> @@ -367,6 +367,7 @@ ixgbe_set_xmit_ctx(struct igb_tx_queue* txq,
> > > > > > > > > >>  		volatile struct ixgbe_adv_tx_context_desc *ctx_txd,
> > > > > > > > > >>  		uint64_t ol_flags, union ixgbe_tx_offload tx_offload)
> > > > > > > > > >>  {
> > > > > > > > > >> +	static const union ixgbe_tx_offload offload_allones = { .data = ~0 };
> > > > > > > > > > Do you want to make this a static data structure?  If you make it a macro like
> > > > > > > > > > this:
> > > > > > > > > > #define ALLONES {.data = ~0}
> > > > > > > > > > Then you save the extra data space in the .data area (not that its that much),
> > > > > > > > > > and you can define it in a header file and use it in multiple c files (if you
> > > > > > > > > > need to)
> > > > > > > > > 
> > > > > > > > > I found that the following code works:
> > > > > > > > > 
> > > > > > > > > 	tx_offload_mask.l2_len |= ~0;
> > > > > > > > > 
> > > > > > > > > (note the '|=' instead of '=')
> > > > > > > > > 
> > > > > > > > How exactly does this work? does the or operator imply some level of type
> > > > > > > > promotion that the assignment doesn't to avoid the truncation?
> > > > > > > > Neil
> > > > > > > > 
> > > > > > > 
> > > > > > > For any aithmetic, and presumably logical, operation on two values, any values
> > > > > > > smaller than "int" are promoted to type int before the operation takes place. I
> > > > > > > believe the exact rules for this are in the C specs e.g. C99.
> > > > > > > 
> > > > > > Yes, but I would have thought that assignment was included in the list of
> > > > > > logical operations for that promotion to occur.  The above change seems to
> > > > > > suggest it isn't, which is why I'm asking.  C99 specifies |= explicitly as a
> > > > > > type of assignment operator (see 6.5.16 here:
> > > > > > http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf
> > > > > > )
> > > > > > 
> > > > > > So I would presume that using = should work exactly the same as |= in terms of
> > > > > > type promotion.  We also don't get this warning on gcc, which concerns me that
> > > > > > we might just be papering over a compiler problem here.
> > > > > > 
> > > > > > Looking at the error, its complaining that we're truncating an int value to a
> > > > > > bitfield (which we are), and that the resultant value is 127 rather that -1
> > > > > > (which it would be if we actually intended to treat it as an integer
> > > > > > 
> > > > > > Which I think is where the problem lies.  That is to say we've typed the
> > > > > > bitfields in ixgbe_tx_offload as uint64_t.  I'm guessing gcc just promotes ~0 to
> > > > > > an unsigned int during the assignment, and supresses the warning (unless you
> > > > > > turn on -pedantic or some such), but clang does not.  The correct solution I
> > > > > > think here is to either:
> > > > > > 
> > > > > > 1) modify the bitfield types so that they are signed integers, thereby
> > > > > > maintaining the resultant value of -1 after the assignment
> > > > > > 
> > > > > > or
> > > > > > 
> > > > > > 2) Modify the ~0 to be ~0UL, so that the clang compiler sees that the resultant
> > > > > > value will be MAXLONG after the assignment
> > > > > > 
> > > > > > I'd think operation 2 would be the better choice
> > > > > > Neil
> > > > > >
> > > > > I'm not a compiler expert, but looking at it a bit more what I think is 
> > > > > happening is that we are simply changing the assignment from a constant one to
> > > > > a computed one instead. With the constant assignment, the compiler can check that
> > > > > the assignment doesn't overflow, while with the computed value, it has no choice
> > > > > to accept the truncation since any computation is going to take place with variables
> > > > > of at least size "int" and there is no way to typecast the resulting value to
> > > > > a bit field.
> > > > > 
> > > > > As for papering-over compiler niggles, possibly so, but this solution is shorter and
> > > > > less impactful than the other solutions which are less workaround-like - i.e. those
> > > > > that assign values of exactly the right size using either magic numbers or a
> > > > > special-value copy of the structure.
> > > > > 
> > > > > Also, in terms of the two options you propose, I tried the second and it still gives
> > > > > errors, so the signed-ness or unsigned-ness is not the problem the compiler has, its
> > > > > the truncation.
> > > > > 
> > > > >   CC ixgbe_rxtx.o
> > > > >   /usr/home/bruce/dpdk.org/lib/librte_pmd_ixgbe/ixgbe_rxtx.c:384:28: fatal error: implicit truncation from 'unsigned long' to bitfield changes value from 18446744073709551615 to 65535
> > > > >         [-Wbitfield-constant-conversion]
> > > > > 	                tx_offload_mask.vlan_tci = ~0UL;
> > > > > 
> > > > > 
> > > > > /Bruce
> > > > > 
> > > > You're right, it does, and looking at it now, I wonder if the warning really
> > > > just needs to be removed.  Gcc has no such warning in it currently (though I
> > > > expect -pedantic would say something here).  Regardless, looking at the clang
> > > > docs I can't find any documentation about the bitfield-constant-conversion
> > > > warning, and it seems to exist only to tell us that we're truncating an integer
> > > > to an integer of a smaller size (which will clearly be the case anytime we are
> > > > assigning a constant to a bitfield).  Instead of avoiding the warning by doing
> > > > any sort of code trickery, why not just remove the warning?
> > > > 
> > > > Neil
> > > >
> > > That's one for a community discussion. My opinion on it is that disabling warnings
> > > is the last option I'd look for - as a point of principle. I'd prefer to take 
> > > Olivier's fix, or my proposed fix, over disabling the warnings. The fix is fairly
> > > trivial, and I think disabling warnings for something like this would set a bad
> > > precedent.
> > > 
> > Nominally I would agree, except in this case I fail to see what the warning buys
> > us.  Its warning about the fact that the value of a constant is being truncated
> > to the size of a bitfield that is smaller than the constants type.  That fact
> > will always be true for any bitfield that is not exactly the size of the
> > constants type, so it doesn't really provide us with any actionable information.
> > Add to that the fact that the proposed patch serves no purpose other than to
> > silence a warning (i.e. its functionally no different from a straight
> > assignment, and theres no clear reasoning to use an |= over an =).  What this
> > leaves us with is a situation where, because of a clang-only warning, future
> > developers are going to have to remember that in any code they write, they will
> > have to assign bitfields using |= and &= to shuffle bits about, if they want to
> > avoid an error they may never see (if they don't test with clang consistently).
> > 
> > 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.

Neil

> /Bruce
> 
>