From: "Zhang, Helin" <helin.zhang@intel.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
Thomas Monjalon <thomas.monjalon@6wind.com>,
"Chen, Jing D" <jing.d.chen@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] i40e: fix shared code compile warning
Date: Tue, 24 Jun 2014 15:25:47 +0000 [thread overview]
Message-ID: <F35DEAC7BCE34641BA9FAC6BCA4A12E70A746859@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <2601191342CEEE43887BDE71AB97725821333FA3@IRSMSX105.ger.corp.intel.com>
-----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
next prev parent reply other threads:[~2014-06-24 15:25 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-24 5:22 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 [this message]
2014-06-26 12:22 ` Thomas Monjalon
2014-06-25 5:36 ` Chen, Jing D
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=F35DEAC7BCE34641BA9FAC6BCA4A12E70A746859@SHSMSX104.ccr.corp.intel.com \
--to=helin.zhang@intel.com \
--cc=dev@dpdk.org \
--cc=jing.d.chen@intel.com \
--cc=konstantin.ananyev@intel.com \
--cc=thomas.monjalon@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).