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

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

[Andrew Rybchenko]

Hi all,

Build is broken in dpdk-next-net for me on Ubuntu 16.04.4:

lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_read_cb’:
lib/librte_vhost/fd_man.c:284:2: error: ignoring return value of ‘read’, declared with attribute warn_unused_result [-Werror=unused-result]
   read(readfd, charbuf, sizeof(charbuf));
   ^
lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_notify’:
lib/librte_vhost/fd_man.c:324:2: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
   write(fdset->u.writefd, "1", 1);
   ^

$ gcc --version
gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609

As I understand it is broken in ad94ade905454401b705be9e06b19e54a1c78148
vhost: add pipe event for optimizing negotiating

Andrew.

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

[Maxime Coquelin]

Hi Andrew,

On 03/30/2018 03:18 PM, Andrew Rybchenko wrote:
> Hi all,
> 
> Build is broken in dpdk-next-net for me on Ubuntu 16.04.4:
> 
> lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_read_cb’:
> lib/librte_vhost/fd_man.c:284:2: error: ignoring return value of ‘read’, declared with attribute warn_unused_result [-Werror=unused-result]
>    read(readfd, charbuf, sizeof(charbuf));
>    ^
> lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_notify’:
> lib/librte_vhost/fd_man.c:324:2: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
>    write(fdset->u.writefd, "1", 1);
>    ^
> 
> $ gcc --version
> gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609

Thanks for reporting, I don't catch it with my Fedora build system:
$ gcc --version
gcc (GCC) 7.3.1 20180303 (Red Hat 7.3.1-5)

> 
> As I understand it is broken in ad94ade905454401b705be9e06b19e54a1c78148
> vhost: add pipe event for optimizing negotiating

I confirm it is this patch.

Xiangxia, can you provide a fix please?

Ferruh, how do you want to proceed?
Would you revert it?

Thanks,
Maxime
> Andrew.
> 

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

[Ferruh Yigit]

On 3/30/2018 2:27 PM, Maxime Coquelin wrote:
> Hi Andrew,
> 
> On 03/30/2018 03:18 PM, Andrew Rybchenko wrote:
>> Hi all,
>>
>> Build is broken in dpdk-next-net for me on Ubuntu 16.04.4:
>>
>> lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_read_cb’:
>> lib/librte_vhost/fd_man.c:284:2: error: ignoring return value of ‘read’, declared with attribute warn_unused_result [-Werror=unused-result]
>>    read(readfd, charbuf, sizeof(charbuf));
>>    ^
>> lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_notify’:
>> lib/librte_vhost/fd_man.c:324:2: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
>>    write(fdset->u.writefd, "1", 1);
>>    ^
>>
>> $ gcc --version
>> gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
> 
> Thanks for reporting, I don't catch it with my Fedora build system:
> $ gcc --version
> gcc (GCC) 7.3.1 20180303 (Red Hat 7.3.1-5)
> 
>>
>> As I understand it is broken in ad94ade905454401b705be9e06b19e54a1c78148
>> vhost: add pipe event for optimizing negotiating
> 
> I confirm it is this patch.
> 
> Xiangxia, can you provide a fix please?
> 
> Ferruh, how do you want to proceed?
> Would you revert it?

next-net not pulled yet, lets try to fix first, at last resort if there is no
solution we can think about revert

> 
> Thanks,
> Maxime
>> Andrew.
>>

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

[Tonghao Zhang]

On Fri, Mar 30, 2018 at 10:13 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 3/30/2018 2:27 PM, Maxime Coquelin wrote:
>> Hi Andrew,
>>
>> On 03/30/2018 03:18 PM, Andrew Rybchenko wrote:
>>> Hi all,
>>>
>>> Build is broken in dpdk-next-net for me on Ubuntu 16.04.4:
>>>
>>> lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_read_cb’:
>>> lib/librte_vhost/fd_man.c:284:2: error: ignoring return value of ‘read’, declared with attribute warn_unused_result [-Werror=unused-result]
>>>    read(readfd, charbuf, sizeof(charbuf));
>>>    ^
>>> lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_notify’:
>>> lib/librte_vhost/fd_man.c:324:2: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
>>>    write(fdset->u.writefd, "1", 1);
>>>    ^
>>>
>>> $ gcc --version
>>> gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
>>
>> Thanks for reporting, I don't catch it with my Fedora build system:
>> $ gcc --version
>> gcc (GCC) 7.3.1 20180303 (Red Hat 7.3.1-5)
>>
>>>
>>> As I understand it is broken in ad94ade905454401b705be9e06b19e54a1c78148
>>> vhost: add pipe event for optimizing negotiating
>>
>> I confirm it is this patch.
>>
>> Xiangxia, can you provide a fix please?
>>
>> Ferruh, how do you want to proceed?
>> Would you revert it?
>
> next-net not pulled yet, lets try to fix first, at last resort if there is no
> solution we can think about revert
I will fix it soon.

