Date: Sun, 28 Sep 2014 09:55:35 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Colin Percival <cperciva@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r272207 - in head/games: factor primes Message-ID: <20140928071459.C927@besplex.bde.org> In-Reply-To: <201409270900.s8R90dWl029070@svn.freebsd.org> References: <201409270900.s8R90dWl029070@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 27 Sep 2014, Colin Percival wrote:
> Log:
> Switch primes(6) from using unsigned long to using uint64_t. This fixes
> 'limited range of type' warnings about comparisons on 32-bit systems, and
> allows 32-bit systems to compute the full range of primes.
Since it no longer pretends to support up to the limit of the type,
using the correct type of uintmax_t doesn't necessarily break anything
immediately. However, blindly changing it gives bugs. Mostly style
bugs like different error messages for different range errors. primes(6)
already has mounds of similar source and output bugs.
> Modified: head/games/primes/primes.c
> ==============================================================================
> --- head/games/primes/primes.c Sat Sep 27 08:59:43 2014 (r272206)
> +++ head/games/primes/primes.c Sat Sep 27 09:00:38 2014 (r272207)
> @@ -111,10 +112,10 @@ main(int argc, char *argv[])
> argv += optind;
>
> start = 0;
> - stop = (sizeof(ubig) > 4) ? SPSPMAX : BIG;
> + stop = SPSPMAX;
>
> /*
> - * Convert low and high args. Strtoul(3) sets errno to
> + * Convert low and high args. Strtoumax(3) sets errno to
> * ERANGE if the number is too large, but, if there's
> * a leading minus sign it returns the negation of the
> * result of the conversion, which we'd rather disallow.
Only some numbers that are too large are detected by strtoumax().
Since SIEVEMAX is now < the return value for a range error, there
is no now need for checking ERANGE.
> @@ -126,19 +127,19 @@ main(int argc, char *argv[])
> errx(1, "negative numbers aren't permitted.");
>
> errno = 0;
> - start = strtoul(argv[0], &p, 0);
> + start = strtoumax(argv[0], &p, 0);
This now truncates the value from uintmax_t to typeof(ubig) == uint64_t
(on unsupported arches with uintmax_t larger than uint64_t). This is
broken (see below). Use uintmax_t for ubig to avoid complications. Or
at least use it to check values before they are truncated.
> if (errno)
> err(1, "%s", argv[0]);
This handles mainly the ERANGE error. Some out-of-range values are
detected by this.
The code is still essentially the same as in 4.4BSD. Then ERANGE was
the only possible error (except preverse functions can set errno to
anything nonzero). POSIX added "helpful" setting to EINVAL which is
actually perverse since it changes the error handling of sloppy code
like this. This code doesn't really expect EINVAL and should have
been written as "if (errno == ERANGE)". This would also fix one of
the style bugs in it (boolean check for non-boolean). Bugs like this
only give minor differences in the error messages. There are many
other style errors in the error messages.
All the parsing and error handling bugs for numbers are quadruplicated,
except for parsing numbers in a file, the bug of misdetecting negative
numbers by checking only the first character in argv[n] is replaced
by the bug of using isblank() on possibly-signed characters to skip
whitespace.
> - if ((uint64_t)stop > SPSPMAX)
> + if (stop > SPSPMAX)
> errx(1, "%s: stop value too large.", argv[1]);
Numbers larger than UINTMAX_MAX are now detected as ERANGE errors and
their error handling is err(1, "%s", argv[0]);. 'stop' numbers
> SPSPAX and <= UINT64_MAX are detected here and their error handling
is better. 'start' and numbers in these ranges are detected as
ERANGE errors or later by comparing them with 'stop'. I'm not sure
if their later error handling is as good (it is unlikely to be so).
Detection of errors for numbers > UINT64_MAX and <= UINTMAX_MAX is
broken by truncation. E.g., UINT64_MAX + (uintmax_t)2 is truncated
to 1.
> @@ -243,7 +244,7 @@ primes(ubig start, ubig stop)
> for (p = &prime[0], factor = prime[0];
> factor < stop && p <= pr_limit; factor = *(++p)) {
> if (factor >= start) {
> - printf(hflag ? "0x%lx\n" : "%lu\n", factor);
> + printf(hflag ? "%" PRIx64 "\n" : "%" PRIu64 "\n", factor);
This adds 2 style bugs and 1 non-style bug:
- it uses the PRI* mistake
- as well as its own ugliness, blind use of PRI* expands the line beyond
80 columns
- the use of PRI* isn't even blind. Blind use would have preserved the
leading "lx" in the hex format. The PRI* mistake is so ugly that bugs
like this might be hard to see.
If the correct type (uintmax_t) were used throughout, then the change here
would be simply to replace 'l' by 'j'.
Otherwise, PRI* should never be used except possibly to micro-optimize
for space on 8-bit systems. Instead, cast 'factor' to uintmax_t and use
%j formats. This doesn't apply here for various reasons. Expansion of
the main code to 64 bits might already be too much bloat for the 8-bit
systems. If they don't mind that, then they shouldn't mind printing
the 64-bit values. They can limit the bloat to 64 bits by not making
uintmax_t larger than that. It should only be larger than that on
64-bit systems.
> }
> }
> /* return early if we are done */
> @@ -306,11 +307,11 @@ primes(ubig start, ubig stop)
> */
> for (q = table; q < tab_lim; ++q, start+=2) {
> if (*q) {
> - if ((uint64_t)start > SIEVEMAX) {
> + if (start > SIEVEMAX) {
Why compare with SIEVEMAX instead of 'stop'? 'start' should be <= 'stop',
and I think there is an up-front check for 'stop'.
> if (!isprime(start))
> continue;
> }
> - printf(hflag ? "0x%lx\n" : "%lu\n", start);
> + printf(hflag ? "%" PRIx64 "\n" : "%" PRIu64 "\n", start);
This set of bugs is only duplicated.
> }
> }
> }
>
> Modified: head/games/primes/primes.h
> ==============================================================================
> --- head/games/primes/primes.h Sat Sep 27 08:59:43 2014 (r272206)
> +++ head/games/primes/primes.h Sat Sep 27 09:00:38 2014 (r272207)
> @@ -41,8 +41,10 @@
> * chongo <for a good prime call: 391581 * 2^216193 - 1> /\oo/\
> */
>
> +#include <stdint.h>
Style bug: nested include.
> +
> /* ubig is the type that holds a large unsigned value */
> -typedef unsigned long ubig; /* must be >=32 bit unsigned value */
> +typedef uint64_t ubig; /* must be >=32 bit unsigned value */
The comment is very wrong now. After the previous commit, 32 bits may have
been sufficient but they gave annoying restrictions. Now, 32 bits isn't
sufficient to give the documented limit. It might be correct to say "must
be large enough to hold <SIEVEMAX>". However, using PRI64 is more broken
than first appeared. It hard-codes the assumption that ubig has type
precisely uint64_t (like old code hard-coded the assumption that ubig
has type unsigned long). So the only correct comment now is
/* must be precisely what it is (duh) */.
It was wrong originally, since >= 32 bits was too large for the algorithm
and there was no range checking other that the ERANGE check related to
the type.
> #define BIG ULONG_MAX /* largest value will sieve */
This bug was pointed out in another reply. BIG might be unused, but its
comment is wrong.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140928071459.C927>