From 296545a2c7f6e9e11dee4440bba80f45d61566d3 Mon Sep 17 00:00:00 2001 From: Yi Wu Date: Fri, 8 Jul 2016 17:50:51 -0700 Subject: [PATCH] Fix clang analyzer errors Summary: Fixing erros reported by clang static analyzer. * Removing some unused variables. * Adding assertions to fix false positives reported by clang analyzer. * Adding `__clang_analyzer__` macro to suppress false positive warnings. Test Plan: USE_CLANG=1 OPT=-g make analyze -j64 Reviewers: andrewkr, sdong Reviewed By: sdong Subscribers: andrewkr, dhruba, leveldb Differential Revision: https://reviews.facebook.net/D60549 --- db/flush_job.cc | 3 --- db/flush_scheduler.cc | 3 +++ db/log_reader.cc | 19 ++++++++++--------- db/merge_helper.cc | 1 + db/version_set.cc | 2 +- table/block.cc | 2 +- table/block_based_table_reader.cc | 8 +++++++- table/block_prefix_index.cc | 1 + table/table_test.cc | 1 - .../gtest-1.7.0/fused-src/gtest/gtest-all.cc | 6 +++++- tools/db_sanity_test.cc | 5 ++++- util/dynamic_bloom_test.cc | 2 ++ util/options_parser.cc | 1 + util/thread_local_test.cc | 3 +++ .../write_batch_with_index.cc | 3 +++ 15 files changed, 42 insertions(+), 18 deletions(-) diff --git a/db/flush_job.cc b/db/flush_job.cc index 8e34df22b..8db50a41a 100644 --- a/db/flush_job.cc +++ b/db/flush_job.cc @@ -292,9 +292,6 @@ Status FlushJob::WriteLevel0Table(const autovector& mems, } base->Unref(); - // re-acquire the most current version - base = cfd_->current(); - // Note that if file_size is zero, the file has been deleted and // should not be added to the manifest. if (s.ok() && meta->fd.GetFileSize() > 0) { diff --git a/db/flush_scheduler.cc b/db/flush_scheduler.cc index 60db59dd4..a961f7f0b 100644 --- a/db/flush_scheduler.cc +++ b/db/flush_scheduler.cc @@ -20,6 +20,8 @@ void FlushScheduler::ScheduleFlush(ColumnFamilyData* cfd) { } #endif // NDEBUG cfd->Ref(); +// Suppress false positive clang analyzer warnings. +#ifndef __clang_analyzer__ Node* node = new Node{cfd, head_.load(std::memory_order_relaxed)}; while (!head_.compare_exchange_strong( node->next, node, std::memory_order_relaxed, std::memory_order_relaxed)) { @@ -28,6 +30,7 @@ void FlushScheduler::ScheduleFlush(ColumnFamilyData* cfd) { // inter-thread synchronization, so we don't even need release // semantics for this CAS } +#endif // __clang_analyzer__ } ColumnFamilyData* FlushScheduler::TakeNextColumnFamily() { diff --git a/db/log_reader.cc b/db/log_reader.cc index a33d7e480..2da16a286 100644 --- a/db/log_reader.cc +++ b/db/log_reader.cc @@ -91,7 +91,7 @@ bool Reader::ReadRecord(Slice* record, std::string* scratch, Slice fragment; while (true) { uint64_t physical_record_offset = end_of_buffer_offset_ - buffer_.size(); - size_t drop_size; + size_t drop_size = 0; const unsigned int record_type = ReadPhysicalRecord(&fragment, &drop_size); switch (record_type) { case kFullType: @@ -199,10 +199,11 @@ bool Reader::ReadRecord(Slice* record, std::string* scratch, scratch->clear(); return false; } - if (record_type == kBadRecordLen) - ReportCorruption(drop_size, "bad record length"); - else - ReportCorruption(drop_size, "checksum mismatch"); + if (record_type == kBadRecordLen) { + ReportCorruption(drop_size, "bad record length"); + } else { + ReportCorruption(drop_size, "checksum mismatch"); + } if (in_fragmented_record) { ReportCorruption(scratch->size(), "error in middle of record"); in_fragmented_record = false; @@ -343,7 +344,7 @@ unsigned int Reader::ReadPhysicalRecord(Slice* result, size_t* drop_size) { if (buffer_.size() < (size_t)kHeaderSize) { int r; if (!ReadMore(drop_size, &r)) { - return r; + return r; } continue; } @@ -362,11 +363,11 @@ unsigned int Reader::ReadPhysicalRecord(Slice* result, size_t* drop_size) { header_size = kRecyclableHeaderSize; // We need enough for the larger header if (buffer_.size() < (size_t)kRecyclableHeaderSize) { - int r; - if (!ReadMore(drop_size, &r)) { + int r; + if (!ReadMore(drop_size, &r)) { return r; } - continue; + continue; } const uint32_t log_num = DecodeFixed32(header + 7); if (log_num != log_number_) { diff --git a/db/merge_helper.cc b/db/merge_helper.cc index 2ba22d977..2fb104dd6 100644 --- a/db/merge_helper.cc +++ b/db/merge_helper.cc @@ -26,6 +26,7 @@ Status MergeHelper::TimedFullMerge(const MergeOperator* merge_operator, assert(merge_operator != nullptr); if (operands.size() == 0) { + assert(value != nullptr && result != nullptr); result->assign(value->data(), value->size()); return Status::OK(); } diff --git a/db/version_set.cc b/db/version_set.cc index 3a7390519..6ded4fdb5 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -2746,7 +2746,7 @@ Status VersionSet::Recover( } } - for (auto builder : builders) { + for (auto& builder : builders) { delete builder.second; } diff --git a/table/block.cc b/table/block.cc index ecc3e4417..fa14a00f0 100644 --- a/table/block.cc +++ b/table/block.cc @@ -73,7 +73,7 @@ void BlockIter::Prev() { const CachedPrevEntry& current_prev_entry = prev_entries_[prev_entries_idx_]; - const char* key_ptr = current_prev_entry.key_ptr; + const char* key_ptr = nullptr; if (current_prev_entry.key_ptr != nullptr) { // The key is not delta encoded and stored in the data block key_ptr = current_prev_entry.key_ptr; diff --git a/table/block_based_table_reader.cc b/table/block_based_table_reader.cc index beb6f74ea..efc619222 100644 --- a/table/block_based_table_reader.cc +++ b/table/block_based_table_reader.cc @@ -1052,6 +1052,7 @@ InternalIterator* BlockBasedTable::NewIndexIterator( Status s; s = CreateIndexReader(&index_reader); if (s.ok()) { + assert(index_reader != nullptr); s = block_cache->Insert(key, index_reader, index_reader->usable_size(), &DeleteCachedIndexEntry, &cache_handle); } @@ -1062,6 +1063,9 @@ InternalIterator* BlockBasedTable::NewIndexIterator( RecordTick(statistics, BLOCK_CACHE_BYTES_WRITE, usable_size); RecordTick(statistics, BLOCK_CACHE_INDEX_BYTES_INSERT, usable_size); } else { + if (index_reader != nullptr) { + delete index_reader; + } RecordTick(statistics, BLOCK_CACHE_ADD_FAILURES); // make sure if something goes wrong, index_reader shall remain intact. if (input_iter != nullptr) { @@ -1189,7 +1193,8 @@ InternalIterator* BlockBasedTable::NewDataBlockIterator( } InternalIterator* iter; - if (s.ok() && block.value != nullptr) { + if (s.ok()) { + assert(block.value != nullptr); iter = block.value->NewIterator(&rep->internal_comparator, input_iter); if (block.cache_handle != nullptr) { iter->RegisterCleanup(&ReleaseCachedEntry, block_cache, @@ -1198,6 +1203,7 @@ InternalIterator* BlockBasedTable::NewDataBlockIterator( iter->RegisterCleanup(&DeleteHeldResource, block.value, nullptr); } } else { + assert(block.value == nullptr); if (input_iter != nullptr) { input_iter->SetStatus(s); iter = input_iter; diff --git a/table/block_prefix_index.cc b/table/block_prefix_index.cc index bc6465a32..10fcb0575 100644 --- a/table/block_prefix_index.cc +++ b/table/block_prefix_index.cc @@ -136,6 +136,7 @@ class BlockPrefixIndex::Builder { assert(prefixes_per_bucket[i]->next == nullptr); buckets[i] = prefixes_per_bucket[i]->start_block; } else { + assert(total_block_array_entries > 0); assert(prefixes_per_bucket[i] != nullptr); buckets[i] = EncodeIndex(offset); block_array_buffer[offset] = num_blocks; diff --git a/table/table_test.cc b/table/table_test.cc index 53aaad532..d9cb799b5 100644 --- a/table/table_test.cc +++ b/table/table_test.cc @@ -1765,7 +1765,6 @@ TEST_F(BlockBasedTableTest, FilterBlockInBlockCache) { ASSERT_GT(props.GetCacheBytesRead(), last_cache_bytes_read); ASSERT_EQ(props.GetCacheBytesWrite(), table_options.block_cache->GetUsage()); - last_cache_bytes_read = props.GetCacheBytesRead(); } // release the iterator so that the block cache can reset correctly. iter.reset(); diff --git a/third-party/gtest-1.7.0/fused-src/gtest/gtest-all.cc b/third-party/gtest-1.7.0/fused-src/gtest/gtest-all.cc index bdb83d553..154ec6f26 100644 --- a/third-party/gtest-1.7.0/fused-src/gtest/gtest-all.cc +++ b/third-party/gtest-1.7.0/fused-src/gtest/gtest-all.cc @@ -34,6 +34,9 @@ // Sometimes it's desirable to build Google Test by compiling a single file. // This file serves this purpose. +// Suppress clang analyzer warnings. +#ifndef __clang_analyzer__ + // This line ensures that gtest.h can be compiled on its own, even // when it's fused. #include "gtest/gtest.h" @@ -109,7 +112,6 @@ #ifndef GTEST_INCLUDE_GTEST_GTEST_SPI_H_ #define GTEST_INCLUDE_GTEST_GTEST_SPI_H_ - namespace testing { // This helper class can be used to mock out Google Test failure reporting @@ -10255,3 +10257,5 @@ const char* TypedTestCasePState::VerifyRegisteredTestNames( } // namespace internal } // namespace testing + +#endif // __clang_analyzer__ diff --git a/tools/db_sanity_test.cc b/tools/db_sanity_test.cc index 773acff6b..84d459993 100644 --- a/tools/db_sanity_test.cc +++ b/tools/db_sanity_test.cc @@ -230,6 +230,9 @@ class SanityTestBloomFilter : public SanityTest { namespace { bool RunSanityTests(const std::string& command, const std::string& path) { + bool result = true; +// Suppress false positive clang static anaylzer warnings. +#ifndef __clang_analyzer__ std::vector sanity_tests = { new SanityTestBasic(path), new SanityTestSpecialComparator(path), @@ -248,7 +251,6 @@ bool RunSanityTests(const std::string& command, const std::string& path) { } else { fprintf(stderr, "Verifying...\n"); } - bool result = true; for (auto sanity_test : sanity_tests) { Status s; fprintf(stderr, "%s -- ", sanity_test->Name().c_str()); @@ -266,6 +268,7 @@ bool RunSanityTests(const std::string& command, const std::string& path) { delete sanity_test; } +#endif // __clang_analyzer__ return result; } } // namespace diff --git a/util/dynamic_bloom_test.cc b/util/dynamic_bloom_test.cc index bad88a94b..5b8ec7493 100644 --- a/util/dynamic_bloom_test.cc +++ b/util/dynamic_bloom_test.cc @@ -201,6 +201,7 @@ TEST_F(DynamicBloomTest, perf) { } ASSERT_EQ(count, num_keys); elapsed = timer.ElapsedNanos(); + assert(count > 0); fprintf(stderr, "standard bloom, avg query latency %" PRIu64 "\n", elapsed / count); @@ -227,6 +228,7 @@ TEST_F(DynamicBloomTest, perf) { } elapsed = timer.ElapsedNanos(); + assert(count > 0); fprintf(stderr, "blocked bloom(enable locality), avg query latency %" PRIu64 "\n", elapsed / count); diff --git a/util/options_parser.cc b/util/options_parser.cc index 0c368c646..e30cf8df9 100644 --- a/util/options_parser.cc +++ b/util/options_parser.cc @@ -756,6 +756,7 @@ Status RocksDBOptionsParser::VerifyBlockBasedTableFactory( if (base_tf == nullptr) { return Status::OK(); } + assert(file_tf != nullptr); const auto& base_opt = base_tf->table_options(); const auto& file_opt = file_tf->table_options(); diff --git a/util/thread_local_test.cc b/util/thread_local_test.cc index 737a2654f..262b6a557 100644 --- a/util/thread_local_test.cc +++ b/util/thread_local_test.cc @@ -57,6 +57,8 @@ class IDChecker : public ThreadLocalPtr { } // anonymous namespace +// Suppress false positive clang analyzer warnings. +#ifndef __clang_analyzer__ TEST_F(ThreadLocalTest, UniqueIdTest) { port::Mutex mu; port::CondVar cv(&mu); @@ -103,6 +105,7 @@ TEST_F(ThreadLocalTest, UniqueIdTest) { // After exit, id sequence in queue: // 3, 1, 2, 0 } +#endif // __clang_analyzer__ TEST_F(ThreadLocalTest, SequentialReadWriteTest) { // global id list carries over 3, 1, 2, 0 diff --git a/utilities/write_batch_with_index/write_batch_with_index.cc b/utilities/write_batch_with_index/write_batch_with_index.cc index 045cf552f..67f3e2895 100644 --- a/utilities/write_batch_with_index/write_batch_with_index.cc +++ b/utilities/write_batch_with_index/write_batch_with_index.cc @@ -226,6 +226,8 @@ class BaseDeltaIterator : public Iterator { bool BaseValid() const { return base_iterator_->Valid(); } bool DeltaValid() const { return delta_iterator_->Valid(); } void UpdateCurrent() { +// Suppress false positive clang analyzer warnings. +#ifndef __clang_analyzer__ while (true) { WriteEntry delta_entry; if (DeltaValid()) { @@ -275,6 +277,7 @@ class BaseDeltaIterator : public Iterator { } AssertInvariants(); +#endif // __clang_analyzer__ } bool forward_;