Skip to content

Commit

Permalink
No elide constructors (facebook#7798)
Browse files Browse the repository at this point in the history 
Summary:
Added "no-elide-constructors to the ASSERT_STATUS_CHECK builds.  This flag gives more errors/warnings for some of the Status checks where an inner class checks a Status and later returns it.  In this case,  without the elide check on, the returned status may not have been checked in the caller, thereby bypassing the checked code.

Pull Request resolved: facebook#7798

Reviewed By: jay-zhuang

Differential Revision: D25680451

Pulled By: pdillinger

fbshipit-source-id: c3f14ed9e2a13f0a8c54d839d5fb4d1fc1e93917
  • Loading branch information
@mrambacher @facebook-github-bot
mrambacher authored and facebook-github-bot committed Dec 24, 2020
1 parent 30a5ed9 commit 55e9968
Show file tree
Hide file tree
Showing 33 changed files with 310 additions and 302 deletions.
5 changes: 5 additions & 0 deletions Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,11 @@ else
endif

ifdef ASSERT_STATUS_CHECKED
# For ASC, turn off constructor elision, preventing the case where a constructor returned
# by a method may pass the ASC check if the status is checked in the inner method. Forcing
# the copy constructor to be invoked disables the optimization and will cause the calling method
# to check the status in order to prevent an error from being raised.
PLATFORM_CXXFLAGS += -fno-elide-constructors
ifeq ($(filter -DROCKSDB_ASSERT_STATUS_CHECKED,$(OPT)),)
OPT += -DROCKSDB_ASSERT_STATUS_CHECKED
endif
Expand Down
4 changes: 3 additions & 1 deletion db/column_family.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,9 @@ ColumnFamilyOptions SanitizeOptions(const ImmutableDBOptions& db_options,
// was not used)
auto sfm = static_cast<SstFileManagerImpl*>(db_options.sst_file_manager.get());
for (size_t i = 0; i < result.cf_paths.size(); i++) {
DeleteScheduler::CleanupDirectory(db_options.env, sfm, result.cf_paths[i].path);
DeleteScheduler::CleanupDirectory(db_options.env, sfm,
result.cf_paths[i].path)
.PermitUncheckedError();
}
#endif

Expand Down
44 changes: 22 additions & 22 deletions db/column_family_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class ColumnFamilyTestBase : public testing::Test {
db_options_.create_if_missing = true;
db_options_.fail_if_options_file_error = true;
db_options_.env = env_;
DestroyDB(dbname_, Options(db_options_, column_family_options_));
EXPECT_OK(DestroyDB(dbname_, Options(db_options_, column_family_options_)));
}

~ColumnFamilyTestBase() override {
Expand Down Expand Up @@ -653,8 +653,8 @@ TEST_P(FlushEmptyCFTestWithParam, FlushEmptyCFTest) {
// after flushing file B is deleted. At the same time, the min log number of
// default CF is not written to manifest. Log file A still remains.
// Flushed to SST file Y.
Flush(1);
Flush(0);
ASSERT_OK(Flush(1));
ASSERT_OK(Flush(0));
ASSERT_OK(Put(1, "bar", "v3")); // seqID 4
ASSERT_OK(Put(1, "foo", "v4")); // seqID 5
ASSERT_OK(db_->FlushWAL(/*sync=*/false));
Expand Down Expand Up @@ -708,15 +708,15 @@ TEST_P(FlushEmptyCFTestWithParam, FlushEmptyCFTest2) {
// and is set to current. Both CFs' min log number is set to file C so after
// flushing file B is deleted. Log file A still remains.
// Flushed to SST file Y.
Flush(1);
ASSERT_OK(Flush(1));
ASSERT_OK(Put(0, "bar", "v2")); // seqID 4
ASSERT_OK(Put(2, "bar", "v2")); // seqID 5
ASSERT_OK(Put(1, "bar", "v3")); // seqID 6
// Flushing all column families. This forces all CFs' min log to current. This
// is written to the manifest file. Log file C is cleared.
Flush(0);
Flush(1);
Flush(2);
ASSERT_OK(Flush(0));
ASSERT_OK(Flush(1));
ASSERT_OK(Flush(2));
// Write to log file D
ASSERT_OK(Put(1, "bar", "v4")); // seqID 7
ASSERT_OK(Put(1, "bar", "v5")); // seqID 8
Expand Down Expand Up @@ -985,7 +985,7 @@ TEST_P(ColumnFamilyTest, FlushTest) {
for (int i = 0; i < 3; ++i) {
uint64_t max_total_in_memory_state =
MaxTotalInMemoryState();
Flush(i);
ASSERT_OK(Flush(i));
AssertMaxTotalInMemoryState(max_total_in_memory_state);
}
ASSERT_OK(Put(1, "foofoo", "bar"));
Expand Down Expand Up @@ -1093,7 +1093,7 @@ TEST_P(ColumnFamilyTest, CrashAfterFlush) {
ASSERT_OK(batch.Put(handles_[0], Slice("foo"), Slice("bar")));
ASSERT_OK(batch.Put(handles_[1], Slice("foo"), Slice("bar")));
ASSERT_OK(db_->Write(WriteOptions(), &batch));
Flush(0);
ASSERT_OK(Flush(0));
fault_env->SetFilesystemActive(false);

std::vector<std::string> names;
Expand All @@ -1103,7 +1103,7 @@ TEST_P(ColumnFamilyTest, CrashAfterFlush) {
}
}
Close();
fault_env->DropUnsyncedFileData();
ASSERT_OK(fault_env->DropUnsyncedFileData());
fault_env->ResetState();
Open(names, {});

Expand Down Expand Up @@ -2236,7 +2236,7 @@ TEST_P(ColumnFamilyTest, FlushStaleColumnFamilies) {
// files for column family [one], because it's empty
AssertCountLiveFiles(4);

Flush(0);
ASSERT_OK(Flush(0));
ASSERT_EQ(0, dbfull()->TEST_total_log_size());
Close();
}
Expand Down Expand Up @@ -3040,7 +3040,7 @@ TEST_P(ColumnFamilyTest, IteratorCloseWALFile1) {
Iterator* it = db_->NewIterator(ReadOptions(), handles_[1]);
ASSERT_OK(it->status());
// A flush will make `it` hold the last reference of its super version.
Flush(1);
ASSERT_OK(Flush(1));

ASSERT_OK(Put(1, "fodor", "mirko"));
ASSERT_OK(Put(0, "fodor", "mirko"));
Expand Down Expand Up @@ -3093,7 +3093,7 @@ TEST_P(ColumnFamilyTest, IteratorCloseWALFile2) {
Iterator* it = db_->NewIterator(ro, handles_[1]);
ASSERT_OK(it->status());
// A flush will make `it` hold the last reference of its super version.
Flush(1);
ASSERT_OK(Flush(1));

ASSERT_OK(Put(1, "fodor", "mirko"));
ASSERT_OK(Put(0, "fodor", "mirko"));
Expand Down Expand Up @@ -3147,7 +3147,7 @@ TEST_P(ColumnFamilyTest, ForwardIteratorCloseWALFile) {
CreateColumnFamilies({"one"});
ASSERT_OK(Put(1, "fodor", "mirko"));
ASSERT_OK(Put(1, "fodar2", "mirko"));
Flush(1);
ASSERT_OK(Flush(1));

// Create an iterator holding the current super version, as well as
// the SST file just flushed.
Expand All @@ -3159,7 +3159,7 @@ TEST_P(ColumnFamilyTest, ForwardIteratorCloseWALFile) {

ASSERT_OK(Put(1, "fodor", "mirko"));
ASSERT_OK(Put(1, "fodar2", "mirko"));
Flush(1);
ASSERT_OK(Flush(1));

WaitForCompaction();

Expand Down Expand Up @@ -3232,9 +3232,9 @@ TEST_P(ColumnFamilyTest, LogSyncConflictFlush) {
ROCKSDB_NAMESPACE::port::Thread thread([&] { ASSERT_OK(db_->SyncWAL()); });

TEST_SYNC_POINT("ColumnFamilyTest::LogSyncConflictFlush:1");
Flush(1);
ASSERT_OK(Flush(1));
ASSERT_OK(Put(1, "foo", "bar"));
Flush(1);
ASSERT_OK(Flush(1));

TEST_SYNC_POINT("ColumnFamilyTest::LogSyncConflictFlush:2");

Expand All @@ -3256,7 +3256,7 @@ TEST_P(ColumnFamilyTest, DISABLED_LogTruncationTest) {
Build(0, 100);

// Flush the 0th column family to force a roll of the wal log
Flush(0);
ASSERT_OK(Flush(0));

// Add some more entries
Build(100, 100);
Expand Down Expand Up @@ -3332,14 +3332,14 @@ TEST_P(ColumnFamilyTest, DefaultCfPathsTest) {

// Fill Column family 1.
PutRandomData(1, 100, 100);
Flush(1);
ASSERT_OK(Flush(1));

ASSERT_EQ(1, GetSstFileCount(cf_opt1.cf_paths[0].path));
ASSERT_EQ(0, GetSstFileCount(dbname_));

// Fill column family 2
PutRandomData(2, 100, 100);
Flush(2);
ASSERT_OK(Flush(2));

// SST from Column family 2 should be generated in
// db_paths which is dbname_ in this case.
Expand All @@ -3358,14 +3358,14 @@ TEST_P(ColumnFamilyTest, MultipleCFPathsTest) {
Reopen({ColumnFamilyOptions(), cf_opt1, cf_opt2});

PutRandomData(1, 100, 100, true /* save */);
Flush(1);
ASSERT_OK(Flush(1));

// Check that files are generated in appropriate paths.
ASSERT_EQ(1, GetSstFileCount(cf_opt1.cf_paths[0].path));
ASSERT_EQ(0, GetSstFileCount(dbname_));

PutRandomData(2, 100, 100, true /* save */);
Flush(2);
ASSERT_OK(Flush(2));

ASSERT_EQ(1, GetSstFileCount(cf_opt2.cf_paths[0].path));
ASSERT_EQ(0, GetSstFileCount(dbname_));
Expand Down
2 changes: 1 addition & 1 deletion db/compaction/compaction_job_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ class CompactionJobTestBase : public testing::Test {
&write_buffer_manager_, &write_controller_,
/*block_cache_tracer=*/nullptr, /*io_tracer=*/nullptr));
compaction_job_stats_.Reset();
SetIdentityFile(env_, dbname_);
ASSERT_OK(SetIdentityFile(env_, dbname_));

VersionEdit new_db;
new_db.SetLogNumber(0);
Expand Down
18 changes: 9 additions & 9 deletions db/corruption_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -579,7 +579,7 @@ TEST_F(CorruptionTest, FileSystemStateCorrupted) {

if (iter == 0) { // corrupt file size
std::unique_ptr<WritableFile> file;
env_->NewWritableFile(filename, &file, EnvOptions());
ASSERT_OK(env_->NewWritableFile(filename, &file, EnvOptions()));
ASSERT_OK(file->Append(Slice("corrupted sst")));
file.reset();
Status x = TryReopen(&options);
Expand Down Expand Up @@ -616,7 +616,7 @@ TEST_F(CorruptionTest, ParanoidFileChecksOnFlush) {
options.table_factory = mock;
mock->SetCorruptionMode(mode);
ASSERT_OK(DB::Open(options, dbname_, &db_));
assert(db_ != nullptr);
assert(db_ != nullptr); // suppress false clang-analyze report
Build(10);
s = db_->Flush(FlushOptions());
if (mode == mock::MockTableFactory::kCorruptNone) {
Expand All @@ -642,7 +642,7 @@ TEST_F(CorruptionTest, ParanoidFileChecksOnCompact) {
std::make_shared<mock::MockTableFactory>();
options.table_factory = mock;
ASSERT_OK(DB::Open(options, dbname_, &db_));
assert(db_ != nullptr);
assert(db_ != nullptr); // suppress false clang-analyze report
Build(100, 2);
// ASSERT_OK(db_->Flush(FlushOptions()));
DBImpl* dbi = static_cast_with_check<DBImpl>(db_);
Expand All @@ -669,7 +669,7 @@ TEST_F(CorruptionTest, ParanoidFileChecksWithDeleteRangeFirst) {
ASSERT_OK(DestroyDB(dbname_, options));
ASSERT_OK(DB::Open(options, dbname_, &db_));
std::string start, end;
assert(db_ != nullptr);
assert(db_ != nullptr); // suppress false clang-analyze report
ASSERT_OK(db_->DeleteRange(WriteOptions(), db_->DefaultColumnFamily(),
Key(3, &start), Key(7, &end)));
auto snap = db_->GetSnapshot();
Expand Down Expand Up @@ -701,7 +701,7 @@ TEST_F(CorruptionTest, ParanoidFileChecksWithDeleteRange) {
db_ = nullptr;
ASSERT_OK(DestroyDB(dbname_, options));
ASSERT_OK(DB::Open(options, dbname_, &db_));
assert(db_ != nullptr);
assert(db_ != nullptr); // suppress false clang-analyze report
Build(10, 0, 0);
std::string start, end;
ASSERT_OK(db_->DeleteRange(WriteOptions(), db_->DefaultColumnFamily(),
Expand Down Expand Up @@ -737,7 +737,7 @@ TEST_F(CorruptionTest, ParanoidFileChecksWithDeleteRangeLast) {
db_ = nullptr;
ASSERT_OK(DestroyDB(dbname_, options));
ASSERT_OK(DB::Open(options, dbname_, &db_));
assert(db_ != nullptr);
assert(db_ != nullptr); // suppress false clang-analyze report
std::string start, end;
Build(10);
ASSERT_OK(db_->DeleteRange(WriteOptions(), db_->DefaultColumnFamily(),
Expand Down Expand Up @@ -775,7 +775,7 @@ TEST_F(CorruptionTest, LogCorruptionErrorsInCompactionIterator) {
options.table_factory = mock;

ASSERT_OK(DB::Open(options, dbname_, &db_));
assert(db_ != nullptr);
assert(db_ != nullptr); // suppress false clang-analyze report
Build(100, 2);

DBImpl* dbi = static_cast_with_check<DBImpl>(db_);
Expand All @@ -798,7 +798,7 @@ TEST_F(CorruptionTest, CompactionKeyOrderCheck) {
std::make_shared<mock::MockTableFactory>();
options.table_factory = mock;
ASSERT_OK(DB::Open(options, dbname_, &db_));
assert(db_ != nullptr);
assert(db_ != nullptr); // suppress false clang-analyze report
mock->SetCorruptionMode(mock::MockTableFactory::kCorruptReorderKey);
Build(100, 2);
DBImpl* dbi = static_cast_with_check<DBImpl>(db_);
Expand Down Expand Up @@ -884,7 +884,7 @@ TEST_F(CorruptionTest, VerifyWholeTableChecksum) {
SyncPoint::GetInstance()->SetCallBack(
"DBImpl::VerifySstFileChecksum:mismatch", [&](void* arg) {
auto* s = reinterpret_cast<Status*>(arg);
assert(s);
ASSERT_NE(s, nullptr);
++count;
ASSERT_NOK(*s);
});
Expand Down
Loading

0 comments on commit 55e9968

Please sign in to comment.