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

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

[Bruce Richardson]

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 };
 	uint32_t type_tucmd_mlhl;
 	uint32_t mss_l4len_idx = 0;
 	uint32_t ctx_idx;
@@ -381,7 +382,7 @@ ixgbe_set_xmit_ctx(struct igb_tx_queue* txq,
 	mss_l4len_idx |= (ctx_idx << IXGBE_ADVTXD_IDX_SHIFT);
 
 	if (ol_flags & PKT_TX_VLAN_PKT) {
-		tx_offload_mask.vlan_tci = ~0;
+		tx_offload_mask.vlan_tci = offload_allones.vlan_tci;
 	}
 
 	/* check if TCP segmentation required for this packet */
@@ -391,17 +392,17 @@ ixgbe_set_xmit_ctx(struct igb_tx_queue* txq,
 			IXGBE_ADVTXD_TUCMD_L4T_TCP |
 			IXGBE_ADVTXD_DTYP_CTXT | IXGBE_ADVTXD_DCMD_DEXT;
 
-		tx_offload_mask.l2_len = ~0;
-		tx_offload_mask.l3_len = ~0;
-		tx_offload_mask.l4_len = ~0;
-		tx_offload_mask.tso_segsz = ~0;
+		tx_offload_mask.l2_len = offload_allones.l2_len;
+		tx_offload_mask.l3_len = offload_allones.l3_len;
+		tx_offload_mask.l4_len = offload_allones.l4_len;
+		tx_offload_mask.tso_segsz = offload_allones.tso_segsz;
 		mss_l4len_idx |= tx_offload.tso_segsz << IXGBE_ADVTXD_MSS_SHIFT;
 		mss_l4len_idx |= tx_offload.l4_len << IXGBE_ADVTXD_L4LEN_SHIFT;
 	} else { /* no TSO, check if hardware checksum is needed */
 		if (ol_flags & PKT_TX_IP_CKSUM) {
 			type_tucmd_mlhl = IXGBE_ADVTXD_TUCMD_IPV4;
-			tx_offload_mask.l2_len = ~0;
-			tx_offload_mask.l3_len = ~0;
+			tx_offload_mask.l2_len = offload_allones.l2_len;
+			tx_offload_mask.l3_len = offload_allones.l3_len;
 		}
 
 		switch (ol_flags & PKT_TX_L4_MASK) {
@@ -409,23 +410,23 @@ ixgbe_set_xmit_ctx(struct igb_tx_queue* txq,
 			type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_L4T_UDP |
 				IXGBE_ADVTXD_DTYP_CTXT | IXGBE_ADVTXD_DCMD_DEXT;
 			mss_l4len_idx |= sizeof(struct udp_hdr) << IXGBE_ADVTXD_L4LEN_SHIFT;
-			tx_offload_mask.l2_len = ~0;
-			tx_offload_mask.l3_len = ~0;
+			tx_offload_mask.l2_len = offload_allones.l2_len;
+			tx_offload_mask.l3_len = offload_allones.l3_len;
 			break;
 		case PKT_TX_TCP_CKSUM:
 			type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_L4T_TCP |
 				IXGBE_ADVTXD_DTYP_CTXT | IXGBE_ADVTXD_DCMD_DEXT;
 			mss_l4len_idx |= sizeof(struct tcp_hdr) << IXGBE_ADVTXD_L4LEN_SHIFT;
-			tx_offload_mask.l2_len = ~0;
-			tx_offload_mask.l3_len = ~0;
-			tx_offload_mask.l4_len = ~0;
+			tx_offload_mask.l2_len = offload_allones.l2_len;
+			tx_offload_mask.l3_len = offload_allones.l3_len;
+			tx_offload_mask.l4_len = offload_allones.l4_len;
 			break;
 		case PKT_TX_SCTP_CKSUM:
 			type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_L4T_SCTP |
 				IXGBE_ADVTXD_DTYP_CTXT | IXGBE_ADVTXD_DCMD_DEXT;
 			mss_l4len_idx |= sizeof(struct sctp_hdr) << IXGBE_ADVTXD_L4LEN_SHIFT;
-			tx_offload_mask.l2_len = ~0;
-			tx_offload_mask.l3_len = ~0;
+			tx_offload_mask.l2_len = offload_allones.l2_len;
+			tx_offload_mask.l3_len = offload_allones.l3_len;
 			break;
 		default:
 			type_tucmd_mlhl |= IXGBE_ADVTXD_TUCMD_L4T_RSV |
-- 
2.1.0

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

[Neil Horman]

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)
Neil

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

[Olivier MATZ]

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 '=')

I would avoid to create a macro. What do you think?

Regards,
Olivier

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

[Bruce Richardson]

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 '=')
> 
> 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

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

[Olivier MATZ]

Hi Bruce,

On 12/01/2014 10:48 AM, Bruce Richardson wrote:
>> 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?

I'll submit the patch, after all it's my mistake!

Regards,
Olivier

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

[Neil Horman]

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

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

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

[Bruce Richardson]

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.

/Bruce

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

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

[Neil Horman]

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

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

[Bruce Richardson]

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

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

[Neil Horman]

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

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

[Bruce Richardson]

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.

/Bruce

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

[Neil Horman]

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?

Neil

> /Bruce
> 

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

[Bruce Richardson]

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.

/Bruce

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

[Neil Horman]

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

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

[Olivier MATZ]

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

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

[Neil Horman]

On Mon, Dec 01, 2014 at 10:55:29PM +0100, Olivier MATZ wrote:
> 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;
> 
Thats true, but the Linux kernel has the luxury of assuming that gcc is the only
compiler that will be building it, and so developers are free to make
assumptions about its behavior.  Thats not true of the DPDK.  DPDK allows at
least 3 compilers to build it in a supported fashion (clan/llvm, icc and gcc).
The behavior of the above may be completely different on each compiler.

> 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.
> 
Thats exactly the problem I'm referring to.  If ixgbe preforms assignment on the
.data value (of anything other than all 1's or all 0's), this will break.  Its
also problematic in a larger scope because rte_mbuf uses this same pattern, and
that interface is exposed to applications, where we have no visibility over what
is being accessed and how.

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