From: Olivier MATZ <olivier.matz@6wind.com>
To: Neil Horman <nhorman@tuxdriver.com>,
Bruce Richardson <bruce.richardson@intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] ixgbe: fix clang compile - remove truncation errors
Date: Mon, 01 Dec 2014 22:55:29 +0100 [thread overview]
Message-ID: <547CE3D1.50104@6wind.com> (raw)
In-Reply-To: <20141201171623.GE15135@hmsreliant.think-freely.org>
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
next prev parent reply other threads:[~2014-12-01 21:55 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-28 15:31 Bruce Richardson
2014-11-30 1:05 ` Neil Horman
2014-12-01 9:09 ` Olivier MATZ
2014-12-01 9:48 ` Bruce Richardson
2014-12-01 9:59 ` Olivier MATZ
2014-12-01 11:18 ` Neil Horman
2014-12-01 11:24 ` Bruce Richardson
2014-12-01 14:25 ` Neil Horman
2014-12-01 14:36 ` Bruce Richardson
2014-12-01 15:18 ` Neil Horman
2014-12-01 15:24 ` Bruce Richardson
2014-12-01 16:35 ` Neil Horman
2014-12-01 16:44 ` Bruce Richardson
2014-12-01 17:16 ` Neil Horman
2014-12-01 21:55 ` Olivier MATZ [this message]
2014-12-02 11:32 ` Neil Horman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=547CE3D1.50104@6wind.com \
--to=olivier.matz@6wind.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=nhorman@tuxdriver.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).