[dpdk-dev] [PATCH] i40e: fix shared code compile warning

[Zhang, Helin]

-----Original Message-----
From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Ananyev, Konstantin
Sent: Tuesday, June 24, 2014 10:44 PM
To: Thomas Monjalon; Chen, Jing D
Cc: dev at dpdk.org
Subject: Re: [dpdk-dev] [PATCH] i40e: fix shared code compile warning

Hi Thomas,

> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, June 24, 2014 11:06 AM
> To: Chen, Jing D
> Cc: dev at 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 at 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