svn commit: r208165 - in head/sys: kern mips/conf mips/include mips/mips mips/rmi mips/rmi/dev/xlr

[Bruce Evans]

On Mon, 9 Aug 2010, M. Warner Losh wrote:

> In message: <AANLkTin9tn+OFOkQSYSewtgVQrDNoa2gE2SqPxA73cih at mail.gmail.com>
>            "Jayachandran C." <c.jayachandran at 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