Skip to content

Commit

Permalink
Suppress a TSAN warning (facebook#7126)
Browse files Browse the repository at this point in the history
Summary:
TSAN shows warning with clang with warning similar to this:

WARNING: ThreadSanitizer: data race (pid=10159)
  Atomic write of size 8 at 0x7b5000002890 by thread T33:
    #0 __tsan_atomic64_store <null> (db_test+0x4ca2b5)
    facebook#1 std::__atomic_base<unsigned long>::store(unsigned long, std::memory_order) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/bits/atomic_base.h:374:2 (db_test+0x774fde)
    facebook#2 rocksdb::VersionSet::SetLastSequence(unsigned long) /home/circleci/project/./db/version_set.h:1057:20 (db_test+0x774fde)
    facebook#3 rocksdb::DBImpl::WriteImpl(rocksdb::WriteOptions const&, rocksdb::WriteBatch*, rocksdb::WriteCallback*, unsigned long*, unsigned long, bool, unsigned long*, unsigned long, rocksdb::PreReleaseCallback*) /home/circleci/project/db/db_impl/db_impl_write.cc:449:18 (db_test+0x774fde)
......
  Previous read of size 8 at 0x7b5000002890 by thread T5 (mutexes: write M1044689462619020832):
    #0 rocksdb::DBImpl::ReleaseSnapshot(rocksdb::Snapshot const*) /home/circleci/project/db/db_impl/db_impl.cc (db_test+0x6f4ae7)
    facebook#1 rocksdb::(anonymous namespace)::MTThreadBody(void*) /home/circleci/project/db/db_test.cc:2514:13 (db_test+0x56ac59)
    facebook#2 rocksdb::(anonymous namespace)::StartThreadWrapper(void*) /home/circleci/project/env/env_posix.cc:443:3 (db_test+0x88c4cd)

It is not limited to ReleaseSnapshot() and rocksdb::DBImpl::MultiCFSnapshot().

While we are not 100% sure it doesn't indicate any correctness violation, we suppress them for now to keep TSAN clean with more tests so that we can cover more bugs with CI.

In the gcc runs we have been running, this warning rarely shows up.

Pull Request resolved: facebook#7126

Test Plan: See the mini-TSAN test to pass with reasonable run time.

Reviewed By: ajkr

Differential Revision: D22552375

fbshipit-source-id: ebdd3854cb3becec3403970326a1ca961db2ab00
  • Loading branch information
siying authored and facebook-github-bot committed Jul 15, 2020
1 parent ee8c79d commit ca5a069
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 5 deletions.
2 changes: 1 addition & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ jobs:
- run: echo "deb http://apt.llvm.org/xenial/ llvm-toolchain-xenial-10 main" | sudo tee -a /etc/apt/sources.list
- run: echo "deb-src http://apt.llvm.org/xenial/ llvm-toolchain-xenial-10 main" | sudo tee -a /etc/apt/sources.list
- run: sudo apt-get update -y && sudo apt-get install -y clang-10 libgflags-dev
- run: SKIP_FORMAT_BUCK_CHECKS=1 COMPILE_WITH_TSAN=1 CC=clang-10 CXX=clang++-10 ROCKSDB_DISABLE_ALIGNED_NEW=1 USE_CLANG=1 PRINT_PARALLEL_OUTPUTS=1 EXCLUDE_TESTS_REGEX="DBTestCompactionFilterWithCompactParam.CompactionFilterWithValueChang|DBTestRandomized/DBTestRandomized.Randomized|DBTestWithParam.MergeCompactionTimeTest|DBTestUniversalCompaction.UniversalCompactionTrivialMoveTest1|DBTestUniversalCompactionMultiLevels.UniversalCompactionMultiLevels|ExternalSSTFileTest.IngestFileWithGlobalSeqnoRandomized|ReadaheadSequentialFileTest|ReadExceedsReadaheadSize/ReadaheadSequentialFileTest|TransactionStressTest|MySQLStyleTransactionTest|SnapshotConcurrentAccessTest|eqAdvanceConcurrentTest|WritePreparedTransactionTest|WriteUnpreparedTransactionTest|WriteUnpreparedStressTest|MultiThreadedDBTest|LogMarkLeakTest" make V=1 -j32 check | .circleci/cat_ignore_eagain # aligned new doesn't work for reason we haven't figured out. Exclude some tests to speed up. MultiThreadedDBTest, TransactionStressTest, TransactionStressTest, TransactionStressTest and TransactionStressTest are showing TSAN warning so excluding them for now. Will investigate later.
- run: SKIP_FORMAT_BUCK_CHECKS=1 COMPILE_WITH_TSAN=1 CC=clang-10 CXX=clang++-10 ROCKSDB_DISABLE_ALIGNED_NEW=1 USE_CLANG=1 PRINT_PARALLEL_OUTPUTS=1 EXCLUDE_TESTS_REGEX="TransactionStressTest|SnapshotConcurrentAccess|SeqAdvanceConcurrent|DeadlockStress|MultiThreadedDBTest.MultiThreaded|WriteUnpreparedStressTest.ReadYourOwnWriteStress|DBAsBaseDB/TransactionStressTest" make V=1 -j32 check | .circleci/cat_ignore_eagain # aligned new doesn't work for reason we haven't figured out.


build-linux-clang10-ubsan:
Expand Down
23 changes: 19 additions & 4 deletions db/version_set.h
Original file line number Diff line number Diff line change
Expand Up @@ -1025,8 +1025,23 @@ class VersionSet {
return next_file_number_.fetch_add(n);
}

// TSAN failure is suppressed in most sequence read/write functions when
// clang is used, because it would show a warning of conflcit for those
// updates. Since we haven't figured out a correctnes violation caused
// by those sharing, we suppress them for now to keep the build clean.
#if defined(__clang__)
#if defined(__has_feature)
#if __has_feature(thread_sanitizer)
#define SUPPRESS_TSAN __attribute__((no_sanitize("thread")))
#endif // __has_feature(thread_sanitizer)
#endif // efined(__has_feature)
#endif // defined(__clang__)
#ifndef SUPPRESS_TSAN
#define SUPPRESS_TSAN
#endif // SUPPRESS_TSAN

// Return the last sequence number.
uint64_t LastSequence() const {
SUPPRESS_TSAN uint64_t LastSequence() const {
return last_sequence_.load(std::memory_order_acquire);
}

Expand All @@ -1036,20 +1051,20 @@ class VersionSet {
}

// Note: memory_order_acquire must be sufficient.
uint64_t LastPublishedSequence() const {
SUPPRESS_TSAN uint64_t LastPublishedSequence() const {
return last_published_sequence_.load(std::memory_order_seq_cst);
}

// Set the last sequence number to s.
void SetLastSequence(uint64_t s) {
SUPPRESS_TSAN void SetLastSequence(uint64_t s) {
assert(s >= last_sequence_);
// Last visible sequence must always be less than last written seq
assert(!db_options_->two_write_queues || s <= last_allocated_sequence_);
last_sequence_.store(s, std::memory_order_release);
}

// Note: memory_order_release must be sufficient
void SetLastPublishedSequence(uint64_t s) {
SUPPRESS_TSAN void SetLastPublishedSequence(uint64_t s) {
assert(s >= last_published_sequence_);
last_published_sequence_.store(s, std::memory_order_seq_cst);
}
Expand Down

0 comments on commit ca5a069

Please sign in to comment.