Add -Werror=return-type to CMakeLists.txt

[james1236]

In C++ (but not C), when a return statement is omitted in a function that is supposed to return an int, it is undefined behaviour, even if the return value isn't being accessed by the caller. A warning is produced for this (-Wreturn-type), but it's somewhat buried among the other warnings produced by the libraries in this repository (microbit-v2-samples).

Here's an example:

#include "MicroBit.h"

MicroBit uBit; // Initialize the MicroBit object

int exampleFunction1() {
    uBit.serial.printf("exampleFunction1()\n");
    // return 0; // This return statement is commented out
}

int exampleFunction2() {
    uBit.serial.printf("exampleFunction2()\n");
    // return 0; // This return statement is commented out
}

int exampleFunction3() {
    uBit.serial.printf("exampleFunction3()\n");
    // return 0; // This return statement is commented out
}

int main() {
    uBit.init();

    uBit.serial.printf("main()\n");

    exampleFunction1();
    exampleFunction2();
    exampleFunction3();
    exampleFunction2();
    exampleFunction1();
}

Serial Output With Return Statements:

main()
exampleFunction1()
exampleFunction2()
exampleFunction3()
exampleFunction2()
exampleFunction1()

Serial Output Without Return Statements:

When the return statements are commented out, you get warnings such as the following:

microbit-v2-samples/source/main.cpp: In function 'int exampleFunction1()':
microbit-v2-samples/source/main.cpp:8:1: warning: no return statement in function returning non-void [-Wreturn-type]
 8 | }
   | ^

And the program outputs:

main()
exampleFunction1()
main()
exampleFunction1()
main()
exampleFunction1()
main()
exampleFunction1()
...

It looks like it's "flowing off the end of the function" and running whatever is next in memory (in this case, instructions from the main function), which is supported by the fact that the behaviour can change when commenting out seemingly unrelated code.

The misunderstanding comes from a belief that in non-void functions, failing to return is fine as long as the return value isn't used by the caller. This isn't the case however, and it's the kind of undefined behaviour that can lead to what we saw above. To add to the confusion, this article suggests this is only undefined behaviour in C++, and not in C which students are taught.

A different article goes into more detail.

Proposed Solution

This problem is symptomatic of a greater issue, which is the long list of warnings produced by the libraries in the microbit-v2-samples repo leading to people becoming "warning blind".

To remediate just this specific issue however, we can change the warning to an error by adding -Werror=return-type to CMakeLists.txt. This would stop compilation if there's no return statement in a function that should return a non-void value. More specifically (from the gcc warning options list):

-Wreturn-type
    Warn whenever a function is defined with a return-type that defaults to int. Also warn about any
    return statement with no return-value in a function whose return-type is not void.

    For C, also warn if the return type of a function has a type qualifier such as const. Such a
    type qualifier has no effect, since the value returned by a function is not an lvalue. ISO C
    prohibits qualified void return types on function definitions, so such return types always receive
    a warning even without this option.

    For C++, a function without return type always produces a diagnostic message, even when
    -Wno-return-type is specified. The only exceptions are `main` and functions defined in system
    headers.

    This warning is enabled by `-Wall`.

The repository still builds with -Werror=return-type, but adding it may cause someone else's code that depends on this repository to stop building with the new repository version.

[microbit-carlos]

Thanks for the PR @james1236!
I think a better approach might be to fix the warnings produced in the samples and CODAL repositories, since as you've mentioned, making warnings errors could break existing projects.

This is something that has been bothering me for a while as well, so I've done the first step in f0c42cb, and will look into silencing the warnings from the codal-microbit-nrf5sdk repository (as that's the Nordic SDK code and we won't be touching that unless strictly necessary).

If you have any other warnings in this repository, or the other CODAL dependencies (for example, by using a different/newer compiler than mine, arm-none-eabi-gcc 10.3) please let me know and we can look into fixing those as well.

[microbit-carlos]

I've raised this PR in the sdk repo:

• Suppress all warnings from this target as it's 3rd party code. microbit-foundation/codal-microbit-nrf5sdk#18

@james1236 if you'd like to review it and test it that would be great!

[james1236]

@microbit-carlos Thank you for looking into this! I spent a bit of time before trying to eliminate warnings in those libraries, however, I believe this repository fetches those libraries at specific old commit IDs, so the changes wouldn't be seen here. I suppose we could change the build script here to fetch the new version, but I was worried that could break things even further than just adding -Werror=return-type.

I'll have a look at your PR!

[microbit-carlos]

The current codal.json file points to the main branch in codal-microbit-v2, so in a brand new build it pulls the latest version from there. Then codal-microbit-v2 points to specific commits for its dependencies, which are listed in its target-locked.json file.

Normally to do a "clean" build I essentially delete the build and libraries folders, and let build.py to git clone them again (there are also specific options in build.py to pull the latest changes, but I this method is simpler for me personally). Otherwise, it is easy to end up with old commits in the libraries directory.

Every time we make a new codal-microbit-v2 release/tag it should update the commits for all its dependencies (codal-core, codal-nrf52, codal-microbit-nrf5sdk), so the next codal-microbit-v2 tag should include the latest changes as well.
In the meantime you can checkout any commit you like after a first build (when libraries directory is populated) and rebuild.

Hope that helps!