diff options
author | Peter Dillinger <peterd@meta.com> | 2024-09-06 10:11:34 -0700 |
---|---|---|
committer | Facebook GitHub Bot <facebook-github-bot@users.noreply.github.com> | 2024-09-06 10:11:34 -0700 |
commit | 4577b672d548e88b58193adcaf05f12eb0fc1835 (patch) | |
tree | 7312fb3e6b49fb51271b3f3836a87a317124e169 | |
parent | 0d3aaf7c0f9c15cb52954e89c171ac880079a8c7 (diff) |
More valgrind fixes (#12990)
Summary:
* https://github.com/facebook/rocksdb/issues/12936 was insufficient to fix the std::optional false positives. Making a fix validated in CI this time (see https://github.com/facebook/rocksdb/issues/12991)
* valgrind grinds to a halt on startup on my dev machine apparently because it expects internet access. Disable its attempts to access the internet when git is using a proxy.
* Move PORTABLE=1 from CI job to the Makefile. Without it, valgrind complains about illegal instructions (too new)
Pull Request resolved: https://github.com/facebook/rocksdb/pull/12990
Test Plan: manual, watch nightly valgrind job
Reviewed By: ltamasi
Differential Revision: D62203242
Pulled By: pdillinger
fbshipit-source-id: a611b08da7dbd173b0709ed7feb0578729553a17
-rw-r--r-- | .github/workflows/nightly.yml | 2 | ||||
-rw-r--r-- | Makefile | 13 | ||||
-rw-r--r-- | table/block_based/filter_policy.cc | 24 |
3 files changed, 25 insertions, 14 deletions
diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index 0bf343639..1370a5460 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -80,7 +80,7 @@ jobs: steps: - uses: actions/checkout@v4.1.0 - uses: "./.github/actions/pre-steps" - - run: PORTABLE=1 make V=1 -j32 valgrind_test + - run: make V=1 -j32 valgrind_test - uses: "./.github/actions/post-steps" build-windows-vs2022-avx2: if: ${{ github.repository_owner == 'facebook' }} @@ -630,6 +630,11 @@ VALGRIND_VER := $(join $(VALGRIND_VER),valgrind) VALGRIND_OPTS = --error-exitcode=$(VALGRIND_ERROR) --leak-check=full # Not yet supported: --show-leak-kinds=definite,possible,reachable --errors-for-leak-kinds=definite,possible,reachable +# Work around valgrind hanging on systems with limited internet access +ifneq ($(shell which git 2>/dev/null && git config --get https.proxy),) + export DEBUGINFOD_URLS= +endif + TEST_OBJECTS = $(patsubst %.cc, $(OBJ_DIR)/%.o, $(TEST_LIB_SOURCES) $(MOCK_LIB_SOURCES)) $(GTEST) BENCH_OBJECTS = $(patsubst %.cc, $(OBJ_DIR)/%.o, $(BENCH_LIB_SOURCES)) CACHE_BENCH_OBJECTS = $(patsubst %.cc, $(OBJ_DIR)/%.o, $(CACHE_BENCH_LIB_SOURCES)) @@ -1164,16 +1169,16 @@ ubsan_crash_test_with_best_efforts_recovery: clean $(MAKE) clean full_valgrind_test: - ROCKSDB_FULL_VALGRIND_RUN=1 DISABLE_JEMALLOC=1 $(MAKE) valgrind_check + ROCKSDB_FULL_VALGRIND_RUN=1 DISABLE_JEMALLOC=1 PORTABLE=1 $(MAKE) valgrind_check full_valgrind_test_some: - ROCKSDB_FULL_VALGRIND_RUN=1 DISABLE_JEMALLOC=1 $(MAKE) valgrind_check_some + ROCKSDB_FULL_VALGRIND_RUN=1 DISABLE_JEMALLOC=1 PORTABLE=1 $(MAKE) valgrind_check_some valgrind_test: - ROCKSDB_VALGRIND_RUN=1 DISABLE_JEMALLOC=1 $(MAKE) valgrind_check + ROCKSDB_VALGRIND_RUN=1 DISABLE_JEMALLOC=1 PORTABLE=1 $(MAKE) valgrind_check valgrind_test_some: - ROCKSDB_VALGRIND_RUN=1 DISABLE_JEMALLOC=1 $(MAKE) valgrind_check_some + ROCKSDB_VALGRIND_RUN=1 DISABLE_JEMALLOC=1 PORTABLE=1 $(MAKE) valgrind_check_some valgrind_check: $(TESTS) $(MAKE) DRIVER="$(VALGRIND_VER) $(VALGRIND_OPTS)" gen_parallel_tests diff --git a/table/block_based/filter_policy.cc b/table/block_based/filter_policy.cc index 233ced703..049148a8a 100644 --- a/table/block_based/filter_policy.cc +++ b/table/block_based/filter_policy.cc @@ -91,9 +91,24 @@ class XXPH3FilterBitsBuilder : public BuiltinFilterBitsBuilder { uint64_t alt_hash = GetSliceHash64(alt); std::optional<uint64_t> prev_key_hash; std::optional<uint64_t> prev_alt_hash = hash_entries_info_.prev_alt_hash; + if (!hash_entries_info_.entries.empty()) { prev_key_hash = hash_entries_info_.entries.back(); } + +#ifdef ROCKSDB_VALGRIND_RUN + // Valgrind can report uninitialized FPs on std::optional usage. See e.g. + // https://stackoverflow.com/q/51616179 + if (!prev_key_hash.has_value()) { + std::memset((void*)&prev_key_hash, 0, sizeof(prev_key_hash)); + prev_key_hash.reset(); + } + if (!prev_alt_hash.has_value()) { + std::memset((void*)&prev_alt_hash, 0, sizeof(prev_key_hash)); + prev_alt_hash.reset(); + } +#endif + // Add alt first, so that entries.back() always contains previous key // ASSUMING a change from one alt to the next implies a change to // corresponding key @@ -295,15 +310,6 @@ class XXPH3FilterBitsBuilder : public BuiltinFilterBitsBuilder { bool detect_filter_construct_corruption_; struct HashEntriesInfo { -#ifdef ROCKSDB_VALGRIND_RUN - HashEntriesInfo() { - // Valgrind can report uninitialized FPs on std::optional usage. See e.g. - // https://stackoverflow.com/q/51616179 - std::memset((void*)&prev_alt_hash, 0, sizeof(prev_alt_hash)); - prev_alt_hash = {}; - } -#endif - // A deque avoids unnecessary copying of already-saved values // and has near-minimal peak memory use. std::deque<uint64_t> entries; |