Hack together a legacy getBitField backed by new bit copying function.

3 files changed, 65 insertions, 55 deletions

diff --git a/Makefile b/Makefile
index 49e22f2c..d841c208 100644
--- a/Makefile
+++ b/Makefile
@@ -1,6 +1,6 @@
 CC = gcc
 INCLUDES = -Isrc
-CFLAGS = $(INCLUDES) -c -w -Wall -Werror -g -ggdb
+CFLAGS = $(INCLUDES) -c -w -Wall -Werror -g -ggdb -std=gnu99
 LDFLAGS =
 LDLIBS = -lcheck
 
diff --git a/src/bitfield/bitfield.c b/src/bitfield/bitfield.c
index b3aef4b2..834e443e 100644
--- a/src/bitfield/bitfield.c
+++ b/src/bitfield/bitfield.c
@@ -23,7 +23,7 @@
  * Returns: a bit position from 0 to 7.
  */
 static uint8_t findEndBit(const uint16_t startBit, const uint16_t numBits) {
-    int endBit = (startBit + numBits) % CHAR_BIT;
+    int endBit = numBits % CHAR_BIT;
     return endBit == 0 ? CHAR_BIT : endBit;
 }
 
@@ -38,8 +38,6 @@ static int byteForBit(const uint16_t startBit) {
  */
 static void bitarray_copy(const uint8_t* source_origin, int source_offset,
         int source_length, uint8_t* destination_origin, int destination_offset) {
-    static const uint8_t mask[] =
-        { 0x55, 0x01, 0x03, 0x07, 0x0f, 0x1f, 0x3f, 0x7f, 0xff };
     static const uint8_t reverse_mask[] =
         { 0x55, 0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe, 0xff };
     static const uint8_t reverse_mask_xor[] =
@@ -49,21 +47,14 @@ static void bitarray_copy(const uint8_t* source_origin, int source_offset,
         return;
     }
 
-    int source_offset_modulo;
-    int destination_offset_modulo;
-
     const uint8_t* source = source_origin + byteForBit(source_offset);
     uint8_t* destination = destination_origin + byteForBit(destination_offset);
-
-    source_offset_modulo = source_offset % CHAR_BIT;
-    destination_offset_modulo = destination_offset % CHAR_BIT;
+    int source_offset_modulo = source_offset % CHAR_BIT;
+    int destination_offset_modulo = destination_offset % CHAR_BIT;
 
     if(source_offset_modulo == destination_offset_modulo) {
-        if(source_offset_modulo) {
-            uint8_t c;
-
-            c = reverse_mask_xor[destination_offset_modulo]     & *source++;
-
+        if(source_offset_modulo > 0) {
+            uint8_t c = reverse_mask_xor[destination_offset_modulo] & *source++;
             PREPARE_FIRST_COPY();
             *destination++ |= c;
         }
@@ -71,35 +62,36 @@ static void bitarray_copy(const uint8_t* source_origin, int source_offset,
         int byte_len = source_length / CHAR_BIT;
         int source_length_modulo = source_length % CHAR_BIT;
 
-        if(byte_len) {
+        if(byte_len > 0) {
             memcpy(destination, source, byte_len);
             source += byte_len;
             destination += byte_len;
         }
-        if(source_length_modulo) {
-            *destination     &= reverse_mask_xor[source_length_modulo];
-            *destination |= reverse_mask[source_length_modulo]     & *source;
+
+        if(source_length_modulo > 0) {
+            *destination &= reverse_mask_xor[source_length_modulo];
+            *destination |= reverse_mask[source_length_modulo] & *source;
         }
     } else {
-        int bit_diff_ls;
-        int bit_diff_rs;
+        int bit_diff_left_shift;
+        int bit_diff_right_shift;
         uint8_t c;
         /*
          * Begin: Line things up on destination.
          */
-        if (source_offset_modulo > destination_offset_modulo) {
-            bit_diff_ls = source_offset_modulo - destination_offset_modulo;
-            bit_diff_rs = CHAR_BIT - bit_diff_ls;
+        if(source_offset_modulo > destination_offset_modulo) {
+            bit_diff_left_shift = source_offset_modulo - destination_offset_modulo;
+            bit_diff_right_shift = CHAR_BIT - bit_diff_left_shift;
 
-            c = *source++ << bit_diff_ls;
-            c |= *source >> bit_diff_rs;
-            c     &= reverse_mask_xor[destination_offset_modulo];
+            c = *source++ << bit_diff_left_shift;
+            c |= *source >> bit_diff_right_shift;
+            c &= reverse_mask_xor[destination_offset_modulo];
         } else {
-            bit_diff_rs = destination_offset_modulo - source_offset_modulo;
-            bit_diff_ls = CHAR_BIT - bit_diff_rs;
+            bit_diff_right_shift = destination_offset_modulo - source_offset_modulo;
+            bit_diff_left_shift = CHAR_BIT - bit_diff_right_shift;
 
-            c = *source >> bit_diff_rs     &
-                reverse_mask_xor[destination_offset_modulo];
+            c = *source >> bit_diff_right_shift &
+                    reverse_mask_xor[destination_offset_modulo];
         }
         PREPARE_FIRST_COPY();
         *destination++ |= c;
@@ -108,10 +100,9 @@ static void bitarray_copy(const uint8_t* source_origin, int source_offset,
          * Middle: copy with only shifting the source.
          */
         int byte_len = source_length / CHAR_BIT;
-
         while(--byte_len >= 0) {
-            c = *source++ << bit_diff_ls;
-            c |= *source >> bit_diff_rs;
+            c = *source++ << bit_diff_left_shift;
+            c |= *source >> bit_diff_right_shift;
             *destination++ = c;
         }
 
@@ -119,9 +110,9 @@ static void bitarray_copy(const uint8_t* source_origin, int source_offset,
          * End: copy the remaing bits;
          */
         int source_length_modulo = source_length % CHAR_BIT;
-        if(source_length_modulo) {
-            c = *source++ << bit_diff_ls;
-            c |= *source >> bit_diff_rs;
+        if(source_length_modulo > 0) {
+            c = *source++ << bit_diff_left_shift;
+            c |= *source >> bit_diff_right_shift;
             c &= reverse_mask[source_length_modulo];
 
             *destination &= reverse_mask_xor[source_length_modulo];
@@ -136,12 +127,30 @@ uint64_t bitmask(const uint8_t numBits) {
 
 uint64_t getBitField(uint64_t data, const uint16_t startBit,
         const uint16_t numBits, bool bigEndian) {
-    uint64_t result;
+    uint8_t result[8] = {0};
+    if(!bigEndian) {
+        data = __builtin_bswap64(data);
+    }
     getBits(startBit, numBits, (const uint8_t*)&data,
             CHAR_BIT * sizeof(uint64_t),
             bigEndian ? ENDIANNESS_BIG_ENDIAN : ENDIANNESS_LITTLE_ENDIAN,
-            &result);
-    return result;
+            result);
+    // TODO the result has already been shifted to be aligned right, so if we
+    // try and bswap here it's going to be screwed up unless it was byte aligned
+    uint64_t int_result = 0;
+    // TODO should the API return the byte length of data in the result array?
+    // i think yes.
+    uint8_t byte_count = numBits / CHAR_BIT;
+    if(numBits % CHAR_BIT != 0) {
+        ++byte_count;
+    }
+
+    // TODO wow, can't believe this works, but something is clearly wrong with
+    // the API!
+    for(int i = 0; i < byte_count; i++) {
+        int_result |= result[byte_count - i - 1] << (CHAR_BIT * i);
+    }
+    return int_result;
 }
 
 /**
@@ -185,5 +194,6 @@ uint8_t getByte(const uint8_t byte_index, const uint8_t data[],
 void getBits(const uint16_t start_index, const uint16_t field_size,
         const uint8_t data[], const uint8_t length, Endianness endianness,
         uint8_t* result) {
-    bitarray_copy(data, start_index, field_size, result, 0);
+    bitarray_copy(data, start_index, field_size, result,
+            CHAR_BIT - findEndBit(start_index, field_size));
 }
diff --git a/tests/bitfield_tests.c b/tests/bitfield_tests.c
index f321e5f2..056f8c13 100644
--- a/tests/bitfield_tests.c
+++ b/tests/bitfield_tests.c
@@ -25,7 +25,7 @@ START_TEST (test_one_bit)
     uint64_t data = 0x8000000000000000;
     uint64_t result = getBitField(data, 0, 1, false);
     fail_unless(result == 0x1,
-            "First bits in 0x%X was 0x%X instead of 0x1", data, result);
+            "First bit in 0x%llx was 0x%llx instead of 0x1", data, result);
 }
 END_TEST
 
@@ -35,7 +35,7 @@ START_TEST (test_32_bit_parse)
     uint64_t result = getBitField(data, 16, 32, false);
     uint64_t expectedValue = 0x574d555a;
     fail_unless(result == expectedValue,
-            "Field retrieved in 0x%X was 0x%X instead of %d", data,
+            "Field retrieved in 0x%llx was 0x%llx instead of 0x%llx", data,
             result, expectedValue);
 }
 END_TEST
@@ -46,7 +46,7 @@ START_TEST (test_16_bit_parse)
     uint64_t result = getBitField(data, 16, 16, false);
     uint64_t expectedValue = 0xFCFF;
     fail_unless(result == expectedValue,
-            "Field retrieved in 0x%X was 0x%X instead of %d", data,
+            "Field retrieved in 0x%llx was 0x%llx instead of 0x%llx", data,
             result, expectedValue);
 }
 END_TEST
@@ -56,13 +56,13 @@ START_TEST (test_one_byte)
     uint64_t data = 0xFA00000000000000;
     uint64_t result = getBitField(data, 0, 4, false);
     fail_unless(result == 0xF,
-            "First 4 bits in 0x%X was 0x%X instead of 0xF", data, result);
+            "First 4 bits in 0x%llx was 0x%llx instead of 0xF", data, result);
     result = getBitField(data, 4, 4, false);
     fail_unless(result == 0xA,
-            "First 4 bits in 0x%X was 0x%X instead of 0xA", data, result);
+            "First 4 bits in 0x%llx was 0x%llx instead of 0xA", data, result);
     result = getBitField(data, 0, 8, false);
     fail_unless(result == 0xFA,
-            "All bits in 0x%X were 0x%X instead of 0x%X", data, result, data);
+            "All bits in 0x%llx were 0x%llx instead of 0x%llx", data, result, data);
 }
 END_TEST
 
@@ -71,19 +71,19 @@ START_TEST (test_multi_byte)
     uint64_t data = 0x12FA000000000000;
     uint64_t result = getBitField(data, 0, 4, false);
     fail_unless(result == 0x1,
-            "First 4 bits in 0x%X was 0x%X instead of 0xF", (data >> 60) & 0xF,
+            "First 4 bits in 0x%llx was 0x%llx instead of 0xF", (data >> 60) & 0xF,
             result);
     result = getBitField(data, 4, 4, false);
     fail_unless(result == 0x2,
-            "Second 4 bits in 0x%X was %d instead of 0xA", (data >> 56) & 0xF,
+            "Second 4 bits in 0x%llx was 0x%llx instead of 0xA", (data >> 56) & 0xF,
             result);
     result = getBitField(data, 8, 4, false);
     fail_unless(result == 0xF,
-            "First 4 bits in 0x%X was %d instead of 0x1", (data >> 52) & 0xF,
+            "First 4 bits in 0x%llx was 0x%llx instead of 0x1", (data >> 52) & 0xF,
             result);
     result = getBitField(data, 12, 4, false);
     fail_unless(result == 0xA,
-            "Second 4 bits in 0x%X was %d instead of 0x2", (data >> 48) % 0xF,
+            "Second 4 bits in 0x%llx was 0x%llx instead of 0x2", (data >> 48) % 0xF,
             result);
 }
 END_TEST
@@ -127,13 +127,13 @@ START_TEST (test_set_doesnt_clobber_existing_data)
     setBitField(&data, 0x4fc8, 16, 16);
     uint64_t result = getBitField(data, 16, 16, false);
     fail_unless(result == 0x4fc8,
-            "Field retrieved in 0x%X was 0x%X instead of 0x%X", data, result,
+            "Field retrieved in 0x%llx was 0x%llx instead of 0x%x", data, result,
             0xc84f);
 
     data = 0x8000000000000000;
     setBitField(&data, 1, 21, 1);
     fail_unless(data == 0x8000040000000000LLU,
-            "Expected combined value 0x8000040000000000 but got 0x%X%X",
+            "Expected combined value 0x8000040000000000 but got 0x%llx%llx",
             data >> 32, data);
 }
 END_TEST
@@ -153,14 +153,14 @@ START_TEST (test_set_odd_number_of_bits)
     setBitField(&data, 0x12, 11, 5);
     uint64_t result = getBitField(data, 11, 5, false);
     fail_unless(result == 0x12,
-            "Field set in 0x%X%X%X%X was %d instead of %d", data, result,
+            "Field set in 0x%llx%llx%llx%llx was 0x%llx instead of 0x%llx", data, result,
             0x12);
 
     data = 0xFFFC4DF300000000LLU;
     setBitField(&data, 0x2, 11, 5);
     result = getBitField(data, 11, 5, false);
     fail_unless(result == 0x2,
-            "Field set in 0x%X%X%X%X was %d instead of %d", data, result,
+            "Field set in 0x%llx%llx%llx%llx was 0x%llx instead of 0x%llx", data, result,
             0x2);
 }
 END_TEST