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

[Neil Horman <nhorman@tuxdriver.com>]

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

> /Bruce
> 
> > > I would avoid to create a macro. What do you think?
> > > 
> > > Regards,
> > > Olivier
> > > 
>