[PATCH 05/18] Fix 64bit arch issue in synaptics eventcomm.c

[Takashi Iwai]

At Mon, 11 Oct 2010 10:24:44 +0200 (CEST),
Mark Kettenis wrote:
> 
> > Date: Mon, 11 Oct 2010 09:47:56 +0200
> > From: Takashi Iwai <tiwai at suse.de>
> > 
> > At Fri, 08 Oct 2010 23:09:26 +0200,
> > Henrik Rydberg wrote:
> > > 
> > > On 10/08/2010 10:51 PM, Mark Kettenis wrote:
> > > 
> > > >> Date: Fri, 08 Oct 2010 21:36:02 +0200
> > > >> From: Takashi Iwai <tiwai at suse.de>
> > > >>
> > > >> At Fri, 8 Oct 2010 20:48:10 +0200 (CEST),
> > > >> Mark Kettenis wrote:
> > > >>>
> > > >>>> From: Takashi Iwai <tiwai at suse.de>
> > > >>>> Date: Fri,  8 Oct 2010 19:22:29 +0200
> > > >>>
> > > >>> Sorry, but it isn't obvious to me what issue this fixes.
> > > >>
> > > >> In C, "1" is an integer, not an unsigned long.
> > > >> Thus (1 << 33) doesn't give you the 33th bit shift, but it's undefined.
> > > > 
> > > > But if you'd actually use 33 (or event 32) as an offset, things
> > > > wouldn't work on a 32-bit platform would they?
> > > 
> > > 
> > > I believe this is what the patch is supposed to fix.
> > 
> > Yep :)
> > 
> > 
> > > > Anyway,
> > > > 
> > > >> If any, this must be (1UL << 32). 
> > > > 
> > > > This is the idiom that is much more common.  I probably wouldn't have
> > > > questioned things if you'd written it like that.  I recommend you to
> > > > stick to this version.
> > > 
> > > 
> > > For a counter-example, see the definition of test_bit() in the Linux kernel.
> > > 
> > > >> Also, it'd be better if such a test macro returns 1 instead of a
> > > >> random non-zero value.  So does my patch.
> > > > 
> > > > In C this doesn't matter.
> > > 
> > > 
> > > It is still useful to know if a logic expression evaluates to (0, 1) or (0,
> > > nonzero).
> > 
> > It does matter if the returned type is bigger.
> > Try an example code below on 32bit and 64bit machines:
> > 
> > ================================================================
> > #include <stdio.h>
> > 
> > #define LONG_BITS (sizeof(long) * 8)
> > #define OFF(x)   ((x) % LONG_BITS)
> > #define LONG(x)  ((x) / LONG_BITS)
> > 
> > #define TEST_BIT1(bit, array) (array[LONG(bit)] & (1 << OFF(bit)))
> > #define TEST_BIT2(bit, array) (array[LONG(bit)] & (1UL << OFF(bit)))
> > #define TEST_BIT3(bit, array) ((array[LONG(bit)] >> OFF(bit)) & 1)
> > 
> > static unsigned long test[4];
> > 
> > int main()
> > {
> > 	const int bit = 33;
> > 	int result;
> > 
> > 	/* set the bit */
> > 	test[LONG(bit)] |= 1UL << OFF(bit);
> > 
> > 	result = TEST_BIT1(bit, test);
> > 	printf("TEST1 %d = %d\n", bit, result);
> > 
> > 	result = TEST_BIT2(bit, test);
> > 	printf("TEST2 %d = %d\n", bit, result);
> > 
> > 	result = TEST_BIT3(bit, test);
> > 	printf("TEST3 %d = %d\n", bit, result);
> > 
> > 	return 0;
> > }
> > ================================================================
> > 
> > On a 32bit machine, it results in:
> >   TEST1 33 = 2
> >   TEST2 33 = 2
> >   TEST3 33 = 1
> > 
> > while on a 64bit machine, it results in:
> >   TEST1 33 = 0
> >   TEST2 33 = 0
> >   TEST3 33 = 1
> > 
> > See TEST2 still failed although it uses 1UL.
> 
> This example is meaningless.  On every reasonable 32-bit architecture,
> long is a 32-bit integer.  So
> 
> > 	/* set the bit */
> > 	test[LONG(bit)] |= 1UL << OFF(bit);
> 
> when bit is 33 (or even 32), is completely undefined on a 32-bit
> machine.  Therefore, you should never call TEST_BIT() with a first
> argument of 32 or larger, even on a 64-bit architecture, since the
> code wouldn't work on a 32-bit machine.

Hm?  TEST_BIT() is used for an long *array*, with the unlimited size.
Otherwise the macro should have been more simplified :)
Ditto for the above code.  It stores the value in an array member.
That's why LONG() and OFF() macros are used there.

> One can even argue that for this reason the change you're suggesting
> is dangerous, since people writing code on a 64-bit machine might not
> notice that they're writing coding that won't run on a 32-bit machine.

I wrote the patch because the detection of MT_* bits don't work on 64
bit machines actually.  So the current code _is_ just broken.


Takashi