>>
>> Thanks,
>> Maxime
>>> Andrew.
>>>
>

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

[Maxime Coquelin]

On 03/30/2018 04:16 PM, Tonghao Zhang wrote:
> On Fri, Mar 30, 2018 at 10:13 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>> On 3/30/2018 2:27 PM, Maxime Coquelin wrote:
>>> Hi Andrew,
>>>
>>> On 03/30/2018 03:18 PM, Andrew Rybchenko wrote:
>>>> Hi all,
>>>>
>>>> Build is broken in dpdk-next-net for me on Ubuntu 16.04.4:
>>>>
>>>> lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_read_cb’:
>>>> lib/librte_vhost/fd_man.c:284:2: error: ignoring return value of ‘read’, declared with attribute warn_unused_result [-Werror=unused-result]
>>>>     read(readfd, charbuf, sizeof(charbuf));
>>>>     ^
>>>> lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_notify’:
>>>> lib/librte_vhost/fd_man.c:324:2: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
>>>>     write(fdset->u.writefd, "1", 1);
>>>>     ^
>>>>
>>>> $ gcc --version
>>>> gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
>>>
>>> Thanks for reporting, I don't catch it with my Fedora build system:
>>> $ gcc --version
>>> gcc (GCC) 7.3.1 20180303 (Red Hat 7.3.1-5)
>>>
>>>>
>>>> As I understand it is broken in ad94ade905454401b705be9e06b19e54a1c78148
>>>> vhost: add pipe event for optimizing negotiating
>>>
>>> I confirm it is this patch.
>>>
>>> Xiangxia, can you provide a fix please?
>>>
>>> Ferruh, how do you want to proceed?
>>> Would you revert it?
>>
>> next-net not pulled yet, lets try to fix first, at last resort if there is no
>> solution we can think about revert
> I will fix it soon.

Thanks! FYI, casting to void have been tried by Andrew but does not
work. See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425

Cheers,
Maxime

>>>
>>> Thanks,
>>> Maxime
>>>> Andrew.
>>>>
>>

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

[Timothy Redaelli]

On Fri, 30 Mar 2018 15:27:55 +0200
Maxime Coquelin <maxime.coquelin@redhat.com> wrote:

> Hi Andrew,
> 
> On 03/30/2018 03:18 PM, Andrew Rybchenko wrote:
> > Hi all,
> > 
> > Build is broken in dpdk-next-net for me on Ubuntu 16.04.4:
> > 
> > lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_read_cb’:
> > lib/librte_vhost/fd_man.c:284:2: error: ignoring return value of
> > ‘read’, declared with attribute warn_unused_result
> > [-Werror=unused-result] read(readfd, charbuf, sizeof(charbuf)); ^
> > lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_notify’:
> > lib/librte_vhost/fd_man.c:324:2: error: ignoring return value of
> > ‘write’, declared with attribute warn_unused_result
> > [-Werror=unused-result] write(fdset->u.writefd, "1", 1); ^
> > 
> > $ gcc --version
> > gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609  
> 
> Thanks for reporting, I don't catch it with my Fedora build system:
> $ gcc --version
> gcc (GCC) 7.3.1 20180303 (Red Hat 7.3.1-5)

Those kind of warnings are enabled by "-D_FORTIFY_SOURCE=2" (enabled by
default in Ubuntu [1]).

To have the same behavior on Fedora you need to set -D_FORTIFY_SOURCE=2
by hand, for example with:

# make EXTRA_CFLAGS="-D_FORTIFY_SOURCE=2"

