[dpdk-dev] Build is broken in dpdk-next-net
Maxime Coquelin
maxime.coquelin at redhat.com
Tue Apr 3 10:31:05 CEST 2018
On 04/02/2018 06:25 PM, Stephen Hemminger wrote:
> On Sat, 31 Mar 2018 14:48:55 -0400
> Neil Horman <nhorman at tuxdriver.com> 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 at neilslaptop ~]$ gcc -g -Wunused-result -Werror ./test.c
>> [nhorman at neilslaptop ~]$ objdump -d -S a.out > ./results
>> [nhorman at 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 at neilslaptop ~]$ clang -g -Wunused-result -Werror ./test.c
>> [nhorman at neilslaptop ~]$ objdump -d -S a.out > ./results
>> [nhorman at 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().
>
Do we really want to call rte_exit() in a library?
More information about the dev
mailing list