[dpdk-dev] Build is broken in dpdk-next-net
Checks
Commit Message
I rebuild it on ubuntu 17.10 and cash it. I use the 'RTE_SET_USED' to fix it.
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
>
Comments
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
>>
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
>>>
>
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
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.
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
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.
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
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().
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?
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.
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
> 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
@@ -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);
}