Roman Kononov <kononov@ftml.net> writes:
> The bitfromint8() and bitfromint4() are hosed. They produce wrong
> results when the BIT size is more than 64 and 32 respectively, and the
> BIT size is not multiple of 8, and the most significant byte of the
> integer value is not 0x00 or 0xff.
Hm, you're right, it's definitely busted ...
> The patch re-implements the conversion functions.
... but I don't much care for this patch. It's unreadable, uncommented,
and doesn't even try to follow Postgres coding conventions.
It looks to me like the actual bug is that the "first fractional byte"
case ought to shift by destbitsleft - 8 not srcbitsleft - 8. A
secondary problem (which your patch fails to fix) is that if the
compiler chooses to implement signed >> as zero-fill rather than
sign-bit-fill (which it is allowed to do per C99) then we'll get
wrong, or at least not the desired, results. So I arrive at
the attached patch.
Unfortunately we've just missed the window for 8.4.2 etc, but I'll
get this committed for the next updates.
regards, tom lane
Index: varbit.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/varbit.c,v
retrieving revision 1.58
diff -c -r1.58 varbit.c
*** varbit.c 1 Jan 2009 17:23:50 -0000 1.58
--- varbit.c 12 Dec 2009 18:53:43 -0000
***************
*** 1346,1352 ****
/* store first fractional byte */
if (destbitsleft > srcbitsleft)
{
! *r++ = (bits8) ((a >> (srcbitsleft - 8)) & BITMASK);
destbitsleft -= 8;
}
/* Now srcbitsleft and destbitsleft are the same, need not track both */
--- 1346,1357 ----
/* store first fractional byte */
if (destbitsleft > srcbitsleft)
{
! int val = (int) (a >> (destbitsleft - 8));
!
! /* Force sign-fill in case the compiler implements >> as zero-fill */
! if (a < 0)
! val |= (-1) << (srcbitsleft + 8 - destbitsleft);
! *r++ = (bits8) (val & BITMASK);
destbitsleft -= 8;
}
/* Now srcbitsleft and destbitsleft are the same, need not track both */
***************
*** 1425,1431 ****
/* store first fractional byte */
if (destbitsleft > srcbitsleft)
{
! *r++ = (bits8) ((a >> (srcbitsleft - 8)) & BITMASK);
destbitsleft -= 8;
}
/* Now srcbitsleft and destbitsleft are the same, need not track both */
--- 1430,1441 ----
/* store first fractional byte */
if (destbitsleft > srcbitsleft)
{
! int val = (int) (a >> (destbitsleft - 8));
!
! /* Force sign-fill in case the compiler implements >> as zero-fill */
! if (a < 0)
! val |= (-1) << (srcbitsleft + 8 - destbitsleft);
! *r++ = (bits8) (val & BITMASK);
destbitsleft -= 8;
}
/* Now srcbitsleft and destbitsleft are the same, need not track both */