DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 966DF3858D35
Subject: Re: FD_{SET,ISSET,CLR} macros from sys/select.h triggerring gcc's
 -Wsign-conversion warnings
To: cygwin@cygwin.com
References: <1f188b7e-6dc4-73af-e458-013760210469@gmail.com>
 <20200803104552.GJ460314@calimero.vinschen.de>
Message-ID: <92f5646d-2a24-e3a4-f3e1-934416335e8d@gmail.com>
Date: Mon, 3 Aug 2020 15:27:11 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20200803104552.GJ460314@calimero.vinschen.de>
Content-Language: en-US
Precedence: list
From: Petr Skocik via Cygwin <cygwin@cygwin.com>
Reply-To: Petr Skocik <pskocik@gmail.com>
Content-Type: text/plain; charset="utf-8"
Errors-To: cygwin-bounces@cygwin.com
Sender: "Cygwin" <cygwin-bounces@cygwin.com>
Content-Transfer-Encoding: 8bit

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
>

--
Problem reports:      https://cygwin.com/problems.html
FAQ:                  https://cygwin.com/faq/
Documentation:        https://cygwin.com/docs.html
Unsubscribe info:     https://cygwin.com/ml/#unsubscribe-simple