Syslinux - Apr 2011 - [PATCH] com32: Do not use centralized bitops header in vsscanf

From: Matt Fleming <matt.fleming at linux.intel.com>

Partially revert "com32: add a centralized bitops header"

This reverts part of commit db74cf6c4182f40ecf7fad1f04799d09d82f896d.

The usage of the centralized bitops in com32/lib/vsscanf.c is not
correct because the bitmap that we're accessing is too large for the
'bt', 'bts' and 'btc' instructions to operate on, i.e.
the
instructions cannot address all the bits of the bitmap as the size of
'matchmap' is 32 bytes.

This commit doesn't entirely revert db74cf6 as having centralised
bitops does make sense in principle, it's just that we can't use it in
vsscanf(). Also, we still need to refrain from marking set_bit() and
test_bit() as static inline otherwise we will end up running into the
compilation issue described in the original commit.

Signed-off-by: Matt Fleming <matt.fleming at linux.intel.com>
---
 com32/lib/vsscanf.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/com32/lib/vsscanf.c b/com32/lib/vsscanf.c
index 751f22a..9575670 100644
--- a/com32/lib/vsscanf.c
+++ b/com32/lib/vsscanf.c
@@ -12,7 +12,6 @@
 #include <string.h>
 #include <limits.h>
 #include <stdio.h>
-#include <sys/bitops.h>
 
 #ifndef LONG_BIT
 #define LONG_BIT (CHAR_BIT*sizeof(long))
@@ -47,6 +46,18 @@ enum bail {
     bail_err			/* Conversion mismatch */
 };
 
+#undef set_bit
+static void set_bit(unsigned long *bitmap, unsigned int bit)
+{
+    bitmap[bit / LONG_BIT] |= 1UL << (bit % LONG_BIT);
+}
+
+#undef test_bit
+static int test_bit(unsigned long *bitmap, unsigned int bit)
+{
+    return (int)(bitmap[bit / LONG_BIT] >> (bit % LONG_BIT)) & 1;
+}
+
 int vsscanf(const char *buffer, const char *format, va_list ap)
 {
     const char *p = format;
@@ -298,7 +309,7 @@ set_integer:
 	    if (ch == '^' && !(flags & FL_INV)) {
 		matchinv = 1;
 	    } else {
-		set_bit((unsigned char)ch, matchmap);
+		set_bit(matchmap, (unsigned char)ch);
 		state = st_match;
 	    }
 	    break;
@@ -310,18 +321,18 @@ set_integer:
 		range_start = (unsigned char)ch;
 		state = st_match_range;
 	    } else {
-		set_bit((unsigned char)ch, matchmap);
+		set_bit(matchmap, (unsigned char)ch);
 	    }
 	    break;
 
 	case st_match_range:	/* %[ match after - */
 	    if (ch == ']') {
-		set_bit((unsigned char)'-', matchmap);	/* - was last character */
+		set_bit(matchmap, (unsigned char)'-');	/* - was last character */
 		goto match_run;
 	    } else {
 		int i;
 		for (i = range_start; i < (unsigned char)ch; i++)
-		    set_bit(i, matchmap);
+		    set_bit(matchmap, i);
 		state = st_match;
 	    }
 	    break;
@@ -329,7 +340,7 @@ set_integer:
 match_run:			/* Match expression finished */
 	    qq = q;
 	    while (width && *q
-		   && test_bit((unsigned char)*q, matchmap) ^ matchinv) {
+		   && test_bit(matchmap, (unsigned char)*q) ^ matchinv) {
 		*sarg++ = *q++;
 	    }
 	    if (q != qq) {
-- 
1.7.4

On 04/04/2011 06:48 AM, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming at linux.intel.com>
> 
> Partially revert "com32: add a centralized bitops header"
> 
> This reverts part of commit db74cf6c4182f40ecf7fad1f04799d09d82f896d.
> 
> The usage of the centralized bitops in com32/lib/vsscanf.c is not
> correct because the bitmap that we're accessing is too large for the
> 'bt', 'bts' and 'btc' instructions to operate on,
i.e. the
> instructions cannot address all the bits of the bitmap as the size of
> 'matchmap' is 32 bytes.
> 
bt/bts/btc can operate on "arbitrary" large memory objects (in
practice
they are limited to 256 GiB in 32-bit mode, but that's not really an
issue here.)

Was this something you observed by code inspection, or did you actually
see a failure?

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.