From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 8D05934F2 for ; Sat, 31 Mar 2018 20:49:42 +0200 (CEST) Received: from cpe-2606-a000-111b-40b7-215-ff-fecc-4872.dyn6.twc.com ([2606:a000:111b:40b7:215:ff:fecc:4872] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.63) (envelope-from ) id 1f2LZC-0000j7-9e; Sat, 31 Mar 2018 14:49:33 -0400 Date: Sat, 31 Mar 2018 14:48:55 -0400 From: Neil Horman To: =?iso-8859-1?Q?Ga=EBtan?= Rivet Cc: Tonghao Zhang , Timothy Redaelli , Maxime Coquelin , Andrew Rybchenko , "dev@dpdk.org" , Ferruh Yigit , Thomas Monjalon Message-ID: <20180331184855.GA4278@neilslaptop.think-freely.org> References: <93d00a28-d52d-25b3-42d0-84b1d95c756a@redhat.com> <20180330162830.1d7ccba2@redhat.com> <20180331133342.GA31292@neilslaptop.think-freely.org> <20180331150947.5ob35sxk2iw4f4fq@bidouze.vm.6wind.com> <20180331152755.GA3261@neilslaptop.think-freely.org> <20180331162141.nmg6awzep53fgakz@bidouze.vm.6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180331162141.nmg6awzep53fgakz@bidouze.vm.6wind.com> User-Agent: Mutt/1.9.2 (2017-12-15) X-Spam-Score: -2.9 (--) X-Spam-Status: No Subject: Re: [dpdk-dev] Build is broken in dpdk-next-net X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 31 Mar 2018 18:49:43 -0000 On Sat, Mar 31, 2018 at 06:21:41PM +0200, Gaëtan Rivet wrote: > On Sat, Mar 31, 2018 at 11:27:55AM -0400, Neil Horman wrote: > > On Sat, Mar 31, 2018 at 05:09:47PM +0200, Gaëtan Rivet wrote: > > > On Sat, Mar 31, 2018 at 09:33:43AM -0400, Neil Horman wrote: > > > > On Fri, Mar 30, 2018 at 10:47:09PM +0800, Tonghao Zhang wrote: > > > > > I rebuild it on ubuntu 17.10 and cash it. I use the 'RTE_SET_USED' to fix it. > > > > > > > > > > > > > > > diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c > > > > > index 771675718..f11803191 100644 > > > > > --- a/lib/librte_vhost/fd_man.c > > > > > +++ b/lib/librte_vhost/fd_man.c > > > > > @@ -279,7 +279,8 @@ fdset_pipe_read_cb(int readfd, void *dat __rte_unused, > > > > > int *remove __rte_unused) > > > > > { > > > > > char charbuf[16]; > > > > > - read(readfd, charbuf, sizeof(charbuf)); > > > > > + int r = read(readfd, charbuf, sizeof(charbuf)); > > > > > + RTE_SET_USED(r); > > > > > } > > > > > > > > > > void > > > > > @@ -319,5 +320,6 @@ fdset_pipe_init(struct fdset *fdset) > > > > > void > > > > > fdset_pipe_notify(struct fdset *fdset) > > > > > { > > > > > - write(fdset->u.writefd, "1", 1); > > > > > + int r = write(fdset->u.writefd, "1", 1); > > > > > + RTE_SET_USED(r); > > > > > } > > > > > > > > > > > > > A better option might be to use _Pragma > > > > > > > > Something like this perhaps > > > > > > > > #define ALLOW_UNUSED(x) \ > > > > _Pragma(push) \ > > > > _Pragma(diagnostic ignored "-Wunused-result") \ > > > > #x;\ > > > > _Pragma(pop) > > > > > > > > This is of course untested, so it probably needs some tweaking, but this method > > > > avoids the need to declare an additional stack variable, which i don't think can > > > > be eliminated due to the cast. I believe that this method should also work > > > > accross compilers (the gcc and clang compilers support this, and i think the > > > > intel compiler should as well) > > > > > > > > Neil > > > > > > > > > > It would be nice to avoid the definition of a useless variable. > > > An alternative could be > > > > > > if (read() < 0) { > > > /* Failure here is acceptable for such and such reason. */ > > > } > > > > > > to ensure all-around compatibility, and the definition or another macro. > > > Just a suggestion. > > > > > That would be a good alternative, but I think its effectiveness is dependent on > > when the compiler does with the return value check. Without any code inside the > > conditional, the compiler may optimize the check out, meaning the warning will > > still be asserted. If it doesn't optimize the check out, then you have a > > useless compare and jump instruction left in the code path. > > > > Best > > Neil > > > > I tested quickly, I see no difference with the three methods: gcc seems to be sufficiently smart to optimize out the conditional, clang not so much: #include #include #include __attribute__((warn_unused_result)) int wur(void) { printf("CALLING WUR!\n"); return read(0, NULL, 0); } #define UNUSED_RESULT(x) if (x) {} int main(void) { UNUSED_RESULT(wur()); return 0; } [nhorman@neilslaptop ~]$ gcc -g -Wunused-result -Werror ./test.c [nhorman@neilslaptop ~]$ objdump -d -S a.out > ./results [nhorman@neilslaptop ~]$ cat results ... 000000000040054b
: #define UNUSED_RESULT(x) if (x) {} int main(void) { 40054b: 55 push %rbp 40054c: 48 89 e5 mov %rsp,%rbp UNUSED_RESULT(wur()); 40054f: e8 d3 ff ff ff callq 400527 return 0; 400554: b8 00 00 00 00 mov $0x0,%eax } 400559: 5d pop %rbp 40055a: c3 retq 40055b: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1) [nhorman@neilslaptop ~]$ clang -g -Wunused-result -Werror ./test.c [nhorman@neilslaptop ~]$ objdump -d -S a.out > ./results [nhorman@neilslaptop ~]$ cat results ... 0000000000400570
: } #define UNUSED_RESULT(x) if (x) {} int main(void) { 400570: 55 push %rbp 400571: 48 89 e5 mov %rsp,%rbp 400574: 48 83 ec 10 sub $0x10,%rsp 400578: c7 45 fc 00 00 00 00 movl $0x0,-0x4(%rbp) UNUSED_RESULT(wur()); 40057f: e8 ac ff ff ff callq 400530 400584: 83 f8 00 cmp $0x0,%eax 400587: 0f 84 05 00 00 00 je 400592 40058d: e9 00 00 00 00 jmpq 400592 400592: 31 c0 xor %eax,%eax return 0; 400594: 48 83 c4 10 add $0x10,%rsp 400598: 5d pop %rbp 400599: c3 retq 40059a: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1) There is an additional compare and two jump statements there. I'm sure eventually most compilers will figure out how to eliminate this, and it might even do so now with the right optimization flags, but I think its best to just organize the source such that no conditional branching is implied. Assuming the intel compiler supports it (which I think it should, can someone with access to it confirm), the _Pragma utility is probably the most clear way to do that. Regards Neil