[dpdk-dev] [PATCH] ixgbe: fix clang compile - remove truncation errors
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
[dpdk-dev] [PATCH] ixgbe: fix clang compile - remove truncation errors
On Mon, Dec 01, 2014 at 11:35:28AM -0500, Neil Horman wrote:
> On Mon, Dec 01, 2014 at 03:24:32PM +, 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 +, 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 +, 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 +, 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 > > > > > > > >> 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
[dpdk-dev] [PATCH] ixgbe: fix clang compile - remove truncation errors
On Mon, Dec 01, 2014 at 10:18:06AM -0500, Neil Horman wrote:
> On Mon, Dec 01, 2014 at 02:36:46PM +, 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 +, 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 +, 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
> > > > > > >> ---
> > > > > > >> 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
[dpdk-dev] [PATCH] ixgbe: fix clang compile - remove truncation errors
On Mon, Dec 01, 2014 at 04:44:39PM +, 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 +, 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 +, 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 +, 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 +, 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 > > > > > > > > >> 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 |=
[dpdk-dev] [PATCH] ixgbe: fix clang compile - remove truncation errors
On Mon, Dec 01, 2014 at 03:24:32PM +, 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 +, 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 +, 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 +, 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
> > > > > > > >> ---
> > > > > > > >> 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
[dpdk-dev] [PATCH] ixgbe: fix clang compile - remove truncation errors
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 +, 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
> > >> ---
> > >> 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.
/Bruce
> > I would avoid to create a macro. What do you think?
> >
> > Regards,
> > Olivier
> >
[dpdk-dev] [PATCH] ixgbe: fix clang compile - remove truncation errors
On Mon, Dec 01, 2014 at 02:36:46PM +, 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 +, 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 +, 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
> > > > > >> ---
> > > > > >> 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
[dpdk-dev] [PATCH] ixgbe: fix clang compile - remove truncation errors
Hi Bruce, Hi Neil,
On 11/30/2014 02:05 AM, Neil Horman wrote:
> On Fri, Nov 28, 2014 at 03:31:00PM +, 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
>> ---
>> 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 '=')
I would avoid to create a macro. What do you think?
Regards,
Olivier
[dpdk-dev] [PATCH] ixgbe: fix clang compile - remove truncation errors
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 +, 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
> >> ---
> >> 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 '=')
>
> I would avoid to create a macro. What do you think?
>
> Regards,
> Olivier
Nice one - cleanest solution thus far, so I suggest we go with that option. Have
you got a patch for it ready?
/Bruce
[dpdk-dev] [PATCH] ixgbe: fix clang compile - remove truncation errors
On Mon, Dec 01, 2014 at 11:24:58AM +, 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 +, 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
> > > >> ---
> > > >> 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
> > >
>
[dpdk-dev] [PATCH] ixgbe: fix clang compile - remove truncation errors
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 +, 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
> >> ---
> >> 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
> I would avoid to create a macro. What do you think?
>
> Regards,
> Olivier
>
[dpdk-dev] [PATCH] ixgbe: fix clang compile - remove truncation errors
On Fri, Nov 28, 2014 at 03:31:00PM +, 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
> ---
> 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)
Neil
