Warnings Series - Return Type

In this series of posts I'm discussing how compiling with multiple compilers can help to catch errors at development time. In the last post I discussed how to detect out of range errors. In this post, I'm going to look at turning some warnings into errors. As an example, I'm using the first warning that I promote to an error on every project I work on.

Background

As discussed in my previous posts, you can enable warnings under GCC and Clang using the -W flags. This can turn on a group of warnings, for example using -Wall, or an individual warning like -Wmaybe-uninitialized. If you want to be stricter, you can promote those warnings to errors that will stop your program from compiling. This can be done by adding -Werror= as a compiler flag, e.g. -Werror=maybe-uninitialized will make unitialized warnings into errors.

Return Type

All compilers have a warning regarding missing the return statement from a function. Consider the following code:

1
2
3
4
5
6
7
8
9
int* allocate_int() {
    int ret = new int(0);
}
 
int main() {
    int* x = allocate_int();
    std::cout << *x;
    delete x;
}

As you can see if you the function allocate_int is missing a return statement, therefore the variable x is undefined. This can then introduce strange logic errors that can be different between builds and runs of your program. One of the first times I ran into this error, the symptom of it was that if logging was enabled a variable was true and the program worked. If logging was disabled, an error was always returned as the variable was false.

Since then, this is always the first warning that I promote to an error on all projects I work on. For new projects, it prevents the error from being introduced. For legacy projects, it helps find and prevent subtle errors. In most legacy projects I've worked on, enabling this error has resulted in a failed compile.

Note: There is no return statement on the main function and this doesn't produce a warning or error. The reason for this is that the C++ standard specifies that main will implicitly return 0 if there is no return statement. This implicit return has resulted in papers such as p1276 calling for main to allow void.

Catching the error

Clang

When compiling with Clang and this is a warning by default when using no compiler flags. To promote the warning to an error you can use -Werror=return-type.

GCC

For GCC the warning is not enabled by default in older versions of the compiler. It is included as part of the -Wall flag or can be enabled individually using -Wreturn-type. To promote it to an error you can use -Werror=return-type.

Note: From v8.0 of GCC you will receive a warning with no additional compiler flags, however, you must still use -Werror=return-type to cause a compilation error.

MSVC

With MSVC this results in a compilation error by default.

Comparing the errors

To view the errors from each compiler you can use godbolt to compare the output.

Warnings Series - autological out of range

In this series of posts I'm discussing how compiling with alternative compilers can help to catch errors at development time. In the last post I discussed how to detect uninitialized errors. In this post, I'm going to look at some logic errors related to using incorrect types. The type of error in this post was encountered when consuming a library and ended up in production code.

Background

For this example, we have a library that returns a result as a uint8_t. The values to return come from defines in the header file.

1
2
3
4
5
6
// coin.h 
#define COIN_HEAD 0
#define COIN_TAIL 1
#define COIN_ERR 0xFFFF
 
uint8_t get_coin(const std::string& coin);

Autological Out of Range

The code to consume the above header file is as follows:

1
2
3
4
5
6
7
8
9
// game.cpp
int main() {
    auto c = get_coin("head");
    if(c != COIN_ERR) {
        std::cout << "valid coin " << c << std::endl;
    }
 
    return 0;
}

In the above code, we get the number representation of a coin face based on the string. If the string is valid, it is printed, otherwise we do nothing.

This code compiles with no warnings on GCC with -Wall

Problems with the code

The problem with the above example is that if you pass an unknown string into get_coin, the return will convert the COIN_ERR definition from 0xFFFF into 0xFF. When you then do the comparison c != COIN_ERR, it is always true because c can never be equal to 0xFFFF. This is caused because COIN_ERR and the return value of get_coin are incompatible.

Catching the error

Clang

When compiling with Clang and any warning level you will get the following warning:

