Why doesn't this swap macro using shifts not work for negative numbers?

Question

I found some code in a built in library that I have and need to extend.

But it appears to be broken:

#define BSWAP16(__x)    ((((__x) >> 8) | ((__x) << 8)))

Does not function the same as:

__builtin_bswap16()

This program proves it.

#include <stdio.h>

#define BSWAP16(__x)    ((((__x) >> 8) | ((__x) << 8)))

int main(int argc, char* argv[])
{
    unsigned short a = (unsigned short)BSWAP16(0xff00);
    unsigned short b = __builtin_bswap16(0xff00);
    short c = (short)BSWAP16(-8);
    short d = __builtin_bswap16(-8);

    printf("a=%04x, b=%04x, c=%04x, d=%04x\n", a,b,c,d);
    return 0;
}

Output:

a=00ff, b=00ff, c=ffffffff, d=fffff8ff

I'm not wanting answers telling me I should use endian.h or __builtin_bswap16. On the target platform used by this in-house library on this platform/compiler configuration, I'm triggering this default code that is defaulting to using the above macro.

So my question is. Why doesn't it work for negative numbers?

If I represent -8 as a short value of 0xfff8 it works.

So I'm guessing it has something to do with internal conversion to int.

How do I fix this macro to work properly?

Shafik Yaghmour · Accepted Answer · 2014-07-04T03:14:52.543

It is undefined behavior to left shift a negative number, from the draft C99 standard section 6.5.7 Bitwise shift operators which says (emphasis mine going forward):

The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are filled with zeros. If E1 has an unsigned type, the value of the result is E1 ´ 2E2, reduced modulo one more than the maximum value representable in the result type. If E1 has a signed type and nonnegative value, and E1 ´ 2E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined.

So the result of the left shift of -8 is unpredictable. Casting to unsigned short should fix the issue:

BSWAP16((unsigned short)-8)

-8 is an integer constant(literal) and since it does not have a suffix will be an int since int can take on it's value. Assuming 32-bit int and twos complement will have have the following value:

FFFFFFF8

casting to unsigned short will remove the unwanted higher bits. Casting to unsigned int won't help since it will preserve the higher bits.

Also right shifting a negative number is implementation defined:

The result of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a signed type and a nonnegative value, the value of the result is the integral part of the quotient of E1 / 2E2. If E1 has a signed type and a negative value, the resulting value is implementation-defined.

@MattMcNabb not correct, `clang` provides the following error for the code `runtime error: left shift of negative value -8` when using `-fsanitize=undefined ` — Shafik Yaghmour, Jul 04 '14 at 02:34
ahh, thanks for that. I'm not actually bit shifting negative numbers at all. This all came about because I was being forced to cast and then wondered what would happen with negative values. Thanks for the feedback. I can avoid the cast with a bitmask. — hookenz, Jul 04 '14 at 02:36
@R.. as far as I can tell the wording is the same as C++11 and the consensus [here](http://stackoverflow.com/questions/19593938/is-left-shifting-a-negative-integer-undefined-behavior-in-c11) was that it was indeed undefined and Howard Hinnant confirmed this to be so but also said post C++11 the wording was tweaked. Also I specifically cited C99 although I don't think the wording has changed. — Shafik Yaghmour, Jul 04 '14 at 03:28

timrau · Answer 2 · 2014-07-04T02:33:23.530

2

You should mask out the unwanted bits before shifting.

#define BSWAP16(__x)    (((((__x) & 0xFF00) >> 8) | (((__x) & 0xFF) << 8)))

Regarding why it doesn't work without shifting, note that -8 is indeed 0xFFFFFFF8. Without masking, there are plenty of higher 1 bits. int is used during computation.

edited Jul 04 '14 at 02:33

answered Jul 04 '14 at 02:26

timrau

22,578
4
51
64

You know I had just figured out that masking seems to fix it although I masked it with 0xffff in both cases. Can you explain why the masking is necessary and why casting to (short) the result doesn't work? – hookenz Jul 04 '14 at 02:28
This still causes undefined behaviour if `int` is 16-bit; to get defined behaviour, convert the value to `unsigned int` before shifting. – M.M Jul 04 '14 at 02:33
@Matt the C standard does not define shifting `1`'s in or out of the sign bit (to put it informally). Masking fixes this (i'm assuming you're using 32-bit ints or larger) because after that mask is applied then only zeroes go through the sign bit. – M.M Jul 04 '14 at 02:40
@MattMcNabb, casting to unsigned int doesn't work. But casting to unsigned short does. #define BSWAP16((((unsigned short)(__x)) >> 8) | (((unsigned short)(__x)) << 8)) – hookenz Jul 04 '14 at 02:57
@Matt that version doesn't work for values greater than 255 , unless you also truncate to 16 bits afterwards – M.M Jul 04 '14 at 03:43

Why doesn't this swap macro using shifts not work for negative numbers?

2 Answers2

Linked