Date: Mon, 30 May 2011 23:57:07 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Attilio Rao <attilio@FreeBSD.org> Cc: svn-src-projects@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r222363 - in projects/largeSMP: gnu/usr.bin/gdb/kgdb lib/libkvm lib/libmemstat usr.sbin/pmccontrol Message-ID: <20110530230553.I3689@besplex.bde.org> In-Reply-To: <201105271601.p4RG1pfn042424@svn.freebsd.org> References: <201105271601.p4RG1pfn042424@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 27 May 2011, Attilio Rao wrote:
> Log:
> Style fix: cast to size_t rather than u_long when comparing to sizeof()
> rets.
>
> Requested by: kib
This was correct before...
> Modified: projects/largeSMP/gnu/usr.bin/gdb/kgdb/kthr.c
> ==============================================================================
> --- projects/largeSMP/gnu/usr.bin/gdb/kgdb/kthr.c Fri May 27 16:00:37 2011 (r222362)
> +++ projects/largeSMP/gnu/usr.bin/gdb/kgdb/kthr.c Fri May 27 16:01:51 2011 (r222363)
> @@ -107,7 +107,7 @@ kgdb_thr_init(void)
> addr = kgdb_lookup("stopped_cpus");
> CPU_ZERO(&stopped_cpus);
> cpusetsize = sysconf(_SC_CPUSET_SIZE);
> - if (cpusetsize != -1 && (u_long)cpusetsize <= sizeof(cpuset_t) &&
> + if (cpusetsize != -1 && (size_t)cpusetsize <= sizeof(cpuset_t) &&
> addr != 0)
> kvm_read(kvm, addr, &stopped_cpus, cpusetsize);
>
sysconf() returns long, and cpusetsize should have type long (can be
a larger signed type, but there is no reason for that), so casting
cpusetsize to anything other than u_long risks truncating it. Casting
it to u_long is also risky, but can possibly be proved to be safe.
Casting the signed type in a comparision with an unsigned type to "fix" the
warning usually gives a style bug and sometimes gives overflow bugs where
there were none before, and the above is no exception. It might have
started as:
if (cpusetsize != -1 && cpusetsize <= sizeof(cpuset_t))
Then cpusetsize and sizeof(cpuset_t) would have been promoted to a
larger common type, with no risk of overflow and no hard-coding of
this common type. The correctness of might can still depend on
cpusetsize never being negative with value other than -1 (which can
probably only be caused by a kernel bug). But the compiler cannot see
that the negative values are handled correctly, so it emits a warning
with certain CFLAGS. Then the problem is often "fixed" by adding a
bogus cast as above.
Casting the sizeof() value usually works better:
if (cpusetsize != -1 && cpusetsize <= (int)sizeof(cpuset_t))
We know that sizeof(cpuset_t) (or sizeof(almost_anything)) is small, so
it can be converted to int without risk of overflow. The int then promotes
naturally in comparisons. The result is equivalent to the original code
but has the same lack of robustness. This can be fixed by treating all
negative values as errors:
if (cpusetsize >= 0 && cpusetsize <= (int)sizeof(cpuset_t))
and now the cast shouldn't be needed to prevent the warning:
if (cpusetsize >= 0 && cpusetsize <= sizeof(cpuset_t))
Now the compiler should see that the cpusetsize value that is compared with
the sizeof(value) is non-negative and therefore that the warning should
never be emitted. Unfortunately, gcc still warns about this. clang has
the same lack of quality. Similarly, if we just initialized cpusetsize to
literal 1, the compilers shouldn't warn if we compare x for <= with 1U,
the compilers shouldn't warn, but they do.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110530230553.I3689>