Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Aug 28, 2023

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 28, 2023

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

Reviews

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.

@maflcko
Copy link
Member Author

maflcko commented Aug 30, 2023

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

 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)
    #0 0x55b9c99524ac in leveldb::DBImpl::DoCompactionWork(leveldb::DBImpl::CompactionState*) src/leveldb/db/db_impl.cc:1032:25
    #1 0x55b9c994d9c4 in leveldb::DBImpl::BackgroundCompaction() src/leveldb/db/db_impl.cc:746:14
    #2 0x55b9c994c1df in leveldb::DBImpl::BackgroundCall() src/leveldb/db/db_impl.cc:687:5
    #3 0x55b9c99f6d96 in leveldb::(anonymous namespace)::PosixEnv::BackgroundThreadMain() src/leveldb/util/env_posix.cc:830:5
    #4 0x55b9c99f6d96 in leveldb::(anonymous namespace)::PosixEnv::BackgroundThreadEntryPoint(leveldb::(anonymous namespace)::PosixEnv*) src/leveldb/util/env_posix.cc:736:10
    #5 0x7fdf9f955362  (/lib/x86_64-linux-gnu/libstdc++.so.6+0xe6362) (BuildId: 1fcdadafe5a79e1031ab0da645aa3798954cf53d)
    #6 0x7fdf9f5d7189  (/lib/x86_64-linux-gnu/libc.so.6+0x8f189) (BuildId: bdb8aa3b1b60f9d43e1c70ba98158e05f765efdc)
    #7 0x7fdf9f665bcf  (/lib/x86_64-linux-gnu/libc.so.6+0x11dbcf) (BuildId: bdb8aa3b1b60f9d43e1c70ba98158e05f765efdc)
SUMMARY: 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?

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&,

@maflcko maflcko force-pushed the 2308-ci-asan-patt- branch from ec9463c to faef41d Compare August 30, 2023 08:30
@maflcko
Copy link
Member Author

maflcko commented Aug 30, 2023

Rebased and made CI green

@fanquake
Copy link
Member

fanquake commented Aug 30, 2023

Concept ACK - Should we just put the leveldb changes into our subtree?

@maflcko
Copy link
Member Author

maflcko commented Aug 30, 2023

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.

@fanquake
Copy link
Member

fanquake commented Sep 5, 2023

Yea. I will get to pulling upstream leveldb & integrating these. Opened bitcoin-core/leveldb-subtree#37.

Copy link
Member

@fanquake fanquake left a 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

@fanquake fanquake merged commit 260445b into bitcoin:master Sep 5, 2023
@maflcko
Copy link
Member Author

maflcko commented Sep 5, 2023

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?

@maflcko maflcko deleted the 2308-ci-asan-patt- branch September 5, 2023 09:10
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 8, 2023
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
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 21, 2024
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.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 21, 2024
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
fanquake added a commit that referenced this pull request Feb 26, 2024
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
@bitcoin bitcoin locked and limited conversation to collaborators Sep 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants