From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id 78695AAEC for ; Tue, 3 Apr 2018 10:31:14 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DF32A4040070; Tue, 3 Apr 2018 08:31:13 +0000 (UTC) Received: from [10.36.112.46] (ovpn-112-46.ams2.redhat.com [10.36.112.46]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 80BB97C49; Tue, 3 Apr 2018 08:31:07 +0000 (UTC) To: Stephen Hemminger , Neil Horman Cc: =?UTF-8?Q?Ga=c3=abtan_Rivet?= , Tonghao Zhang , Timothy Redaelli , Andrew Rybchenko , "dev@dpdk.org" , Ferruh Yigit , Thomas Monjalon 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> <20180331184855.GA4278@neilslaptop.think-freely.org> <20180402092515.3e5ccea4@xeon-e3> From: Maxime Coquelin Message-ID: <02f1e234-e02c-332c-bf09-77584621cef1@redhat.com> Date: Tue, 3 Apr 2018 10:31:05 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180402092515.3e5ccea4@xeon-e3> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Tue, 03 Apr 2018 08:31:13 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Tue, 03 Apr 2018 08:31:13 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'maxime.coquelin@redhat.com' RCPT:'' 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: Tue, 03 Apr 2018 08:31:14 -0000 On 04/02/2018 06:25 PM, Stephen Hemminger wrote: > On Sat, 31 Mar 2018 14:48:55 -0400 > Neil Horman wrote: > >> 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 > > > Rather than wallpapering over the unused result, why not do real error checking? > If the program was run in a non-Linux environment (such as WSL etc), maybe an error > could occur. Best to return an error; or at least call rte_exit(). > Do we really want to call rte_exit() in a library?