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

[Arun Raghavan]

On Sat, 18 Jun 2016, at 07:07 PM, Ahmed S. Darwish wrote:
> 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.

I don't think I want to try to fix these for 9.0 (which I hope to roll
out today). GCC 6 does seem to have become a bit overzealous, so I'd
rather see if that changes by 10.0.

I'm interested to hear packagers' opinions about this too, though.

-- Arun