From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: from second.openwall.net (second.openwall.net [193.110.157.125]) by inbox.vuxu.org (Postfix) with SMTP id 692272203E for ; Thu, 18 Jul 2024 18:25:56 +0200 (CEST) Received: (qmail 15918 invoked by uid 550); 18 Jul 2024 16:25:49 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 15871 invoked from network); 18 Jul 2024 16:25:49 -0000 Date: Thu, 18 Jul 2024 12:25:41 -0400 From: Rich Felker To: Brad House Cc: musl@lists.openwall.com Message-ID: <20240718162541.GO10433@brightrain.aerifal.cx> References: <10dead9f-55f9-4426-adcb-1d25769e6fc0@brad-house.com> <20240718131949.GQ3766212@port70.net> <0c1ddb63-8bb9-4bfc-918d-3fa4f55fcfa4@brad-house.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <0c1ddb63-8bb9-4bfc-918d-3fa4f55fcfa4@brad-house.com> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion On Thu, Jul 18, 2024 at 11:21:59AM -0400, Brad House wrote: > 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. That's not acceptable, but I think you're confusing "functions" with "functions with external linkage". There's no reason you can't do functions with static linkage. However there is probably also some fancy solution with the ternary operator that avoids the type-unsafety. I haven't thought through the pros and cons of either option. What's really frustrating about these kinds of garbage warnings is that they encourage (as in the proposed patch) writing casts that *remove type-safety* and make very-wrong code silently compile "fine", for the purpose of suppressing a warning that's supposedly about a type-safety issue. Rich