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

[Takashi Iwai]

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.


Takashi