mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] Musl's FD_{SET,ISSET,CLR} macros from sys/select.h trigger gcc's -Wsign-conversion warnings
@ 2020-08-02 18:24 Petr Skocik
  2020-08-02 18:53 ` Alexander Monakov
  0 siblings, 1 reply; 6+ messages in thread
From: Petr Skocik @ 2020-08-02 18:24 UTC (permalink / raw)
  To: musl

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [musl] Musl's FD_{SET,ISSET,CLR} macros from sys/select.h trigger gcc's -Wsign-conversion warnings
  2020-08-02 18:24 [musl] Musl's FD_{SET,ISSET,CLR} macros from sys/select.h trigger gcc's -Wsign-conversion warnings Petr Skocik
@ 2020-08-02 18:53 ` Alexander Monakov
  2020-08-02 18:56   ` Alexander Monakov
  2020-08-02 19:43   ` Petr Skocik
  0 siblings, 2 replies; 6+ messages in thread
From: Alexander Monakov @ 2020-08-02 18:53 UTC (permalink / raw)
  To: musl

[-- 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [musl] Musl's FD_{SET,ISSET,CLR} macros from sys/select.h trigger gcc's -Wsign-conversion warnings
  2020-08-02 18:53 ` Alexander Monakov
@ 2020-08-02 18:56   ` Alexander Monakov
  2020-08-02 19:43   ` Petr Skocik
  1 sibling, 0 replies; 6+ messages in thread
From: Alexander Monakov @ 2020-08-02 18:56 UTC (permalink / raw)
  To: musl

[-- 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [musl] Musl's FD_{SET,ISSET,CLR} macros from sys/select.h trigger gcc's -Wsign-conversion warnings
  2020-08-02 18:53 ` Alexander Monakov
  2020-08-02 18:56   ` Alexander Monakov
@ 2020-08-02 19:43   ` Petr Skocik
  2020-08-03  5:43     ` Markus Wichmann
  1 sibling, 1 reply; 6+ messages in thread
From: Petr Skocik @ 2020-08-02 19:43 UTC (permalink / raw)
  To: Alexander Monakov, musl

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
>
>
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [musl] Musl's FD_{SET,ISSET,CLR} macros from sys/select.h trigger gcc's -Wsign-conversion warnings
  2020-08-02 19:43   ` Petr Skocik
@ 2020-08-03  5:43     ` Markus Wichmann
  2020-08-03 17:47       ` Rich Felker
  0 siblings, 1 reply; 6+ messages in thread
From: Markus Wichmann @ 2020-08-03  5:43 UTC (permalink / raw)
  To: musl

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [musl] Musl's FD_{SET,ISSET,CLR} macros from sys/select.h trigger gcc's -Wsign-conversion warnings
  2020-08-03  5:43     ` Markus Wichmann
@ 2020-08-03 17:47       ` Rich Felker
  0 siblings, 0 replies; 6+ messages in thread
From: Rich Felker @ 2020-08-03 17:47 UTC (permalink / raw)
  To: Markus Wichmann; +Cc: musl

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-08-03 17:47 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-02 18:24 [musl] Musl's FD_{SET,ISSET,CLR} macros from sys/select.h trigger gcc's -Wsign-conversion warnings Petr Skocik
2020-08-02 18:53 ` Alexander Monakov
2020-08-02 18:56   ` Alexander Monakov
2020-08-02 19:43   ` Petr Skocik
2020-08-03  5:43     ` Markus Wichmann
2020-08-03 17:47       ` Rich Felker

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).