5

I'm writing a C program and need to create a bitwise mask that could potentially fill a computer word (64 or 32 bits) with all 1s. I found a bug in my code but I can't make sense of it because when I run the operation using literal values it gives the expected value, but when I store the parts in variables and do the same operation I get a different result.

#include <stdio.h>
#include <stdint.h>

#define min(a,b) (((a) < (b)) ? (a) : (b))

typedef uint64_t WORD;

int main() {
    
    WORD one = 1;
    
    printf("mask0=%lu\n", (one<<64) - one);
    
    uint32_t shift_amt = 2;
    uint32_t pos_in_word = 62;
    uint32_t my_min = min((uint32_t)64, pos_in_word + shift_amt);
    WORD mask = (one<<my_min) - one;
    
    printf("one=%lu, my_min=%d pos_in_word=%d, shift_amt=%d, mask1=%lu\n", 
            one, my_min, pos_in_word, shift_amt, mask);

    return 0;
}

Building with command: gcc -Wall -std=c99 main.c -lm -o main under Linux Ubuntu.

I expect the value to be 18446744073709551615 (all 1s) in both cases. In the first case, mask0=18446744073709551615. In the second case, mask1=0. I don't understand how. I'd really appreciate some help please.

3
  • This compiler gives a nice warning about how the requested shift is too large for the variable type used in the code. It might be a useful tool for you in the future when such issues arise. I'm guessing your current compiler gave you no warning for this issue. Commented Jun 15 at 3:16
  • 1
    New users rarely have their compiler warnings set to anything useful. They have to be taught, and I have yet to see a tutorial or instructional video do more than mention it, let alone demonstrate how-to. Commented Jun 15 at 4:23
  • Note that if one<<64 had been defined to have the same result as (one<<32)<<32, the result would be 0 so your (one<<64) - one is effectively the same as -one. Commented Jun 15 at 5:06

2 Answers 2

10

You're performing an invalid shift.

Shifting by a value greater than or equal to the bit size of the operand being shifted triggers undefined behavior in your code. This is spelled out in section 6.5.8p3 of the C standard:

The integer promotions are performed on each of the operands. The type of the result is that of the promoted left operand. If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined

If you had done multiple shifts to keep under this limit, you would get the expected result:

printf("mask0=%" PRIu64 "\n", ((one<<32)<<32) - one);

Note that using the wrong conversion specifier in printf also makes your program have undefined behavior, and you are consistently using conversion specifiers that may be invalid in some implementations and valid in other implementations. When printing variables of fixed width integer types, use the macros defined in <inttypes.h> to make sure you get the correct conversion specifiers. Example:

printf("one=%" PRIu64 ", my_min=%" PRIu32 " pos_in_word=%" PRIu32
       ", shift_amt=%" PRIu32 ", mask1=%" PRIu64 "\n",
       one, my_min, pos_in_word, shift_amt, mask);
Sign up to request clarification or add additional context in comments.

2 Comments

This is, IMHO, the dumbest UB spec in C. Too bad people keep discovering it the hard way...
@Dúthomhas: The problem is that CPU architectures vary in the behavior of their shift instructions. On x86 for instance, getting mathematically correct behavior for shifts would require several additional instructions, and make every non-constant shift operation several times more expensive - even if all your shifts are actually within range. This runs counter to "pay for what you use".
4

There are multiple issues in your code:

  • the macro min expands to an expression that evaluates the arguments twice, which may pose a problem if you pass arguments with side effects. You do not do this in the posted code, but you might want to check other uses in your code base... Macros with such issues used to be named in uppercase to warn programmers of the different semantics from functions. With modern compilers, defining min as an inline function is highly recommended.

  • you shift values by 64, which is the width in bits of the shifted type. This has undefined behavior. The reason for this is most recent CPUs only use the low 6 bits of the shift count for 64-bit values, resulting in no change to the argument value (shifting by 0 is a no-op) while some more ancient or exotic hardware use the full value of the shift argument and evaluate to a different value. In such ambiguous cases, the C Committee decides to make the behavior undefined to keep the translation to code as minimal as possible. Other languages do specify that such shift operations mask the shift count by 63.

  • you should use the macros defined in <inttypes.h> to pass uint32_t and uint64_t values to printf instead of hard-coding %u and %lu as some legacy systems define long as 32-bit types instead of 64-bit types.

    You should compile with extra warnings to let the compiler flag these operations as having undefined behavior.

Note that (one << 64) would be 0 if the full shift did occur, so you could just write (WORD)0 instead. In your program, you compute one << my_min, which has undefined behavior if my_min is greater or equal to 64, so you just cannot use this expression, you must add a test:

WORD mask = (my_min >= 64 ? (WORD)0 : one << my_min) - one;

Also note that printing bit patterns in hexadecimal is much more readable.

Here is a modified version:

#include <stdio.h>
#include <stdint.h>

uint32_t min_u32(uint32_t a, uint32_t b) { return a < b ? a : b; }

typedef uint64_t WORD;

int main(void) {
    
    WORD one = 1;
    
    printf("mask0=%#"PRIx64"\n", -one);
    
    uint32_t shift_amt = 2;
    uint32_t pos_in_word = 62;
    uint32_t my_min = min_u32(64, pos_in_word + shift_amt);
    WORD mask = (my_min >= 64 ? (WORD)0 : one << my_min) - one;
    
    printf("one=%#"PRIx64", my_min=%"PRIu32" pos_in_word=%"PRIu32", shift_amt=%"PRIu32", mask1=%#"PRIx64"\n", 
            one, my_min, pos_in_word, shift_amt, mask);

    return 0;
}

For your purpose, filling a word with all one bits, here are 2 simple portable methods:

  • use the bit complement operator on a suitably typed 0 value:

    WORD mask = ~(WORD)0;
    
  • even simpler: just use -1:

    WORD mask = -1;
    

7 Comments

Converting a signed type to unsigned has always been defined to give you "wrapping" behavior, on every architecture, since C89. Non-two's-complement architectures just had to deal with it and emit extra instructions as needed. So WORD mask = -1;, when WORD is an unsigned integer type, has always been guaranteed to give you the maximum value of WORD, i.e. all-bits-one. What changed in C23 was defining the behavior of converting unsigned to signed, which previously was implementation-defined and now follows two's-complement behavior.
C89 draft 3.2.1.2: "When a signed integer is converted to an unsigned integer with equal or greater size, if the value of the signed integer is nonnegative, its value is unchanged. Otherwise: if the unsigned integer has greater size, the signed integer is first promoted to the signed integer corresponding to the unsigned integer; the value is converted to unsigned by adding to it one greater than the largest number that can be represented in the unsigned integer type."
@NateEldredge: good point, I shall simplify the answer.
@chqrlie Interestingly the 8086 would correctly do a shift 0-255. With 16-bit registers, any shift past 16 was not that useful. Since a shift of n took n cycles, a bad attribute was that a shift of 255 took 255 clock ticks! This was not interruptible and made a a very bad worst case interrupt latency time. The 80186 and later only use the least 3,4 and later (80x86) 5, 6 bits to perform the shift. Later processors also used a barrel shifter requiring only O(1) time rather than O(n).
@ARetha: it works as long as you do not pass expressions with side effects. For example min(rand(), 10000) will expand to (((rand()) < (10000)) ? (rand()) : (10000)): you can see that rand() get evaluated twice if its first return value was less than 10000 and the second call might return a value larger than 10000, defeating the purpose of the min macro. No such problem with an inline function. There are ways to define type generic functions in C now, but nothing as simple as these macros. Same problem with min(*p++, *q++): p or q gets incremented twice and the result is wrong
|

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.