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

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

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