Usually I build dpdk with the flags we also use when we build the
Fedora/RHEL package. To do that I have in my .bashrc (all in one line):

export EXTRA_CFLAGS="-O2 -g -pipe -Werror=format-security
-Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong
--param=ssp-buffer-size=4 -grecord-gcc-switches
-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic
-fasynchronous-unwind-tables -Wformat"

> > 
> > As I understand it is broken in
> > ad94ade905454401b705be9e06b19e54a1c78148 vhost: add pipe event for
> > optimizing negotiating  
> 
> I confirm it is this patch.
> 
> Xiangxia, can you provide a fix please?
> 
> Ferruh, how do you want to proceed?
> Would you revert it?
> 
> Thanks,
> Maxime
> > Andrew.
> >   

[1]
https://wiki.ubuntu.com/ToolChain/CompilerFlags#A-D_FORTIFY_SOURCE.3D2

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

[Tonghao Zhang]

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);
 }

On Fri, Mar 30, 2018 at 10:28 PM, Timothy Redaelli <tredaelli@redhat.com> wrote:
> On Fri, 30 Mar 2018 15:27:55 +0200
> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
>
>> Hi Andrew,
>>
>> On 03/30/2018 03:18 PM, Andrew Rybchenko wrote:
>> > Hi all,
>> >
>> > Build is broken in dpdk-next-net for me on Ubuntu 16.04.4:
>> >
>> > lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_read_cb’:
>> > lib/librte_vhost/fd_man.c:284:2: error: ignoring return value of
>> > ‘read’, declared with attribute warn_unused_result
>> > [-Werror=unused-result] read(readfd, charbuf, sizeof(charbuf)); ^
>> > lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_notify’:
>> > lib/librte_vhost/fd_man.c:324:2: error: ignoring return value of
>> > ‘write’, declared with attribute warn_unused_result
>> > [-Werror=unused-result] write(fdset->u.writefd, "1", 1); ^
>> >
>> > $ gcc --version
>> > gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
>>
>> Thanks for reporting, I don't catch it with my Fedora build system:
>> $ gcc --version
>> gcc (GCC) 7.3.1 20180303 (Red Hat 7.3.1-5)
>
> Those kind of warnings are enabled by "-D_FORTIFY_SOURCE=2" (enabled by
> default in Ubuntu [1]).
>
> To have the same behavior on Fedora you need to set -D_FORTIFY_SOURCE=2
> by hand, for example with:
>
> # make EXTRA_CFLAGS="-D_FORTIFY_SOURCE=2"
>
> Usually I build dpdk with the flags we also use when we build the
> Fedora/RHEL package. To do that I have in my .bashrc (all in one line):
>
> export EXTRA_CFLAGS="-O2 -g -pipe -Werror=format-security
> -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong
> --param=ssp-buffer-size=4 -grecord-gcc-switches
> -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic
> -fasynchronous-unwind-tables -Wformat"
>
>> >
>> > As I understand it is broken in
>> > ad94ade905454401b705be9e06b19e54a1c78148 vhost: add pipe event for
>> > optimizing negotiating
>>
>> I confirm it is this patch.
>>
>> Xiangxia, can you provide a fix please?
>>
>> Ferruh, how do you want to proceed?
>> Would you revert it?
>>
>> Thanks,
>> Maxime
>> > Andrew.
>> >
>
> [1]
> https://wiki.ubuntu.com/ToolChain/CompilerFlags#A-D_FORTIFY_SOURCE.3D2
>

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

[Maxime Coquelin]

