[pulseaudio-discuss] Notes on pulse latest GCC 6 warnings

[Ahmed S. Darwish]

Hi :-)

Compiling pulse master with gcc v6.1.1 emits several warnings from
the `padsp' utility. I was going to send a fixing patch, but upon
further inspection most of them seems to be problems either from
GCC itself, or from its zealous interpreatation of glibc headers.

Do we want to release a v9.0 with such warnings? Your input is
appreciated...


==> GCC -Wlogical-op not probably understanding macros
------------------------------------------------------

The first warning is the following:

   utils/padsp.c: In function ‘sndstat_open’:
   utils/padsp.c:1440:9: warning: logical ‘and’ of equal expressions [-Wlogical-op]
            && flags != (O_RDONLY|O_LARGEFILE)
            ^~

and it comes from below if condition:

       if (flags != O_RDONLY
   #ifdef O_LARGEFILE
           && flags != (O_RDONLY|O_LARGEFILE)
   #endif
          )

Since O_RDONLY and O_LARGEFILE effectively have the value of
_zero_ in modern Linux systems, thus it complains because we are
effectively doing:

   if (flags != 0 && flags != 0)

Nonetheless, gcc should've been smart enough to detect we're
comparing flags with different macros. Other projects also raised
a similar concern, and this is tracked in the following GCC
bugzilla entry:

   "Over-ambitious logical-op warning on EAGAIN vs EWOULDBLOCK"
   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69602


==> GCC's tough interpreatation of __nonnull arguments
------------------------------------------------------

The second class of warnigns is the following:

   utils/padsp.c: In function ‘access’:
   utils/padsp.c:2450:8: warning: nonnull argument ‘pathname’ compared to NULL [-Wnonnull-compare]
        if (!pathname ||
           ^
   ... and similar other 7 warnings ...

Here padsp is re-implementing libc's access(3) function. GCC is
complaining because glibc's declaration of access() marks the
first string parameter with gcc's __attribute__((nonnull)):

   /* /usr/include/unistd.h */
   extern int access (const char *__name, int __type) __THROW __nonnull ((1));

The same warnings are also issued for padsp's re-implementation
of stat(), stat64(), __xstat(), etc.

This is even more strange because the GCC nonnull attribute docs
state that "The compiler may also choose to make optimizations
based on the knowledge that certain function arguments will not be
null":

   https://gcc.gnu.org/onlinedocs/gcc-6.1.0/gcc/Common-Function-Attributes.html

And this is stated to be the main reason of the new GCC 6 warning.
Namely, that GCC __will optimize away__ the NULL checks and the
main reason of this warning is to warn the developer about that:

   "GCC might optimize such checks away so warn the user when the
   function contains such comparisions."
   https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00621.html

   "So the new warning essentially says 'you promised this check
   is redundant and it will be removed'."
   https://gnu.wildebeest.org/blog/mjw/2016/02/15/looking-forward-to-gcc6-many-new-warnings/#comment-5716

So what is the suggested action for pulse here? For now, we're at
odds with the system call headers provided by glibc.


Thanks,   

-- 
Darwish
http://darwish.chasingpointers.com