0

I have the following piece of code in C where I am trying to extract specific bits from values stored in val1 and val2 respectively and then store back the decimal equivalent of extracted bit to val1 and val2 for later comparison of val1 and val2. nbits gives the range of bits to be extracted here for example from 2 to 2 meaning just extract the third bit.

static unsigned int BIT[] = {
0x1,        0x2,        0x4,        0x8,
0x10,       0x20,       0x40,       0x80,
0x100,      0x200,      0x400,      0x800,
0x1000,     0x2000,     0x4000,     0x8000,
0x10000,    0x20000,    0x40000,    0x80000,
0x100000,   0x200000,   0x400000,   0x800000,
0x1000000,  0x2000000,  0x4000000,  0x8000000,
0x10000000, 0x20000000, 0x40000000, 0x80000000
};
int val1, val2;
unsigned long mask_val;
int nbits[2];

nbits[0] = nbits[1] = 2;
val1 = -41;
val2 = -45;

for (i = nbits[0]; i <= nbits[1]; i++)
     mask_val = mask_val|BIT[i];
val1 = (val1 & mask_val) >> nbits[0];
val2 = (val2 & mask_val) >> nbits[0];

This piece of code above works as desired for a 32 bit machine but gives wrong results for a 64 bit machine specially when the initial values stored in val1 and val2 are negative. What changes do I have to make to my code above to make it machine independent?

srsci
  • 239
  • 5
  • 10
  • 3
    `int` and `long` types are not portable. Use the `stdint.h` types, instead: `int32_t`, `uint32_t`, `int64_t`, and `uint64_t`. – sfstewman Jun 27 '12 at 18:20
  • @sfstewman That seems like the right answer to me, so you should probably post it as such, not just a comment – Dan F Jun 27 '12 at 18:22
  • 2
    Not initializing this makes me uncomfortable: `unsigned long mask_val;` but I don't know if it is causing any of your problems. – Justin Jun 27 '12 at 18:23
  • Also why is your mask (potentially) larger than your vals? If you have a certain size val, it is no use having a larger mask is it? – Justin Jun 27 '12 at 18:39

4 Answers4

3

1, not initializing mask_val will cause you problems on many compilers 2, use stdint.h and it's types for vals and mask to be system independent

int8_t
int16_t
int32_t
uint8_t
uint16_t
uint32_t
int64_t
uint64_t

3, Why not just bitshift from 0x1 instead of using the table. ie

mask_val = mask_val| ( 0x1 << (i - 1) );
8bitwide
  • 2,071
  • 1
  • 17
  • 24
  • Better yet: `mask_val = ~((1U << nbits[0]) - 1) & ((1U << nbits[1]) - 1);` and just get rid of the `for` loop. – twalberg Jun 27 '12 at 18:47
  • I also found that changing the datatype of val1 and val2 to unsigned int solved the problem – srsci Jun 27 '12 at 18:53
  • Actually, minor error with that expression. I think this works better: `mask_val = ~((1U << nbits[0]) - 1) & ((1U << (nbits[1] + 1)) - 1);`. The first one failed when `nbits[0] == nbits[1]`. – twalberg Jun 27 '12 at 18:58
  • @srsci, just curious are you aware that ints are stored in 2's compliment(http://en.wikipedia.org/wiki/Two's_complement). I ask because I am get the feeling from your above post your expecting -42 to contain the bit pattern of binary 42. – 8bitwide Jun 27 '12 at 18:58
  • yes i realized my foolishness in this regard. I need to do more reading regarding this. Thanks for pointing this out to me – srsci Jun 27 '12 at 19:03
1

While I was developing various versions of your code, others essentially stated the same things I show below.

I think the reason your code worked on the 32-bit version was a fluke; apparently on that system your uninitialized mask_val just happened to be zero, you were "lucky".

Below I show a progression of three versions of your own code. First shows the code essentially unchanged, but initializing mask_val to 0, and using a typedef for whatever type you want the code to run with. Second is the same, but without the BIT[] table. Third eliminates the loop.

#if 0
#include <stdio.h>

typedef long long valTy;