On 03/30/2018 04:28 PM, Timothy Redaelli wrote:
> On Fri, 30 Mar 2018 15:27:55 +0200
> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
> 
>> Hi Andrew,
>>
>> On 03/30/2018 03:18 PM, Andrew Rybchenko wrote:
>>> Hi all,
>>>
>>> Build is broken in dpdk-next-net for me on Ubuntu 16.04.4:
>>>
>>> lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_read_cb’:
>>> lib/librte_vhost/fd_man.c:284:2: error: ignoring return value of
>>> ‘read’, declared with attribute warn_unused_result
>>> [-Werror=unused-result] read(readfd, charbuf, sizeof(charbuf)); ^
>>> lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_notify’:
>>> lib/librte_vhost/fd_man.c:324:2: error: ignoring return value of
>>> ‘write’, declared with attribute warn_unused_result
>>> [-Werror=unused-result] write(fdset->u.writefd, "1", 1); ^
>>>
>>> $ gcc --version
>>> gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
>>
>> Thanks for reporting, I don't catch it with my Fedora build system:
>> $ gcc --version
>> gcc (GCC) 7.3.1 20180303 (Red Hat 7.3.1-5)
> 
> Those kind of warnings are enabled by "-D_FORTIFY_SOURCE=2" (enabled by
> default in Ubuntu [1]).
> 
> To have the same behavior on Fedora you need to set -D_FORTIFY_SOURCE=2
> by hand, for example with:
> 
> # make EXTRA_CFLAGS="-D_FORTIFY_SOURCE=2"
> 
> Usually I build dpdk with the flags we also use when we build the
> Fedora/RHEL package. To do that I have in my .bashrc (all in one line):
> 
> export EXTRA_CFLAGS="-O2 -g -pipe -Werror=format-security
> -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong
> --param=ssp-buffer-size=4 -grecord-gcc-switches
> -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic
> -fasynchronous-unwind-tables -Wformat"

Thanks for the hint! I indeed reproduce with -D_FORTIFY_SOURCE=2
Maxime

>>>
>>> As I understand it is broken in
>>> ad94ade905454401b705be9e06b19e54a1c78148 vhost: add pipe event for
>>> optimizing negotiating
>>
>> I confirm it is this patch.
>>
>> Xiangxia, can you provide a fix please?
>>
>> Ferruh, how do you want to proceed?
>> Would you revert it?
>>
>> Thanks,
>> Maxime
>>> Andrew.
>>>    
> 
> [1]
> https://wiki.ubuntu.com/ToolChain/CompilerFlags#A-D_FORTIFY_SOURCE.3D2
> 

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

[Maxime Coquelin]

Thanks Xiangxia,

I can post it, do you agree if I add below comments?

On 03/30/2018 04:47 PM, 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));
/* Just an optimization, we don't care if read() failed so ignore 
explicitly its return value to make the compiler happy */
> +       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);
/* Just an optimization, we don't care if write() failed so ignore 
explicitly its return value to make the compiler happy */
> +       int r = write(fdset->u.writefd, "1", 1);
> +       RTE_SET_USED(r);
>   }
> 
> On Fri, Mar 30, 2018 at 10:28 PM, Timothy Redaelli <tredaelli@redhat.com> wrote:
>> On Fri, 30 Mar 2018 15:27:55 +0200
>> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
>>
>>> Hi Andrew,
>>>
>>> On 03/30/2018 03:18 PM, Andrew Rybchenko wrote:
>>>> Hi all,
>>>>
>>>> Build is broken in dpdk-next-net for me on Ubuntu 16.04.4:
>>>>
>>>> lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_read_cb’:
>>>> lib/librte_vhost/fd_man.c:284:2: error: ignoring return value of
>>>> ‘read’, declared with attribute warn_unused_result
>>>> [-Werror=unused-result] read(readfd, charbuf, sizeof(charbuf)); ^
>>>> lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_notify’:
>>>> lib/librte_vhost/fd_man.c:324:2: error: ignoring return value of
>>>> ‘write’, declared with attribute warn_unused_result
>>>> [-Werror=unused-result] write(fdset->u.writefd, "1", 1); ^
>>>>
>>>> $ gcc --version
>>>> gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
>>>
>>> Thanks for reporting, I don't catch it with my Fedora build system:
>>> $ gcc --version
>>> gcc (GCC) 7.3.1 20180303 (Red Hat 7.3.1-5)
>>
>> Those kind of warnings are enabled by "-D_FORTIFY_SOURCE=2" (enabled by
>> default in Ubuntu [1]).
>>
>> To have the same behavior on Fedora you need to set -D_FORTIFY_SOURCE=2
>> by hand, for example with:
>>
>> # make EXTRA_CFLAGS="-D_FORTIFY_SOURCE=2"
>>
>> Usually I build dpdk with the flags we also use when we build the
>> Fedora/RHEL package. To do that I have in my .bashrc (all in one line):
>>
>> export EXTRA_CFLAGS="-O2 -g -pipe -Werror=format-security
>> -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong
>> --param=ssp-buffer-size=4 -grecord-gcc-switches
>> -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic
>> -fasynchronous-unwind-tables -Wformat"
>>
>>>>
>>>> As I understand it is broken in
>>>> ad94ade905454401b705be9e06b19e54a1c78148 vhost: add pipe event for
>>>> optimizing negotiating
>>>
>>> I confirm it is this patch.
>>>
>>> Xiangxia, can you provide a fix please?
>>>
>>> Ferruh, how do you want to proceed?
>>> Would you revert it?
>>>
>>> Thanks,
>>> Maxime
>>>> Andrew.
>>>>
>>
>> [1]
>> https://wiki.ubuntu.com/ToolChain/CompilerFlags#A-D_FORTIFY_SOURCE.3D2
>>

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