1
2
3
4
<source>:35:10: warning: comparison of constant 65535 with expression of type 'unsigned char' is always true [-Wtautological-constant-out-of-range-compare]
    if(c != COIN_ERR) {
       ~ ^  ~~~~~~~~
1 warning generated.

GCC

As mentioned, on GCC with the compiler flags -Wall, there are no errors in the code. Enabling other warnings with -Wextra and -pedantic fails to trigger a warning.

MSVC

The warning failed to trigger on MVSC with any warning level.

Catching early

If we go back and look at the library code that this came from, the definition of get_coin is as follows:

1
2
3
4
5
6
7
8
9
10
11
// coin.cpp
uint8_t get_coin(const std::string& coin)
{
    if(coin == "head") {
        return COIN_HEAD;
    } else if(coin == "tail") {
        return COIN_TAIL;
    }
 
    return COIN_ERR;
}

If you recompile the library, this code will produce a warning on GCC, Clang, and MSVC. The Clang version of this warning is:

1
2
3
4
5
<source>:29:12: warning: implicit conversion from 'int' to 'uint8_t' (aka 'unsigned char') changes value from 65535 to 255 [-Wconstant-conversion]
    return COIN_ERR;
    ~~~~~~ ^~~~~~~~
<source>:16:18: note: expanded from macro 'COIN_ERR'
#define COIN_ERR 0xFFFF

If the original library author had paid attention to the warnings, they could have correctly caught the incompatibility in the code and fixed it before it reached our final application. This shows one of the reasons to aim for warning free code across multiple compilers.

Comparing the errors

To view the errors from each compiler you can use godbolt to compare the output.

Warnings Series - Sometimes Uninitialized

In this series of posts I'm discussing how compiling with alternative compilers can help to catch errors at development time. In the last post I discussed how to detect hidden overloads. In this post, I'm going to look at some recent errors I came across when getting a legacy library to compile under clang.

Background

The library is an old library that was written with C++98 and was in the style of "C with classes" rather than C++. When written the author followed a single return policy from functions, which was achieved by using a goto cleanup pattern. For a simple example see:

1
2
3
4
5
6
7
8
9
10
11
12
int func(int num)
{
    int ret = 0;
 
    if(num < 50)
        goto cleanup;
 
    ret = num;
 
cleanup:
    return ret;
}

The error handling was normally activated by the use of macros that would check a condition and if false, log a message, set the return code, and then goto cleanup. This programming style lead to a lot of functions that have the following pattern:

• Declare all variables at the start of the function.
• Check inputs.
• Function logic (with checks).
• Cleanup.

Uninitialized Variables

All compilers have some form of uninitialized checking in place. For example GCC, Clang and MSVC will all warn about the uninitialized use of mid in the following function:

1
2
3
4
5
6
7
8
int test(int num)
{
    int mid;
    if(num < mid)
        return 100;
 
    return 0;
}

Maybe / Sometimes Uninitialized

However, with more complex use cases some compilers can miss the use of an uninitialized variable. Below is a simplified (and nonsensical) example of triggering such a use case:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
// error handling macro
#define check(A, E)               \
    if ((A)) {                    \
        ret = E;                  \
        goto cleanup;             \
    }
 
int* get_mid() 
{
    return new int(50);
}
 
int round_100(int num) 
{
    int* mid;
    int ret = 0;
 
    check(num < 0, -1) // make sure input was valid
 
    mid = get_mid(); // get mid point
    check(mid, -1) // check mid was allocated
 
    check(num < *mid, 0)
    ret = 100;
cleanup:
    delete mid;
    return ret;
}

As mentioned above, this uses a macro for error checking and the goto-cleanup pattern of error handling. If a number is entered and is less that 0, we return -1. If it is greater than a median value (which we declare as the pointer mid), we return the value of mid. Otherwise we return 100.

Some basic unit tests for this might be created as:

1
2
3
4
5
6
7
void test_round() 
{
    assert(round_100(0) == 0);
    assert(round_100(45) == 0);
    assert(round_100(55) == 100);
    assert(round_100(1000) == 100);
}

Using the above code on GCC (with -Wall), everything compiles without warning and all tests pass.

Problem with the code

There are a lot of problems with this code including lack of RAII, use of goto, etc. but for this post I'm only going to look at the code as it currently is without a full refactor.

On the happy path, the code above works fine and does what it is supposed to do. However, if the user enters a number of less than 0 then we go to the cleanup path and attempt to delete the mid pointer. This results in us calling delete on uninitialized memory and leads to undefined behaviour.

Catching the error

Clang

If you compile the library code with clang (again with -Wall) it gets the following warning:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
<source>:24:5: warning: variable 'mid' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
    check(num < 0, -1);
    ^~~~~~~~~~~~~~~~~~
<source>:14:9: note: expanded from macro 'check'
    if ((A)) {                    \
        ^~~
<source>:30:12: note: uninitialized use occurs here
    delete mid;
           ^~~
<source>:24:5: note: remove the 'if' if its condition is always false
    check(num < 0, -1);
    ^~~~~~~~~~~~~~~~~~
<source>:14:5: note: expanded from macro 'check'
    if ((A)) {                    \
    ^
<source>:21:13: note: initialize the variable 'mid' to silence this warning
    int* mid;
            ^
             = nullptr

This warning is long but tells us where the error happens, and even gives a helpful tip on how to fix it by initializing mid to nullptr.

GCC

As mentioned, on GCC with the compiler flags -Wall, there are no errors in the code. Enabling other warnings with -Wextra, -pedantic, and even -Wmaybe-uninitialized all fail to trigger the warnings.

MSVC

On MSVC compiling with the /W4 flag, the warning appears as below:

1
2
z:\tmp\example.cpp(30) : warning C4701: potentially uninitialized local variable 'mid' used
z:\tmp\example.cpp(30) : warning C4703: potentially uninitialized local pointer variable 'mid' used

While this isn't as helpful as the clang warning, it does tell us that there is a problem with the code.

Comparing the errors

To view the errors from each compiler you can use godbolt to compare the output

Warnings Series - Hidden Overloads

In previous posts I've described how to build Clang on RHEL. This allows us to have access to an alternative compiler to help catch errors early. In this and some upcoming blog posts I'm going to go over some of the errors that using multiple compilers has helped me catch.

Hidden Overloads

Below is some example code which has multiple types of Dice. The base type is a standard 6 sided Dice and it is using inheritance to make a 20 sided Dice.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
// library code
struct Dice {
    virtual ~Dice() = default;
    // Do a "random" roll of the dice 
    virtual int roll() { return 6; }
    virtual int min() { return 1; }
    virtual int max() { return 6; }
};
 
struct TwentySidedDice : public Dice {
    virtual ~TwentySidedDice() = default;
    // Do multiple random rolls of the dice
    virtual int roll(int times) { return times * 3; }
    virtual int max() { return 20; }
};

As with all good code, we have some unit tests to make sure everything is working:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
// unit tests
void test_dice() {
    Dice die;
    assert(die.roll() == 6);
    assert(die.min() == 1);
    assert(die.max() == 6);
}
 
void test_twenty_sided() {
    TwentySidedDice die;
    assert(die.roll(4) == 12);
    assert(die.min() == 1);
    assert(die.max() == 20);
}

The above is compiled without any warnings on GCC (using -Wall) and as all unit tests pass, we feel like we can ship the code as a library.

Problem with the code

If a client attempted to use the above code, they might try something like this:

1
2
3
4
5
6
7
8
9
// client code
int main()
{
    auto die = std::make_unique<TwentySidedDice>();
    std::cout << "min is " << die->min() << std::endl;
    std::cout << "max is " << die->max() << std::endl;
    std::cout << "roll is " << die->roll() << std::endl;
    return 0;
}

On first look this code seems reasonable and looks like it should work. However, you would see the following error:

1
2
3
4
5
6
7
8
9
<source>: In function 'int main()':
<source>:43:42: error: no matching function for call to 'TwentySidedDice::roll()'
     std::cout << "roll is " << die->roll() << std::endl; // no matching function error
                                          ^
<source>:16:17: note: candidate: 'virtual int TwentySidedDice::roll(int)'
     virtual int roll(int times) { return times * 3; }
                 ^~~~
<source>:16:17: note:   candidate expects 1 argument, 0 provided
Compiler returned: 1

The reason for the error is that based on the rules of C++ overload resolution the int roll(int i); function in TwentySidedDice is hiding the int roll(); function from Dice.

Catching the error

Clang

If you compile the library code with clang (again with -Wall) it gets the following warning:

1
2
3
4
5
6
7
8
<source>:16:17: warning: 'TwentySidedDice::roll' hides overloaded virtual function [-Woverloaded-virtual]
    virtual int roll(int times) { return times * 3; }
                ^
<source>:9:17: note: hidden overloaded virtual function 'Dice::roll' declared here: different number of parameters (0 vs 1)
    virtual int roll() { return 6; }
                ^
1 warning generated.
Compiler returned: 0

This shows the error early and would let a developer know about the potential problem, so that it can be resolved before shipping to users.

GCC

As mentioned, on GCC with the compiler flags -Wall, there are no errors in the code. To enable this warning on GCC you can explicitly add the -Woverloaded-virtual flag.

MSVC

On MSVC compiling with the /W4 flag, doesn't show any warnings as the C4264 warning is off by default. To see the error you can enable warning C4264 or you can see the following using `/Wall:

1
2
3
4
<source>(16): warning C4263: 'int TwentySidedDice::roll(int)': member function does not override any base class virtual member function
<source>(18): warning C4264: 'int Dice::roll(void)': no override available for virtual member function from base 'Dice'; function is hidden
<source>(9): note: see declaration of 'Dice::roll'
<source>(7): note: see declaration of 'Dice'

Comparing the errors

To view the errors from each compiler you can use godbolt to compare the output