Does it adds new warning messages on master?

@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:

random.cpp:136:12: warning: variable 'r1' may be uninitialized when used here [-Wconditional-uninitialized]
    return r1;
           ^~
random.cpp:131:16: note: initialize the variable 'r1' to silence this warning
    uint64_t r1;
               ^
                = 0
…
leveldb/db/version_set.cc:1053:55: warning: variable 'manifest_size' may be uninitialized when used here [-Wconditional-uninitialized]
  descriptor_log_ = new log::Writer(descriptor_file_, manifest_size);
                                                      ^~~~~~~~~~~~~
leveldb/db/version_set.cc:1034:25: note: initialize the variable 'manifest_size' to silence this warning
  uint64_t manifest_size;
                        ^
                         = 0

I think there are also some new warnings in qt?

@MarcoFalke I don't see any Qt warnings of type -Wconditional-uninitialized: what commands should I run to reproduce? :)

I'm up for doing cleanup work in the dependencies, e.g. google/leveldb#774

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• build: ensure we aren't using GNU extensions #18088 (build: ensure we aren't using GNU extensions by fanquake)

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.

Commenters + @Sjors @fanquake @laanwj who have all expressed interest in enabling compiler warnings: would you mind reviewing this PR to make it move forward towards either merge or close? :)

Concept ACK

I get the same two warnings on macOS 10.15.3 (without depends).

1. I can live with the leveldb warning since there's work in progress to kill it
2. In Suppress false positive warning about uninitialized entropy buffers #17627 we had a long discussion about intentionally uninitialized variables in the RNG code. Although this is a different variable, I assume we also don't want to initialise it. 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 and #17895 could help, but isn't there a macro to suppress it?

@hebasto @Sjors

A source code comment // intentionally uninitialized, see #17627 and #17895 could help, but isn't there a macro to suppress it?

Regarding …

random.cpp:136:12: warning: variable 'r1' may be uninitialized when used here [-Wconditional-uninitialized]
    return r1;
           ^~
random.cpp:131:16: note: initialize the variable 'r1' to silence this warning
    uint64_t r1;
               ^
                = 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 :)

Concept ACK:ers @Sjors, @fanquake and others: would you mind reviewing the change as well? Would love to see this merged to reduce the risk of more uninitialised reads reaching master :)

Concept ACK, but it seems the only effect of this for now is to enable several "false" positives

@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 :)

I think as long as there are false positives, we should run the check once on every released version to prevent any mishaps. Not sure about forcing the false positives into every developer's eye on every compile.

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).

diff --git i/src/leveldb/db/version_set.cc w/src/leveldb/db/version_set.cc
index cd07346ea..d1845d7e4 100644
--- i/src/leveldb/db/version_set.cc
+++ w/src/leveldb/db/version_set.cc
@@ -991,13 +991,13 @@ bool VersionSet::ReuseManifest(const std::string& dscname,
                                const std::string& dscbase) {
   if (!options_->reuse_logs) {
     return false;
   }
   FileType manifest_type;
   uint64_t manifest_number;
-  uint64_t manifest_size;
+  uint64_t manifest_size = 0; // https://github.com/google/leveldb/pull/774
   if (!ParseFileName(dscbase, &manifest_number, &manifest_type) ||
       manifest_type != kDescriptorFile ||
       !env_->GetFileSize(dscname, &manifest_size).ok() ||
       // Make new compacted MANIFEST if old one is too big
       manifest_size >= TargetFileSize(options_)) {
     return false;
diff --git i/src/random.cpp w/src/random.cpp
index 765239e1f..50d06d513 100644
--- i/src/random.cpp
+++ w/src/random.cpp
@@ -113,25 +113,25 @@ static void ReportHardwareRand()
  */
 static uint64_t GetRdRand() noexcept
 {
     // RdRand may very rarely fail. Invoke it up to 10 times in a loop to reduce this risk.
 #ifdef __i386__
     uint8_t ok;
-    uint32_t r1, r2;
+    uint32_t r1 = 0, r2 = 0;
     for (int i = 0; i < 10; ++i) {
         __asm__ volatile (".byte 0x0f, 0xc7, 0xf0; setc %1" : "=a"(r1), "=q"(ok) :: "cc"); // rdrand %eax
         if (ok) break;
     }
     for (int i = 0; i < 10; ++i) {
         __asm__ volatile (".byte 0x0f, 0xc7, 0xf0; setc %1" : "=a"(r2), "=q"(ok) :: "cc"); // rdrand %eax
         if (ok) break;
     }
     return (((uint64_t)r2) << 32) | r1;
 #elif defined(__x86_64__) || defined(__amd64__)
     uint8_t ok;
-    uint64_t r1;
+    uint64_t r1 = 0;
     for (int i = 0; i < 10; ++i) {
         __asm__ volatile (".byte 0x48, 0x0f, 0xc7, 0xf0; setc %1" : "=a"(r1), "=q"(ok) :: "cc"); // rdrand %rax
         if (ok) break;
     }
     return r1;
 #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.

@MarcoFalke I see your point. Would you ACK with @vasild's suggested random.cpp patch applied?

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".

@vasild I think our policy is to not make any local changes to leveldb :)

What about removing -Wconditional-uninitialized from flags when compiling LevelDB? I.e. change src/Makefile.leveldb.include like this:

-leveldb_libleveldb_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
+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" :)

A followup of this: #18843