[Tonghao Zhang]

On Fri, Mar 30, 2018 at 10:59 PM, Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> Thanks Xiangxia,
>
> I can post it, do you agree if I add below comments?
yes, thanks.

> On 03/30/2018 04:47 PM, 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));
>
> /* Just an optimization, we don't care if read() failed so ignore explicitly
> its return value to make the compiler happy */
>>
>> +       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);
>
> /* Just an optimization, we don't care if write() failed so ignore
> explicitly its return value to make the compiler happy */
>
>> +       int r = write(fdset->u.writefd, "1", 1);
>> +       RTE_SET_USED(r);
>>   }
>>
>> On Fri, Mar 30, 2018 at 10:28 PM, Timothy Redaelli <tredaelli@redhat.com>
>> wrote:
>>>
>>> On Fri, 30 Mar 2018 15:27:55 +0200
>>> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
>>>
>>>> Hi Andrew,
>>>>
>>>> On 03/30/2018 03:18 PM, Andrew Rybchenko wrote:
>>>>>
>>>>> Hi all,
>>>>>
>>>>> Build is broken in dpdk-next-net for me on Ubuntu 16.04.4:
>>>>>
>>>>> lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_read_cb’:
>>>>> lib/librte_vhost/fd_man.c:284:2: error: ignoring return value of
>>>>> ‘read’, declared with attribute warn_unused_result
>>>>> [-Werror=unused-result] read(readfd, charbuf, sizeof(charbuf)); ^
>>>>> lib/librte_vhost/fd_man.c: In function ‘fdset_pipe_notify’:
>>>>> lib/librte_vhost/fd_man.c:324:2: error: ignoring return value of
>>>>> ‘write’, declared with attribute warn_unused_result
>>>>> [-Werror=unused-result] write(fdset->u.writefd, "1", 1); ^
>>>>>
>>>>> $ gcc --version
>>>>> gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
>>>>
>>>>
>>>> Thanks for reporting, I don't catch it with my Fedora build system:
>>>> $ gcc --version
>>>> gcc (GCC) 7.3.1 20180303 (Red Hat 7.3.1-5)
>>>
>>>
>>> Those kind of warnings are enabled by "-D_FORTIFY_SOURCE=2" (enabled by
>>> default in Ubuntu [1]).
>>>
>>> To have the same behavior on Fedora you need to set -D_FORTIFY_SOURCE=2
>>> by hand, for example with:
>>>
>>> # make EXTRA_CFLAGS="-D_FORTIFY_SOURCE=2"
>>>
>>> Usually I build dpdk with the flags we also use when we build the
>>> Fedora/RHEL package. To do that I have in my .bashrc (all in one line):
>>>
>>> export EXTRA_CFLAGS="-O2 -g -pipe -Werror=format-security
>>> -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong
>>> --param=ssp-buffer-size=4 -grecord-gcc-switches
>>> -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic
>>> -fasynchronous-unwind-tables -Wformat"
>>>
>>>>>
>>>>> As I understand it is broken in
>>>>> ad94ade905454401b705be9e06b19e54a1c78148 vhost: add pipe event for
>>>>> optimizing negotiating
>>>>
>>>>
>>>> I confirm it is this patch.
>>>>
>>>> Xiangxia, can you provide a fix please?
>>>>
>>>> Ferruh, how do you want to proceed?
>>>> Would you revert it?
>>>>
>>>> Thanks,
>>>> Maxime
>>>>>
>>>>> Andrew.
>>>>>
>>>
>>> [1]
>>> https://wiki.ubuntu.com/ToolChain/CompilerFlags#A-D_FORTIFY_SOURCE.3D2
>>>
>

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

