This makes memory bugs deterministic. -ftrivial-auto-var-init=pattern is incompatible with other memory sanitizers (like valgrind and msan), but that is irrelevant here, because the address sanitizer in this fuzz CI config is already incompatible with them.
-ftrivial-auto-var-init=pattern goes well with -fsanitize=bool and -fsanitize=enum, but those are already enabled via -fsanitize=undefined. See https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#available-checks
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | fanquake |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
For reference, the CI failure, which should also reproduce in valgrind/msan if forcing a side-effect: https://cirrus-ci.com/task/6321987505618944?logs=ci#L3413
0 node0 stderr leveldb/db/db_impl.cc:1032:25: runtime error: implicit conversion from type 'uint64_t' (aka 'unsigned long') of value 12297829382473034410 (64-bit, unsigned) to type 'int64_t' (aka 'long') changed the value to -6148914691236517206 (64-bit, signed)
1 [#0](/bitcoin-bitcoin/0/) 0x55b9c99524ac in leveldb::DBImpl::DoCompactionWork(leveldb::DBImpl::CompactionState*) src/leveldb/db/db_impl.cc:1032:25
2 [#1](/bitcoin-bitcoin/1/) 0x55b9c994d9c4 in leveldb::DBImpl::BackgroundCompaction() src/leveldb/db/db_impl.cc:746:14
3 [#2](/bitcoin-bitcoin/2/) 0x55b9c994c1df in leveldb::DBImpl::BackgroundCall() src/leveldb/db/db_impl.cc:687:5
4 [#3](/bitcoin-bitcoin/3/) 0x55b9c99f6d96 in leveldb::(anonymous namespace)::PosixEnv::BackgroundThreadMain() src/leveldb/util/env_posix.cc:830:5
5 [#4](/bitcoin-bitcoin/4/) 0x55b9c99f6d96 in leveldb::(anonymous namespace)::PosixEnv::BackgroundThreadEntryPoint(leveldb::(anonymous namespace)::PosixEnv*) src/leveldb/util/env_posix.cc:736:10
6 [#5](/bitcoin-bitcoin/5/) 0x7fdf9f955362 (/lib/x86_64-linux-gnu/libstdc++.so.6+0xe6362) (BuildId: 1fcdadafe5a79e1031ab0da645aa3798954cf53d)
7 [#6](/bitcoin-bitcoin/6/) 0x7fdf9f5d7189 (/lib/x86_64-linux-gnu/libc.so.6+0x8f189) (BuildId: bdb8aa3b1b60f9d43e1c70ba98158e05f765efdc)
8 [#7](/bitcoin-bitcoin/7/) 0x7fdf9f665bcf (/lib/x86_64-linux-gnu/libc.so.6+0x11dbcf) (BuildId: bdb8aa3b1b60f9d43e1c70ba98158e05f765efdc)
9SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change leveldb/db/db_impl.cc:1032:25 in
Since leveldb is unmaintained, the easiest fix would be to remove the buggy part of the logging code, which I guess never worked and no one noticed anyway?
0diff --git a/src/leveldb/db/db_impl.cc b/src/leveldb/db/db_impl.cc
1index 65e31724bc..a0f8eda244 100644
2--- a/src/leveldb/db/db_impl.cc
3+++ b/src/leveldb/db/db_impl.cc
4@@ -537,7 +537,6 @@ Status DBImpl::WriteLevel0Table(MemTable* mem, VersionEdit* edit,
5
6 CompactionStats stats;
7 stats.micros = env_->NowMicros() - start_micros;
8- stats.bytes_written = meta.file_size;
9 stats_[level].Add(stats);
10 return s;
11 }
12@@ -1028,9 +1027,6 @@ Status DBImpl::DoCompactionWork(CompactionState* compact) {
13 stats.bytes_read += compact->compaction->input(which, i)->file_size;
14 }
15 }
16- for (size_t i = 0; i < compact->outputs.size(); i++) {
17- stats.bytes_written += compact->outputs[i].file_size;
18- }
19
20 mutex_.Lock();
21 stats_[compact->compaction->level() + 1].Add(stats);
22@@ -1416,7 +1412,7 @@ bool DBImpl::GetProperty(const Slice& property, std::string* value) {
23 files, versions_->NumLevelBytes(level) / 1048576.0,
24 stats_[level].micros / 1e6,
25 stats_[level].bytes_read / 1048576.0,
26- stats_[level].bytes_written / 1048576.0);
27+ 0 / 1048576.0);
28 value->append(buf);
29 }
30 }
31diff --git a/src/leveldb/db/db_impl.h b/src/leveldb/db/db_impl.h
32index 685735c733..8816a30a33 100644
33--- a/src/leveldb/db/db_impl.h
34+++ b/src/leveldb/db/db_impl.h
35@@ -88,17 +88,15 @@ class DBImpl : public DB {
36 // Per level compaction stats. stats_[level] stores the stats for
37 // compactions that produced data for the specified "level".
38 struct CompactionStats {
39- CompactionStats() : micros(0), bytes_read(0), bytes_written(0) {}
40+ CompactionStats() : micros(0), bytes_read(0) {}
41
42 void Add(const CompactionStats& c) {
43 this->micros += c.micros;
44 this->bytes_read += c.bytes_read;
45- this->bytes_written += c.bytes_written;
46 }
47
48 int64_t micros;
49 int64_t bytes_read;
50- int64_t bytes_written;
51 };
52
53 Iterator* NewInternalIterator(const ReadOptions&,
Concept ACK - Should we just put the leveldb changes into our subtree?
Yes, someone can create a pull request and see what happens. I am happy to review, but will probably not create it. Seems fine to have a test-only trivial patch to enable a useful sanitizer.
ACK fa07ac48d804beac38a98d23be2167f78cadefae - going to get back to fixing up the cxxflags usage in CI, but not a blocker here:
0 CC = /usr/bin/ccache clang-16 -ftrivial-auto-var-init=pattern
1 CFLAGS = -pthread -g -O2
2 CPPFLAGS = -fmacro-prefix-map=$(abs_top_srcdir)=. -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -DHAVE_BUILD_INFO -DPROVIDE_FUZZ_MAIN_FUNCTION -DARENA_DEBUG -DDEBUG_LOCKORDER
3 CXX = /usr/bin/ccache clang++-16 -ftrivial-auto-var-init=pattern -std=c++20
4 CXXFLAGS = -fdebug-prefix-map=$(abs_top_srcdir)=. -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wunreachable-code-loop-increment -Wimplicit-fallthrough -Wdocumentation -Wno-unused-parameter -Wno-self-assign -Werror -g -O2
going to get back to fixing up the cxxflags usage in CI, but not a blocker here:
Yeah, that is intentional and the recommended way to set compiler flags in the CI, no? Otherwise, it will just disable the -O2 again, which would be bad?