* Re: [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error
[not found] ` <1417606099-3489-1-git-send-email-michael.qiu@intel.com>
@ 2014-12-03 11:42 ` Bruce Richardson
2014-12-03 13:59 ` Qiu, Michael
` (2 more replies)
0 siblings, 3 replies; 32+ messages in thread
From: Bruce Richardson @ 2014-12-03 11:42 UTC (permalink / raw)
To: Michael Qiu; +Cc: dev
On Wed, Dec 03, 2014 at 07:28:19PM +0800, Michael Qiu wrote:
> app/test-pmd/csumonly.c: In function ‘get_psd_sum’:
> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’
> does break strict-aliasing rules
> build/include/rte_ip.h:157: note: initialized from here
> ...
>
> The root cause is that, compile enable strict aliasing by default,
> while in function rte_raw_cksum() try to convert 'const char *'
> to 'const uint16_t *'.
>
What compiler version is this with? Is there any other way to fix this
other than disabling the compiler warnings. Turning off strict aliasing may
affect performance as it reduces the number of optimizations that the compiler
can perform.
/Bruce
> Need to add CFLAG '-Wno-strict-aliasing' to avoid this issue.
>
> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> ---
> app/test-pmd/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/app/test-pmd/Makefile b/app/test-pmd/Makefile
> index 97dc2e6..995c874 100644
> --- a/app/test-pmd/Makefile
> +++ b/app/test-pmd/Makefile
> @@ -38,7 +38,7 @@ ifeq ($(CONFIG_RTE_TEST_PMD),y)
> #
> APP = testpmd
>
> -CFLAGS += -O3
> +CFLAGS += -O3 -Wno-strict-aliasing
> CFLAGS += $(WERROR_FLAGS)
>
> ifeq ($(CONFIG_RTE_LIBRTE_PMD_PCAP),y)
> --
> 1.9.3
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error
2014-12-03 11:42 ` [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error Bruce Richardson
@ 2014-12-03 13:59 ` Qiu, Michael
2014-12-03 14:51 ` Bruce Richardson
2014-12-03 15:24 ` [dpdk-dev] [PATCH] " Olivier MATZ
2014-12-03 15:57 ` Dayu Qiu
2 siblings, 1 reply; 32+ messages in thread
From: Qiu, Michael @ 2014-12-03 13:59 UTC (permalink / raw)
To: Richardson, Bruce, Michael Qiu; +Cc: dev
On 2014/12/3 19:43, Richardson, Bruce wrote:
> On Wed, Dec 03, 2014 at 07:28:19PM +0800, Michael Qiu wrote:
>> app/test-pmd/csumonly.c: In function ‘get_psd_sum’:
>> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’
>> does break strict-aliasing rules
>> build/include/rte_ip.h:157: note: initialized from here
>> ...
>>
>> The root cause is that, compile enable strict aliasing by default,
>> while in function rte_raw_cksum() try to convert 'const char *'
>> to 'const uint16_t *'.
>>
> What compiler version is this with? Is there any other way to fix this
> other than disabling the compiler warnings. Turning off strict aliasing may
> affect performance as it reduces the number of optimizations that the compiler
> can perform.
The compile version is:
$ gcc -v
Using built-in specs.
Target: x86_64-redhat-linux
...
gcc version 4.4.7 20120313 (Red Hat 4.4.7-4) (GCC)
The OS is centos6.5 x86_64
Actually, another possible solution is, as gcc manual shows, use union.
In function rte_raw_cksum() of lib/librte_net/rte_ip.h:
static inline uint16_t
rte_raw_cksum(const char *buf, size_t len){
union {
const char *ubuf;
const uint16_t *uu16;
} convert;
convert.ubuf = buf;
const uint16_t *u16 = convert.uu16;
...
}
This may be work, but not test yet.
Any comments about this solution?
Thanks,
Michael
>
> /Bruce
>
>> Need to add CFLAG '-Wno-strict-aliasing' to avoid this issue.
>>
>> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
>> ---
>> app/test-pmd/Makefile | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/app/test-pmd/Makefile b/app/test-pmd/Makefile
>> index 97dc2e6..995c874 100644
>> --- a/app/test-pmd/Makefile
>> +++ b/app/test-pmd/Makefile
>> @@ -38,7 +38,7 @@ ifeq ($(CONFIG_RTE_TEST_PMD),y)
>> #
>> APP = testpmd
>>
>> -CFLAGS += -O3
>> +CFLAGS += -O3 -Wno-strict-aliasing
>> CFLAGS += $(WERROR_FLAGS)
>>
>> ifeq ($(CONFIG_RTE_LIBRTE_PMD_PCAP),y)
>> --
>> 1.9.3
>>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error
2014-12-03 13:59 ` Qiu, Michael
@ 2014-12-03 14:51 ` Bruce Richardson
2014-12-03 15:19 ` Qiu, Michael
0 siblings, 1 reply; 32+ messages in thread
From: Bruce Richardson @ 2014-12-03 14:51 UTC (permalink / raw)
To: Qiu, Michael; +Cc: dev, Michael Qiu
On Wed, Dec 03, 2014 at 01:59:58PM +0000, Qiu, Michael wrote:
> On 2014/12/3 19:43, Richardson, Bruce wrote:
> > On Wed, Dec 03, 2014 at 07:28:19PM +0800, Michael Qiu wrote:
> >> app/test-pmd/csumonly.c: In function ‘get_psd_sum’:
> >> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’
> >> does break strict-aliasing rules
> >> build/include/rte_ip.h:157: note: initialized from here
> >> ...
> >>
> >> The root cause is that, compile enable strict aliasing by default,
> >> while in function rte_raw_cksum() try to convert 'const char *'
> >> to 'const uint16_t *'.
> >>
> > What compiler version is this with? Is there any other way to fix this
> > other than disabling the compiler warnings. Turning off strict aliasing may
> > affect performance as it reduces the number of optimizations that the compiler
> > can perform.
>
> The compile version is:
>
> $ gcc -v
> Using built-in specs.
> Target: x86_64-redhat-linux
> ...
> gcc version 4.4.7 20120313 (Red Hat 4.4.7-4) (GCC)
>
>
> The OS is centos6.5 x86_64
>
>
> Actually, another possible solution is, as gcc manual shows, use union.
> In function rte_raw_cksum() of lib/librte_net/rte_ip.h:
>
> static inline uint16_t
> rte_raw_cksum(const char *buf, size_t len){
> union {
> const char *ubuf;
> const uint16_t *uu16;
> } convert;
>
> convert.ubuf = buf;
> const uint16_t *u16 = convert.uu16;
> ...
> }
>
> This may be work, but not test yet.
>
> Any comments about this solution?
what happens if you make rte_raw_cksum take a void * (or const void *) parameter
instead of "const char *"?
/Bruce
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error
2014-12-03 14:51 ` Bruce Richardson
@ 2014-12-03 15:19 ` Qiu, Michael
2014-12-03 15:36 ` Bruce Richardson
0 siblings, 1 reply; 32+ messages in thread
From: Qiu, Michael @ 2014-12-03 15:19 UTC (permalink / raw)
To: Richardson, Bruce; +Cc: dev, Michael Qiu
On 2014/12/3 22:51, Richardson, Bruce wrote:
> On Wed, Dec 03, 2014 at 01:59:58PM +0000, Qiu, Michael wrote:
>> On 2014/12/3 19:43, Richardson, Bruce wrote:
>>> On Wed, Dec 03, 2014 at 07:28:19PM +0800, Michael Qiu wrote:
>>>> app/test-pmd/csumonly.c: In function ‘get_psd_sum’:
>>>> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’
>>>> does break strict-aliasing rules
>>>> build/include/rte_ip.h:157: note: initialized from here
>>>> ...
>>>>
>>>> The root cause is that, compile enable strict aliasing by default,
>>>> while in function rte_raw_cksum() try to convert 'const char *'
>>>> to 'const uint16_t *'.
>>>>
>>> What compiler version is this with? Is there any other way to fix this
>>> other than disabling the compiler warnings. Turning off strict aliasing may
>>> affect performance as it reduces the number of optimizations that the compiler
>>> can perform.
>> The compile version is:
>>
>> $ gcc -v
>> Using built-in specs.
>> Target: x86_64-redhat-linux
>> ...
>> gcc version 4.4.7 20120313 (Red Hat 4.4.7-4) (GCC)
>>
>>
>> The OS is centos6.5 x86_64
>>
>>
>> Actually, another possible solution is, as gcc manual shows, use union.
>> In function rte_raw_cksum() of lib/librte_net/rte_ip.h:
>>
>> static inline uint16_t
>> rte_raw_cksum(const char *buf, size_t len){
>> union {
>> const char *ubuf;
>> const uint16_t *uu16;
>> } convert;
>>
>> convert.ubuf = buf;
>> const uint16_t *u16 = convert.uu16;
>> ...
>> }
>>
>> This may be work, but not test yet.
>>
>> Any comments about this solution?
> what happens if you make rte_raw_cksum take a void * (or const void *) parameter
> instead of "const char *"?
"size_t len" is for type char, it is the the array size(for char array
is byte numbers), if we use void *, the meaning maybe confuse I think.
But it should work with other code change.
Thanks,
Michael
>
> /Bruce
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error
2014-12-03 11:42 ` [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error Bruce Richardson
2014-12-03 13:59 ` Qiu, Michael
@ 2014-12-03 15:24 ` Olivier MATZ
2014-12-03 16:03 ` Dayu Qiu
2014-12-03 15:57 ` Dayu Qiu
2 siblings, 1 reply; 32+ messages in thread
From: Olivier MATZ @ 2014-12-03 15:24 UTC (permalink / raw)
To: Bruce Richardson, Michael Qiu; +Cc: dev
Hi Bruce,
On 12/03/2014 12:42 PM, Bruce Richardson wrote:
> On Wed, Dec 03, 2014 at 07:28:19PM +0800, Michael Qiu wrote:
>> app/test-pmd/csumonly.c: In function ‘get_psd_sum’:
>> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’
>> does break strict-aliasing rules
>> build/include/rte_ip.h:157: note: initialized from here
>> ...
>>
>> The root cause is that, compile enable strict aliasing by default,
>> while in function rte_raw_cksum() try to convert 'const char *'
>> to 'const uint16_t *'.
>>
>
> What compiler version is this with? Is there any other way to fix this
> other than disabling the compiler warnings. Turning off strict aliasing may
> affect performance as it reduces the number of optimizations that the compiler
> can perform.
I can reproduce the issue with a gcc-4.4.6 toolchain. But I think it's
a toolchain bug as the warning does not occur with other versions I've
tested.
If it's the case, we could either:
- do nothing: in this case the user need to upgrade its toolchain, or
pass the -Wno-strict-aliasing manually in EXTRA_CFLAGS
- add the -Wno-strict-aliasing only for gcc 4.4 in the Makefile
What do you think?
Regards,
Olivier
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error
2014-12-03 15:19 ` Qiu, Michael
@ 2014-12-03 15:36 ` Bruce Richardson
2014-12-04 2:38 ` Qiu, Michael
0 siblings, 1 reply; 32+ messages in thread
From: Bruce Richardson @ 2014-12-03 15:36 UTC (permalink / raw)
To: Qiu, Michael; +Cc: dev, Michael Qiu
On Wed, Dec 03, 2014 at 03:19:34PM +0000, Qiu, Michael wrote:
> On 2014/12/3 22:51, Richardson, Bruce wrote:
> > On Wed, Dec 03, 2014 at 01:59:58PM +0000, Qiu, Michael wrote:
> >> On 2014/12/3 19:43, Richardson, Bruce wrote:
> >>> On Wed, Dec 03, 2014 at 07:28:19PM +0800, Michael Qiu wrote:
> >>>> app/test-pmd/csumonly.c: In function ‘get_psd_sum’:
> >>>> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’
> >>>> does break strict-aliasing rules
> >>>> build/include/rte_ip.h:157: note: initialized from here
> >>>> ...
> >>>>
> >>>> The root cause is that, compile enable strict aliasing by default,
> >>>> while in function rte_raw_cksum() try to convert 'const char *'
> >>>> to 'const uint16_t *'.
> >>>>
> >>> What compiler version is this with? Is there any other way to fix this
> >>> other than disabling the compiler warnings. Turning off strict aliasing may
> >>> affect performance as it reduces the number of optimizations that the compiler
> >>> can perform.
> >> The compile version is:
> >>
> >> $ gcc -v
> >> Using built-in specs.
> >> Target: x86_64-redhat-linux
> >> ...
> >> gcc version 4.4.7 20120313 (Red Hat 4.4.7-4) (GCC)
> >>
> >>
> >> The OS is centos6.5 x86_64
> >>
> >>
> >> Actually, another possible solution is, as gcc manual shows, use union.
> >> In function rte_raw_cksum() of lib/librte_net/rte_ip.h:
> >>
> >> static inline uint16_t
> >> rte_raw_cksum(const char *buf, size_t len){
> >> union {
> >> const char *ubuf;
> >> const uint16_t *uu16;
> >> } convert;
> >>
> >> convert.ubuf = buf;
> >> const uint16_t *u16 = convert.uu16;
> >> ...
> >> }
> >>
> >> This may be work, but not test yet.
> >>
> >> Any comments about this solution?
> > what happens if you make rte_raw_cksum take a void * (or const void *) parameter
> > instead of "const char *"?
>
> "size_t len" is for type char, it is the the array size(for char array
> is byte numbers), if we use void *, the meaning maybe confuse I think.
That shouldn't be a big issue. We can rename it to "size" instead of "len" if
needed.
> But it should work with other code change.
>
> Thanks,
> Michael
> >
> > /Bruce
> >
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error
2014-12-03 11:42 ` [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error Bruce Richardson
2014-12-03 13:59 ` Qiu, Michael
2014-12-03 15:24 ` [dpdk-dev] [PATCH] " Olivier MATZ
@ 2014-12-03 15:57 ` Dayu Qiu
2 siblings, 0 replies; 32+ messages in thread
From: Dayu Qiu @ 2014-12-03 15:57 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
Just re-post this mail as Thomas said it missed in mail list.
On Wed, Dec 3, 2014 at 7:42 PM, Bruce Richardson <bruce.richardson@intel.com
> wrote:
> On Wed, Dec 03, 2014 at 07:28:19PM +0800, Michael Qiu wrote:
> > app/test-pmd/csumonly.c: In function ‘get_psd_sum’:
> > build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’
> > does break strict-aliasing rules
> > build/include/rte_ip.h:157: note: initialized from here
> > ...
> >
> > The root cause is that, compile enable strict aliasing by default,
> > while in function rte_raw_cksum() try to convert 'const char *'
> > to 'const uint16_t *'.
> >
>
> What compiler version is this with? Is there any other way to fix this
> other than disabling the compiler warnings. Turning off strict aliasing may
> affect performance as it reduces the number of optimizations that the
> compiler
> can perform.
>
>
The compile version is:
$ gcc -v
Using built-in specs.
Target: x86_64-redhat-linux
...
gcc version 4.4.7 20120313 (Red Hat 4.4.7-4) (GCC)
The OS is centos6.5 x86_64
Actually, another possible solution is, as gcc manual shows, use union.
In function rte_raw_cksum() of lib/librte_net/rte_ip.h:
static inline uint16_t
rte_raw_cksum(const char *buf, size_t len){
union {
const char *ubuf;
const uint16_t *uu16;
} convert;
convert.ubuf = buf;
const uint16_t *u16 = convert.uu16;
...
}
This may be work, but not test yet.
Thanks,
Michael
> /Bruce
>
> > Need to add CFLAG '-Wno-strict-aliasing' to avoid this issue.
> >
> > Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> > ---
> > app/test-pmd/Makefile | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/app/test-pmd/Makefile b/app/test-pmd/Makefile
> > index 97dc2e6..995c874 100644
> > --- a/app/test-pmd/Makefile
> > +++ b/app/test-pmd/Makefile
> > @@ -38,7 +38,7 @@ ifeq ($(CONFIG_RTE_TEST_PMD),y)
> > #
> > APP = testpmd
> >
> > -CFLAGS += -O3
> > +CFLAGS += -O3 -Wno-strict-aliasing
> > CFLAGS += $(WERROR_FLAGS)
> >
> > ifeq ($(CONFIG_RTE_LIBRTE_PMD_PCAP),y)
> > --
> > 1.9.3
> >
>
--
Thanks & Best Regards
Mike
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error
2014-12-03 15:24 ` [dpdk-dev] [PATCH] " Olivier MATZ
@ 2014-12-03 16:03 ` Dayu Qiu
0 siblings, 0 replies; 32+ messages in thread
From: Dayu Qiu @ 2014-12-03 16:03 UTC (permalink / raw)
To: Olivier MATZ; +Cc: dev
Hi Olivier,
You can check gcc manual.
-fstrict-aliasing
Allow the compiler to assume the strictest aliasing rules
applicable to the language being
compiled. For C (and C++), this activates optimizations based
on the type of expressions. In
particular, an object of one type is assumed never to reside at
the same address as an object of a
different type, unless the types are almost the same. For
example, an "unsigned int" can alias an
"int", but not a "void*" or a "double". A character type may
alias any other type.
So it should not be a bug, but I have no idea about why other version does
not reproduce.
Thanks,
Michael
On Wed, Dec 3, 2014 at 11:24 PM, Olivier MATZ <olivier.matz@6wind.com>
wrote:
> Hi Bruce,
>
> On 12/03/2014 12:42 PM, Bruce Richardson wrote:
>
>> On Wed, Dec 03, 2014 at 07:28:19PM +0800, Michael Qiu wrote:
>>
>>> app/test-pmd/csumonly.c: In function ‘get_psd_sum’:
>>> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’
>>> does break strict-aliasing rules
>>> build/include/rte_ip.h:157: note: initialized from here
>>> ...
>>>
>>> The root cause is that, compile enable strict aliasing by default,
>>> while in function rte_raw_cksum() try to convert 'const char *'
>>> to 'const uint16_t *'.
>>>
>>>
>> What compiler version is this with? Is there any other way to fix this
>> other than disabling the compiler warnings. Turning off strict aliasing
>> may
>> affect performance as it reduces the number of optimizations that the
>> compiler
>> can perform.
>>
>
> I can reproduce the issue with a gcc-4.4.6 toolchain. But I think it's
> a toolchain bug as the warning does not occur with other versions I've
> tested.
>
> If it's the case, we could either:
>
> - do nothing: in this case the user need to upgrade its toolchain, or
> pass the -Wno-strict-aliasing manually in EXTRA_CFLAGS
>
> - add the -Wno-strict-aliasing only for gcc 4.4 in the Makefile
>
> What do you think?
>
> Regards,
> Olivier
>
>
--
Thanks & Best Regards
Mike
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix "__BYTE_ORDER__" not defined error
[not found] <1417606044-3432-1-git-send-email-michael.qiu@intel.com>
[not found] ` <1417606099-3489-1-git-send-email-michael.qiu@intel.com>
@ 2014-12-03 16:26 ` Qiu, Michael
2014-12-03 19:59 ` Thomas Monjalon
1 sibling, 1 reply; 32+ messages in thread
From: Qiu, Michael @ 2014-12-03 16:26 UTC (permalink / raw)
To: Michael Qiu, dev
Hi all,
What about this patch?
It may be some network or mail client issue of me, so not sure this
patch posted to mail list successful.
If failed, I will re-post it later.
Thanks,
Michael
On 2014/12/3 19:28, Michael Qiu wrote:
> app/test-pmd/csumonly.c:84:5: error: "__BYTE_ORDER__" is not defined
> app/test-pmd/csumonly.c:84:23: error: "__ORDER_LITTLE_ENDIAN__" is not defined
>
> This because old gcc version, like 4.4.7, does not has these buildin macros.
>
> $ gcc -v
> Using built-in specs.
> Target: x86_64-redhat-linux
> ...
> gcc version 4.4.7 20120313 (Red Hat 4.4.7-4) (GCC)
>
> $ echo | gcc -dM -E -| grep "LITTLE"
> (none)
>
> In this situation, we should back to use the macros defined in
> <endian.h>
>
> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> ---
> app/test-pmd/csumonly.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index 6f43761..3fa81a2 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -81,7 +81,13 @@
>
> /* we cannot use htons() from arpa/inet.h due to name conflicts, and we
> * cannot use rte_cpu_to_be_16() on a constant in a switch/case */
> -#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +#ifdef __BYTE_ORDER__
> +#define LITTLE_ENDIAN_CHECK (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
> +#else
> +#define LITTLE_ENDIAN_CHECK (__BYTE_ORDER == __LITTLE_ENDIAN)
> +#endif
> +
> +#if LITTLE_ENDIAN_CHECK
> #define _htons(x) ((uint16_t)((((x) & 0x00ffU) << 8) | (((x) & 0xff00U) >> 8)))
> #else
> #define _htons(x) (x)
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix "__BYTE_ORDER__" not defined error
2014-12-03 16:26 ` [dpdk-dev] [PATCH] test-pmd: Fix "__BYTE_ORDER__" not defined error Qiu, Michael
@ 2014-12-03 19:59 ` Thomas Monjalon
2014-12-03 20:47 ` [dpdk-dev] [PATCH 0/2] fix endianness in EAL Thomas Monjalon
0 siblings, 1 reply; 32+ messages in thread
From: Thomas Monjalon @ 2014-12-03 19:59 UTC (permalink / raw)
To: Qiu, Michael; +Cc: dev
2014-12-03 16:26, Qiu, Michael:
> > --- a/app/test-pmd/csumonly.c
> > +++ b/app/test-pmd/csumonly.c
> > @@ -81,7 +81,13 @@
> >
> > /* we cannot use htons() from arpa/inet.h due to name conflicts, and we
> > * cannot use rte_cpu_to_be_16() on a constant in a switch/case */
> > -#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> > +#ifdef __BYTE_ORDER__
> > +#define LITTLE_ENDIAN_CHECK (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
> > +#else
> > +#define LITTLE_ENDIAN_CHECK (__BYTE_ORDER == __LITTLE_ENDIAN)
> > +#endif
> > +
> > +#if LITTLE_ENDIAN_CHECK
> > #define _htons(x) ((uint16_t)((((x) & 0x00ffU) << 8) | (((x) & 0xff00U) >> 8)))
> > #else
> > #define _htons(x) (x)
I don't agree we should fix it in testpmd.
It's a more general problem which must be solved in EAL.
I'm preparing a patchset to fix it.
--
Thomas
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH 0/2] fix endianness in EAL
2014-12-03 19:59 ` Thomas Monjalon
@ 2014-12-03 20:47 ` Thomas Monjalon
2014-12-03 20:47 ` [dpdk-dev] [PATCH 1/2] eal: detect endianness Thomas Monjalon
` (2 more replies)
0 siblings, 3 replies; 32+ messages in thread
From: Thomas Monjalon @ 2014-12-03 20:47 UTC (permalink / raw)
To: dev
It can be hard to implement a reliable detection of endianness.
Previous trials in testpmd failed.
The IBM Power patchset introduced a config option.
This patchset try to improve the situation by having a detection
in EAL headers.
Please test it (especially on IBM Power) with different toolchains
or distributions.
Thomas Monjalon (2):
eal: detect endianness
app/testpmd: fix endianness detection
--
2.1.3
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH 1/2] eal: detect endianness
2014-12-03 20:47 ` [dpdk-dev] [PATCH 0/2] fix endianness in EAL Thomas Monjalon
@ 2014-12-03 20:47 ` Thomas Monjalon
2014-12-04 2:28 ` Qiu, Michael
2014-12-03 20:47 ` [dpdk-dev] [PATCH 2/2] app/testpmd: fix endianness detection Thomas Monjalon
2014-12-04 9:28 ` [dpdk-dev] [PATCH 0/2] fix endianness in EAL Chao Zhu
2 siblings, 1 reply; 32+ messages in thread
From: Thomas Monjalon @ 2014-12-03 20:47 UTC (permalink / raw)
To: dev
There is no standard to check endianness.
So we need to try different checks.
Previous trials were done in testpmd (see commits
51f694dd40f56 and 64741f237cf29) without full success.
This one is not guaranteed to work everywhere so it could
evolve when exceptions are found.
If endianness is not detected, there is a fallback on x86
to little endian. It could be forced before doing detection
but it would add some arch-dependent code in the generic header.
The option CONFIG_RTE_ARCH_BIG_ENDIAN introduced for IBM Power only
(commit a982ec81d84d53) can be removed. A compile-time check is better.
Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
config/defconfig_ppc_64-power8-linuxapp-gcc | 1 -
.../common/include/arch/ppc_64/rte_byteorder.h | 4 ++--
.../common/include/arch/x86/rte_byteorder.h | 4 ++++
.../common/include/generic/rte_byteorder.h | 28 ++++++++++++++++++++++
4 files changed, 34 insertions(+), 3 deletions(-)
diff --git a/config/defconfig_ppc_64-power8-linuxapp-gcc b/config/defconfig_ppc_64-power8-linuxapp-gcc
index 48018c3..d97a885 100644
--- a/config/defconfig_ppc_64-power8-linuxapp-gcc
+++ b/config/defconfig_ppc_64-power8-linuxapp-gcc
@@ -34,7 +34,6 @@ CONFIG_RTE_MACHINE="power8"
CONFIG_RTE_ARCH="ppc_64"
CONFIG_RTE_ARCH_PPC_64=y
-CONFIG_RTE_ARCH_BIG_ENDIAN=y
CONFIG_RTE_ARCH_64=y
CONFIG_RTE_TOOLCHAIN="gcc"
diff --git a/lib/librte_eal/common/include/arch/ppc_64/rte_byteorder.h b/lib/librte_eal/common/include/arch/ppc_64/rte_byteorder.h
index 1a89051..80436f2 100644
--- a/lib/librte_eal/common/include/arch/ppc_64/rte_byteorder.h
+++ b/lib/librte_eal/common/include/arch/ppc_64/rte_byteorder.h
@@ -105,7 +105,7 @@ static inline uint64_t rte_arch_bswap64(uint64_t _x)
/* Power 8 have both little endian and big endian mode
* Power 7 only support big endian
*/
-#ifndef RTE_ARCH_BIG_ENDIAN
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
#define rte_cpu_to_le_16(x) (x)
#define rte_cpu_to_le_32(x) (x)
@@ -123,7 +123,7 @@ static inline uint64_t rte_arch_bswap64(uint64_t _x)
#define rte_be_to_cpu_32(x) rte_bswap32(x)
#define rte_be_to_cpu_64(x) rte_bswap64(x)
-#else
+#else /* RTE_BIG_ENDIAN */
#define rte_cpu_to_le_16(x) rte_bswap16(x)
#define rte_cpu_to_le_32(x) rte_bswap32(x)
diff --git a/lib/librte_eal/common/include/arch/x86/rte_byteorder.h b/lib/librte_eal/common/include/arch/x86/rte_byteorder.h
index 1aa6985..ffdb6ef 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_byteorder.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_byteorder.h
@@ -40,6 +40,10 @@ extern "C" {
#include "generic/rte_byteorder.h"
+#ifndef RTE_BYTE_ORDER
+#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
+#endif
+
/*
* An architecture-optimized byte swap for a 16-bit value.
*
diff --git a/lib/librte_eal/common/include/generic/rte_byteorder.h b/lib/librte_eal/common/include/generic/rte_byteorder.h
index 9358136..ea23fdf 100644
--- a/lib/librte_eal/common/include/generic/rte_byteorder.h
+++ b/lib/librte_eal/common/include/generic/rte_byteorder.h
@@ -44,6 +44,34 @@
*/
#include <stdint.h>
+#ifdef RTE_EXEC_ENV_BSDAPP
+#include <sys/endian.h>
+#else
+#include <endian.h>
+#endif
+
+/*
+ * Compile-time endianness detection
+ */
+#define RTE_BIG_ENDIAN 1
+#define RTE_LITTLE_ENDIAN 2
+#if defined __BYTE_ORDER
+#if __BYTE_ORDER == __BIG_ENDIAN
+#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
+#elif __BYTE_ORDER == __LITTLE_ENDIAN
+#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
+#endif /* __BYTE_ORDER */
+#elif defined __BYTE_ORDER__
+#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
+#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
+#endif /* __BYTE_ORDER__ */
+#elif defined __BIG_ENDIAN__
+#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
+#elif defined __LITTLE_ENDIAN__
+#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
+#endif
/*
* An internal function to swap bytes in a 16-bit value.
--
2.1.3
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH 2/2] app/testpmd: fix endianness detection
2014-12-03 20:47 ` [dpdk-dev] [PATCH 0/2] fix endianness in EAL Thomas Monjalon
2014-12-03 20:47 ` [dpdk-dev] [PATCH 1/2] eal: detect endianness Thomas Monjalon
@ 2014-12-03 20:47 ` Thomas Monjalon
2014-12-04 9:28 ` [dpdk-dev] [PATCH 0/2] fix endianness in EAL Chao Zhu
2 siblings, 0 replies; 32+ messages in thread
From: Thomas Monjalon @ 2014-12-03 20:47 UTC (permalink / raw)
To: dev
Use endianness detection factorized in EAL.
The comment about arpa/inet.h is not valid anymore since
commit d07180f211c08 ("net: fix conflict with libc").
The macro _htons could also be moved in rte_byteorder.h
by providing some constant byte swapping.
Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
app/test-pmd/config.c | 2 +-
app/test-pmd/csumonly.c | 5 ++---
app/test-pmd/testpmd.h | 2 +-
3 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index f0b770e..69a83c2 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -628,7 +628,7 @@ ring_dma_zone_lookup(const char *ring_name, uint8_t port_id, uint16_t q_id)
union igb_ring_dword {
uint64_t dword;
struct {
-#ifdef RTE_ARCH_BIG_ENDIAN
+#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
uint32_t lo;
uint32_t hi;
#else
diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 6f43761..da4bca4 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -79,9 +79,8 @@
#define IP_HDRLEN 0x05 /* default IP header length == five 32-bits words. */
#define IP_VHL_DEF (IP_VERSION | IP_HDRLEN)
-/* we cannot use htons() from arpa/inet.h due to name conflicts, and we
- * cannot use rte_cpu_to_be_16() on a constant in a switch/case */
-#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+/* We cannot use rte_cpu_to_be_16() on a constant in a switch/case */
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
#define _htons(x) ((uint16_t)((((x) & 0x00ffU) << 8) | (((x) & 0xff00U) >> 8)))
#else
#define _htons(x) (x)
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index c925910..f8b0740 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -567,7 +567,7 @@ int tx_queue_id_is_invalid(queueid_t txq_id);
#define RTE_BE_TO_CPU_16(be_16_v) rte_be_to_cpu_16((be_16_v))
#define RTE_CPU_TO_BE_16(cpu_16_v) rte_cpu_to_be_16((cpu_16_v))
#else
-#ifdef __big_endian__
+#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
#define RTE_BE_TO_CPU_16(be_16_v) (be_16_v)
#define RTE_CPU_TO_BE_16(cpu_16_v) (cpu_16_v)
#else
--
2.1.3
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH 1/2] eal: detect endianness
2014-12-03 20:47 ` [dpdk-dev] [PATCH 1/2] eal: detect endianness Thomas Monjalon
@ 2014-12-04 2:28 ` Qiu, Michael
2014-12-04 9:00 ` Thomas Monjalon
0 siblings, 1 reply; 32+ messages in thread
From: Qiu, Michael @ 2014-12-04 2:28 UTC (permalink / raw)
To: Thomas Monjalon, dev; +Cc: Chao Zhu
On 12/4/2014 5:26 AM, Thomas Monjalon wrote:
> There is no standard to check endianness.
> So we need to try different checks.
> Previous trials were done in testpmd (see commits
> 51f694dd40f56 and 64741f237cf29) without full success.
> This one is not guaranteed to work everywhere so it could
> evolve when exceptions are found.
>
> If endianness is not detected, there is a fallback on x86
> to little endian. It could be forced before doing detection
> but it would add some arch-dependent code in the generic header.
>
> The option CONFIG_RTE_ARCH_BIG_ENDIAN introduced for IBM Power only
> (commit a982ec81d84d53) can be removed. A compile-time check is better.
>
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> ---
> config/defconfig_ppc_64-power8-linuxapp-gcc | 1 -
> .../common/include/arch/ppc_64/rte_byteorder.h | 4 ++--
> .../common/include/arch/x86/rte_byteorder.h | 4 ++++
> .../common/include/generic/rte_byteorder.h | 28 ++++++++++++++++++++++
> 4 files changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/config/defconfig_ppc_64-power8-linuxapp-gcc b/config/defconfig_ppc_64-power8-linuxapp-gcc
> index 48018c3..d97a885 100644
> --- a/config/defconfig_ppc_64-power8-linuxapp-gcc
> +++ b/config/defconfig_ppc_64-power8-linuxapp-gcc
> @@ -34,7 +34,6 @@ CONFIG_RTE_MACHINE="power8"
>
> CONFIG_RTE_ARCH="ppc_64"
> CONFIG_RTE_ARCH_PPC_64=y
> -CONFIG_RTE_ARCH_BIG_ENDIAN=y
> CONFIG_RTE_ARCH_64=y
>
> CONFIG_RTE_TOOLCHAIN="gcc"
> diff --git a/lib/librte_eal/common/include/arch/ppc_64/rte_byteorder.h b/lib/librte_eal/common/include/arch/ppc_64/rte_byteorder.h
> index 1a89051..80436f2 100644
> --- a/lib/librte_eal/common/include/arch/ppc_64/rte_byteorder.h
> +++ b/lib/librte_eal/common/include/arch/ppc_64/rte_byteorder.h
> @@ -105,7 +105,7 @@ static inline uint64_t rte_arch_bswap64(uint64_t _x)
> /* Power 8 have both little endian and big endian mode
> * Power 7 only support big endian
> */
> -#ifndef RTE_ARCH_BIG_ENDIAN
> +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
>
> #define rte_cpu_to_le_16(x) (x)
> #define rte_cpu_to_le_32(x) (x)
> @@ -123,7 +123,7 @@ static inline uint64_t rte_arch_bswap64(uint64_t _x)
> #define rte_be_to_cpu_32(x) rte_bswap32(x)
> #define rte_be_to_cpu_64(x) rte_bswap64(x)
>
> -#else
> +#else /* RTE_BIG_ENDIAN */
>
> #define rte_cpu_to_le_16(x) rte_bswap16(x)
> #define rte_cpu_to_le_32(x) rte_bswap32(x)
> diff --git a/lib/librte_eal/common/include/arch/x86/rte_byteorder.h b/lib/librte_eal/common/include/arch/x86/rte_byteorder.h
> index 1aa6985..ffdb6ef 100644
> --- a/lib/librte_eal/common/include/arch/x86/rte_byteorder.h
> +++ b/lib/librte_eal/common/include/arch/x86/rte_byteorder.h
> @@ -40,6 +40,10 @@ extern "C" {
>
> #include "generic/rte_byteorder.h"
>
> +#ifndef RTE_BYTE_ORDER
> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
> +#endif
> +
> /*
> * An architecture-optimized byte swap for a 16-bit value.
> *
> diff --git a/lib/librte_eal/common/include/generic/rte_byteorder.h b/lib/librte_eal/common/include/generic/rte_byteorder.h
> index 9358136..ea23fdf 100644
> --- a/lib/librte_eal/common/include/generic/rte_byteorder.h
> +++ b/lib/librte_eal/common/include/generic/rte_byteorder.h
> @@ -44,6 +44,34 @@
> */
>
> #include <stdint.h>
> +#ifdef RTE_EXEC_ENV_BSDAPP
> +#include <sys/endian.h>
> +#else
> +#include <endian.h>
> +#endif
> +
> +/*
> + * Compile-time endianness detection
> + */
> +#define RTE_BIG_ENDIAN 1
> +#define RTE_LITTLE_ENDIAN 2
> +#if defined __BYTE_ORDER
> +#if __BYTE_ORDER == __BIG_ENDIAN
> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
> +#elif __BYTE_ORDER == __LITTLE_ENDIAN
> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
> +#endif /* __BYTE_ORDER */
> +#elif defined __BYTE_ORDER__
> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
> +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
> +#endif /* __BYTE_ORDER__ */
> +#elif defined __BIG_ENDIAN__
> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
> +#elif defined __LITTLE_ENDIAN__
> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
> +#endif
What do you think about :
+/*
+ * Compile-time endianness detection
+ */
+#define RTE_BIG_ENDIAN 1
+#define RTE_LITTLE_ENDIAN 2
+if defined __BYTE_ORDER__ /* Prefer gcc build-in macros */
+#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
+#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
+#endif /* __BYTE_ORDER__ */
+#else
+#if defined RTE_EXEC_ENV_BSDAPP
+#include <sys/endian.h>
+#else
+#include <endian.h>
+#endif
+#if defined __BYTE_ORDER
+#if __BYTE_ORDER == __BIG_ENDIAN
+#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
+#elif __BYTE_ORDER == __LITTLE_ENDIAN
+#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
+#endif /* __BYTE_ORDER */
+#elif defined __BIG_ENDIAN__
+#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
+#elif defined __LITTLE_ENDIAN__
+#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
+#endif
+#endif
>
> /*
> * An internal function to swap bytes in a 16-bit value.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error
2014-12-03 15:36 ` Bruce Richardson
@ 2014-12-04 2:38 ` Qiu, Michael
2014-12-04 3:28 ` [dpdk-dev] [PATCH v2] " Michael Qiu
0 siblings, 1 reply; 32+ messages in thread
From: Qiu, Michael @ 2014-12-04 2:38 UTC (permalink / raw)
To: Richardson, Bruce; +Cc: dev, Michael Qiu
On 12/3/2014 11:36 PM, Richardson, Bruce wrote:
> On Wed, Dec 03, 2014 at 03:19:34PM +0000, Qiu, Michael wrote:
>> On 2014/12/3 22:51, Richardson, Bruce wrote:
>>> On Wed, Dec 03, 2014 at 01:59:58PM +0000, Qiu, Michael wrote:
>>>> On 2014/12/3 19:43, Richardson, Bruce wrote:
>>>>> On Wed, Dec 03, 2014 at 07:28:19PM +0800, Michael Qiu wrote:
>>>>>> app/test-pmd/csumonly.c: In function ‘get_psd_sum’:
>>>>>> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’
>>>>>> does break strict-aliasing rules
>>>>>> build/include/rte_ip.h:157: note: initialized from here
>>>>>> ...
>>>>>>
>>>>>> The root cause is that, compile enable strict aliasing by default,
>>>>>> while in function rte_raw_cksum() try to convert 'const char *'
>>>>>> to 'const uint16_t *'.
>>>>>>
>>>>> What compiler version is this with? Is there any other way to fix this
>>>>> other than disabling the compiler warnings. Turning off strict aliasing may
>>>>> affect performance as it reduces the number of optimizations that the compiler
>>>>> can perform.
>>>> The compile version is:
>>>>
>>>> $ gcc -v
>>>> Using built-in specs.
>>>> Target: x86_64-redhat-linux
>>>> ...
>>>> gcc version 4.4.7 20120313 (Red Hat 4.4.7-4) (GCC)
>>>>
>>>>
>>>> The OS is centos6.5 x86_64
>>>>
>>>>
>>>> Actually, another possible solution is, as gcc manual shows, use union.
>>>> In function rte_raw_cksum() of lib/librte_net/rte_ip.h:
>>>>
>>>> static inline uint16_t
>>>> rte_raw_cksum(const char *buf, size_t len){
>>>> union {
>>>> const char *ubuf;
>>>> const uint16_t *uu16;
>>>> } convert;
>>>>
>>>> convert.ubuf = buf;
>>>> const uint16_t *u16 = convert.uu16;
>>>> ...
>>>> }
>>>>
>>>> This may be work, but not test yet.
>>>>
>>>> Any comments about this solution?
>>> what happens if you make rte_raw_cksum take a void * (or const void *) parameter
>>> instead of "const char *"?
>> "size_t len" is for type char, it is the the array size(for char array
>> is byte numbers), if we use void *, the meaning maybe confuse I think.
> That shouldn't be a big issue. We can rename it to "size" instead of "len" if
> needed.
As I tested in my env just, void * is still not work, the same errors
post when compile.
Then the solution for DPDK should be:
- As Olivier suggest, add the -Wno-strict-aliasing only for gcc 4.4 in
the Makefile
- As gcc manual suggest, use "union" to work around this issue.
Thanks,
Michael
>> But it should work with other code change.
>>
>> Thanks,
>> Michael
>>> /Bruce
>>>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v2] test-pmd: Fix pointer aliasing error
2014-12-04 2:38 ` Qiu, Michael
@ 2014-12-04 3:28 ` Michael Qiu
2014-12-04 4:16 ` [dpdk-dev] [PATCH v3] " Michael Qiu
2014-12-04 12:54 ` [dpdk-dev] [PATCH v2] " Ananyev, Konstantin
0 siblings, 2 replies; 32+ messages in thread
From: Michael Qiu @ 2014-12-04 3:28 UTC (permalink / raw)
To: dev
app/test-pmd/csumonly.c: In function ‘get_psd_sum’:
build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’
does break strict-aliasing rules
build/include/rte_ip.h:157: note: initialized from here
...
The root cause is that, compile enable strict aliasing by default,
while in function rte_raw_cksum() try to convert 'const char *'
to 'const uint16_t *'.
This patch is one workaround fix.
Signed-off-by: Michael Qiu <michael.qiu@intel.com>
---
lib/librte_net/rte_ip.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
index 61e4457..f1c7087 100644
--- a/lib/librte_net/rte_ip.h
+++ b/lib/librte_net/rte_ip.h
@@ -154,7 +154,8 @@ struct ipv4_hdr {
static inline uint16_t
rte_raw_cksum(const char *buf, size_t len)
{
- const uint16_t *u16 = (const uint16_t *)buf;
+ unsigned long ptr = (unsigned long)buf;
+ const uint16_t *u16 = (const uint16_t *)ptr;
uint32_t sum = 0;
while (len >= (sizeof(*u16) * 4)) {
--
1.9.3
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v3] test-pmd: Fix pointer aliasing error
2014-12-04 3:28 ` [dpdk-dev] [PATCH v2] " Michael Qiu
@ 2014-12-04 4:16 ` Michael Qiu
2014-12-05 5:34 ` Qiu, Michael
2014-12-08 1:30 ` Qiu, Michael
2014-12-04 12:54 ` [dpdk-dev] [PATCH v2] " Ananyev, Konstantin
1 sibling, 2 replies; 32+ messages in thread
From: Michael Qiu @ 2014-12-04 4:16 UTC (permalink / raw)
To: dev
app/test-pmd/csumonly.c: In function ‘get_psd_sum’:
build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’
does break strict-aliasing rules
build/include/rte_ip.h:157: note: initialized from here
...
The root cause is that, compile enable strict aliasing by default,
while in function rte_raw_cksum() try to convert 'const char *'
to 'const uint16_t *'.
This patch is one workaround fix.
Signed-off-by: Michael Qiu <michael.qiu@intel.com>
---
v3 --> v2:
use uintptr_t instead of unsigned long to
save pointer.
v2 --> v1:
Workaround solution instead of shut off the
gcc params.
lib/librte_net/rte_ip.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
index 61e4457..cda3436 100644
--- a/lib/librte_net/rte_ip.h
+++ b/lib/librte_net/rte_ip.h
@@ -154,7 +154,8 @@ struct ipv4_hdr {
static inline uint16_t
rte_raw_cksum(const char *buf, size_t len)
{
- const uint16_t *u16 = (const uint16_t *)buf;
+ uintptr_t ptr = (uintptr_t)buf;
+ const uint16_t *u16 = (const uint16_t *)ptr;
uint32_t sum = 0;
while (len >= (sizeof(*u16) * 4)) {
--
1.9.3
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH 1/2] eal: detect endianness
2014-12-04 2:28 ` Qiu, Michael
@ 2014-12-04 9:00 ` Thomas Monjalon
2014-12-04 10:28 ` Qiu, Michael
0 siblings, 1 reply; 32+ messages in thread
From: Thomas Monjalon @ 2014-12-04 9:00 UTC (permalink / raw)
To: Qiu, Michael; +Cc: dev, Chao Zhu
2014-12-04 02:28, Qiu, Michael:
> On 12/4/2014 5:26 AM, Thomas Monjalon wrote:
> > There is no standard to check endianness.
> > So we need to try different checks.
> > Previous trials were done in testpmd (see commits
> > 51f694dd40f56 and 64741f237cf29) without full success.
> > This one is not guaranteed to work everywhere so it could
> > evolve when exceptions are found.
[...]
> > #include <stdint.h>
> > +#ifdef RTE_EXEC_ENV_BSDAPP
> > +#include <sys/endian.h>
> > +#else
> > +#include <endian.h>
> > +#endif
> > +
> > +/*
> > + * Compile-time endianness detection
> > + */
> > +#define RTE_BIG_ENDIAN 1
> > +#define RTE_LITTLE_ENDIAN 2
> > +#if defined __BYTE_ORDER
> > +#if __BYTE_ORDER == __BIG_ENDIAN
> > +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
> > +#elif __BYTE_ORDER == __LITTLE_ENDIAN
> > +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
> > +#endif /* __BYTE_ORDER */
> > +#elif defined __BYTE_ORDER__
> > +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> > +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
> > +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> > +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
> > +#endif /* __BYTE_ORDER__ */
> > +#elif defined __BIG_ENDIAN__
> > +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
> > +#elif defined __LITTLE_ENDIAN__
> > +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
> > +#endif
>
> What do you think about :
>
> +/*
> + * Compile-time endianness detection
> + */
> +#define RTE_BIG_ENDIAN 1
> +#define RTE_LITTLE_ENDIAN 2
> +if defined __BYTE_ORDER__ /* Prefer gcc build-in macros */
> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
> +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
> +#endif /* __BYTE_ORDER__ */
> +#else
> +#if defined RTE_EXEC_ENV_BSDAPP
> +#include <sys/endian.h>
> +#else
> +#include <endian.h>
> +#endif
> +#if defined __BYTE_ORDER
> +#if __BYTE_ORDER == __BIG_ENDIAN
> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
> +#elif __BYTE_ORDER == __LITTLE_ENDIAN
> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
> +#endif /* __BYTE_ORDER */
> +#elif defined __BIG_ENDIAN__
> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
> +#elif defined __LITTLE_ENDIAN__
> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
> +#endif
> +#endif
Please, could you give more explanations about your proposal?
Why not always try to include endian.h?
Why giving high priority to __BYTE_ORDER__?
--
Thomas
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH 0/2] fix endianness in EAL
2014-12-03 20:47 ` [dpdk-dev] [PATCH 0/2] fix endianness in EAL Thomas Monjalon
2014-12-03 20:47 ` [dpdk-dev] [PATCH 1/2] eal: detect endianness Thomas Monjalon
2014-12-03 20:47 ` [dpdk-dev] [PATCH 2/2] app/testpmd: fix endianness detection Thomas Monjalon
@ 2014-12-04 9:28 ` Chao Zhu
2014-12-05 16:01 ` Thomas Monjalon
2 siblings, 1 reply; 32+ messages in thread
From: Chao Zhu @ 2014-12-04 9:28 UTC (permalink / raw)
To: Thomas Monjalon, dev
On 2014/12/4 4:47, Thomas Monjalon wrote:
> It can be hard to implement a reliable detection of endianness.
> Previous trials in testpmd failed.
> The IBM Power patchset introduced a config option.
>
> This patchset try to improve the situation by having a detection
> in EAL headers.
>
> Please test it (especially on IBM Power) with different toolchains
> or distributions.
>
> Thomas Monjalon (2):
> eal: detect endianness
> app/testpmd: fix endianness detection
>
I tested it on IBM Power with GCC 4.8. It works fine.
If there is no other better way to do the detection, I think this patch
is good enough.
Acked-by: Chao Zhu<chaozhu@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH 1/2] eal: detect endianness
2014-12-04 9:00 ` Thomas Monjalon
@ 2014-12-04 10:28 ` Qiu, Michael
2014-12-04 12:19 ` Thomas Monjalon
0 siblings, 1 reply; 32+ messages in thread
From: Qiu, Michael @ 2014-12-04 10:28 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, Chao Zhu
On 12/4/2014 5:01 PM, Thomas Monjalon wrote:
> 2014-12-04 02:28, Qiu, Michael:
>> On 12/4/2014 5:26 AM, Thomas Monjalon wrote:
>>> There is no standard to check endianness.
>>> So we need to try different checks.
>>> Previous trials were done in testpmd (see commits
>>> 51f694dd40f56 and 64741f237cf29) without full success.
>>> This one is not guaranteed to work everywhere so it could
>>> evolve when exceptions are found.
> [...]
>>> #include <stdint.h>
>>> +#ifdef RTE_EXEC_ENV_BSDAPP
>>> +#include <sys/endian.h>
>>> +#else
>>> +#include <endian.h>
>>> +#endif
>>> +
>>> +/*
>>> + * Compile-time endianness detection
>>> + */
>>> +#define RTE_BIG_ENDIAN 1
>>> +#define RTE_LITTLE_ENDIAN 2
>>> +#if defined __BYTE_ORDER
>>> +#if __BYTE_ORDER == __BIG_ENDIAN
>>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
>>> +#elif __BYTE_ORDER == __LITTLE_ENDIAN
>>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
>>> +#endif /* __BYTE_ORDER */
>>> +#elif defined __BYTE_ORDER__
>>> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
>>> +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
>>> +#endif /* __BYTE_ORDER__ */
>>> +#elif defined __BIG_ENDIAN__
>>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
>>> +#elif defined __LITTLE_ENDIAN__
>>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
>>> +#endif
>> What do you think about :
>>
>> +/*
>> + * Compile-time endianness detection
>> + */
>> +#define RTE_BIG_ENDIAN 1
>> +#define RTE_LITTLE_ENDIAN 2
>> +if defined __BYTE_ORDER__ /* Prefer gcc build-in macros */
>> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
>> +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
>> +#endif /* __BYTE_ORDER__ */
>> +#else
>> +#if defined RTE_EXEC_ENV_BSDAPP
>> +#include <sys/endian.h>
>> +#else
>> +#include <endian.h>
>> +#endif
>> +#if defined __BYTE_ORDER
>> +#if __BYTE_ORDER == __BIG_ENDIAN
>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
>> +#elif __BYTE_ORDER == __LITTLE_ENDIAN
>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
>> +#endif /* __BYTE_ORDER */
>> +#elif defined __BIG_ENDIAN__
>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
>> +#elif defined __LITTLE_ENDIAN__
>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
>> +#endif
>> +#endif
> Please, could you give more explanations about your proposal?
> Why not always try to include endian.h?
I assume that if gcc can handler why we need include that file?
Also it seems that only old version could have this issue, newer
versions has build in this marcos.
So that's why I prefer "__BYTE_ORDER__" for high priority.
Thanks,
Michael
> Why giving high priority to __BYTE_ORDER__?
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH 1/2] eal: detect endianness
2014-12-04 10:28 ` Qiu, Michael
@ 2014-12-04 12:19 ` Thomas Monjalon
2014-12-04 12:50 ` Qiu, Michael
0 siblings, 1 reply; 32+ messages in thread
From: Thomas Monjalon @ 2014-12-04 12:19 UTC (permalink / raw)
To: Qiu, Michael; +Cc: dev, Chao Zhu
2014-12-04 10:28, Qiu, Michael:
> On 12/4/2014 5:01 PM, Thomas Monjalon wrote:
> > 2014-12-04 02:28, Qiu, Michael:
> >> On 12/4/2014 5:26 AM, Thomas Monjalon wrote:
> >>> There is no standard to check endianness.
> >>> So we need to try different checks.
> >>> Previous trials were done in testpmd (see commits
> >>> 51f694dd40f56 and 64741f237cf29) without full success.
> >>> This one is not guaranteed to work everywhere so it could
> >>> evolve when exceptions are found.
> > [...]
> >>> #include <stdint.h>
> >>> +#ifdef RTE_EXEC_ENV_BSDAPP
> >>> +#include <sys/endian.h>
> >>> +#else
> >>> +#include <endian.h>
> >>> +#endif
> >>> +
> >>> +/*
> >>> + * Compile-time endianness detection
> >>> + */
> >>> +#define RTE_BIG_ENDIAN 1
> >>> +#define RTE_LITTLE_ENDIAN 2
> >>> +#if defined __BYTE_ORDER
> >>> +#if __BYTE_ORDER == __BIG_ENDIAN
> >>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
> >>> +#elif __BYTE_ORDER == __LITTLE_ENDIAN
> >>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
> >>> +#endif /* __BYTE_ORDER */
> >>> +#elif defined __BYTE_ORDER__
> >>> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> >>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
> >>> +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> >>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
> >>> +#endif /* __BYTE_ORDER__ */
> >>> +#elif defined __BIG_ENDIAN__
> >>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
> >>> +#elif defined __LITTLE_ENDIAN__
> >>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
> >>> +#endif
> >> What do you think about :
> >>
> >> +/*
> >> + * Compile-time endianness detection
> >> + */
> >> +#define RTE_BIG_ENDIAN 1
> >> +#define RTE_LITTLE_ENDIAN 2
> >> +if defined __BYTE_ORDER__ /* Prefer gcc build-in macros */
> >> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> >> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
> >> +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> >> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
> >> +#endif /* __BYTE_ORDER__ */
> >> +#else
> >> +#if defined RTE_EXEC_ENV_BSDAPP
> >> +#include <sys/endian.h>
> >> +#else
> >> +#include <endian.h>
> >> +#endif
> >> +#if defined __BYTE_ORDER
> >> +#if __BYTE_ORDER == __BIG_ENDIAN
> >> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
> >> +#elif __BYTE_ORDER == __LITTLE_ENDIAN
> >> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
> >> +#endif /* __BYTE_ORDER */
> >> +#elif defined __BIG_ENDIAN__
> >> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
> >> +#elif defined __LITTLE_ENDIAN__
> >> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
> >> +#endif
> >> +#endif
> >
> > Please, could you give more explanations about your proposal?
> > Why not always try to include endian.h?
>
> I assume that if gcc can handler why we need include that file?
Separating include on top is easier to read, and I'm not sure it won't
be needed for __BYTE_ORDER__ with some toolchains.
> Also it seems that only old version could have this issue, newer
> versions has build in this marcos.
>
> So that's why I prefer "__BYTE_ORDER__" for high priority.
I have no problem with reversing this priority.
> > Why giving high priority to __BYTE_ORDER__?
Any other comment? May I apply with above change?
--
Thomas
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH 1/2] eal: detect endianness
2014-12-04 12:19 ` Thomas Monjalon
@ 2014-12-04 12:50 ` Qiu, Michael
0 siblings, 0 replies; 32+ messages in thread
From: Qiu, Michael @ 2014-12-04 12:50 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, Chao Zhu
On 12/4/2014 8:20 PM, Thomas Monjalon wrote:
> 2014-12-04 10:28, Qiu, Michael:
>> On 12/4/2014 5:01 PM, Thomas Monjalon wrote:
>>> 2014-12-04 02:28, Qiu, Michael:
>>>> On 12/4/2014 5:26 AM, Thomas Monjalon wrote:
>>>>> There is no standard to check endianness.
>>>>> So we need to try different checks.
>>>>> Previous trials were done in testpmd (see commits
>>>>> 51f694dd40f56 and 64741f237cf29) without full success.
>>>>> This one is not guaranteed to work everywhere so it could
>>>>> evolve when exceptions are found.
>>> [...]
>>>>> #include <stdint.h>
>>>>> +#ifdef RTE_EXEC_ENV_BSDAPP
>>>>> +#include <sys/endian.h>
>>>>> +#else
>>>>> +#include <endian.h>
>>>>> +#endif
>>>>> +
>>>>> +/*
>>>>> + * Compile-time endianness detection
>>>>> + */
>>>>> +#define RTE_BIG_ENDIAN 1
>>>>> +#define RTE_LITTLE_ENDIAN 2
>>>>> +#if defined __BYTE_ORDER
>>>>> +#if __BYTE_ORDER == __BIG_ENDIAN
>>>>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
>>>>> +#elif __BYTE_ORDER == __LITTLE_ENDIAN
>>>>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
>>>>> +#endif /* __BYTE_ORDER */
>>>>> +#elif defined __BYTE_ORDER__
>>>>> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>>>>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
>>>>> +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
>>>>> +#endif /* __BYTE_ORDER__ */
>>>>> +#elif defined __BIG_ENDIAN__
>>>>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
>>>>> +#elif defined __LITTLE_ENDIAN__
>>>>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
>>>>> +#endif
>>>> What do you think about :
>>>>
>>>> +/*
>>>> + * Compile-time endianness detection
>>>> + */
>>>> +#define RTE_BIG_ENDIAN 1
>>>> +#define RTE_LITTLE_ENDIAN 2
>>>> +if defined __BYTE_ORDER__ /* Prefer gcc build-in macros */
>>>> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>>>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
>>>> +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
>>>> +#endif /* __BYTE_ORDER__ */
>>>> +#else
>>>> +#if defined RTE_EXEC_ENV_BSDAPP
>>>> +#include <sys/endian.h>
>>>> +#else
>>>> +#include <endian.h>
>>>> +#endif
>>>> +#if defined __BYTE_ORDER
>>>> +#if __BYTE_ORDER == __BIG_ENDIAN
>>>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
>>>> +#elif __BYTE_ORDER == __LITTLE_ENDIAN
>>>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
>>>> +#endif /* __BYTE_ORDER */
>>>> +#elif defined __BIG_ENDIAN__
>>>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN
>>>> +#elif defined __LITTLE_ENDIAN__
>>>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
>>>> +#endif
>>>> +#endif
>>> Please, could you give more explanations about your proposal?
>>> Why not always try to include endian.h?
>> I assume that if gcc can handler why we need include that file?
> Separating include on top is easier to read, and I'm not sure it won't
> be needed for __BYTE_ORDER__ with some toolchains.
>
>> Also it seems that only old version could have this issue, newer
>> versions has build in this marcos.
>>
>> So that's why I prefer "__BYTE_ORDER__" for high priority.
> I have no problem with reversing this priority.
>
>>> Why giving high priority to __BYTE_ORDER__?
> Any other comment? May I apply with above change?
Acked-by: Michael Qiu <michael.qiu@intel.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v2] test-pmd: Fix pointer aliasing error
2014-12-04 3:28 ` [dpdk-dev] [PATCH v2] " Michael Qiu
2014-12-04 4:16 ` [dpdk-dev] [PATCH v3] " Michael Qiu
@ 2014-12-04 12:54 ` Ananyev, Konstantin
1 sibling, 0 replies; 32+ messages in thread
From: Ananyev, Konstantin @ 2014-12-04 12:54 UTC (permalink / raw)
To: Michael Qiu, dev
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Michael Qiu
> Sent: Thursday, December 04, 2014 3:29 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v2] test-pmd: Fix pointer aliasing error
>
>
> app/test-pmd/csumonly.c: In function ‘get_psd_sum’:
> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’
> does break strict-aliasing rules
> build/include/rte_ip.h:157: note: initialized from here
> ...
>
> The root cause is that, compile enable strict aliasing by default,
> while in function rte_raw_cksum() try to convert 'const char *'
> to 'const uint16_t *'.
>
> This patch is one workaround fix.
>
> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> ---
> lib/librte_net/rte_ip.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> index 61e4457..f1c7087 100644
> --- a/lib/librte_net/rte_ip.h
> +++ b/lib/librte_net/rte_ip.h
> @@ -154,7 +154,8 @@ struct ipv4_hdr {
> static inline uint16_t
> rte_raw_cksum(const char *buf, size_t len)
> {
> - const uint16_t *u16 = (const uint16_t *)buf;
> + unsigned long ptr = (unsigned long)buf;
I think better use uintptr_t here.
It is specially designed to be long enough to hold pointer value.
> + const uint16_t *u16 = (const uint16_t *)ptr;
> uint32_t sum = 0;
>
> while (len >= (sizeof(*u16) * 4)) {
> --
> 1.9.3
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test-pmd: Fix pointer aliasing error
2014-12-04 4:16 ` [dpdk-dev] [PATCH v3] " Michael Qiu
@ 2014-12-05 5:34 ` Qiu, Michael
2014-12-05 9:24 ` Thomas Monjalon
2014-12-08 1:30 ` Qiu, Michael
1 sibling, 1 reply; 32+ messages in thread
From: Qiu, Michael @ 2014-12-05 5:34 UTC (permalink / raw)
To: Michael Qiu, dev
Any comments about this version? a new workaround solution :)
Thanks,
Michael
On 12/4/2014 9:35 PM, Michael Qiu wrote:
> app/test-pmd/csumonly.c: In function ‘get_psd_sum’:
> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’
> does break strict-aliasing rules
> build/include/rte_ip.h:157: note: initialized from here
> ...
>
> The root cause is that, compile enable strict aliasing by default,
> while in function rte_raw_cksum() try to convert 'const char *'
> to 'const uint16_t *'.
>
> This patch is one workaround fix.
>
> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> ---
> v3 --> v2:
> use uintptr_t instead of unsigned long to
> save pointer.
>
> v2 --> v1:
> Workaround solution instead of shut off the
> gcc params.
>
> lib/librte_net/rte_ip.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> index 61e4457..cda3436 100644
> --- a/lib/librte_net/rte_ip.h
> +++ b/lib/librte_net/rte_ip.h
> @@ -154,7 +154,8 @@ struct ipv4_hdr {
> static inline uint16_t
> rte_raw_cksum(const char *buf, size_t len)
> {
> - const uint16_t *u16 = (const uint16_t *)buf;
> + uintptr_t ptr = (uintptr_t)buf;
> + const uint16_t *u16 = (const uint16_t *)ptr;
> uint32_t sum = 0;
>
> while (len >= (sizeof(*u16) * 4)) {
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test-pmd: Fix pointer aliasing error
2014-12-05 5:34 ` Qiu, Michael
@ 2014-12-05 9:24 ` Thomas Monjalon
2014-12-08 1:28 ` Qiu, Michael
0 siblings, 1 reply; 32+ messages in thread
From: Thomas Monjalon @ 2014-12-05 9:24 UTC (permalink / raw)
To: Qiu, Michael; +Cc: dev
2014-12-05 05:34, Qiu, Michael:
> Any comments about this version? a new workaround solution :)
Yes, one comment: I think it's ugly :)
These aliasing errors are not reliable so I think we can disable it (like
Linux does).
But in case you don't want to disable the warning, please add a comment to your
workaround to explain it is caused by GCC strict-aliasing check.
> > - const uint16_t *u16 = (const uint16_t *)buf;
> > + uintptr_t ptr = (uintptr_t)buf;
> > + const uint16_t *u16 = (const uint16_t *)ptr;
Thanks
--
Thomas
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH 0/2] fix endianness in EAL
2014-12-04 9:28 ` [dpdk-dev] [PATCH 0/2] fix endianness in EAL Chao Zhu
@ 2014-12-05 16:01 ` Thomas Monjalon
0 siblings, 0 replies; 32+ messages in thread
From: Thomas Monjalon @ 2014-12-05 16:01 UTC (permalink / raw)
To: Chao Zhu; +Cc: dev
> > It can be hard to implement a reliable detection of endianness.
> > Previous trials in testpmd failed.
> > The IBM Power patchset introduced a config option.
> >
> > This patchset try to improve the situation by having a detection
> > in EAL headers.
> >
> > Please test it (especially on IBM Power) with different toolchains
> > or distributions.
> >
> > Thomas Monjalon (2):
> > eal: detect endianness
> > app/testpmd: fix endianness detection
> >
> I tested it on IBM Power with GCC 4.8. It works fine.
> If there is no other better way to do the detection, I think this patch
> is good enough.
>
> Acked-by: Chao Zhu<chaozhu@linux.vnet.ibm.com>
Applied
--
Thomas
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test-pmd: Fix pointer aliasing error
2014-12-05 9:24 ` Thomas Monjalon
@ 2014-12-08 1:28 ` Qiu, Michael
0 siblings, 0 replies; 32+ messages in thread
From: Qiu, Michael @ 2014-12-08 1:28 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev
On 12/5/2014 5:26 PM, Thomas Monjalon wrote:
> 2014-12-05 05:34, Qiu, Michael:
>> Any comments about this version? a new workaround solution :)
> Yes, one comment: I think it's ugly :)
> These aliasing errors are not reliable so I think we can disable it (like
> Linux does).
> But in case you don't want to disable the warning, please add a comment to your
> workaround to explain it is caused by GCC strict-aliasing check.
Yes, really ugly ....
But lots of reviewer voted against with disable it as my first version
is disable.
I could not think out a better solution for all reviewers can accept.
I will add the comment with the thread of v3 patch.
Thanks,
Michael
>>> - const uint16_t *u16 = (const uint16_t *)buf;
>>> + uintptr_t ptr = (uintptr_t)buf;
>>> + const uint16_t *u16 = (const uint16_t *)ptr;
> Thanks
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test-pmd: Fix pointer aliasing error
2014-12-04 4:16 ` [dpdk-dev] [PATCH v3] " Michael Qiu
2014-12-05 5:34 ` Qiu, Michael
@ 2014-12-08 1:30 ` Qiu, Michael
2014-12-10 3:44 ` Qiu, Michael
2014-12-11 0:54 ` Thomas Monjalon
1 sibling, 2 replies; 32+ messages in thread
From: Qiu, Michael @ 2014-12-08 1:30 UTC (permalink / raw)
To: Michael Qiu, dev
On 12/4/2014 9:35 PM, Michael Qiu wrote:
> app/test-pmd/csumonly.c: In function ‘get_psd_sum’:
> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’
> does break strict-aliasing rules
> build/include/rte_ip.h:157: note: initialized from here
> ...
>
> The root cause is that, compile enable strict aliasing by default,
> while in function rte_raw_cksum() try to convert 'const char *'
> to 'const uint16_t *'.
>
> This patch is one workaround fix.
>
> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> ---
> v3 --> v2:
> use uintptr_t instead of unsigned long to
> save pointer.
>
> v2 --> v1:
> Workaround solution instead of shut off the
> gcc params.
>
> lib/librte_net/rte_ip.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> index 61e4457..cda3436 100644
> --- a/lib/librte_net/rte_ip.h
> +++ b/lib/librte_net/rte_ip.h
> @@ -154,7 +154,8 @@ struct ipv4_hdr {
> static inline uint16_t
> rte_raw_cksum(const char *buf, size_t len)
> {
> - const uint16_t *u16 = (const uint16_t *)buf;
> + uintptr_t ptr = (uintptr_t)buf;
> + const uint16_t *u16 = (const uint16_t *)ptr;
> uint32_t sum = 0;
>
> while (len >= (sizeof(*u16) * 4)) {
This workaround is to solve the compile issue of GCC strict-aliasing(Two
different type pointers should not be point to the same memory address).
For GCC 4.4.7 it will definitely occurs if flags "-fstrict-aliasing"
and "-Wall" used.
Thanks,
Michael
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test-pmd: Fix pointer aliasing error
2014-12-08 1:30 ` Qiu, Michael
@ 2014-12-10 3:44 ` Qiu, Michael
2014-12-11 0:54 ` Thomas Monjalon
1 sibling, 0 replies; 32+ messages in thread
From: Qiu, Michael @ 2014-12-10 3:44 UTC (permalink / raw)
To: Michael Qiu, dev, Thomas.monjalon@6wind.com
Hi Thomas,
What's going on with this patch?
I really do not have other better solution.
Thanks,
Michael
On 12/8/2014 9:30 AM, Qiu, Michael wrote:
> On 12/4/2014 9:35 PM, Michael Qiu wrote:
>> app/test-pmd/csumonly.c: In function ‘get_psd_sum’:
>> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’
>> does break strict-aliasing rules
>> build/include/rte_ip.h:157: note: initialized from here
>> ...
>>
>> The root cause is that, compile enable strict aliasing by default,
>> while in function rte_raw_cksum() try to convert 'const char *'
>> to 'const uint16_t *'.
>>
>> This patch is one workaround fix.
>>
>> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
>> ---
>> v3 --> v2:
>> use uintptr_t instead of unsigned long to
>> save pointer.
>>
>> v2 --> v1:
>> Workaround solution instead of shut off the
>> gcc params.
>>
>> lib/librte_net/rte_ip.h | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
>> index 61e4457..cda3436 100644
>> --- a/lib/librte_net/rte_ip.h
>> +++ b/lib/librte_net/rte_ip.h
>> @@ -154,7 +154,8 @@ struct ipv4_hdr {
>> static inline uint16_t
>> rte_raw_cksum(const char *buf, size_t len)
>> {
>> - const uint16_t *u16 = (const uint16_t *)buf;
>> + uintptr_t ptr = (uintptr_t)buf;
>> + const uint16_t *u16 = (const uint16_t *)ptr;
>> uint32_t sum = 0;
>>
>> while (len >= (sizeof(*u16) * 4)) {
> This workaround is to solve the compile issue of GCC strict-aliasing(Two
> different type pointers should not be point to the same memory address).
>
> For GCC 4.4.7 it will definitely occurs if flags "-fstrict-aliasing"
> and "-Wall" used.
>
> Thanks,
> Michael
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test-pmd: Fix pointer aliasing error
2014-12-08 1:30 ` Qiu, Michael
2014-12-10 3:44 ` Qiu, Michael
@ 2014-12-11 0:54 ` Thomas Monjalon
2014-12-11 17:51 ` r k
1 sibling, 1 reply; 32+ messages in thread
From: Thomas Monjalon @ 2014-12-11 0:54 UTC (permalink / raw)
To: Qiu, Michael, dev
> > app/test-pmd/csumonly.c: In function ‘get_psd_sum’:
> > build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’
> > does break strict-aliasing rules
> > build/include/rte_ip.h:157: note: initialized from here
> > ...
> >
> > The root cause is that, compile enable strict aliasing by default,
> > while in function rte_raw_cksum() try to convert 'const char *'
> > to 'const uint16_t *'.
> >
> > This patch is one workaround fix.
> >
> > Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> > ---
> > v3 --> v2:
> > use uintptr_t instead of unsigned long to
> > save pointer.
> >
> > v2 --> v1:
> > Workaround solution instead of shut off the
> > gcc params.
>
> This workaround is to solve the compile issue of GCC strict-aliasing(Two
> different type pointers should not be point to the same memory address).
>
> For GCC 4.4.7 it will definitely occurs if flags "-fstrict-aliasing"
> and "-Wall" used.
Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>>
Applied with a comment in the code.
Thanks
--
Thomas
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test-pmd: Fix pointer aliasing error
2014-12-11 0:54 ` Thomas Monjalon
@ 2014-12-11 17:51 ` r k
2014-12-12 6:49 ` Qiu, Michael
0 siblings, 1 reply; 32+ messages in thread
From: r k @ 2014-12-11 17:51 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev
Thomas, Michael,
Wouldn't it cause unaligned memory access (new changes as well as the
previous code)? Wondering if get_unaligned/put_unaligned macros similar to
the ones used in kernel be ported to user-space?
Thanks,
Ravi
On Wed, Dec 10, 2014 at 4:54 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:
>
> > > app/test-pmd/csumonly.c: In function 'get_psd_sum':
> > > build/include/rte_ip.h:161: error: dereferencing pointer 'u16'
> > > does break strict-aliasing rules
> > > build/include/rte_ip.h:157: note: initialized from here
> > > ...
> > >
> > > The root cause is that, compile enable strict aliasing by default,
> > > while in function rte_raw_cksum() try to convert 'const char *'
> > > to 'const uint16_t *'.
> > >
> > > This patch is one workaround fix.
> > >
> > > Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> > > ---
> > > v3 --> v2:
> > > use uintptr_t instead of unsigned long to
> > > save pointer.
> > >
> > > v2 --> v1:
> > > Workaround solution instead of shut off the
> > > gcc params.
> >
> > This workaround is to solve the compile issue of GCC strict-aliasing(Two
> > different type pointers should not be point to the same memory address).
> >
> > For GCC 4.4.7 it will definitely occurs if flags "-fstrict-aliasing"
> > and "-Wall" used.
>
> Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>>
>
> Applied with a comment in the code.
>
> Thanks
> --
> Thomas
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test-pmd: Fix pointer aliasing error
2014-12-11 17:51 ` r k
@ 2014-12-12 6:49 ` Qiu, Michael
0 siblings, 0 replies; 32+ messages in thread
From: Qiu, Michael @ 2014-12-12 6:49 UTC (permalink / raw)
To: r k, Thomas Monjalon; +Cc: dev
On 2014/12/12 1:51, r k wrote:
Thomas, Michael,
Wouldn't it cause unaligned memory access (new changes as well as the previous code)? Wondering if get_unaligned/put_unaligned macros similar to the ones used in kernel be ported to user-space?
I think it will not, as all buf point to are struct udp_hdr/struct tcp_hdr/struct ipv6_psd_header/struct ipv4_psd_header, they are all aligned with uint16_t.
Thanks
Michael
Thanks,
Ravi
On Wed, Dec 10, 2014 at 4:54 PM, Thomas Monjalon <thomas.monjalon@6wind.com<mailto:thomas.monjalon@6wind.com>> wrote:
> > app/test-pmd/csumonly.c: In function ‘get_psd_sum’:
> > build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’
> > does break strict-aliasing rules
> > build/include/rte_ip.h:157: note: initialized from here
> > ...
> >
> > The root cause is that, compile enable strict aliasing by default,
> > while in function rte_raw_cksum() try to convert 'const char *'
> > to 'const uint16_t *'.
> >
> > This patch is one workaround fix.
> >
> > Signed-off-by: Michael Qiu <michael.qiu@intel.com<mailto:michael.qiu@intel.com>>
> > ---
> > v3 --> v2:
> > use uintptr_t instead of unsigned long to
> > save pointer.
> >
> > v2 --> v1:
> > Workaround solution instead of shut off the
> > gcc params.
>
> This workaround is to solve the compile issue of GCC strict-aliasing(Two
> different type pointers should not be point to the same memory address).
>
> For GCC 4.4.7 it will definitely occurs if flags "-fstrict-aliasing"
> and "-Wall" used.
Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com<mailto:thomas.monjalon@6wind.com>>>
Applied with a comment in the code.
Thanks
--
Thomas
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2014-12-12 6:51 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1417606044-3432-1-git-send-email-michael.qiu@intel.com>
[not found] ` <1417606099-3489-1-git-send-email-michael.qiu@intel.com>
2014-12-03 11:42 ` [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error Bruce Richardson
2014-12-03 13:59 ` Qiu, Michael
2014-12-03 14:51 ` Bruce Richardson
2014-12-03 15:19 ` Qiu, Michael
2014-12-03 15:36 ` Bruce Richardson
2014-12-04 2:38 ` Qiu, Michael
2014-12-04 3:28 ` [dpdk-dev] [PATCH v2] " Michael Qiu
2014-12-04 4:16 ` [dpdk-dev] [PATCH v3] " Michael Qiu
2014-12-05 5:34 ` Qiu, Michael
2014-12-05 9:24 ` Thomas Monjalon
2014-12-08 1:28 ` Qiu, Michael
2014-12-08 1:30 ` Qiu, Michael
2014-12-10 3:44 ` Qiu, Michael
2014-12-11 0:54 ` Thomas Monjalon
2014-12-11 17:51 ` r k
2014-12-12 6:49 ` Qiu, Michael
2014-12-04 12:54 ` [dpdk-dev] [PATCH v2] " Ananyev, Konstantin
2014-12-03 15:24 ` [dpdk-dev] [PATCH] " Olivier MATZ
2014-12-03 16:03 ` Dayu Qiu
2014-12-03 15:57 ` Dayu Qiu
2014-12-03 16:26 ` [dpdk-dev] [PATCH] test-pmd: Fix "__BYTE_ORDER__" not defined error Qiu, Michael
2014-12-03 19:59 ` Thomas Monjalon
2014-12-03 20:47 ` [dpdk-dev] [PATCH 0/2] fix endianness in EAL Thomas Monjalon
2014-12-03 20:47 ` [dpdk-dev] [PATCH 1/2] eal: detect endianness Thomas Monjalon
2014-12-04 2:28 ` Qiu, Michael
2014-12-04 9:00 ` Thomas Monjalon
2014-12-04 10:28 ` Qiu, Michael
2014-12-04 12:19 ` Thomas Monjalon
2014-12-04 12:50 ` Qiu, Michael
2014-12-03 20:47 ` [dpdk-dev] [PATCH 2/2] app/testpmd: fix endianness detection Thomas Monjalon
2014-12-04 9:28 ` [dpdk-dev] [PATCH 0/2] fix endianness in EAL Chao Zhu
2014-12-05 16:01 ` Thomas Monjalon
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).