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

[Thomas Monjalon <thomas.monjalon@6wind.com>]

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