-
Notifications
You must be signed in to change notification settings - Fork 38.7k
ci: Asan with -ftrivial-auto-var-init=pattern #28359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
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 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? diff --git a/src/leveldb/db/db_impl.cc b/src/leveldb/db/db_impl.cc
index 65e31724bc..a0f8eda244 100644
--- a/src/leveldb/db/db_impl.cc
+++ b/src/leveldb/db/db_impl.cc
@@ -537,7 +537,6 @@ Status DBImpl::WriteLevel0Table(MemTable* mem, VersionEdit* edit,
CompactionStats stats;
stats.micros = env_->NowMicros() - start_micros;
- stats.bytes_written = meta.file_size;
stats_[level].Add(stats);
return s;
}
@@ -1028,9 +1027,6 @@ Status DBImpl::DoCompactionWork(CompactionState* compact) {
stats.bytes_read += compact->compaction->input(which, i)->file_size;
}
}
- for (size_t i = 0; i < compact->outputs.size(); i++) {
- stats.bytes_written += compact->outputs[i].file_size;
- }
mutex_.Lock();
stats_[compact->compaction->level() + 1].Add(stats);
@@ -1416,7 +1412,7 @@ bool DBImpl::GetProperty(const Slice& property, std::string* value) {
files, versions_->NumLevelBytes(level) / 1048576.0,
stats_[level].micros / 1e6,
stats_[level].bytes_read / 1048576.0,
- stats_[level].bytes_written / 1048576.0);
+ 0 / 1048576.0);
value->append(buf);
}
}
diff --git a/src/leveldb/db/db_impl.h b/src/leveldb/db/db_impl.h
index 685735c733..8816a30a33 100644
--- a/src/leveldb/db/db_impl.h
+++ b/src/leveldb/db/db_impl.h
@@ -88,17 +88,15 @@ class DBImpl : public DB {
// Per level compaction stats. stats_[level] stores the stats for
// compactions that produced data for the specified "level".
struct CompactionStats {
- CompactionStats() : micros(0), bytes_read(0), bytes_written(0) {}
+ CompactionStats() : micros(0), bytes_read(0) {}
void Add(const CompactionStats& c) {
this->micros += c.micros;
this->bytes_read += c.bytes_read;
- this->bytes_written += c.bytes_written;
}
int64_t micros;
int64_t bytes_read;
- int64_t bytes_written;
};
Iterator* NewInternalIterator(const ReadOptions&, |
ec9463c to
faef41d
Compare
faef41d to
fa07ac4
Compare
|
Rebased and made CI green |
|
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. |
|
Yea. I will get to pulling upstream leveldb & integrating these. Opened bitcoin-core/leveldb-subtree#37. |
fanquake
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fa07ac4 - going to get back to fixing up the cxxflags usage in CI, but not a blocker here:
CC = /usr/bin/ccache clang-16 -ftrivial-auto-var-init=pattern
CFLAGS = -pthread -g -O2
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
CXX = /usr/bin/ccache clang++-16 -ftrivial-auto-var-init=pattern -std=c++20
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
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? |
fa07ac4 ci: Asan with -ftrivial-auto-var-init=pattern (MarcoFalke) Pull request description: 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 ACKs for top commit: fanquake: ACK fa07ac4 - going to get back to fixing up the cxxflags usage in CI, but not a blocker here: Tree-SHA512: 2ea6c5128a9cd262bdd1b1475c7e1be23ce2c520fad05f6c2aace6c29e203573323c0564d272e25da35ad5ff46fde488a5eae1ed14d3349e793d14a5e2533fb1
Drop `git diff` command so it is easier to run CI locally if git checkout is a worktree. Currently it fails because the directory is not recognized as a git repository. The `git diff` command was added recently in bitcoin#28359 commit fa07ac4 and isn't neccessary. It is also probably not useful for debugging.
Drop `git diff` command so it is easier to run CI locally if git checkout is a worktree. Currently it fails because the directory is not recognized as a git repository. The `git diff` command was added recently in bitcoin#28359 commit fa07ac4 and can be avoided just by teeing the patch to stdout
84388c9 ci: avoid running git diff after patching (Ryan Ofsky) Pull request description: Drop `git diff` command so it is easier to run CI locally if git checkout is a worktree. Currently it fails because the directory is not recognized as a git repository. The `git diff` command was added recently in #28359 commit fa07ac4 and can be avoided just by teeing the patch to stdout ACKs for top commit: maflcko: lgtm ACK 84388c9 TheCharlatan: ACK 84388c9 Tree-SHA512: 089c8ff62f9c56a1df06686e72420a9a54a079d2ef9eaf7c9cfcd97cb5cce50c8c169890e599ef875aaf1ee426f590851b1f19d6c9e386671460ee6507d8d872
This makes memory bugs deterministic.
-ftrivial-auto-var-init=patternis 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=patterngoes well with-fsanitize=booland-fsanitize=enum, but those are already enabled via-fsanitize=undefined. See https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#available-checks