Avoid overflows in left shifts

[ilammy]

Left-shifting uint8_t types kinda works because of integer promotion rules. However, this is incorrect on platforms with 16-bit int types. We should explicitly use 32-bit unsigned integers to get the correct behavior on all platforms (and avoid warnings by undefined behavior sanitizer).

[Lagovas]

I don't understand why we should use 32 bit variables for 8bit results? we use explicitly defined 32 bit variable which stores 32 bits and then we place each byte to separate 1 byte variable. where is here undefined behavior? Anyway, we need here 32 bits to store crc32. And we don't tell that we support 16-bits platforms, so it's not our problem that old platform can't work with *32_t types.
@vixentael what do you think?

[ilammy]

@Lagovas, the issue is with this expression:

result = ((byte0 << 24) | (byte1 << 16) | (byte2 << 8) | byte3);

Regardless of the result type, if byteN values are smaller than int then they are first extended to int (or unsigned int if they are unsigned), then all the shifts and bitwise operations are performed, and only then the result is extended or truncated to the result type. (These are know as integer promotion rules in the standard.)

This means that if byteN are uint8_t and unsigned int is 16-bit then the compiler is free to interpret the shifts as unsigned left shifts of 16-bit values, making << 16 and << 24 technically undefined behavior as these are shifting more that the width of the left operand.

Obviously, it works as intended even with uint8_t on x86 and ARM because of the way left shifts are implemented there,* but strictly speaking this may cause undefined behavior and sanitizers do warn about that.

* It’s not like that. After reading through Intel’s manual it seems that if the shifts are** interpreted as 16-bit on x86 then the byte values will be really lost and the carry flag will contain some undefined value. ARM is the same if it has 16-bit registers.

** gcc and clang do not generate 16-bit code anymore, so again we’re ‘safe’.

Switching the types to uint32_t is the cleanest way to avoid this issue. Keeping the variables uint8_t and inserting explicit casts into that expression will work too, but that’s less readable and produces the same machine code.

[Lagovas]

but our left operand has enough size to store result value. result defined as 32 bit variable, not 16.

[ilammy]

The type of the result is not important here. Even if it were bool the computation should be first performed with extended integer values and only then converted to the result type. It’s the types of the operands that matter. If they are uint32_t then on platforms with 32-bit int types no promotions are performed, and on 16-bit platforms the operations are performed with 32-bit values (with whatever tricks the compiler uses for that).

[vixentael]

we don't support 16-bit platforms, we haven't tried even testing on 'em