Fix TSAN build and re-enable the tests (facebook#7386)

Summary:
Resolve TSAN build warnings and re-enable disabled TSAN tests.

Not sure if it's a compiler issue or TSAN check issue. Switching from
conditional operator to if-else mitigated the problem.

Pull Request resolved: facebook#7386

Test Plan:
run TSAN check 10 times in circleci.

```
WARNING: ThreadSanitizer: data race (pid=27735)
  Atomic write of size 8 at 0x7b54000005e8 by thread T32:
    #0 __tsan_atomic64_store <null> (db_test+0x4cee95)
    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+0x78460e)
    facebook#2 rocksdb::VersionSet::SetLastSequence(unsigned long) /home/circleci/project/./db/version_set.h:1058:20 (db_test+0x78460e)
...
  Previous read of size 8 at 0x7b54000005e8 by thread T31:
    #0 bool rocksdb::DBImpl::MultiCFSnapshot<std::unordered_map<unsigned int, rocksdb::DBImpl::MultiGetColumnFamilyData, std::hash<unsigned int>, std::equal_to<unsigned int>, std::allocator<std::pair<unsigned int const, rocksdb::DBImpl::MultiGetColumnFamilyData> > > >(rocksdb::ReadOptions const&, rocksdb::ReadCallback*, std::function<rocksdb::DBImpl::MultiGetColumnFamilyData* (std::unordered_map<unsigned int, rocksdb::DBImpl::MultiGetColumnFamilyData, std::hash<unsigned int>, std::equal_to<unsigned int>, std::allocator<std::pair<unsigned int const, rocksdb::DBImpl::MultiGetColumnFamilyData> > >::iterator&)>&, std::unordered_map<unsigned int, rocksdb::DBImpl::MultiGetColumnFamilyData, std::hash<unsigned int>, std::equal_to<unsigned int>, std::allocator<std::pair<unsigned int const, rocksdb::DBImpl::MultiGetColumnFamilyData> > >*, unsigned long*) /home/circleci/project/db/db_impl/db_impl.cc (db_test+0x715087)
```

Reviewed By: ltamasi

Differential Revision: D23725226

Pulled By: jay-zhuang

fbshipit-source-id: a6d662a5ea68111246cd32ec95f3411a25f76bc6

.circleci/config.yml

@@ -163,7 +163,7 @@ jobs:
       - pre-steps
       - install-gflags
       - install-clang-10
-      - run: COMPILE_WITH_TSAN=1 CC=clang-10 CXX=clang++-10 ROCKSDB_DISABLE_ALIGNED_NEW=1 USE_CLANG=1 EXCLUDE_TESTS_REGEX="TransactionStressTest|SnapshotConcurrentAccess|SeqAdvanceConcurrent|DeadlockStress|MultiThreadedDBTest.MultiThreaded|WriteUnpreparedStressTest.ReadYourOwnWriteStress|DBAsBaseDB/TransactionStressTest|FlushCloseWALFiles|BackgroundPurgeCFDropTest" make V=1 -j32 check | .circleci/cat_ignore_eagain # aligned new doesn't work for reason we haven't figured out. Exclude FlushCloseWALFiles and BackgroundPurgeCFDropTest for occasional failures.
+      - run: COMPILE_WITH_TSAN=1 CC=clang-10 CXX=clang++-10 ROCKSDB_DISABLE_ALIGNED_NEW=1 USE_CLANG=1 make V=1 -j32 check | .circleci/cat_ignore_eagain # aligned new doesn't work for reason we haven't figured out.
       - post-steps
 
   build-linux-clang10-ubsan:

db/db_impl/db_impl.cc

@@ -1625,9 +1625,11 @@ Status DBImpl::GetImpl(const ReadOptions& read_options, const Slice& key,
     // data for the snapshot, so the reader would see neither data that was be
     // visible to the snapshot before compaction nor the newer data inserted
     // afterwards.
-    snapshot = last_seq_same_as_publish_seq_
-                   ? versions_->LastSequence()
-                   : versions_->LastPublishedSequence();
+    if (last_seq_same_as_publish_seq_) {
+      snapshot = versions_->LastSequence();
+    } else {
+      snapshot = versions_->LastPublishedSequence();
+    }
     if (get_impl_options.callback) {
       // The unprep_seqs are not published for write unprepared, so it could be
       // that max_visible_seq is larger. Seek to the std::max of the two.
@@ -1984,9 +1986,11 @@ bool DBImpl::MultiCFSnapshot(
       // version because a flush happening in between may compact away data for
       // the snapshot, but the snapshot is earlier than the data overwriting it,
       // so users may see wrong results.
-      *snapshot = last_seq_same_as_publish_seq_
-                      ? versions_->LastSequence()
-                      : versions_->LastPublishedSequence();
+      if (last_seq_same_as_publish_seq_) {
+        *snapshot = versions_->LastSequence();
+      } else {
+        *snapshot = versions_->LastPublishedSequence();
+      }
     }
   } else {
     // If we end up with the same issue of memtable geting sealed during 2
@@ -2017,9 +2021,11 @@ bool DBImpl::MultiCFSnapshot(
           // acquire the lock so we're sure to succeed
           mutex_.Lock();
         }
-        *snapshot = last_seq_same_as_publish_seq_
-                        ? versions_->LastSequence()
-                        : versions_->LastPublishedSequence();
+        if (last_seq_same_as_publish_seq_) {
+          *snapshot = versions_->LastSequence();
+        } else {
+          *snapshot = versions_->LastPublishedSequence();
+        }
       } else {
         *snapshot = reinterpret_cast<const SnapshotImpl*>(read_options.snapshot)
                         ->number_;
@@ -2961,9 +2967,11 @@ void DBImpl::ReleaseSnapshot(const Snapshot* s) {
     snapshots_.Delete(casted_s);
     uint64_t oldest_snapshot;
     if (snapshots_.empty()) {
-      oldest_snapshot = last_seq_same_as_publish_seq_
-                            ? versions_->LastSequence()
-                            : versions_->LastPublishedSequence();
+      if (last_seq_same_as_publish_seq_) {
+        oldest_snapshot = versions_->LastSequence();
+      } else {
+        oldest_snapshot = versions_->LastPublishedSequence();
+      }
     } else {
       oldest_snapshot = snapshots_.oldest()->number_;
     }

db/version_set.h

@@ -1035,23 +1035,8 @@ 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.
-  SUPPRESS_TSAN uint64_t LastSequence() const {
+  uint64_t LastSequence() const {
     return last_sequence_.load(std::memory_order_acquire);
   }
 
@@ -1061,20 +1046,20 @@ class VersionSet {
   }
 
   // Note: memory_order_acquire must be sufficient.
-  SUPPRESS_TSAN uint64_t LastPublishedSequence() const {
+  uint64_t LastPublishedSequence() const {
     return last_published_sequence_.load(std::memory_order_seq_cst);
   }
 
   // Set the last sequence number to s.
-  SUPPRESS_TSAN void SetLastSequence(uint64_t s) {
+  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
-  SUPPRESS_TSAN void SetLastPublishedSequence(uint64_t s) {
+  void SetLastPublishedSequence(uint64_t s) {
     assert(s >= last_published_sequence_);
     last_published_sequence_.store(s, std::memory_order_seq_cst);
   }