[Neil Horman]

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

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

[Gaëtan Rivet]

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.

-- 
Gaëtan Rivet
6WIND

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

[Neil Horman]

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

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

[Gaëtan Rivet]

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:

#include <unistd.h>

__attribute__((warn_unused_result))
int wur(void)
{
        return read(0, NULL, 0);
}

void with_void(void)
{
        int ret;

        ret = wur();
        (void) ret;
}

void with_pragma(void)
{
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wunused-result"
        wur();
#pragma GCC diagnostic pop
}

void with_if(void)
{
        if (wur() < 0) {
                /* I do not care for errors. */
        }
}

int main(void)
{
        return 0;
}

----------------------------

gcc -D_FORTIFY_SOURCE=2 -Wunused-result -Werror -O3 _wur.c -o wur_O3.out
gcc -D_FORTIFY_SOURCE=2 -Wunused-result -Werror -O0 _wur.c -o wur_O0.out
diff -Napy <(gdb -batch -ex 'file ./wur_O0.out' -ex 'disassemble with_void') <(gdb -batch -ex 'file ./wur_O3.out' -ex 'disassemble with_void') || true
Dump of assembler code for function with_void:                  Dump of assembler code for function with_void:
   0x00000000000006ca <+0>:     push   %rbp                   |    0x00000000000006e0 <+0>:     xor    %edx,%edx
   0x00000000000006cb <+1>:     mov    %rsp,%rbp              |    0x00000000000006e2 <+2>:     xor    %esi,%esi
   0x00000000000006ce <+4>:     sub    $0x10,%rsp             |    0x00000000000006e4 <+4>:     xor    %edi,%edi
   0x00000000000006d2 <+8>:     callq  0x6b0 <wur>            |    0x00000000000006e6 <+6>:     jmpq   0x560 <read@plt>
   0x00000000000006d7 <+13>:    mov    %eax,-0x4(%rbp)        <
   0x00000000000006da <+16>:    nop                           <
   0x00000000000006db <+17>:    leaveq                        <
   0x00000000000006dc <+18>:    retq                          <
End of assembler dump.                                          End of assembler dump.
diff -Napy <(gdb -batch -ex 'file ./wur_O0.out' -ex 'disassemble with_pragma') <(gdb -batch -ex 'file ./wur_O3.out' -ex 'disassemble with_pragma') || true
Dump of assembler code for function with_pragma:                Dump of assembler code for function with_pragma:
   0x00000000000006dd <+0>:     push   %rbp                   |    0x00000000000006f0 <+0>:     xor    %edx,%edx
   0x00000000000006de <+1>:     mov    %rsp,%rbp              |    0x00000000000006f2 <+2>:     xor    %esi,%esi
   0x00000000000006e1 <+4>:     callq  0x6b0 <wur>            |    0x00000000000006f4 <+4>:     xor    %edi,%edi
   0x00000000000006e6 <+9>:     nop                           |    0x00000000000006f6 <+6>:     jmpq   0x560 <read@plt>
   0x00000000000006e7 <+10>:    pop    %rbp                   <
   0x00000000000006e8 <+11>:    retq                          <
End of assembler dump.                                          End of assembler dump.
diff -Napy <(gdb -batch -ex 'file ./wur_O0.out' -ex 'disassemble with_if') <(gdb -batch -ex 'file ./wur_O3.out' -ex 'disassemble with_if') || true
Dump of assembler code for function with_if:                    Dump of assembler code for function with_if:
   0x00000000000006e9 <+0>:     push   %rbp                   |    0x0000000000000700 <+0>:     xor    %edx,%edx
   0x00000000000006ea <+1>:     mov    %rsp,%rbp              |    0x0000000000000702 <+2>:     xor    %esi,%esi
   0x00000000000006ed <+4>:     callq  0x6b0 <wur>            |    0x0000000000000704 <+4>:     xor    %edi,%edi
   0x00000000000006f2 <+9>:     nop                           |    0x0000000000000706 <+6>:     jmpq   0x560 <read@plt>
   0x00000000000006f3 <+10>:    pop    %rbp                   <
   0x00000000000006f4 <+11>:    retq                          <
