1

Okay,

I wrote a function that takes an unsigned char from a hex file and then shifts it to the left to fit inside a WORD,DWORD or QWORD, like follows:

retVal |= ((unsigned char)(memory[i]) << (8 * j));

(inside a loop, hence variables i and j).

Now visual studio reminds me of possible arithmetik overflow.

My question: If I limit j to be never more than 8 (size of a uint64_t), can I safely ignore this message? I am always a little bummed about warnings and try to eliminate them.

In my understanding it should not matter how much you shift to the left before saving the value, am I mistaken?

EDIT:

here is an example (it's my function):

int getValuePNTR(const char* memory, int &start, int size)
{
    uint64_t retVal = 0;

    //now just add up array fields 
    for (int i = start + size-1,j = size-1; j >= 0; --j, i--)
    {
        //fprintf(stdout, "\ncycle: %d, memory: [%x]", j, memory[i]);

        if ((unsigned char)memory[i] == 00 && j > 0)
            retVal <<= 8;
        else
            retVal |= ((unsigned char)(memory[i]) << (8 * j));
    }
    //get the next field after this one
    start += size;
    return retVal;
}
clockw0rk
  • 576
  • 5
  • 26
  • For cases when `j` equals `8`, what bit position will the originally 7th bit occupy? – JohnFilleau Jan 27 '20 at 21:38
  • 1
    How confident are you that your compiler is warning you about the left shift operation, and not about the `|=` operator? Or, how confident are you that your compiler is not warning you about an overflow due to the left-handside getting promoted only to `int`, which is likely to be only a 32 bit value, and not a 64 bit value as you're assuming? – Sam Varshavchik Jan 27 '20 at 21:39
  • I'm not able to reproduce the warning. Can you share a [MCVE]? Are you sure that this line is the one that is producing a warning? – François Andrieux Jan 27 '20 at 21:47
  • @SamVarshavchik as you can see, I'm using a uint64_t , not a normal DWORD – clockw0rk Jan 27 '20 at 22:58
  • Hm, you wrote that comment ***after*** the answer was already posted, which clearly explained the error in your understanding of integer promotion rules. No, you are not using 64 bit values in the shift operation. – Sam Varshavchik Jan 27 '20 at 23:01

1 Answers1

4

You need to limit (8 * j) to be less than sizeof(int) * CHAR_BIT in order to keep you code legal in all cases (assuming a standard x86-64 implementation).

First, when you do (unsigned char)(memory[i]) << (8 * j) integer promotion happens and then type of the expression is the type of the promoted left hand side. In this case unsigned char is promoted to int if sizeof(unsigned char) < sizeof(int) and unsigned int otherwise.

Then [expr.shift]/1 has

The behavior is undefined if the right operand is negative, or greater than or equal to the width of the promoted left operand.

Which is the reason why (8 * j) needs to be less than sizeof(promoted_type) * CHAR_BIT

NathanOliver
  • 171,901
  • 28
  • 288
  • 402
  • okay, I have a limit build in, I expanded the question to include actual code. I have macros for the size-argument, and the biggest value it can take per makro is 8. so, if i understood you correctly, this should be fine then? – clockw0rk Jan 27 '20 at 23:00
  • 1
    @clockw0rk it depends on what `j` will be. For your platform `8 * j` needs to be less than 32 or you'll have undefined behavior. – NathanOliver Jan 27 '20 at 23:28