From owner-cvs-src@FreeBSD.ORG Tue Jun 29 02:30:25 2004 Return-Path: Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id E1F9A16A4D0; Tue, 29 Jun 2004 02:30:24 +0000 (GMT) Received: from mailout2.pacific.net.au (mailout2.pacific.net.au [61.8.0.85]) by mx1.FreeBSD.org (Postfix) with ESMTP id 3046743D41; Tue, 29 Jun 2004 02:30:23 +0000 (GMT) (envelope-from bde@zeta.org.au) Received: from mailproxy2.pacific.net.au (mailproxy2.pacific.net.au [61.8.0.87])i5T2UG5v030665; Tue, 29 Jun 2004 12:30:16 +1000 Received: from gamplex.bde.org (katana.zip.com.au [61.8.7.246]) i5T2UDnl010660; Tue, 29 Jun 2004 12:30:14 +1000 Date: Tue, 29 Jun 2004 12:30:12 +1000 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Andrew Gallatin In-Reply-To: <16608.30892.745161.730935@grasshopper.cs.duke.edu> Message-ID: <20040629114614.T2908@gamplex.bde.org> References: <200406281915.i5SJFeaV060231@repoman.freebsd.org> <20040628193858.GG5635@green.homeunix.org> <16608.30892.745161.730935@grasshopper.cs.duke.edu> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: Brian Fundakowski Feldman cc: src-committers@freebsd.org cc: cvs-all@freebsd.org cc: cvs-src@freebsd.org Subject: Re: cvs commit: src/sys/vm vm_map.c X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Jun 2004 02:30:25 -0000 On Mon, 28 Jun 2004, Andrew Gallatin wrote: > Brian Fundakowski Feldman writes: > > On Mon, Jun 28, 2004 at 03:22:33PM -0400, Andrew Gallatin wrote: > > > Andrew Gallatin [gallatin@FreeBSD.org] wrote: > > > > gallatin 2004-06-28 19:15:40 UTC > > > > > > > > FreeBSD src repository > > > > > > > > Modified files: > > > > sys/vm vm_map.c > > > > Log: > > > > Fix alpha - the use of min() on longs was loosing the high bits and > > > > returning wrong answers, leading to strange values vm2->vm_{s,t,d}size. MIN() and MAX() should not be used in the kernel. 4.4BSD removed them in the kernel, but FreeBSD broke this in rev.1.141 of sys/param.h. They remain removed in RELENG_4. > > > Why are min() and max() inlines which operate on ints? This seems > > > like a real landmine for 64-bit platforms.. They actually operate on unsigned ints. This is because they are declared to do so. C doesn't have any overloaded or type-generic functions, so min()/max() must operate on some type, and that type happens to be unsigned int. Another common error is sign extension bugs from using min() and max() where imin() or imax() should be used, or vice versa. The min()/max() family is hard to use, especially on typedefed types. MIN()/MAX() is more type-generic, but has side effects and has the same problems for mixing of signed types with unsigned types. gcc with certain warning options now detects sign mismatches in comparisions, but the implicit casts in the function versions lose the sign information before the comparisions are done. > > Also, why is GCC not generating the correct warnings? The values passed > > in were definitely a 64-bit type. Thanks for finding and fixing this. > > I wish I knew. I'm afraid this may bite us at some other point? gcc -Wconversion reports both size and sign mismatches: %%% Script started on Tue Jun 29 12:05:20 2004 ttyv6:bde@gamplex:/tmp> cat z.c #define _KERNEL #include #include long long x; int i; int main() { return (min(x, i)); } ttyv6:bde@gamplex:/tmp> cc -Wconversion -c z.c z.c: In function `main': z.c:11: warning: passing arg 1 of `min' with different width due to prototype z.c:11: warning: passing arg 2 of `min' as unsigned due to prototype ttyv6:bde@gamplex:/tmp> exit Script done on Tue Jun 29 12:05:30 2004 %%% but -Wconversion is not used by default since it would be too noisy. It also complains about harmless conversions like promoting 32 bit ints to 64-bit ints. > > The inlines seem to exist to work around the effect of using macros > > unknowingly on statements with side effects. These should really be > > MIN(), and there seems to have been an extra tab that crept in. Do > > you think you could change those things? > > Sure. Already done. Thanks for the blessing to use MIN(). MIN() should really not be used. I once started to fix this by using sizeof() and __typeof() to decide the correct version of the min()/max() family to call, and comparing the choice with programmers' choices by examing the object files. __typeof() can be used even more simply to write safe versions of MIN() and MAX(), but it shouldn't be used because it is a gccism. Part of the unfinished code is: %%% #define __min(x, y) \ ( \ (sizeof(x) == 8 || sizeof(y) == 8) ? \ ((__typeof(x))-1 == -1 && (__typeof(y))-1 == -1) ? \ _qmin((x), (y)) \ : \ _uqmin((x), (y)) \ : \ ((__typeof(x))-1 == -1 && (__typeof(y))-1 == -1) ? \ _imin((x), (y)) \ : \ _min((x), (y)) \ ) %%% (where _min(), etc. are inline functions identical to the current _min(), etc., and all current min functions are macros expanding to __min(...)) There was little interest in finishing this. I only fixed about 10 serious type mismatches discovered by using it. Things are more complicated now with multiple arches and more typdefed types, but the hard-coded 8 in the above still works for all supported arches. It depends mainly on ints being smaller than quads and longs having the same size as either ints or quads. Bruce