End of assembler dump.                                          End of assembler dump.

And then with

__attribute__((warn_unused_result))
int wur(void)
{
        return -1;
}

-----------------------------------

gcc -D_FORTIFY_SOURCE=2 -Wunused-result -Werror -O3 _wur.c -o wur_O3.out
gcc -D_FORTIFY_SOURCE=2 -Wunused-result -Werror -O0 _wur.c -o wur_O0.out
diff -Napy <(gdb -batch -ex 'file ./wur_O0.out' -ex 'disassemble with_void') <(gdb -batch -ex 'file ./wur_O3.out' -ex 'disassemble with_void') || true
Dump of assembler code for function with_void:                  Dump of assembler code for function with_void:
   0x000000000000066b <+0>:     push   %rbp                   |    0x0000000000000680 <+0>:     repz retq
   0x000000000000066c <+1>:     mov    %rsp,%rbp              <
   0x000000000000066f <+4>:     sub    $0x10,%rsp             <
   0x0000000000000673 <+8>:     callq  0x660 <wur>            <
   0x0000000000000678 <+13>:    mov    %eax,-0x4(%rbp)        <
   0x000000000000067b <+16>:    nop                           <
   0x000000000000067c <+17>:    leaveq                        <
   0x000000000000067d <+18>:    retq                          <
End of assembler dump.                                          End of assembler dump.
diff -Napy <(gdb -batch -ex 'file ./wur_O0.out' -ex 'disassemble with_pragma') <(gdb -batch -ex 'file ./wur_O3.out' -ex 'disassemble with_pragma') || true
Dump of assembler code for function with_pragma:                Dump of assembler code for function with_pragma:
   0x000000000000067e <+0>:     push   %rbp                   |    0x0000000000000690 <+0>:     repz retq
   0x000000000000067f <+1>:     mov    %rsp,%rbp              <
   0x0000000000000682 <+4>:     callq  0x660 <wur>            <
   0x0000000000000687 <+9>:     nop                           <
   0x0000000000000688 <+10>:    pop    %rbp                   <
   0x0000000000000689 <+11>:    retq                          <
End of assembler dump.                                          End of assembler dump.
diff -Napy <(gdb -batch -ex 'file ./wur_O0.out' -ex 'disassemble with_if') <(gdb -batch -ex 'file ./wur_O3.out' -ex 'disassemble with_if') || true
Dump of assembler code for function with_if:                    Dump of assembler code for function with_if:
   0x000000000000068a <+0>:     push   %rbp                   |    0x00000000000006a0 <+0>:     repz retq
   0x000000000000068b <+1>:     mov    %rsp,%rbp              <
   0x000000000000068e <+4>:     callq  0x660 <wur>            <
   0x0000000000000693 <+9>:     nop                           <
   0x0000000000000694 <+10>:    pop    %rbp                   <
   0x0000000000000695 <+11>:    retq                          <
End of assembler dump.                                          End of assembler dump.

There is a slight difference with the unoptimized version, otherwise any
solution should work. We can also note that in -O3, having the
additional variable on the stack makes no difference.

-- 
Gaëtan Rivet
6WIND

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

[Neil Horman]

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

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

[Stephen Hemminger]

On Sat, 31 Mar 2018 14:48:55 -0400
Neil Horman <nhorman@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@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().

Re: [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@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@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().
> 

Do we really want to call rte_exit() in a library?

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

[Thomas Monjalon]

03/04/2018 10:31, Maxime Coquelin:
> On 04/02/2018 06:25 PM, Stephen Hemminger wrote:
> > 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?

No, we must use return.

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

[Neil Horman]

On Mon, Apr 02, 2018 at 09:25:15AM -0700, Stephen Hemminger wrote:
> On Sat, 31 Mar 2018 14:48:55 -0400
> Neil Horman <nhorman@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@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.

Neil

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

[Wiles, Keith]

> On Apr 3, 2018, at 5:41 AM, Neil Horman <nhorman@tuxdriver.com> 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 <nhorman@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@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