[musl] Sign conversion warning in FD_ISSET()

[musl] Sign conversion warning in FD_ISSET()

[David Steele]

Greetings,

I'm getting a warning when compiling on Alpine 3.21 with musl libc 1.2.5:

# /lib/libc.musl-aarch64.so.1
musl libc (aarch64)
Version 1.2.5

The code looks like this (simplified for this example, see original at 
(https://github.com/pgbackrest/pgbackrest/blob/release/2.55.1/src/protocol/parallel.c#L163):

int fd = getFdFunc();

if (FD_ISSET(fd, &selectSet))

And I get this warning when -Wsign-conversion is enabled on gcc:

../../pgbackrest/src/protocol/parallel.c:164:25: error: conversion to 
'long unsigned int' from 'int' may change the sign of the result 
[-Werror=sign-conversion]
   164 |                     if (FD_ISSET(fd, &selectSet))

Here's the cc output from ninja:

[3/4] cc -Isrc/pgbackrest.p -Isrc -I../../pgbackrest/src 
-Isrc/command/help -Isrc/postgres -I/usr/include/postgresql 
-I/usr/include/libxml2 -fdiagnostics-color=always -DNDEBUG 
-D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Werror -std=c99 -O3 
-Wcast-qual -Wconversion -Wduplicated-cond -Wduplicated-branches 
-Wformat=2 -Wformat-overflow=2 -Wformat-signedness -Winit-self 
-Wmissing-prototypes -Wmissing-variable-declarations -Wpointer-arith 
-Wredundant-decls -Wstrict-prototypes -Wvla -Wwrite-strings 
-Wno-clobbered -Wno-missing-field-initializers -funroll-loops 
-ftree-vectorize -fmacro-prefix-map=../../pgbackrest/src/= 
-fmacro-prefix-map=../../pgbackrest/test/src/=test/ 
-D_POSIX_C_SOURCE=200809L -MD -MQ src/pgbackrest.p/protocol_parallel.c.o 
-MF src/pgbackrest.p/protocol_parallel.c.o.d -o 
src/pgbackrest.p/protocol_parallel.c.o -c 
../../pgbackrest/src/protocol/parallel.c

I can fix this by casting to long unsigned int, but it seems like 
FD_ISSET() should accept int without complaint like the associate macros 
and functions.

Please CC me on any response since I am not subscribed to the mailing list.

Thanks,
-David

Re: [musl] Sign conversion warning in FD_ISSET()

[Rich Felker]

On Fri, Jun 06, 2025 at 12:17:19AM +0200, Thorsten Glaser wrote:
> On Thu, 5 Jun 2025, Rich Felker wrote:
> 
> >In general, it looks like -Wsign-conversion is a very badly-behaved
> >warning that's not interacting right with the normal logic to suppress
> >warnings from system headers.
> 
> I thought so at first, too. But this is not code that is contained
> wholly in the system header: the warning thinks the macro must be
> called with an unsigned type, but the user calls it with signed int,
> so it triggers.

But that's what I'm saying they're doing wrong. Warnings from
*anything* in the expansion of the macro should be suppressed. It
should not be trying to infer a correct type and warning for mismatch.

Rich

Re: [musl] Sign conversion warning in FD_ISSET()

[Rich Felker]

On Thu, Jun 05, 2025 at 10:26:21PM +0000, David Steele wrote:
> On 6/5/25 17:50, Rich Felker wrote:
> > On Thu, Jun 05, 2025 at 09:42:27PM +0000, David Steele wrote:
> > > On 5/23/25 12:07, Jeffrey Walton wrote:
> > > > For some background, see the discussion at
> > > > <https://www.openwall.com/lists/musl/2024/07/16/1>.
> > > 
> > > Well, OK, but I must say that "this is a bad warning" is not a very
> > > satisfying answer. I get that casting a pointer to int would be bad but is
> > > having the compiler do a silent implicit conversion better? To be clear, in
> > > this case I have not enabled this particular warning but instead
> > > -Wconversion, which includes it. In general I have found these warnings to
> > > be useful -- usually the warnings point out mistakes but in edge cases a
> > > cast can be added and documented so it is clear why the conversion is being
> > > done.
> > > 
> > > But this is clearly not a battle that I'm going to win so I wrote a meson
> > > probe to detect the issue and disable the warning:
> > > 
> > > # Suppress -Wsign-conversion for C libraries (e.g. musl) that do not accept
> > > int without warning for FD_*() macros
> > > if not cc.compiles(
> > >          '''#include <sys/select.h>
> > >          int main(int arg, char **argv) {int fd = 1; fd_set s; FD_ZERO(&s);
> > > FD_SET(fd, &s); FD_ISSET(fd, &s);}''',
> > >          args: ['-Wsign-conversion', '-Werror'])
> > >      message('Disabling -Wsign-conversion because int is not accepted without
> > > warning by FD_*() macros')
> > >    add_project_arguments(cc.get_supported_arguments('-Wno-sign-conversion'),
> > > language: 'c')
> > > endif
> > > 
> > > I post this in case it is of use to others who run into this issue.
> > 
> > This is still in the domain of "stuff we may change", but there does
> > not seem to be a clear right way to do it without masking warnings for
> > more serious bugs like passing an actually-wrong type. If you or
> > anyone else has good ideas for a recipe that wouldn't accept pointers
> > and cast them to ints (and that would leave all constraint violations
> > as constraint violations), please share it for discussion as a
> > potential way forward.
> 
> I wonder if it would be possible to use _Static_assert for this purpose,
> when it is available, i.e. cast to unsigned when the input is int. Something
> like this (but not this):
> 
> #ifdef HAVE_BUILTIN_TYPES_COMPATIBLE_P
> #define UNCONSTIFY(type, expression)
> \
>   (STATIC_ASSERT_EXPR(__builtin_types_compatible_p(__typeof(expression),
> const type), "invalid cast"), (type)(expression))
> #else
> #define UNCONSTIFY(type, expression)
> \
>     ((type)(expression))
> #endif

Any kind of GNUC monstrosity like this is a non-starter. Proposals
need to actually be C. And C89-compatible, since the header can be
used in C89 mode.

There probably is some way to get it right with clever use of the
ternary operator but I haven't come up with anything in the first few
seconds thinking about it.

Rich

Re: [musl] Sign conversion warning in FD_ISSET()

[Andy Caldwell]

> > There probably is some way to get it right with clever use of the
> > ternary operator but I haven't come up with anything in the first few
> > seconds thinking about it.
> >
> > Rich

Yes, the ternary operator can be used to create a C89-compatible
version (this allows anything coercible to an `int` or an `unsigned`,
so it mostly only refuses pointers and structures).

#define SOCKET_TO_UNSIGNED(s) ((unsigned)(1?(s):0))

Andy

Re: [musl] Sign conversion warning in FD_ISSET()

[Rich Felker]

On Fri, Jun 06, 2025 at 01:23:14AM +0100, Andy Caldwell wrote:
> > > There probably is some way to get it right with clever use of the
> > > ternary operator but I haven't come up with anything in the first few
> > > seconds thinking about it.
> > >
> > > Rich
> 
> Yes, the ternary operator can be used to create a C89-compatible
> version (this allows anything coercible to an `int` or an `unsigned`,
> so it mostly only refuses pointers and structures).
> 
> #define SOCKET_TO_UNSIGNED(s) ((unsigned)(1?(s):0))

Nope, that accepts a pointer. Make it 42 instead of 0. Or just 1.

Re: [musl] Sign conversion warning in FD_ISSET()

[Nick Wellnhofer]

On Jun 6, 2025, at 02:23, Andy Caldwell <andy.m.caldwell@googlemail.com> wrote:
> 
>>> There probably is some way to get it right with clever use of the
>>> ternary operator but I haven't come up with anything in the first few
>>> seconds thinking about it.
>>> 
>>> Rich
> 
> Yes, the ternary operator can be used to create a C89-compatible
> version (this allows anything coercible to an `int` or an `unsigned`,
> so it mostly only refuses pointers and structures).
> 
> #define SOCKET_TO_UNSIGNED(s) ((unsigned)(1?(s):0))

This doesn't work with pointers because 0 is a null pointer constant. But if you change the value to 1, it should work.

Nick

Re: [musl] Sign conversion warning in FD_ISSET()

[Andy Caldwell]

On Fri, Jun 6, 2025 at 1:29 AM Nick Wellnhofer <wellnhofer@aevum.de> wrote:
>
> On Jun 6, 2025, at 02:23, Andy Caldwell <andy.m.caldwell@googlemail.com> wrote:
> >
> >>> There probably is some way to get it right with clever use of the
> >>> ternary operator but I haven't come up with anything in the first few
> >>> seconds thinking about it.
> >>>
> >>> Rich
> >
> > Yes, the ternary operator can be used to create a C89-compatible
> > version (this allows anything coercible to an `int` or an `unsigned`,
> > so it mostly only refuses pointers and structures).
> >
> > #define SOCKET_TO_UNSIGNED(s) ((unsigned)(1?(s):0))
>
> This doesn't work with pointers because 0 is a null pointer constant. But if you change the value to 1, it should work.
>
> Nick
>

Thanks Nick and Rich, you are both correct, I originally had it as
`0?1:(s)` then changed it (without re-testing) for the email, like a
fool.  However in re-testing I notice that this only triggers a
warning `-Wint-conversion` rather than an error when passed a pointer
in C89 mode (it gives an error when passed a structure in any mode).

Andy