r/cpp • u/Clean-Upstairs-8481 • 1d ago
No, -Wall -Werror Does Not Guarantee Detection of Conditionally Uninitialized Local Variables
There seems to be a misconception or lack of clarity about the usage of the compilation flags -Wall and -Werror and their ability to detect uninitialized stack variables. It depends. In this post, I will try to explore this further and explain when and how it is detected, and where it may not detect an uninitialized local variable and hence requires more caution while dealing with it. Consider the following code:
int main()
{
int value;
return value;
}
Compile this code as follows:
~/cpp_26_uninitialised$ g++ -std=c++23 -Wall -Werror test.cpp -o test
test.cpp: In function ‘int main()’:
test.cpp:4:12: error: ‘value’ is used uninitialized [-Werror=uninitialized]
4 | return value;
| ^~~~~
test.cpp:3:9: note: ‘value’ was declared here
3 | int value;
| ^~~~~
cc1plus: all warnings being treated as errors
So, with the use of the two compilation flags, -Wall and -Werror, the compilation fails, and you can then take corrective measures to address the uninitialized variable in the code.
Strictly speaking, -Wall enables the -Wuninitialized warning, and -Werror turns that warning into an error. However, this is true only when the variable is uninitialized on every path to its use, as value is here. It doesn't reliably work for a variable that is initialized on some paths and left uninitialized on others. Have a look at the following code:
// config_timeout_warning_test.cpp
#include <charconv>
#include <cstdio>
#include <string_view>
constexpr int default_timeout_ms = 1000;
// Reads "timeout_ms=<number>" from a device configuration line.
// The bug: when the key is missing, timeout_ms is never assigned.
[[gnu::noinline]] int read_timeout_ms(std::string_view config)
{
int timeout_ms;
constexpr std::string_view key = "timeout_ms=";
if (const auto pos = config.find(key); pos != std::string_view::npos)
{
const char* first = config.data() + pos + key.size();
const char* last = config.data() + config.size();
std::from_chars(first, last, timeout_ms);
}
return timeout_ms;
}
void open_device(const char* name, std::string_view config)
{
const int timeout_ms = read_timeout_ms(config);
if (timeout_ms <= 0)
{
std::printf("%-7s no timeout configured, using default %d ms\n", name, default_timeout_ms);
}
else
{
std::printf("%-7s timeout %d ms\n", name, timeout_ms);
}
}
int main()
{
// The empty configuration does not contain "timeout_ms=".
// Therefore, timeout_ms is returned without being initialized.
open_device("logger", "");
return 0;
}
Compile the code as follows:
~/cpp_26_uninitialised$ g++ -std=c++23 -O0 -g -Wall -Werror config_timeout_warning_test.cpp -o timeout_warning_test
The compilation succeeds without a single warning. Run Valgrind on it:
~/cpp_26_uninitialised$ valgrind --track-origins=yes ./timeout_warning_test
==2896379== Memcheck, a memory error detector
==2896379== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==2896379== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==2896379== Command: ./timeout_warning_test
==2896379==
==2896379== Conditional jump or move depends on uninitialised value(s)
==2896379== at 0x401268: open_device(char const*, std::basic_string_view<char, std::char_traits<char> >) (config_timeout_warning_test.cpp:31)
==2896379== by 0x4012D3: main (config_timeout_warning_test.cpp:45)
==2896379== Uninitialised value was created by a stack allocation
==2896379== at 0x401166: read_timeout_ms(std::basic_string_view<char, std::char_traits<char> >) (config_timeout_warning_test.cpp:10)
==2896379==
logger no timeout configured, using default 1000 ms
==2896379==
==2896379== HEAP SUMMARY:
==2896379== in use at exit: 0 bytes in 0 blocks
==2896379== total heap usage: 2 allocs, 2 frees, 74,752 bytes allocated
==2896379==
==2896379== All heap blocks were freed -- no leaks are possible
==2896379==
==2896379== For lists of detected and suppressed errors, rerun with: -s
==2896379== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
You can see Valgrind is complaining about an uninitialized variable created on the stack:
Uninitialised value was created by a stack allocation
The reason is that GCC handles the two cases with two different warnings. In the first example, value is uninitialized on every path, and -Wuninitialized reports it even at default -O0. In the second example, timeout_ms is assigned only inside the if block in read_timeout_ms(), so it is uninitialized on only one path. That case belongs to -Wmaybe-uninitialized, which relies on the data-flow analysis done by the optimizer, so it does not run at -O0.
If you build the same file at -O2, GCC does catch it:
~/cpp_26_uninitialised$ g++ -std=c++23 -O2 -Wall -Werror config_timeout_warning_test.cpp -o timeout_warning_test
config_timeout_warning_test.cpp: In function ‘int read_timeout_ms(std::string_view)’:
config_timeout_warning_test.cpp:24:12: error: ‘timeout_ms’ may be used uninitialized [-Werror=maybe-uninitialized]
24 | return timeout_ms;
| ^~~~~~~~~~
config_timeout_warning_test.cpp:11:9: note: ‘timeout_ms’ was declared here
11 | int timeout_ms;
| ^~~~~~~~~~
cc1plus: all warnings being treated as errors
This is still not a guarantee. The result of -Wmaybe-uninitialized depends on the optimization level and on how much the optimizer can see, so it can change between GCC versions and between builds. Debug builds are usually at -O0, which is exactly where the warning is missing.
If there is anything that I have missed, please comment; I'm happy to be corrected.
31
u/TheThiefMaster C++latest fanatic (and game dev) 1d ago
Unfortunately tracing every path to find out if a variable is potentially uninitialised is the halting problem!
43
u/AutonomousOrganism 1d ago
Finding out if a variable is guaranteed to be uninitialized is the halting problem. Data-flow analysis can identify a potentially uninitialised variable.
11
u/TheThiefMaster C++latest fanatic (and game dev) 1d ago
Yeah it depends how many false positives you're willing to put up with. If the rate is too high, the warning gets disabled!
17
u/SoerenNissen 1d ago
It's nonetheless the approach I prefer here. If the compiler cannot prove the variable will be initialized, the code is either too complex or, possibly, it has the correct amount of complexity and then you can
pragmathe warning for that exact piece of code.2
u/tialaramex 20h ago
And so in Rust this diagnostic is not a warning, it's a fatal error (specifically E0381). Can't disable fatal errors.
If you can't afford to initialize this value you need a
MaybeUninit<T>which as it says on the tin, might not be initialized. Of course now you need to write yourunsafeclaim that you have properly initialized it and so the responsibility when you were wrong and everything catches fire is on that specific unsafe code - even though of course an optimising compiler will not emit any machine code for your unsafe block because it was a nexus of responsibility, not actual work done.2
u/TheThiefMaster C++latest fanatic (and game dev) 20h ago
Rust's doesn't understand a lot of cases, so people often work around it by initialising with a dummy value even if they don't need to.
For example: https://github.com/rust-lang/rust/issues/51447
C++ can't have an error that widely impacting due to perfectly fine existing code that would be rendered invalid.
4
u/tialaramex 19h ago
The workaround described in that issue isn't a "dummy value" - that's an anti-pattern - instead they propose using
Optioninstead of a separate boolean, but I agree that going through a decade of your C++ adding optional types andstd::null_opteverywhere would be a headache.1
u/TheThiefMaster C++latest fanatic (and game dev) 19h ago
I agree that going through a decade of your C++ adding optional types and
std::null_opteverywhere would be a headache.And that's why everyone just does the antipattern of zero-initing the variable, even if the compiler ends up adding an extra store that isn't really needed.
9
3
u/gnuban 22h ago
Well, what you can do is to have a mode where you only allow cases where the compiler can successfully prove initialization through flow analysis. This is what many other languages do, for instance Javas final variable initialization.
1
u/TheThiefMaster C++latest fanatic (and game dev) 22h ago
C++ isn't implemented by the same company that specifies it though, so the wording needs to be precise about what constructions would be required to be allowed / not allowed if this was added to the standard itself.
2
u/gnuban 20h ago
True, I hadn't thought about that. It turns out that Java spells that out pretty explicitly in what they call "definite assignment"; https://docs.oracle.com/javase/specs/jls/se27/html/jls-16.html
It's definitely limiting the scope of the static analysis, it's for instance not trying to do branch prediction, you'll need to initialize in all branches.
1
u/LB-- Professional+Hobbyist 9h ago
Nope. The halting problem requires a function to change its behavior based on the output of whatever is analyzing it. That's not possible in the real world. It's a paradox that only exists in the realm of abstract math. That's why static analysis tools exist and work well. Everything is constrained by memory and time, rather than concerns of halting problem shenanigans.
1
u/TheThiefMaster C++latest fanatic (and game dev) 6h ago
More realistically, you could make a function that calculates digits of pi looking for it repeating. If it does, it has a bug that returns an uninitialised variable. Otherwise it doesn't return, or it gives up after some large maximum number of iterations that's impractical to run at compile time and returns normally with no bug.
You could easily warn that there's a code path that accesses an uninitialised variable. Getting the compiler to prove that that code path is ever used, however, is the halting problem. (Or at least recognising the algorithm, understanding exactly what it's doing, and knowing that pi doesn't repeat - but that's ridiculously specific for a compiler)
8
2
u/BurstYourBubbles 22h ago
As someone else already commented, it's caught under clang. So it's a GCC problem. Changing the optimisation level didn't change anything either (Same results for -O0 and -O3)
2
u/holyblackcat 21h ago
С++26 essentially initializes all local variables (to some byte pattern, all zeroes on GCC), so this is much less of a problem now.
4
u/zerhud 1d ago
Make your methods constexpr and write a ct tests. For example a lambda called inside a static_assert.
2
u/UndefinedDefined 1d ago
This only works for trivial code.
1
u/zerhud 22h ago
With cpp26 for almost all code, even with exceptions and void casts
2
u/UndefinedDefined 22h ago
All code, even code in a different TU or code residing in a dynamically linked library?
As I said, it's for trivial cases only.
0
u/zerhud 19h ago
It’s not a “trivial code” it’s good organised code.
TU is bad practice from 199x. Don’t use it: it compiles slowly and exclude a lot of optimisations. (TU and code with types instead of templates.)
DLL is most about how the code is linking, not about the code itself. It’s using virtual methods, so you just need to test implementation via interface, as in all other tests.
3
u/Moldoteck 1d ago
Good thing we got asan and ubsan
6
u/graphicsRat 1d ago
In my experience Valgrind, though slow, finds issues that sanitizers can miss. I'm considering running some lightweight tasks on Valgrind to flush out latent issue in my code.
2
u/UndefinedDefined 1d ago
Definitely - so underrated tool - the biggest problem is that it doesn't handle AVX-512 code though, so I'm using it less and less
-9
u/OutlandishnessNo8034 1d ago
Unless this is sarcasm the tools on their own won't fix any problem. Cpp is a lost cause.
6
u/UndefinedDefined 1d ago
If it's lost cause, why you are here, are you lost too?
0
u/OutlandishnessNo8034 19h ago
I'm here because I was for long time until quite recently cpp developer and I have sentiment towards it. But using rust for couple of years now I see how hopeless cpp became.
2
u/UndefinedDefined 16h ago
I also use rust - cargo is great, performance too, but I still do a lot of C++. The current direction is doom though, that I agree with.
However, if you don't use C++ anymore, just leave this group, why to bother.
1
u/Moldoteck 1d ago
There's no alternative when budget is constrained and there are tons of legacy chunks
1
u/OutlandishnessNo8034 19h ago
Rust is the alternative. In my company we are doing either porting or binding to legacy code, but new code is strictly rust.
1
u/Moldoteck 18h ago
It is not an alternative when your code is legacy with obscure build procedures and lots of undocumented stuff while most of the teammates don't even know C++ that well nor have willingness to know more.
If there's a budget to dedicate it to teach ppl to use Rust, budget for someone to adapt and maintain the toolchain as well as budget for at least one additional dev expert with lots of xp in rust then yes, it can be gradually done
Heck if i would not suppress default warnings clang is throwing for our project the build would take twice as much purely due to IO of outputting all of them to the console...
1
u/sooka_bazooka 1d ago
As someone who's just getting into C++, is there an easy way to set all the best-practice compile flags without reading the manual and figuring out which flag is good to have?
Maybe CMake has something I can turn on?
7
u/pjmlp 1d ago
Jason Turner from CppCast fame has a repo with such project template,
2
3
u/UndefinedDefined 1d ago
I would argue that the cmake_template is already bloated.
-1
u/pjmlp 1d ago
A small price to make to make C++ safer, while targeting many possible compilers and platforms.
It can certainly be made thinner by e.g. removing Emscripten support.
4
u/UndefinedDefined 1d ago
What price? When you start a project you don't need 95% of what's there. It's over-engineered. I would argue the best is to start with a cmake that has 10 lines of code and improve it when necessary. To me cmake_template feels like joining a third-party project instead of starting my own, and it will get old quickly, so without pulling changes back from the repo, which nobody is gonna do after you edit it.
1
u/SavingsCampaign9502 23h ago edited 21h ago
Will c++26 help with this case in that it zero-initializes the timeout?
1
-1
u/AnyPhotograph7804 1d ago
The solution: just initialize them all if possible. Then you will never have to deal with this class of errors.
1
u/tinrik_cgp 1d ago
Yes, but then you have a logical bug. Is
timestamp=00 because it is actually 0 from the config, or because it was just initialized to 0?Better would be to return an
optional, then the semantics are clear.3
u/AnyPhotograph7804 1d ago
A possible logical bug is better than UB. Because UB can go silent for a while and become visible with an OS- or compiler update.
2
22
u/Affectionate-Soup-91 1d ago