Date: Fri, 27 Feb 2015 20:56:39 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Andrew Turner <andrew@fubar.geek.nz> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Warner Losh <imp@freebsd.org> Subject: Re: svn commit: r279349 - head/sys/kern Message-ID: <20150227202646.I2088@besplex.bde.org> In-Reply-To: <20150227082257.3fb1081c@bender.Home> References: <201502270256.t1R2uxnv085328@svn.freebsd.org> <20150227082257.3fb1081c@bender.Home>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 27 Feb 2015, Andrew Turner wrote:
> On Fri, 27 Feb 2015 02:56:59 +0000 (UTC)
> Warner Losh <imp@FreeBSD.org> wrote:
> ...
>> /*
>> + * We need some randomness. Implement the classic Linear Congruential
>> + * generator X_{n+1}=(aX_n+c) mod m. These values are optimized for
>> + * m = 2^32, a = 69069 and c = 5. This is signed so that we can get
>> + * both positive and negative values from it by shifting the value
>> + * right.
>> + */
>> +static int sched_random()
>> +{
>> + int rnd, *rndptr;
>> + rndptr = DPCPU_PTR(randomval);
>> + rnd = *rndptr * 69069 + 5;
>> + *rndptr = rnd;
>> + return(rnd);
>> +}
>
> Didn't we recently have issues with signed integer overflow being
> undefined? Even though we worked around it with a compiler flag it
> would be better to not rely on undefined behaviour in the first place.
It just moves bad code from inside 1 function, adds mounds of style bugs,
(about 10 style bugs of 6 different types in the above; a few more
elsewhere) and calls the bad code from where it was moved from and
another function.
The undefined behaviour is missing in old rand() in libc. That uses
unsigned long internally to avoid the undefined behaviour and to not
depend on ints being 32 bits, but returns only 15 bits so that the
value can be represented as a (nonnegative) 16-bit int. Normally,
LCGs have a large multiplier that puts most randomness in the top
bits so lower bits should be discarded. This one does the opposite.
Here the comment can be read as admitting that the undefined behaviour
is intentional. "This" in the comment is ambiguous. I think it means
the return value. The result of right shifting a negative int is only
implementation-defined.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150227202646.I2088>