* [dpdk-dev] [PATCH] i40e: fix shared code compile warning
@ 2014-06-24 5:22 Chen Jing D(Mark)
2014-06-24 8:46 ` Thomas Monjalon
0 siblings, 1 reply; 8+ messages in thread
From: Chen Jing D(Mark) @ 2014-06-24 5:22 UTC (permalink / raw)
To: dev
From: "Chen Jing D(Mark)" <jing.d.chen@intel.com>
Fix a compile warning in shared code on 32-bits RHEL6.3/6.5.
Signed-off-by: Chen Jing D(Mark) <jing.d.chen@intel.com>
---
lib/librte_pmd_i40e/Makefile | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/lib/librte_pmd_i40e/Makefile b/lib/librte_pmd_i40e/Makefile
index 09f2087..1d9dcb3 100644
--- a/lib/librte_pmd_i40e/Makefile
+++ b/lib/librte_pmd_i40e/Makefile
@@ -52,6 +52,7 @@ CFLAGS_SHARED_DRIVERS += -Wno-missing-field-initializers
CFLAGS_SHARED_DRIVERS += -Wno-pointer-to-int-cast
CFLAGS_SHARED_DRIVERS += -Wno-format-nonliteral
CFLAGS_SHARED_DRIVERS += -Wno-format-security
+CFLAGS_i40e_lan_hmc.o += -Wno-error
endif
#
--
1.7.7.6
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] i40e: fix shared code compile warning
2014-06-24 5:22 [dpdk-dev] [PATCH] i40e: fix shared code compile warning Chen Jing D(Mark)
@ 2014-06-24 8:46 ` Thomas Monjalon
2014-06-24 9:47 ` Chen, Jing D
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Monjalon @ 2014-06-24 8:46 UTC (permalink / raw)
To: Chen Jing D(Mark); +Cc: dev
Hi,
2014-06-24 13:22, Chen Jing D:
> Fix a compile warning in shared code on 32-bits RHEL6.3/6.5.
>
> Signed-off-by: Chen Jing D(Mark) <jing.d.chen@intel.com>
In such case you should show the error message in the log.
lib/librte_pmd_i40e/i40e/i40e_lan_hmc.c:917:
error: integer constant is too large for ‘long’ type
> --- a/lib/librte_pmd_i40e/Makefile
> +++ b/lib/librte_pmd_i40e/Makefile
> @@ -52,6 +52,7 @@ CFLAGS_SHARED_DRIVERS += -Wno-missing-field-initializers
> CFLAGS_SHARED_DRIVERS += -Wno-pointer-to-int-cast
> CFLAGS_SHARED_DRIVERS += -Wno-format-nonliteral
> CFLAGS_SHARED_DRIVERS += -Wno-format-security
> +CFLAGS_i40e_lan_hmc.o += -Wno-error
I know we shouldn't modify base drivers. But this one seems to be an important
error. In such case, we already modified base driver. Recently:
http://dpdk.org/ml/archives/dev/2014-June/003498.html
--
Thomas
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] i40e: fix shared code compile warning
2014-06-24 8:46 ` Thomas Monjalon
@ 2014-06-24 9:47 ` Chen, Jing D
2014-06-24 10:06 ` Thomas Monjalon
0 siblings, 1 reply; 8+ messages in thread
From: Chen, Jing D @ 2014-06-24 9:47 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev
Hi Thomas,
-----Original Message-----
From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
Sent: Tuesday, June 24, 2014 4:47 PM
To: Chen, Jing D
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] i40e: fix shared code compile warning
Hi,
2014-06-24 13:22, Chen Jing D:
> Fix a compile warning in shared code on 32-bits RHEL6.3/6.5.
>
> Signed-off-by: Chen Jing D(Mark) <jing.d.chen@intel.com>
In such case you should show the error message in the log.
lib/librte_pmd_i40e/i40e/i40e_lan_hmc.c:917:
error: integer constant is too large for ‘long’ type
> --- a/lib/librte_pmd_i40e/Makefile
> +++ b/lib/librte_pmd_i40e/Makefile
> @@ -52,6 +52,7 @@ CFLAGS_SHARED_DRIVERS +=
> -Wno-missing-field-initializers CFLAGS_SHARED_DRIVERS +=
> -Wno-pointer-to-int-cast CFLAGS_SHARED_DRIVERS +=
> -Wno-format-nonliteral CFLAGS_SHARED_DRIVERS += -Wno-format-security
> +CFLAGS_i40e_lan_hmc.o += -Wno-error
I know we shouldn't modify base drivers. But this one seems to be an important error. In such case, we already modified base driver. Recently:
http://dpdk.org/ml/archives/dev/2014-June/003498.html
I think it's different. The logic is right after adding the patch. Below is my finding.
In this case, it met the error when compile on 32-bits OS. The message is :
/jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/librte_pmd_i40e/i40e/i40e_lan_hmc.c: In function ‘i40e_write_qword’:
/jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/librte_pmd_i40e/i40e/i40e_lan_hmc.c:917: error: integer constant is too large for ‘long’ type
/jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/librte_pmd_i40e/i40e/i40e_lan_hmc.c: In function ‘i40e_read_qword’:
/jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/librte_pmd_i40e/i40e/i40e_lan_hmc.c:1097: error: integer constant is too large for ‘long’ type
I found the code that cause errors. 'mask' is 'uint64_t' type and is assigned to value 0Xffff_ffff_ffff_ffff. Compiler assumes the constant is 'int' type by default. If changed it to oxffff_ffff_ffff_ffffULL, the warning will be gone.
if (ce_info->width < 64)
mask = ((u64)1 << ce_info->width) - 1;
else
mask = 0xFFFFFFFFFFFFFFFF;
besides that, I dis-assembler the code with the patch and get below segment. It seems right.
if (ce_info->width < 64)
1946: 8b 45 0c mov 0xc(%ebp),%eax
1949: 0f b7 40 04 movzwl 0x4(%eax),%eax
194d: 66 83 f8 3f cmp $0x3f,%ax
1951: 77 30 ja 1983 <i40e_write_qword+0x62>
mask = ((u64)1 << ce_info->width) - 1;
1953: 8b 45 0c mov 0xc(%ebp),%eax
1956: 0f b7 40 04 movzwl 0x4(%eax),%eax
195a: 0f b7 c8 movzwl %ax,%ecx
195d: b8 01 00 00 00 mov $0x1,%eax
1962: ba 00 00 00 00 mov $0x0,%edx
1967: 0f a5 c2 shld %cl,%eax,%edx
196a: d3 e0 shl %cl,%eax
196c: f6 c1 20 test $0x20,%cl
196f: 74 04 je 1975 <i40e_write_qword+0x54>
1971: 89 c2 mov %eax,%edx
1973: 31 c0 xor %eax,%eax
1975: 83 c0 ff add $0xffffffff,%eax
1978: 83 d2 ff adc $0xffffffff,%edx
197b: 89 45 e0 mov %eax,-0x20(%ebp)
197e: 89 55 e4 mov %edx,-0x1c(%ebp)
1981: eb 0e jmp 1991 <i40e_write_qword+0x70>
else
mask = 0xFFFFFFFFFFFFFFFF;
1983: c7 45 e0 ff ff ff ff movl $0xffffffff,-0x20(%ebp)
198a: c7 45 e4 ff ff ff ff movl $0xffffffff,-0x1c(%ebp)
--
Thomas
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] i40e: fix shared code compile warning
2014-06-24 9:47 ` Chen, Jing D
@ 2014-06-24 10:06 ` Thomas Monjalon
2014-06-24 14:43 ` Ananyev, Konstantin
2014-06-25 5:36 ` Chen, Jing D
0 siblings, 2 replies; 8+ messages in thread
From: Thomas Monjalon @ 2014-06-24 10:06 UTC (permalink / raw)
To: Chen, Jing D; +Cc: dev
2014-06-24 09:47, Chen, Jing D:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2014-06-24 13:22, Chen Jing D:
> > > +CFLAGS_i40e_lan_hmc.o += -Wno-error
> >
> > I know we shouldn't modify base drivers. But this one seems to be an
> > important error. In such case, we already modified base driver. Recently:
> > http://dpdk.org/ml/archives/dev/2014-June/003498.html
>
> I think it's different. The logic is right after adding the patch. Below is
> my finding.
> In this case, it met the error when compile on 32-bits OS. The message is :
>
> /jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/librte_pmd_i40e
> /i40e/i40e_lan_hmc.c: In function ‘i40e_write_qword’:
> /jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/librte_pmd_i40
> e/i40e/i40e_lan_hmc.c:917: error: integer constant is too large for ‘long’
> type
> /jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/librte_pmd_i40
> e/i40e/i40e_lan_hmc.c: In function ‘i40e_read_qword’:
> /jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/librte_pmd_i40
> e/i40e/i40e_lan_hmc.c:1097: error: integer constant is too large for ‘long’
> type
> I found the code that cause errors. 'mask' is 'uint64_t' type and is
> assigned to value 0Xffff_ffff_ffff_ffff. Compiler assumes the constant is
> 'int' type by default. If changed it to oxffff_ffff_ffff_ffffULL, the
> warning will be gone.
> if (ce_info->width < 64)
> mask = ((u64)1 << ce_info->width) - 1;
> else
> mask = 0xFFFFFFFFFFFFFFFF;
>
> besides that, I dis-assembler the code with the patch and get below segment.
> It seems right.
> if (ce_info->width < 64)
> 1946: 8b 45 0c mov 0xc(%ebp),%eax
> 1949: 0f b7 40 04 movzwl 0x4(%eax),%eax
> 194d: 66 83 f8 3f cmp $0x3f,%ax
> 1951: 77 30 ja 1983 <i40e_write_qword+0x62>
> mask = ((u64)1 << ce_info->width) - 1;
> 1953: 8b 45 0c mov 0xc(%ebp),%eax
> 1956: 0f b7 40 04 movzwl 0x4(%eax),%eax
> 195a: 0f b7 c8 movzwl %ax,%ecx
> 195d: b8 01 00 00 00 mov $0x1,%eax
> 1962: ba 00 00 00 00 mov $0x0,%edx
> 1967: 0f a5 c2 shld %cl,%eax,%edx
> 196a: d3 e0 shl %cl,%eax
> 196c: f6 c1 20 test $0x20,%cl
> 196f: 74 04 je 1975 <i40e_write_qword+0x54>
> 1971: 89 c2 mov %eax,%edx
> 1973: 31 c0 xor %eax,%eax
> 1975: 83 c0 ff add $0xffffffff,%eax
> 1978: 83 d2 ff adc $0xffffffff,%edx
> 197b: 89 45 e0 mov %eax,-0x20(%ebp)
> 197e: 89 55 e4 mov %edx,-0x1c(%ebp)
> 1981: eb 0e jmp 1991 <i40e_write_qword+0x70>
> else
> mask = 0xFFFFFFFFFFFFFFFF;
> 1983: c7 45 e0 ff ff ff ff movl $0xffffffff,-0x20(%ebp)
> 198a: c7 45 e4 ff ff ff ff movl $0xffffffff,-0x1c(%ebp)
Maybe I don't understand. You are saying you can fix the compiler warning by
adding ULL to the constant. This is a simple patch and is a lot nicer than
CFLAGS_i40e_lan_hmc.o += -Wno-error
Even if the asm code seems right, it would be more secure to remove this
warning.
PS: please try to configure your mailer to add citation marks.
--
Thomas
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] i40e: fix shared code compile warning
2014-06-24 10:06 ` Thomas Monjalon
@ 2014-06-24 14:43 ` Ananyev, Konstantin
2014-06-24 15:25 ` Zhang, Helin
2014-06-25 5:36 ` Chen, Jing D
1 sibling, 1 reply; 8+ messages in thread
From: Ananyev, Konstantin @ 2014-06-24 14:43 UTC (permalink / raw)
To: Thomas Monjalon, Chen, Jing D; +Cc: dev
Hi Thomas,
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, June 24, 2014 11:06 AM
> To: Chen, Jing D
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] i40e: fix shared code compile warning
>
> 2014-06-24 09:47, Chen, Jing D:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2014-06-24 13:22, Chen Jing D:
> > > > +CFLAGS_i40e_lan_hmc.o += -Wno-error
> > >
> > > I know we shouldn't modify base drivers. But this one seems to be an
> > > important error. In such case, we already modified base driver. Recently:
> > > http://dpdk.org/ml/archives/dev/2014-June/003498.html
> >
Actually, I suppose there is a way to fix that issue without modifying shared code.
As I know Pablo plans to prepare another patch to deal with it in a proper way.
> > I think it's different. The logic is right after adding the patch. Below is
> > my finding.
>
> > In this case, it met the error when compile on 32-bits OS. The message is :
> >
> > /jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/librte_pmd_i40e
> > /i40e/i40e_lan_hmc.c: In function ‘i40e_write_qword’:
> > /jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/librte_pmd_i40
> > e/i40e/i40e_lan_hmc.c:917: error: integer constant is too large for ‘long’
> > type
> > /jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/librte_pmd_i40
> > e/i40e/i40e_lan_hmc.c: In function ‘i40e_read_qword’:
> > /jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/librte_pmd_i40
> > e/i40e/i40e_lan_hmc.c:1097: error: integer constant is too large for ‘long’
> > type
> > I found the code that cause errors. 'mask' is 'uint64_t' type and is
> > assigned to value 0Xffff_ffff_ffff_ffff. Compiler assumes the constant is
> > 'int' type by default. If changed it to oxffff_ffff_ffff_ffffULL, the
> > warning will be gone.
>
> > if (ce_info->width < 64)
> > mask = ((u64)1 << ce_info->width) - 1;
> > else
> > mask = 0xFFFFFFFFFFFFFFFF;
> >
> > besides that, I dis-assembler the code with the patch and get below segment.
> > It seems right.
>
> > if (ce_info->width < 64)
> > 1946: 8b 45 0c mov 0xc(%ebp),%eax
> > 1949: 0f b7 40 04 movzwl 0x4(%eax),%eax
> > 194d: 66 83 f8 3f cmp $0x3f,%ax
> > 1951: 77 30 ja 1983 <i40e_write_qword+0x62>
> > mask = ((u64)1 << ce_info->width) - 1;
> > 1953: 8b 45 0c mov 0xc(%ebp),%eax
> > 1956: 0f b7 40 04 movzwl 0x4(%eax),%eax
> > 195a: 0f b7 c8 movzwl %ax,%ecx
> > 195d: b8 01 00 00 00 mov $0x1,%eax
> > 1962: ba 00 00 00 00 mov $0x0,%edx
> > 1967: 0f a5 c2 shld %cl,%eax,%edx
> > 196a: d3 e0 shl %cl,%eax
> > 196c: f6 c1 20 test $0x20,%cl
> > 196f: 74 04 je 1975 <i40e_write_qword+0x54>
> > 1971: 89 c2 mov %eax,%edx
> > 1973: 31 c0 xor %eax,%eax
> > 1975: 83 c0 ff add $0xffffffff,%eax
> > 1978: 83 d2 ff adc $0xffffffff,%edx
> > 197b: 89 45 e0 mov %eax,-0x20(%ebp)
> > 197e: 89 55 e4 mov %edx,-0x1c(%ebp)
> > 1981: eb 0e jmp 1991 <i40e_write_qword+0x70>
> > else
> > mask = 0xFFFFFFFFFFFFFFFF;
> > 1983: c7 45 e0 ff ff ff ff movl $0xffffffff,-0x20(%ebp)
> > 198a: c7 45 e4 ff ff ff ff movl $0xffffffff,-0x1c(%ebp)
>
> Maybe I don't understand. You are saying you can fix the compiler warning by
> adding ULL to the constant. This is a simple patch and is a lot nicer than
> CFLAGS_i40e_lan_hmc.o += -Wno-error
> Even if the asm code seems right, it would be more secure to remove this
> warning.
>
Yes, it is much nicer to fix it in i40e_lan_hmc.c.
But I don't really want us to open that door.
So my vote would be to initial Mark's patch: add '-Wno-error' in the Makefile.
Konstantin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] i40e: fix shared code compile warning
2014-06-24 14:43 ` Ananyev, Konstantin
@ 2014-06-24 15:25 ` Zhang, Helin
2014-06-26 12:22 ` Thomas Monjalon
0 siblings, 1 reply; 8+ messages in thread
From: Zhang, Helin @ 2014-06-24 15:25 UTC (permalink / raw)
To: Ananyev, Konstantin, Thomas Monjalon, Chen, Jing D; +Cc: dev
-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin
Sent: Tuesday, June 24, 2014 10:44 PM
To: Thomas Monjalon; Chen, Jing D
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] i40e: fix shared code compile warning
Hi Thomas,
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, June 24, 2014 11:06 AM
> To: Chen, Jing D
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] i40e: fix shared code compile warning
>
> 2014-06-24 09:47, Chen, Jing D:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2014-06-24 13:22, Chen Jing D:
> > > > +CFLAGS_i40e_lan_hmc.o += -Wno-error
> > >
> > > I know we shouldn't modify base drivers. But this one seems to be
> > > an important error. In such case, we already modified base driver. Recently:
> > > http://dpdk.org/ml/archives/dev/2014-June/003498.html
> >
Actually, I suppose there is a way to fix that issue without modifying shared code.
As I know Pablo plans to prepare another patch to deal with it in a proper way.
> > I think it's different. The logic is right after adding the patch.
> > Below is my finding.
>
> > In this case, it met the error when compile on 32-bits OS. The message is :
> >
> > /jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/librte_
> > pmd_i40e
> > /i40e/i40e_lan_hmc.c: In function ‘i40e_write_qword’:
> > /jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/librte_
> > pmd_i40
> > e/i40e/i40e_lan_hmc.c:917: error: integer constant is too large for ‘long’
> > type
> > /jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/librte_
> > pmd_i40
> > e/i40e/i40e_lan_hmc.c: In function ‘i40e_read_qword’:
> > /jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/librte_
> > pmd_i40
> > e/i40e/i40e_lan_hmc.c:1097: error: integer constant is too large for ‘long’
> > type
> > I found the code that cause errors. 'mask' is 'uint64_t' type and is
> > assigned to value 0Xffff_ffff_ffff_ffff. Compiler assumes the
> > constant is 'int' type by default. If changed it to
> > oxffff_ffff_ffff_ffffULL, the warning will be gone.
>
> > if (ce_info->width < 64)
> > mask = ((u64)1 << ce_info->width) - 1;
> > else
> > mask = 0xFFFFFFFFFFFFFFFF;
> >
> > besides that, I dis-assembler the code with the patch and get below segment.
> > It seems right.
>
> > if (ce_info->width < 64)
> > 1946: 8b 45 0c mov 0xc(%ebp),%eax
> > 1949: 0f b7 40 04 movzwl 0x4(%eax),%eax
> > 194d: 66 83 f8 3f cmp $0x3f,%ax
> > 1951: 77 30 ja 1983 <i40e_write_qword+0x62>
> > mask = ((u64)1 << ce_info->width) - 1;
> > 1953: 8b 45 0c mov 0xc(%ebp),%eax
> > 1956: 0f b7 40 04 movzwl 0x4(%eax),%eax
> > 195a: 0f b7 c8 movzwl %ax,%ecx
> > 195d: b8 01 00 00 00 mov $0x1,%eax
> > 1962: ba 00 00 00 00 mov $0x0,%edx
> > 1967: 0f a5 c2 shld %cl,%eax,%edx
> > 196a: d3 e0 shl %cl,%eax
> > 196c: f6 c1 20 test $0x20,%cl
> > 196f: 74 04 je 1975 <i40e_write_qword+0x54>
> > 1971: 89 c2 mov %eax,%edx
> > 1973: 31 c0 xor %eax,%eax
> > 1975: 83 c0 ff add $0xffffffff,%eax
> > 1978: 83 d2 ff adc $0xffffffff,%edx
> > 197b: 89 45 e0 mov %eax,-0x20(%ebp)
> > 197e: 89 55 e4 mov %edx,-0x1c(%ebp)
> > 1981: eb 0e jmp 1991 <i40e_write_qword+0x70>
> > else
> > mask = 0xFFFFFFFFFFFFFFFF;
> > 1983: c7 45 e0 ff ff ff ff movl $0xffffffff,-0x20(%ebp)
> > 198a: c7 45 e4 ff ff ff ff movl $0xffffffff,-0x1c(%ebp)
>
> Maybe I don't understand. You are saying you can fix the compiler
> warning by adding ULL to the constant. This is a simple patch and is a lot nicer than
> CFLAGS_i40e_lan_hmc.o += -Wno-error
> Even if the asm code seems right, it would be more secure to remove
> this warning.
>
Yes, it is much nicer to fix it in i40e_lan_hmc.c.
But I don't really want us to open that door.
So my vote would be to initial Mark's patch: add '-Wno-error' in the Makefile.
Konstantin
---------------------------------------------------------------------------------------------------------------------
Hi guys
We should not modify code in shared code, if we do not want to maintain those huge code base. I think Mark's patch good for now, we can report that issue to shared code maintainers, and try to get them to fix it later.
Regards,
Helin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] i40e: fix shared code compile warning
2014-06-24 10:06 ` Thomas Monjalon
2014-06-24 14:43 ` Ananyev, Konstantin
@ 2014-06-25 5:36 ` Chen, Jing D
1 sibling, 0 replies; 8+ messages in thread
From: Chen, Jing D @ 2014-06-25 5:36 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev
Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, June 24, 2014 6:06 PM
> To: Chen, Jing D
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] i40e: fix shared code compile warning
>
> 2014-06-24 09:47, Chen, Jing D:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2014-06-24 13:22, Chen Jing D:
> > > > +CFLAGS_i40e_lan_hmc.o += -Wno-error
> > >
> > > I know we shouldn't modify base drivers. But this one seems to be an
> > > important error. In such case, we already modified base driver. Recently:
> > > http://dpdk.org/ml/archives/dev/2014-June/003498.html
> >
> > I think it's different. The logic is right after adding the patch.
> > Below is my finding.
>
> > In this case, it met the error when compile on 32-bits OS. The message is :
> >
> >
> /jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/libr
> te_pm
> > d_i40e
> > /i40e/i40e_lan_hmc.c: In function ‘i40e_write_qword’:
> >
> /jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/libr
> te_pm
> > d_i40
> > e/i40e/i40e_lan_hmc.c:917: error: integer constant is too large for ‘long’
> > type
> >
> /jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/libr
> te_pm
> > d_i40
> > e/i40e/i40e_lan_hmc.c: In function ‘i40e_read_qword’:
> >
> /jenkins/workspace/DPDK_AUTO_IDT_VM_RHEL65_32_BUILD/DPDK/lib/libr
> te_pm
> > d_i40
> > e/i40e/i40e_lan_hmc.c:1097: error: integer constant is too large for ‘long’
> > type
> > I found the code that cause errors. 'mask' is 'uint64_t' type and is
> > assigned to value 0Xffff_ffff_ffff_ffff. Compiler assumes the constant
> > is 'int' type by default. If changed it to oxffff_ffff_ffff_ffffULL,
> > the warning will be gone.
>
> > if (ce_info->width < 64)
> > mask = ((u64)1 << ce_info->width) - 1;
> > else
> > mask = 0xFFFFFFFFFFFFFFFF;
> >
> > besides that, I dis-assembler the code with the patch and get below
> segment.
> > It seems right.
>
> > if (ce_info->width < 64)
> > 1946: 8b 45 0c mov 0xc(%ebp),%eax
> > 1949: 0f b7 40 04 movzwl 0x4(%eax),%eax
> > 194d: 66 83 f8 3f cmp $0x3f,%ax
> > 1951: 77 30 ja 1983 <i40e_write_qword+0x62>
> > mask = ((u64)1 << ce_info->width) - 1;
> > 1953: 8b 45 0c mov 0xc(%ebp),%eax
> > 1956: 0f b7 40 04 movzwl 0x4(%eax),%eax
> > 195a: 0f b7 c8 movzwl %ax,%ecx
> > 195d: b8 01 00 00 00 mov $0x1,%eax
> > 1962: ba 00 00 00 00 mov $0x0,%edx
> > 1967: 0f a5 c2 shld %cl,%eax,%edx
> > 196a: d3 e0 shl %cl,%eax
> > 196c: f6 c1 20 test $0x20,%cl
> > 196f: 74 04 je 1975 <i40e_write_qword+0x54>
> > 1971: 89 c2 mov %eax,%edx
> > 1973: 31 c0 xor %eax,%eax
> > 1975: 83 c0 ff add $0xffffffff,%eax
> > 1978: 83 d2 ff adc $0xffffffff,%edx
> > 197b: 89 45 e0 mov %eax,-0x20(%ebp)
> > 197e: 89 55 e4 mov %edx,-0x1c(%ebp)
> > 1981: eb 0e jmp 1991 <i40e_write_qword+0x70>
> > else
> > mask = 0xFFFFFFFFFFFFFFFF;
> > 1983: c7 45 e0 ff ff ff ff movl $0xffffffff,-0x20(%ebp)
> > 198a: c7 45 e4 ff ff ff ff movl $0xffffffff,-0x1c(%ebp)
>
> Maybe I don't understand. You are saying you can fix the compiler warning
> by adding ULL to the constant. This is a simple patch and is a lot nicer than
> CFLAGS_i40e_lan_hmc.o += -Wno-error
> Even if the asm code seems right, it would be more secure to remove this
> warning.
>
> PS: please try to configure your mailer to add citation marks.
>
> --
> Thomas
[Chen, Jing D]
Generally speaking, I can fix the compile error by 2 ways.
1. change shared code with "ULL" suffix, which means that we had to maintain it. In any time shared code updates,
we needs to find out what change we've made in history and compare with new shared code.
I don’t know who should be responsible for that and how we record the changes. If you have good idea,
please share with me.
2. my committed patch to ignore the warning on that file and continue the compile . By doing so, the logic is right and
didn't change code's behavior. We also needn't maintain it. Even in bad case we find bugs hidden behind the
'--CFLAGS_i40e_lan_hmc.0' warning in future, we can fix it after bug occurred. That's simple.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] i40e: fix shared code compile warning
2014-06-24 15:25 ` Zhang, Helin
@ 2014-06-26 12:22 ` Thomas Monjalon
0 siblings, 0 replies; 8+ messages in thread
From: Thomas Monjalon @ 2014-06-26 12:22 UTC (permalink / raw)
To: Zhang, Helin, Ananyev, Konstantin, Chen, Jing D; +Cc: dev
2014-06-24 14:43, Ananyev, Konstantin:
> Yes, it is much nicer to fix it in i40e_lan_hmc.c.
> But I don't really want us to open that door.
> So my vote would be to initial Mark's patch: add '-Wno-error' in the
> Makefile.
2014-06-24 15:25, Zhang, Helin:
> We should not modify code in shared code, if we do not want to maintain
> those huge code base. I think Mark's patch good for now, we can report that
> issue to shared code maintainers, and try to get them to fix it later.
2014-06-25 05:36, Chen, Jing D:
> my committed patch to ignore the warning on that file and continue the
> compile . By doing so, the logic is right and didn't change code's
> behavior. We also needn't maintain it. Even in bad case we find bugs
> hidden behind the '--CFLAGS_i40e_lan_hmc.0' warning in future, we can fix
> it after bug occurred. That's simple.
Applied for version 1.7.0.
Thanks
--
Thomas
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-06-26 12:22 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-24 5:22 [dpdk-dev] [PATCH] i40e: fix shared code compile warning Chen Jing D(Mark)
2014-06-24 8:46 ` Thomas Monjalon
2014-06-24 9:47 ` Chen, Jing D
2014-06-24 10:06 ` Thomas Monjalon
2014-06-24 14:43 ` Ananyev, Konstantin
2014-06-24 15:25 ` Zhang, Helin
2014-06-26 12:22 ` Thomas Monjalon
2014-06-25 5:36 ` Chen, Jing D
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).