DPDK patches and discussions
 help / color / mirror / Atom feed
* [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).