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); } results in fds.c: In function ‘f’: fds.c:17:2: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion] 17 | FD_SET(X,&set); | ^~~~~~ fds.c:17:2: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion] fds.c:18:2: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion] 18 | FD_CLR(X+1,&set); | ^~~~~~ fds.c:18:2: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion] fds.c:19:8: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion] 19 | (void)FD_ISSET(X+2,&set); | ^~~~~~~~ fds.c:19:8: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion] on `gcc -Wconversion`. Adding an int-cast before each `sizeof` in the definition of these macros eliminates the warning: #define FD_SET(d, s) ((s)->fds_bits[(d)/(8*(int)sizeof(long))] |= (1UL<<((d)%(8*(int)sizeof(long))))) #define FD_CLR(d, s) ((s)->fds_bits[(d)/(8*(int)sizeof(long))] &= ~(1UL<<((d)%(8*(int)sizeof(long))))) #define FD_ISSET(d, s) !!((s)->fds_bits[(d)/(8*(int)sizeof(long))] & (1UL<<((d)%(8*(int)sizeof(long))))) You might want to add them. Regards, Petr Skocik
[-- Attachment #1: Type: text/plain, Size: 844 bytes --] On Sun, 2 Aug 2020, Petr Skocik wrote: > #define FD_SET(d, s) ((s)->fds_bits[(d)/(8*(int)sizeof(long))] |= > (1UL<<((d)%(8*(int)sizeof(long))))) > #define FD_CLR(d, s) ((s)->fds_bits[(d)/(8*(int)sizeof(long))] &= > ~(1UL<<((d)%(8*(int)sizeof(long))))) > #define FD_ISSET(d, s) !!((s)->fds_bits[(d)/(8*(int)sizeof(long))] & > (1UL<<((d)%(8*(int)sizeof(long))))) > > You might want to add them. (casting 'd' to size_t would have been more appropriate, as there's no need to perform signed division and modulus here) This is one of the cases where the warning should have been suppressed by GCC unless -Wsystem-headers is also given: the problem appears when expanding a macro defined in a system header, so the user can't cleanly avoid it. Would you care to open an issue in the GCC Bugzilla about it? Alexander
[-- Attachment #1: Type: text/plain, Size: 1155 bytes --] On Sun, 2 Aug 2020, Alexander Monakov wrote: > On Sun, 2 Aug 2020, Petr Skocik wrote: > > > #define FD_SET(d, s) ((s)->fds_bits[(d)/(8*(int)sizeof(long))] |= > > (1UL<<((d)%(8*(int)sizeof(long))))) > > #define FD_CLR(d, s) ((s)->fds_bits[(d)/(8*(int)sizeof(long))] &= > > ~(1UL<<((d)%(8*(int)sizeof(long))))) > > #define FD_ISSET(d, s) !!((s)->fds_bits[(d)/(8*(int)sizeof(long))] & > > (1UL<<((d)%(8*(int)sizeof(long))))) > > > > You might want to add them. > > (casting 'd' to size_t would have been more appropriate, as there's no need > to perform signed division and modulus here) > > This is one of the cases where the warning should have been suppressed by > GCC unless -Wsystem-headers is also given: the problem appears when expanding > a macro defined in a system header, so the user can't cleanly avoid it. I meant to also mention that as -Wsign-conversion is not part of -Wall -Wextra, it's relatively rarely used, so lack of suppression is probably just an oversight in the implementation, not a deliberate choice on GCC part. > Would you care to open an issue in the GCC Bugzilla about it? > > Alexander
Yeah, you're right. The signedness does make the codegen worse because of the signed division (glibc still does the (int) cast of sizeof).
A direct size_t cast of (d) could potentially do some type-unsafe things (like convert a pointer), but I guess you could
do something like `(size_t)(int){d}` or use an inline function to silence the warnings without worsening the codegen and without worsening the typesafety of the macro.
I don't super care about this in Musl as it's quite easily silencable with an `unsigned` cast from the user-side but Cygwin had the macros written in such a way that the `unsigned` cast on the descriptor argument wouldn't help, so I reported it there and also sent one bug report to Musl while I was at it, but I will the -Wsystem-headers problem to gcc.
Thanks for the suggestion, Alexander Monakov.
Regards,
Petr Skocik
On 8/2/20 8:53 PM, Alexander Monakov wrote:
> On Sun, 2 Aug 2020, Petr Skocik wrote:
>
>> #define FD_SET(d, s) ((s)->fds_bits[(d)/(8*(int)sizeof(long))] |=
>> (1UL<<((d)%(8*(int)sizeof(long)))))
>> #define FD_CLR(d, s) ((s)->fds_bits[(d)/(8*(int)sizeof(long))] &=
>> ~(1UL<<((d)%(8*(int)sizeof(long)))))
>> #define FD_ISSET(d, s) !!((s)->fds_bits[(d)/(8*(int)sizeof(long))] &
>> (1UL<<((d)%(8*(int)sizeof(long)))))
>>
>> You might want to add them.
> (casting 'd' to size_t would have been more appropriate, as there's no need
> to perform signed division and modulus here)
>
> This is one of the cases where the warning should have been suppressed by
> GCC unless -Wsystem-headers is also given: the problem appears when expanding
> a macro defined in a system header, so the user can't cleanly avoid it.
>
> Would you care to open an issue in the GCC Bugzilla about it?
>
> Alexander
>
>
>
On Sun, Aug 02, 2020 at 09:43:56PM +0200, Petr Skocik wrote:
> A direct size_t cast of (d) could potentially do some type-unsafe things (like convert a pointer),
Then just add 0ul? Pointers stay pointers, and ints become unsigned
longs. Only signed long longs remain signed, but using 0ull would turn
this into 64-bit calculations on 32-bit systems, typically requiring
libgcc calls. Also, storing FDs in long longs is a tad overkill, and
probably not the normal case.
Ciao,
Markus
On Mon, Aug 03, 2020 at 07:43:43AM +0200, Markus Wichmann wrote:
> On Sun, Aug 02, 2020 at 09:43:56PM +0200, Petr Skocik wrote:
> > A direct size_t cast of (d) could potentially do some type-unsafe things (like convert a pointer),
>
> Then just add 0ul? Pointers stay pointers, and ints become unsigned
> longs. Only signed long longs remain signed, but using 0ull would turn
> this into 64-bit calculations on 32-bit systems, typically requiring
> libgcc calls. Also, storing FDs in long longs is a tad overkill, and
> probably not the normal case.
That's no different than what the code is already doing, getting the
desired promotion via C's promotion rules. Unfortunately compilers are
wrongly complaining about it because it doesn't meet their preferred
style rules.
Rich