Enable Clang’s -Wconditional-uninitialized to warn on potential uninitialized reads.
Clang’s -Wconditional-uninitialized warns in cases where a simple control flow analysis cannot prove that a variable was written to prior to each use.
Diagnostics texts:
0warning: variable foo may be uninitialized when used here
1warning: variable foo may be uninitialized when captured by block
@hebasto Good question! Yes, it warns about two variables where Clang’s control flow analysis cannot prove that the variables were not written to prior to use.
Given the severeness of the bugs that this diagnostic can help prevent I think that this slight increase in warning volume is a cost worth taking :)
The two warnings emitted:
0random.cpp:136:12: warning: variable 'r1' may be uninitialized when used here [-Wconditional-uninitialized]
1 return r1;
2 ^~
3random.cpp:131:16: note: initialize the variable 'r1' to silence this warning
4 uint64_t r1;
5 ^
6 = 0
7…
8leveldb/db/version_set.cc:1053:55: warning: variable 'manifest_size' may be uninitialized when used here [-Wconditional-uninitialized]
9 descriptor_log_ = new log::Writer(descriptor_file_, manifest_size);
10 ^~~~~~~~~~~~~
11leveldb/db/version_set.cc:1034:25: note: initialize the variable 'manifest_size' to silence this warning
12 uint64_t manifest_size;
13 ^
14 = 0
-Wconditional-uninitialized: what commands should I run to reproduce? :)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Concept ACK
I get the same two warnings on macOS 10.15.3 (without depends).
// intentionally uninitialized, see [#17627](/bitcoin-bitcoin/17627/) and [#17895](/bitcoin-bitcoin/17895/) could help, but isn’t there a macro to suppress it?
ACK 6f84c2fe07c329a19c356e82db300d7429caaf11, tested on macOS 10.14.
Two new warnings are emitted:
0...
1random.cpp:136:12: warning: variable 'r1' may be uninitialized when used here [-Wconditional-uninitialized]
2 return r1;
3 ^~
4random.cpp:131:16: note: initialize the variable 'r1' to silence this warning
5 uint64_t r1;
6 ^
7 = 0
8...
9leveldb/db/version_set.cc:1016:55: warning: variable 'manifest_size' may be uninitialized when used here [-Wconditional-uninitialized]
10 descriptor_log_ = new log::Writer(descriptor_file_, manifest_size);
11 ^~~~~~~~~~~~~
12leveldb/db/version_set.cc:997:25: note: initialize the variable 'manifest_size' to silence this warning
13 uint64_t manifest_size;
14 ^
15 = 0
16...
Agree with @Sjors:
Keeping the warning around until the end of time seems annoying (though not deal-breaking), and will likely lead to PRs every few months of people trying to fix it. A source code comment
// intentionally uninitialized, see [#17627](/bitcoin-bitcoin/17627/) and [#17895](/bitcoin-bitcoin/17895/)could help, but isn’t there a macro to suppress it?
A source code comment
// intentionally uninitialized, see [#17627](/bitcoin-bitcoin/17627/) and [#17895](/bitcoin-bitcoin/17895/)could help, but isn’t there a macro to suppress it?
Regarding …
0random.cpp:136:12: warning: variable 'r1' may be uninitialized when used here [-Wconditional-uninitialized]
1 return r1;
2 ^~
3random.cpp:131:16: note: initialize the variable 'r1' to silence this warning
4 uint64_t r1;
5 ^
6 = 0
… I’m not entirely sure that is intentional. The author was asked about it in #17313 (Theoretical (astronomically small) possibility of uninitialized read in GetRdRand()?) back in October last year but he hasn’t responded yet :)
@MarcoFalke The effect is hopefully that if someone introduces an uninitialized read they will notice immediately thanks to the warning, but you’re right that the random.cpp case and the two leveldb/ cases will warn too :)
It is a trade-off I guess: benefit of reducing the risk of introducing new uninitialized reads vs cost of the spurious random.cpp warning (+ leveldb). Personally I think the price is worth paying :)
Concept ACK
However I cannot sleep well when warnings have been printed on my screen. Once you allow one warning, this opens the door to others and eventually it becomes very difficult to close the door (clean all of them). It is easier to just never open the door (don’t allow any warnings in the first place). For example, in this particular case if a warning is printed, then it is not possible to compile with full -Werror anymore. Also, if an “uninitialized read” warning is introduced in new code the chances are it will go unnoticed because, even though it is printed it will be buried somewhere in build output and CI logs.
So, I would suggest to introduce -Wconditional-uninitialized and fix the warnings in the code:
the random.cpp one - given that r1 would be overwritten with zeroes even on rdrand failure, it means that if we initialize it to 0, then that will be a non-functional change.
the leveldb one - fix it and add a reference to the upstream pull request. Even if the local fix gets wiped on the next “import”, it is not a big deal - one warning will pop up (if not already fixed upstream).
0diff --git i/src/leveldb/db/version_set.cc w/src/leveldb/db/version_set.cc
1index cd07346ea..d1845d7e4 100644
2--- i/src/leveldb/db/version_set.cc
3+++ w/src/leveldb/db/version_set.cc
4@@ -991,13 +991,13 @@ bool VersionSet::ReuseManifest(const std::string& dscname,
5 const std::string& dscbase) {
6 if (!options_->reuse_logs) {
7 return false;
8 }
9 FileType manifest_type;
10 uint64_t manifest_number;
11- uint64_t manifest_size;
12+ uint64_t manifest_size = 0; // https://github.com/google/leveldb/pull/774
13 if (!ParseFileName(dscbase, &manifest_number, &manifest_type) ||
14 manifest_type != kDescriptorFile ||
15 !env_->GetFileSize(dscname, &manifest_size).ok() ||
16 // Make new compacted MANIFEST if old one is too big
17 manifest_size >= TargetFileSize(options_)) {
18 return false;
19diff --git i/src/random.cpp w/src/random.cpp
20index 765239e1f..50d06d513 100644
21--- i/src/random.cpp
22+++ w/src/random.cpp
23@@ -113,25 +113,25 @@ static void ReportHardwareRand()
24 */
25 static uint64_t GetRdRand() noexcept
26 {
27 // RdRand may very rarely fail. Invoke it up to 10 times in a loop to reduce this risk.
28 #ifdef __i386__
29 uint8_t ok;
30- uint32_t r1, r2;
31+ uint32_t r1 = 0, r2 = 0;
32 for (int i = 0; i < 10; ++i) {
33 __asm__ volatile (".byte 0x0f, 0xc7, 0xf0; setc %1" : "=a"(r1), "=q"(ok) :: "cc"); // rdrand %eax
34 if (ok) break;
35 }
36 for (int i = 0; i < 10; ++i) {
37 __asm__ volatile (".byte 0x0f, 0xc7, 0xf0; setc %1" : "=a"(r2), "=q"(ok) :: "cc"); // rdrand %eax
38 if (ok) break;
39 }
40 return (((uint64_t)r2) << 32) | r1;
41 #elif defined(__x86_64__) || defined(__amd64__)
42 uint8_t ok;
43- uint64_t r1;
44+ uint64_t r1 = 0;
45 for (int i = 0; i < 10; ++i) {
46 __asm__ volatile (".byte 0x48, 0x0f, 0xc7, 0xf0; setc %1" : "=a"(r1), "=q"(ok) :: "cc"); // rdrand %rax
47 if (ok) break;
48 }
49 return r1;
50 #else
Also, I would suggest to enable -Werror=conditional-uninitialized if Travis is happy with it. I confirm no such warnings come from external headers - Boost 1.72.0, Qt 5.14.2 and db 6.2.32 with Clang 11.
Given the four concept ACKs: what is the best way to move forward on this PR?
Empirically I seems like we need all help we can get with regards to avoiding uninitialized reads :)
Would the one-line change in src/leveldb/db/version_set.cc cause any trouble with the next “import” of LevelDB, if that line is not changed upstream?
I guess git-substree(1) is used for that and it should handle it transparently?
If the line is changed upstream and a conflict arises, then either 1) upstream accepted the fix we sent and the conflict should be resolved by “take theirs” or 2) some refactor upstream in which case we should again “take theirs”.
What about removing -Wconditional-uninitialized from flags when compiling LevelDB? I.e. change src/Makefile.leveldb.include like this:
0-leveldb_libleveldb_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
1+leveldb_libleveldb_a_CXXFLAGS = $(AM_CXXFLAGS:-Wconditional-uninitialized=) $(PIE_FLAGS)
(untested, GNU make doc)
@vasild That sounds like a way forward! Feel free to tackle this issue. I’d be glad to review!
I’m closing this PR as “up for grabs” :)