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

Bruce Richardson bruce.richardson at intel.com
Mon Dec 1 15:36:46 CET 2014


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 at 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


More information about the dev mailing list