[dpdk-dev] Build is broken in dpdk-next-net

[Maxime Coquelin]

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?