On 7/18/24 9:19 AM, Szabolcs Nagy wrote:

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

I'm not sure about your statement above about a behavior change if d is 
0.5, file descriptors are required to be int, which can't represent 
0.5.  Passing in a garbage value would clearly fall into "Undefined 
Behavior".

We really can't wrap FD_SET/FD_ISSET in c-ares, since wrapping it would 
require the library to know the internals about the operations of the 
fd_set for each and every platform, which could change over time if 
there are ABI changes.

I understand your concern regarding casting away real programming errors 
by accident.  The only solution to that I see would be to turn the 
macros into functions, POSIX says either are valid: 
https://pubs.opengroup.org/onlinepubs/009604499/basedefs/sys/select.h.html

Of course, that would introduce a backwards-incompatible ABI change 
where an application compiled against a newer version of musl wouldn't 
run on an older version.  I don't know what your ABI policies look like 
to know if that's acceptable.

-Brad