Date: Tue, 10 Aug 2010 21:03:42 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: "M. Warner Losh" <imp@bsdimp.com> Cc: src-committers@FreeBSD.org, jhb@FreeBSD.org, c.jayachandran@gmail.com, jchandra@FreeBSD.org, svn-src-all@FreeBSD.org, jlanders@vmware.com, attilio@FreeBSD.org, rrs@FreeBSD.org, sbruno@FreeBSD.org, svn-src-head@FreeBSD.org Subject: Re: svn commit: r208165 - in head/sys: kern mips/conf mips/include mips/mips mips/rmi mips/rmi/dev/xlr Message-ID: <20100810202538.G10110@delplex.bde.org> In-Reply-To: <20100809.111922.276219111637867136.imp@bsdimp.com> References: <AANLkTi=jkUiXStPV3Gq206GgmctM9%2B1ofbNWuh5CSeWw@mail.gmail.com> <AANLkTim_pTQgquRRuz4G=jC5G0Amc%2ByF6VeDcg-cpTCX@mail.gmail.com> <AANLkTin9tn%2BOFOkQSYSewtgVQrDNoa2gE2SqPxA73cih@mail.gmail.com> <20100809.111922.276219111637867136.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 9 Aug 2010, M. Warner Losh wrote: > In message: <AANLkTin9tn+OFOkQSYSewtgVQrDNoa2gE2SqPxA73cih@mail.gmail.com> > "Jayachandran C." <c.jayachandran@gmail.com> writes: > : > Actually the 32 bits limit is well aware and acknowledged in cpumask_t. > : > While it is true that it should be an 'opaque' type, in reality it is > : > not. The maximum limit of 32 CPUs is a reality due to cpumask_t on all > : > our architectures. > : > That is why we are going to use cpuset_t for dealing with CPUs numbers > : > (which is really opaque, at same extent, and is not bound to any > : > size). > : > : In my opinion, your changes added another pitfall for the person who > : tries to make the cpumask_t 64 bit to support more cpus, which really > : could have been avoided. To avoid the pitfall, the 32 in the latest should be spelled something like (sizeof(cpu_mask_t) * CHAR_BIT). Perhaps some literal constants also need to be cast to cpu_mask_t. > Yes, the correct fix, short of cpuset_t, is not: > >>>>> - top->cg_mask = (1 << mp_ncpus) - 1; >>>>> + if (mp_ncpus == sizeof(top->cg_mask) * 8) This spelling of 32 is better than the hard 32. >>>>> + top->cg_mask = -1; >>>>> + else >>>>> + top->cg_mask = (1 << mp_ncpus) - 1; > > but rather > > if (mp_ncpus > sizeof(top->cg_mask) * NBBY) > mp_ncpus = sizeof(top->cg_mask) * NBBY; /* Or panic */ When fixing the spelling of sizeof(char), please catch up with C90 and spell it CHAR_BIT. > if (mp_ncpus > sizeof(top->cg_mask) * NBBY) > top->cg_mask = ~0; /* which avoids the signed error */ > else > top->cg_mask = (1 << mp_ncpus) - 1; This is missing a cast of 1. With 16-bit ints, this would overflow even with a 32-bit cpumask_t. > I'm not sure why the expression would fail (1 << 32 == 0 -1 == all > bits set), but I've not looked at the old thread to see why the > compiler is generating bogus code. (1 << 32) gives undefined behaviour unless ints have at least 33 value bits (and 1 sign bit). IIRC, even within the x86 family, some actual behaviours are to give a result of either 0 or 1, depending on whether the shift count is masked with 0x1F before shifting. Even (1 << 31) with normal 32-bit ints gives undefined behaviour. C90 says that it gives the result of shifting, but that is unpredictable and nothing is required for it. C99 fixes this and makes it explicitly undefined (since normal 32-bit ints have 31 value bits and (1 << 31) requires at least 32 value bits to represent in the type of the left operand 1, i.e., int). The actual behaviour for broken expressions like top->cg_mask = (1 << mp_ncpus) - 1; is: - first (1 << 31) overflows benignly to the negative value INT_MIN (with normal 2's complement 32-bit ints) - next, INT_MIN - 1 overflows benignly to the positive value INT_MAX - the signed value INT_MAX is assigned to the unsigned variable cg_mask. Since INT_MAX is positive and has a type smaller than cg_mask, there are no further overflows or other significant conversions - long mails about this are sent by language lawyers - the overflows accidentally resulted in the correct value, so there are no further problems. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100810202538.G10110>