Add missing status check when compiling with ASSERT_STATUS_CHECKED=1
#11686
Conversation
options/options_settable_test.cc
Outdated
| @@ -34,6 +34,8 @@ namespace ROCKSDB_NAMESPACE { | |||
| #if defined OS_LINUX || defined OS_WIN | |||
| #ifndef __clang__ | |||
| #ifndef ROCKSDB_UBSAN_RUN | |||
| // status check adds CXX flag -fno-elide-constructors which fails this test. | |||
| #ifndef ROCKSDB_ASSERT_STATUS_CHECKED | |||
There was a problem hiding this comment.
Tests in this file fail when ROCKSDB_ASSERT_STATUS_CHECKED is enabled, so I disabled these tests for now.
There was a problem hiding this comment.
For some reason, I'm only seeing one test failure. Are you seeing more failures for disabling the whole test?
[ RUN ] OptionsSettableTest.ColumnFamilyOptionsAllFieldsSettable
options/options_settable_test.cc:568: Failure
Expected equality of these values:
unset_bytes_base
Which is: 115
NumUnsetBytes(new_options_ptr, sizeof(ColumnFamilyOptions), kColumnFamilyOptionsExcluded)
There was a problem hiding this comment.
You are right, only one test failed. Updated to only skip that test.
| @@ -269,7 +269,7 @@ Status BlobDBImpl::Open(std::vector<ColumnFamilyHandle*>* handles) { | |||
| // Add trash files in blob dir to file delete scheduler. | |||
| SstFileManagerImpl* sfm = static_cast<SstFileManagerImpl*>( | |||
| db_impl_->immutable_db_options().sst_file_manager.get()); | |||
| DeleteScheduler::CleanupDirectory(env_, sfm, blob_dir_); | |||
| s = DeleteScheduler::CleanupDirectory(env_, sfm, blob_dir_); | |||
There was a problem hiding this comment.
Change in non-unit-test code.
There was a problem hiding this comment.
I suggest following the pattern in this function that if(!s.ok()) {return s;} (optionally with some error logging if you'd like) following a s=... as we can't guarantee s = DeleteScheduler::... is the last s of the function in the future.
There was a problem hiding this comment.
Added status check and error logging.
|
@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
The unit tests are compiled with Line 160 in 9a03480 Line 243 in 9a03480 -fno-elide-constructors is not set, I'm guessing Status construction is optimized away, as well as the ASC check in Status destructor .
|
|
@cbi42 has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
|
@cbi42 has updated the pull request. You must reimport the pull request before landing. |
|
@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
…` (#11686) Summary: It seems the flag `-fno-elide-constructors` is incorrectly overwritten in Makefile by https://github.com/facebook/rocksdb/blob/9c2ebcc2c365bb89af566b3076f813d7bf11146b/Makefile#L243 Applying the change in PR facebook/rocksdb#11675 shows a lot of missing status checks. This PR adds the missing status checks. Most of changes are just adding asserts in unit tests. I'll add pr comment around more interesting changes that need review. Pull Request resolved: facebook/rocksdb#11686 Test Plan: change Makefile as in facebook/rocksdb#11675, and run `ASSERT_STATUS_CHECKED=1 TEST_UINT128_COMPAT=1 ROCKSDB_MODIFY_NPHASH=1 LIB_MODE=static OPT="-DROCKSDB_NAMESPACE=alternative_rocksdb_ns" make V=1 -j24 J=24 check` Reviewed By: hx235 Differential Revision: D48176132 Pulled By: cbi42 fbshipit-source-id: 6758946cfb1c6ff84c4c1e0ca540d05e6fc390bd
…` (#11686) Summary: It seems the flag `-fno-elide-constructors` is incorrectly overwritten in Makefile by https://github.com/facebook/rocksdb/blob/9c2ebcc2c365bb89af566b3076f813d7bf11146b/Makefile#L243 Applying the change in PR facebook/rocksdb#11675 shows a lot of missing status checks. This PR adds the missing status checks. Most of changes are just adding asserts in unit tests. I'll add pr comment around more interesting changes that need review. Pull Request resolved: facebook/rocksdb#11686 Test Plan: change Makefile as in facebook/rocksdb#11675, and run `ASSERT_STATUS_CHECKED=1 TEST_UINT128_COMPAT=1 ROCKSDB_MODIFY_NPHASH=1 LIB_MODE=static OPT="-DROCKSDB_NAMESPACE=alternative_rocksdb_ns" make V=1 -j24 J=24 check` Reviewed By: hx235 Differential Revision: D48176132 Pulled By: cbi42 fbshipit-source-id: 6758946cfb1c6ff84c4c1e0ca540d05e6fc390bd
It seems the flag
-fno-elide-constructorsis incorrectly overwritten in Makefile byrocksdb/Makefile
Line 243 in 9c2ebcc
Applying the change in PR #11675 shows a lot of missing status checks. This PR adds the missing status checks.
Most of changes are just adding asserts in unit tests. I'll add pr comment around more interesting changes that need review.
Test plan: change Makefile as in #11675, and run
ASSERT_STATUS_CHECKED=1 TEST_UINT128_COMPAT=1 ROCKSDB_MODIFY_NPHASH=1 LIB_MODE=static OPT="-DROCKSDB_NAMESPACE=alternative_rocksdb_ns" make V=1 -j24 J=24 check