win32: fix -Wmissing-field-initializers warnings

[fanquake]

When building with -Wmissing-field-initializers, the following warning is emitted:

leveldb/util/env_windows.cc: In member function ‘virtual leveldb::Status leveldb::{anonymous}::WindowsRandomAccessFile::Read(uint64_t, size_t, leveldb::Slice*, char*) const’:
leveldb/util/env_windows.cc:197:31: warning: missing initializer for member ‘_OVERLAPPED::InternalHigh’ [-Wmissing-field-initializers]
  197 |     OVERLAPPED overlapped = {0};
      |                               ^
leveldb/util/env_windows.cc:197:31: warning: missing initializer for member ‘_OVERLAPPED::<anonymous>’ [-Wmissing-field-initializers]
leveldb/util/env_windows.cc:197:31: warning: missing initializer for member ‘_OVERLAPPED::hEvent’ [-Wmissing-field-initializers]

Submitted upstream: google#1053

[hebasto]

ACK 12c52b3, I have reviewed the code and it looks OK.

[fanquake]

cc @theuni @sipsorcery

[sipsorcery]

ACK 12c52b3.

[theuni]

Copy/pasting from bitcoin/bitcoin#26090

From: https://learn.microsoft.com/en-us/windows/win32/api/minwinbase/ns-minwinbase-overlapped

Any unused members of this structure should always be initialized to zero before the structure is used in a function call. Otherwise, the function may fail and return ERROR_INVALID_PARAMETER.

According to that note at least, it's possible that this was always failing before.

As opposed to bitcoin/bitcoin#26090, it looks like this is probably called pretty often, so if it was causing problems it might've manifested as random-looking read-errors. So it seems even if it was happening radomly/rarely, fixing it could only be a good thing as obviously no new codepaths are being taken.

[theuni]

ACK 12c52b3

[maflcko]

I don't follow, just like bitcoin/bitcoin#26090 this should be a refactor that doesn't change the binary. The elements that are not mentioned in the initializer list will be initialized with their C++11 default member initializer or with {} if there is none.

Given that we suppress external warnings, there is no need to change the code here.

[fanquake]

Given that we suppress external warnings, there is no need to change the code here.

These warnings will show up after bitcoin/bitcoin#25972, and cause failures in our CI (-Werror), unless we then choose to suppress them in Makefile.leveldb. They are not currently suppressed by --enable-suppress-external-warnings.

[theuni]

I don't follow, just like bitcoin/bitcoin#26090 this should be a refactor that doesn't change the binary. The elements that are not mentioned in the initializer list will be initialized with their C++11 default member initializer or with {} if there is none.

Given that we suppress external warnings, there is no need to change the code here.

Yes, looks like you're right. I would've thought that initializing the first field would've prevented the initialization of the rest, but docs and local asm comparisons show that you're correct here. TIL, thanks :)

So it seems the warning is simply pointing out possible unintended code as opposed to possible undefined behavior. Note that the warning is also present with a minimal test-case with modern gcc and clang 14.

I still think the change is worthwhile to avoid disabling the warning wholesale, but agree that this is a non-binary changing refactor.