[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250605215040.GF1827@brightrain.aerifal.cx>
Date: Thu, 5 Jun 2025 17:50:40 -0400
From: Rich Felker <dalias@...c.org>
To: David Steele <david@...ackrest.org>
Cc: noloader@...il.com, musl@...ts.openwall.com
Subject: Re: Sign conversion warning in FD_ISSET()
On Thu, Jun 05, 2025 at 09:42:27PM +0000, David Steele wrote:
> On 5/23/25 12:07, Jeffrey Walton wrote:
> > On Fri, May 23, 2025 at 11:31 AM David Steele <david@...ackrest.org> wrote:
> > >
> > > 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.
> >
> > 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.
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. It would be nice if the compiler side
could fix that.
Rich
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.
