> On Apr 3, 2018, at 5:41 AM, Neil Horman <[email protected]> wrote: > > On Mon, Apr 02, 2018 at 09:25:15AM -0700, Stephen Hemminger wrote: >> On Sat, 31 Mar 2018 14:48:55 -0400 >> Neil Horman <[email protected]> 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 <stdio.h> >>> #include <stdlib.h> >>> #include <unistd.h> >>> >>> __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 <main>: >>> >>> #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 <wur> >>> 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 <main>: >>> } >>> >>> #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 <wur> >>> 400584: 83 f8 00 cmp $0x0,%eax >>> 400587: 0f 84 05 00 00 00 je 400592 <main+0x22> >>> 40058d: e9 00 00 00 00 jmpq 400592 <main+0x22> >>> 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(). >> > Thats a fair point, but I think there are legitimate situations where the > return > value of a function is really a don't care state. In those, it doesn't hurt > to > have a proscribed method of ignoring said result.
Providing a standard solution for developers to ignore a returned value is a good thing as it clearly provides the reader a hint the value should be ignored as Neil is pointing out. We need to make sure the code is readable and understandable by non-experts of DPDK. > > Neil > Regards, Keith
