feat: add checked return values diagnostic

[camshaft]

Description of changes:

Currently, if you use POSIX_BAIL in a function that returns an unsigned integer, it will not fail. This is a problem as the -1 is cast to the maximum value for the integer:

uint8_t my_fun()
{
    POSIX_BAIL(MY_ERROR); // will not emit a warning
}

Luckily, some versions of C compilers provide a way to add diagnostics to a specific region of code. In this case, we wrap the return -1 in a -Wconversion diagnostic and now the code above will fail. See a working example here.

Call-outs:

This depends on the work in #3921.

I refactored and moved the s2n_extensions_client_key_share_size function, which was only used in a single test. This was the only function in the codebase that failed this check (which is good!).

Testing:

All of the existing tests should be sufficient to test for this, since it's just a compilation flag.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[camshaft]

fun fact: <GCC5 doesn't implement diagnostic pop correctly and preserves the configuration for the rest of the file: https://godbolt.org/z/qdKrhWMo6

[goatgoose]

Is there ever an instance where we would want to use GCC diagnostics where diagnostic pop isn't supported? Like, should checking if diagnostic pop works correctly just be a part of the normal GCC diagnostic feature probe?

[camshaft]

Great question (and this ties into your other question). There's two different uses of the diagnostics in the codebase: you can ignore warnings that are on by default and you can turn on warnings that are off by default. In the case of ignoring warnings, we want to include as many compilers in that list as possible, otherwise we'd get warnings on those platforms and fail the build (since -Werror is on). The worst case is we ignore warnings later on, but since we're also testing on platforms that do support popping diagnostics, it's not going to affect those.

On the other hand, we definitely can't turn on warnings for compilers that don't support pop without fixing every instance in the file after that triggers the warning.

[goatgoose]

Ah okay, that makes sense. Thanks!

[lrstewart]

How many of our CI jobs use GCC that supports these checks? A few, most, or all? Since this is on by default, I'm a little worried about a function in some ifdef block not getting caught until a customer uses a modern compiler.

But I guess that's better then their application running with the type conversion...

[camshaft]

TODO: rename the two probes to PUSH_SUPPORTED and POP_SUPPORTED so it's clear how to use them correctly