FD_{SET,ISSET,CLR} macros from sys/select.h triggerring gcc's -Wsign-conversion warnings

FD_{SET,ISSET,CLR} macros from sys/select.h triggerring gcc's -Wsign-conversion warnings

[Petr Skocik]

Example:

#include <sys/select.h>
void f(int X)
{
    fd_set set;
    FD_ZERO(&set);
    FD_SET(X,&set);
    FD_CLR(X+1,&set);
    (void)FD_ISSET(X+2,&set);
}

causes

fds.c:7:2: warning: conversion to ‘long unsigned int’ from ‘int’ may
change the sign of the result [-Wsign-conversion]
  FD_SET(X,&set);
  ^~~~~~
fds.c:7:2: warning: conversion to ‘long unsigned int’ from ‘int’ may
change the sign of the result [-Wsign-conversion]
fds.c:7:2: warning: conversion to ‘long unsigned int’ from ‘long int’
may change the sign of the result [-Wsign-conversion]
fds.c:8:2: warning: conversion to ‘long unsigned int’ from ‘int’ may
change the sign of the result [-Wsign-conversion]
  FD_CLR(X+1,&set);
  ^~~~~~
fds.c:8:2: warning: conversion to ‘long unsigned int’ from ‘int’ may
change the sign of the result [-Wsign-conversion]
fds.c:8:2: warning: conversion to ‘long unsigned int’ from ‘long int’
may change the sign of the result [-Wsign-conversion]
fds.c:9:8: warning: conversion to ‘long unsigned int’ from ‘int’ may
change the sign of the result [-Wsign-conversion]
  (void)FD_ISSET(X+2,&set);
        ^~~~~~~~
fds.c:9:8: warning: conversion to ‘long unsigned int’ from ‘int’ may
change the sign of the result [-Wsign-conversion]
fds.c:9:8: warning: conversion to ‘long unsigned int’ from ‘long int’
may change the sign of the result [-Wsign-conversion]

on gcc with -Wconversion -Wsign-conversion.


The problem is caused by the following macros:

    #  define    NFDBITS    (sizeof (fd_mask) * 8)    /* bits per mask */
    #  define    FD_SET(n, p)    ((p)->fds_bits[(n)/NFDBITS] |= (1L <<
((n) % NFDBITS)))
    #  define    FD_CLR(n, p)    ((p)->fds_bits[(n)/NFDBITS] &= ~(1L <<
((n) % NFDBITS)))
    #  define    FD_ISSET(n, p)    ((p)->fds_bits[(n)/NFDBITS] & (1L <<
((n) % NFDBITS)))

int-casting the sizeof and using 1UL instead of 1L fixes the problem:

#include <sys/select.h>

#if __CYGWIN__
    //current defs #  define    NFDBITS    (sizeof (fd_mask) * 8)    /*
bits per mask */
    #  define    NFDBITS    (sizeof (fd_mask) * 8)    /* bits per mask */
    #  define    FD_SET(n, p)    ((p)->fds_bits[(n)/NFDBITS] |= (1L <<
((n) % NFDBITS)))
    #  define    FD_CLR(n, p)    ((p)->fds_bits[(n)/NFDBITS] &= ~(1L <<
((n) % NFDBITS)))
    #  define    FD_ISSET(n, p)    ((p)->fds_bits[(n)/NFDBITS] & (1L <<
((n) % NFDBITS)))

    #if CYGWIN_FD_REDEFS
    #undef NFDBITS
    #undef FD_SET
    #undef FD_CLR
    #undef FD_ISSET

    //redefs that don't trigger gcc's  -Wsign-conversion
    #  define    NFDBITS    ((int)sizeof (fd_mask) * 8)    /* bits per
mask */
    #  define    FD_SET(n, p)    ((p)->fds_bits[(n)/NFDBITS] |= (1UL <<
((n) % NFDBITS)))
    #  define    FD_CLR(n, p)    ((p)->fds_bits[(n)/NFDBITS] &= ~(1UL <<