int main()
{
static unsigned int BIT[] = {
0x1,        0x2,        0x4,        0x8,
0x10,       0x20,       0x40,       0x80,
0x100,      0x200,      0x400,      0x800,
0x1000,     0x2000,     0x4000,     0x8000,
0x10000,    0x20000,    0x40000,    0x80000,
0x100000,   0x200000,   0x400000,   0x800000,
0x1000000,  0x2000000,  0x4000000,  0x8000000,
0x10000000, 0x20000000, 0x40000000, 0x80000000
};
valTy val1, val2;
valTy mask_val;
int i, nbits[2];

printf("\n");

nbits[0] = nbits[1] = 2;
val1 = -41;
val2 = -45;

printf("    val1 = 0x%016llx (%lld)\n", (long long)val1, (long long)val1);
printf("    val2 = 0x%016llx (%lld)\n", (long long)val2, (long long)val2);
printf("\n");

mask_val = 0;
for (i = nbits[0]; i <= nbits[1]; i++)
    mask_val = mask_val|BIT[i];
printf("mask_val = 0x%016llx\n\n", (long long)mask_val);
val1 = (val1 & mask_val) >> nbits[0];
val2 = (val2 & mask_val) >> nbits[0];

printf("    val1 = 0x%016llx (%lld)\n", (long long)val1, (long long)val1);
printf("    val2 = 0x%016llx (%lld)\n", (long long)val2, (long long)val2);
printf("\n");
}

#endif




#if 0
#include <stdio.h>

typedef long long valTy;
#define BIT(bitnum) (1<<bitnum)

int main()
{
valTy val1, val2;
valTy mask_val;
int i, nbits[2];

printf("\n");

nbits[0] = nbits[1] = 2;
val1 = -41;
val2 = -45;

printf("    val1 = 0x%016llx (%lld)\n", (long long)val1, (long long)val1);
printf("    val2 = 0x%016llx (%lld)\n", (long long)val2, (long long)val2);
printf("\n");

mask_val = 0;
for (i = nbits[0]; i <= nbits[1]; i++)
    mask_val = mask_val | BIT(i);
printf("mask_val = 0x%016llx\n\n", (long long)mask_val);
val1 = (val1 & mask_val) >> nbits[0];
val2 = (val2 & mask_val) >> nbits[0];

printf("    val1 = 0x%016llx (%lld)\n", (long long)val1, (long long)val1);
printf("    val2 = 0x%016llx (%lld)\n", (long long)val2, (long long)val2);
printf("\n");
}

#endif

#include <stdio.h>

typedef long long valTy;
#define BIT(bitnum) (1<<bitnum)

int main()
{
valTy val1, val2;
valTy mask_val;
int i, nbits[2];

printf("\n");

nbits[0] = nbits[1] = 2;
val1 = -41;
val2 = -45;

printf("    val1 = 0x%016llx (%lld)\n", (long long)val1, (long long)val1);
printf("    val2 = 0x%016llx (%lld)\n", (long long)val2, (long long)val2);
printf("\n");

mask_val = 0;
{
    valTy maskTop = ((BIT(nbits[0])-1)<<1)|1;
    valTy maskBottom = BIT(nbits[1])-1;
    mask_val = maskTop & ~maskBottom;
}
printf("mask_val = 0x%016llx\n\n", (long long)mask_val);
val1 = (val1 & mask_val) >> nbits[0];
val2 = (val2 & mask_val) >> nbits[0];

printf("    val1 = 0x%016llx (%lld)\n", (long long)val1, (long long)val1);
printf("    val2 = 0x%016llx (%lld)\n", (long long)val2, (long long)val2);
printf("\n");
}
phonetagger
  • 7,701
  • 3
  • 31
  • 55
  • Yeah, that's the conclusion that I came to. Negative values of `val1` and `val2` cause the higher-order bits to be set (twos-complement notation and all), and are probably exposing an uninitialized `mask_val`. – sfstewman Jun 27 '12 at 19:18
0

the code is still working as written but I suspect your sign bit is higher than the 32 bit mark. Please refer to this post and let me know if it is helpful: What is the bit size of long on 64-bit Windows?

Community
  • 1
  • 1
Ryan
  • 2,755
  • 16
  • 30
0

I think your problem comes from the fact that mask_val isn't initialized. You should initialize it with 0 in order to ensure no bit is set before your loop starts.

Nicolas Bachschmidt
  • 6,475
  • 2
  • 26
  • 36