[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240718131949.GQ3766212@port70.net>
Date: Thu, 18 Jul 2024 15:19:49 +0200
From: Szabolcs Nagy <nsz@...t70.net>
To: Brad House <brad@...d-house.com>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH 1/1] FD_SET() and FD_ISSET() warn on
-Wsign-conversion
* Brad House <brad@...d-house.com> [2024-07-16 10:05:06 -0400]:
> I'm the maintainer of c-ares (https://c-ares.org) and have been scanning the
> CI build logs for various systems to catch warnings, and on Alpine Linux
> (which obviously uses musl c) we always get these warnings:
>
> /tmp/cirrus-ci-build/src/lib/ares_event_select.c: In function
> 'ares_evsys_select_wait':
> /tmp/cirrus-ci-build/src/lib/ares_event_select.c:95:7: warning: conversion
> to 'long unsigned int' from 'ares_socket_t' {aka 'int'} may change the sign
> of the result [-Wsign-conversion]
> 95 | FD_SET(ev->fd, &read_fds);
> | ^~~~~~
...
> full build output:
> https://cirrus-ci.com/task/6416020481507328?logs=main#L401
>
> Sockets / File Descriptors in POSIX are defined as 'int', so it seems bad to
> have the standard C library complain when you pass an 'int' to a macro that
> should be expecting an 'int'. I know -Wsign-conversion probably isn't a
> common warning flag to test with but its part of our default set of
> warnings.
note: -Wsign-conversion is not enabled by -Wall
nor by -Wextra.
>
> I've attached a patch that will silence this warning, but otherwise not
> impact the behavior.
>
> -Brad
> diff --git a/include/sys/select.h b/include/sys/select.h
> index b3bab1d5..8982f39b 100644
> --- a/include/sys/select.h
> +++ b/include/sys/select.h
> @@ -24,9 +24,9 @@ typedef struct {
> } fd_set;
>
> #define FD_ZERO(s) do { int __i; unsigned long *__b=(s)->fds_bits; for(__i=sizeof (fd_set)/sizeof (long); __i; __i--) *__b++=0; } while(0)
> -#define FD_SET(d, s) ((s)->fds_bits[(d)/(8*sizeof(long))] |= (1UL<<((d)%(8*sizeof(long)))))
> -#define FD_CLR(d, s) ((s)->fds_bits[(d)/(8*sizeof(long))] &= ~(1UL<<((d)%(8*sizeof(long)))))
> -#define FD_ISSET(d, s) !!((s)->fds_bits[(d)/(8*sizeof(long))] & (1UL<<((d)%(8*sizeof(long)))))
> +#define FD_SET(d, s) ((s)->fds_bits[((unsigned int)d)/(8*sizeof(long))] |= (1UL<<(((unsigned int)d)%(8*sizeof(long)))))
> +#define FD_CLR(d, s) ((s)->fds_bits[((unsigned int)d)/(8*sizeof(long))] &= ~(1UL<<(((unsigned int)d)%(8*sizeof(long)))))
> +#define FD_ISSET(d, s) !!((s)->fds_bits[((unsigned int)d)/(8*sizeof(long))] & (1UL<<(((unsigned int)d)%(8*sizeof(long)))))
the macro argument must be protected with ()
e.g. the patch changes behaviour if d is 0.5?x:y
and musl prefers 'unsigned' to 'unsigned int', so
#define FD_SET(d, s) ((s)->fds_bits[(unsigned)(d)/(8*sizeof(long))] |= (1UL<<((unsigned)(d)%(8*sizeof(long)))))
note that this change can hide real bugs: now
int (d) is implicit converted to size_t and e.g
pointer types error out, but the cast suppresses
the error for pointers, not just the signedness
warning.
i think only cast can suppress the warning (at
least if we stick to standard c) and suspect
this is why the warning is not used widely: it
forces changes that make the code more buggy.
we used to argue that compilers should not warn
about system headers even if it's macro expansion
but when it's partly system header and partly
user code (d) then compilers tend to warn anyway.
i think for your project the cleanest is to wrap
FD_* and then you can do whatever.
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