((n) % NFDBITS)))
    #  define    FD_ISSET(n, p)    ((p)->fds_bits[(n)/NFDBITS] & (1UL <<
((n) % NFDBITS)))
    #endif
#endif

void f(int X)
{
    fd_set set;
    FD_ZERO(&set);
    FD_SET(X,&set);
    FD_CLR(X+1,&set);
    (void)FD_ISSET(X+2,&set);
}

Regards,
Petr Skocik

Re: FD_{SET,ISSET,CLR} macros from sys/select.h triggerring gcc's -Wsign-conversion warnings

[Corinna Vinschen]

Hi Petr,

On Aug  2 20:08, Petr Skocik via Cygwin wrote:
> Example:
> 
> #include <sys/select.h>
> void f(int X)
> {
>     fd_set set;
>     FD_ZERO(&set);
>     FD_SET(X,&set);
>     FD_CLR(X+1,&set);
>     (void)FD_ISSET(X+2,&set);
> }
> 
> causes
> 
> fds.c:7:2: warning: conversion to ‘long unsigned int’ from ‘int’ may
> change the sign of the result [-Wsign-conversion]
>   FD_SET(X,&set);
>   ^~~~~~
> [...]
> on gcc with -Wconversion -Wsign-conversion.
> 
> The problem is caused by the following macros:
> 
>     #  define    NFDBITS    (sizeof (fd_mask) * 8)    /* bits per mask */
>     #  define    FD_SET(n, p)    ((p)->fds_bits[(n)/NFDBITS] |= (1L <<
> ((n) % NFDBITS)))
>     #  define    FD_CLR(n, p)    ((p)->fds_bits[(n)/NFDBITS] &= ~(1L <<
> ((n) % NFDBITS)))
>     #  define    FD_ISSET(n, p)    ((p)->fds_bits[(n)/NFDBITS] & (1L <<
> ((n) % NFDBITS)))
> 
> int-casting the sizeof and using 1UL instead of 1L fixes the problem:

Thanks.  I pushed a patch which will show up in the next Cygwin release:
https://sourceware.org/git/?p=newlib-cygwin.git;a=commitdiff;h=5717262b8ecf

Funny enough, while the GLibc version of sys/select.h does not generate
these warnings, the same problem still exists in the upstream FreeBSD
code.

For testing I uploaded a new developer snapshot to
https://cygwin.com/snapshots/  You only need to fetch the new
sys/select.h file.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

Re: FD_{SET,ISSET,CLR} macros from sys/select.h triggerring gcc's -Wsign-conversion warnings

[Petr Skocik]

Thanks for the patch, Corinna!

I also reported it to Musl which has very similar code (I guess because
of the same FreeBSD origins)
and it's been pointed out to me by Alexander Monakov that the int-cast
results in worse codegen because then the modulos/divisions need signed
division instructions and can no longer be optimized to shifts and masks
(glibc does the int-cast still).

