From owner-svn-src-projects@FreeBSD.ORG Mon May 30 13:57:12 2011 Return-Path: Delivered-To: svn-src-projects@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B43E2106564A; Mon, 30 May 2011 13:57:12 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail07.syd.optusnet.com.au (mail07.syd.optusnet.com.au [211.29.132.188]) by mx1.freebsd.org (Postfix) with ESMTP id 518698FC12; Mon, 30 May 2011 13:57:11 +0000 (UTC) Received: from c122-106-165-191.carlnfd1.nsw.optusnet.com.au (c122-106-165-191.carlnfd1.nsw.optusnet.com.au [122.106.165.191]) by mail07.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id p4UDv7v4004678 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 30 May 2011 23:57:09 +1000 Date: Mon, 30 May 2011 23:57:07 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Attilio Rao In-Reply-To: <201105271601.p4RG1pfn042424@svn.freebsd.org> Message-ID: <20110530230553.I3689@besplex.bde.org> References: <201105271601.p4RG1pfn042424@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed 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 X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 30 May 2011 13:57:12 -0000 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