Saturday 6 September 2014

C++ - Integer Comparison: A Hidden Bug in Warning

Recently I was asked to review a fix on a C++ warning on Linux by one of my colleagues. Firstly I was put on a test if I can spot the bug, which is pointed out by a warning generated by gcc.

// Code 1 - warning/bug
//********************************************************************************
char buf[16];
// ......
if (buf[i] == 0xC0 && buf[i+1] == 0x80) {
    // ......
}
//********************************************************************************

The warning points to the if statement in the above code. Before I was presented the warning message, my colleague asked if I can spot the bug on that line. I wasn't able to spot it. Then I was shown the warning message,
    "warning: comparison is always false due to limited range of data type"

This message actually is very helpful. It is telling me that in the comparison statement the two data types do not match and one data type actually has limited range of representing another one. For sure buf[i] has data type of char, with 1 byte size, and by default char is singed value. (This singed/unsigned is configurable in compiler. In my case by default char/int/long/... are all signed data type.) Because signed char is the smallest integer data type, it makes me think that it must the 0xC0 and 0x80 is treated as another data type by the compiler and for sure this data type is not signed char.

1. Investigation and  Fix
After googling this warning message and further checking the standard, it turns out that these constant values, 0xC0 and 0x80, can only be treated as right-hand-value (RHS). And if not data type specified the compiler will treat them as data type of signed/unsigned int. If the constant value is less than ~2 billion, then it is treated as (signed) int and if more than that, it is treated as unsigned int. (Of course the standard allows to specify the type deterministically, for instance 0xC0L as long type, 0xC0UL as unsigned long type.)

Let's come back to the above if statement. 0xC0 and 0x80 is treated as (signedint data type by default, as they are both less than 2 billion. However buf[i] and buf[i+1] has data type of signed char, which can only represent [-128, 127]. However 0xC0 and 0x80 has value of 192 and 128 respectively, which are out of the range of signed char can represent. Therefore the comparison will be always false. The warning message is spot-on. And the fix is rather simple - casting the constant value into the data type that you want.

// Code 2 - fix of Code 1
//********************************************************************************
char buf[16];
// ......
if (buf[i] == (char)0xC0 && buf[i+1] == (char)0x80) {
    // ......
}
//********************************************************************************

However this warning is not always happening. It depends on what the constant value is. In this case if the const value is within [-128, 127], then there will be no such warning, which is expected as residing in the range of (signed) char can represent.

// Code 3 - no warning
//********************************************************************************
char buf[16];
// ......
if (buf[i] == 0x50 && buf[i+1] == 0x30) {
    // ......
}
//********************************************************************************

2. Data Type Truncation and Promotion
Be careful of these warning on data type truncation, promotion and data type casting. These kind of data type truncation and promotion is quite common in string/char/file/Unicode  manipulation.

// Code 4 - data type truncation, promotion
//********************************************************************************
char buf[] = {0x4D, 0x5A, 0x80, 0xC0}; // warning on data type truncation
                                                                // on 0x80 and 0xC0
                                                                // buf[2] and buf[3] are both negative value
int negativeC0 = buf[4]; // negativeC0 = 0xffffffC0, data promotion
int positiveC0 = 0xC0; // positiveC0 = 0x000000C0, 0xC0 default type as int
//********************************************************************************

3. MSVC Compiler
Also tried Code 1 on Miscrosoft Visual Studio Expression 2013. Unfortunately MSVC does not generate any warning to pin-point out the problem as gcc does, even though all level warnings are enabled.

Even worse MSVC generates a rather misleading warning against Code 2, "warning C4310: cast truncates constant value". This warning is rather a red-herring and does not help us to spot this bug at all. Overall MSVC compiler seems not doing a good job against this hidden bug.

4. Other Useful Threads on This Warning
http://www.linuxtopia.org/online_books/an_introduction_to_gcc/gccintro_71.html
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
http://stackoverflow.com/questions/13666033/c-truncation-of-constant-value
http://msdn.microsoft.com/en-us/library/z8f60833.aspx

No comments:

Post a Comment