In light of that, if I were the maintainer, I think I'd just just make
the macros into inline functions (POSIX says it's unspecified whether
they're functions or macros) and cast the filedescriptor number to
unsigned internally or something similar.

#define __fdset_mask(n)    ((__fd_mask)1 << ((unsigned)(n) % _NFDBITS))

static inline void FD_CLR(int __fd, fd_set *__fdset)
{
    __fdset->__fds_bits[((unsigned)__fd)/_NFDBITS] &= ~__fdset_mask(__fd);
}

static inline int FD_ISSET(int __fd, fd_set *__fdset)
{
    return ((__fdset->__fds_bits[(unsigned)__fd/_NFDBITS] &
__fdset_mask(n)) != 0);
}

static inline void FD_SET(int __fd, fd_set *__fdset)
{
    __fdset->__fds_bits[(unsigned)__fd/_NFDBITS] |= __fdset_mask(__fd);
}

//perhaps also:
static inline void FD_ZERO(fd_set *__fdset)
{
    memset(__fdset,0,sizeof(*__fdset)); //shorter + should have the same
codegen on gcc/clang
}

Alternatively, keeping them macros and casting the n argument to
unsigned (rather than casting sizeof to int) would preserve the better
codegen, but it would be a bit of a loss in terms of type safety.

(The issue could well be attributed to gcc, too, because, arguably, (the
implicit) -Wno-system-headers should have silenced it even without the
explicit casts.)

Anyway, sorry for all the fuss about such a negligible issue, but I
guess fixing it (in whatever way) does make
the lib a tiny bit more pleasant to use.

Regards,
Petr Skocik


On 8/3/20 12:45 PM, Corinna Vinschen wrote:
> Hi Petr,
>
> On Aug  2 20:08, Petr Skocik via Cygwin wrote:
>> Example:
>>
>> #include <sys/select.h>
>> void f(int X)
>> {
>>     fd_set set;
>>     FD_ZERO(&set);
>>     FD_SET(X,&set);
>>     FD_CLR(X+1,&set);
>>     (void)FD_ISSET(X+2,&set);
>> }
>>
>> causes
>>
>> fds.c:7:2: warning: conversion to ‘long unsigned int’ from ‘int’ may
>> change the sign of the result [-Wsign-conversion]
>>   FD_SET(X,&set);
>>   ^~~~~~
>> [...]
>> on gcc with -Wconversion -Wsign-conversion.
>>
>> The problem is caused by the following macros:
>>
>>     #  define    NFDBITS    (sizeof (fd_mask) * 8)    /* bits per mask */
>>     #  define    FD_SET(n, p)    ((p)->fds_bits[(n)/NFDBITS] |= (1L <<
>> ((n) % NFDBITS)))
>>     #  define    FD_CLR(n, p)    ((p)->fds_bits[(n)/NFDBITS] &= ~(1L <<
>> ((n) % NFDBITS)))
>>     #  define    FD_ISSET(n, p)    ((p)->fds_bits[(n)/NFDBITS] & (1L <<
>> ((n) % NFDBITS)))
>>
>> int-casting the sizeof and using 1UL instead of 1L fixes the problem:
> Thanks.  I pushed a patch which will show up in the next Cygwin release:
> https://sourceware.org/git/?p=newlib-cygwin.git;a=commitdiff;h=5717262b8ecf
>
> Funny enough, while the GLibc version of sys/select.h does not generate
> these warnings, the same problem still exists in the upstream FreeBSD
> code.
>
> For testing I uploaded a new developer snapshot to
> https://cygwin.com/snapshots/  You only need to fetch the new
> sys/select.h file.
>
>
> Thanks,
> Corinna
>

Re: FD_{SET,ISSET,CLR} macros from sys/select.h triggerring gcc's -Wsign-conversion warnings

[Corinna Vinschen]

On Aug  3 15:27, Petr Skocik via Cygwin wrote:
> Thanks for the patch, Corinna!
> 
> I also reported it to Musl which has very similar code (I guess because
> of the same FreeBSD origins)
> and it's been pointed out to me by Alexander Monakov that the int-cast
> results in worse codegen because then the modulos/divisions need signed
> division instructions and can no longer be optimized to shifts and masks
> (glibc does the int-cast still).
> 
> In light of that, if I were the maintainer, I think I'd just just make
> the macros into inline functions (POSIX says it's unspecified whether
> they're functions or macros) and cast the filedescriptor number to
> unsigned internally or something similar.

I thought about that as well, but I'd like to keep it as close to FreeBSD
as possible.  In theory, it would be even better to revert the additional
cast and just live with the gcc warnings.

> (The issue could well be attributed to gcc, too, because, arguably, (the
> implicit) -Wno-system-headers should have silenced it even without the
> explicit casts.)

I didn't realize that.  It's weird behaviour.

> Anyway, sorry for all the fuss about such a negligible issue, but I
> guess fixing it (in whatever way) does make
> the lib a tiny bit more pleasant to use.

No worries.


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer