From 1e8d7d6f1e2d17cde2126144ed3a8fc5e7555f7a Mon Sep 17 00:00:00 2001 From: tzaeschke Date: Sat, 28 Oct 2023 13:28:17 +0200 Subject: [PATCH] WIP - test does not fail yet --- include/phtree/converter.h | 12 ++ include/phtree/phtree_multimap.h | 3 +- include/phtree/v16/node.h | 3 +- include/phtree/v16/phtree_v16.h | 2 +- test/BUILD | 13 +++ test/phtree_test_issue_153.cc | 191 +++++++++++++++++++++++++++++++ 6 files changed, 221 insertions(+), 3 deletions(-) create mode 100644 test/phtree_test_issue_153.cc diff --git a/include/phtree/converter.h b/include/phtree/converter.h index 9781d39b..463778f3 100644 --- a/include/phtree/converter.h +++ b/include/phtree/converter.h @@ -71,6 +71,18 @@ class ScalarConverterIEEE { } }; +template +class ScalarConverterNoOp { + public: + static SCALAR pre(SCALAR value) noexcept { + return value; + } + + static SCALAR post(SCALAR value) { + return value; + } +}; + /* * The ScalarMultiplyConverter converts floating point scalars 'f' (double or float) to integral * scalars 'i' and back. diff --git a/include/phtree/phtree_multimap.h b/include/phtree/phtree_multimap.h index f449d5aa..eced2e81 100644 --- a/include/phtree/phtree_multimap.h +++ b/include/phtree/phtree_multimap.h @@ -829,7 +829,8 @@ using PhTreeMultiMapF = PhTreeMultiMap; template < dimension_t DIM, typename T, - typename CONVERTER_BOX, + //typename CONVERTER_BOX = ConverterBoxBase, + typename CONVERTER_BOX = SimpleBoxConverter>, typename BUCKET = b_plus_tree_hash_set> using PhTreeMultiMapBox = PhTreeMultiMap; diff --git a/include/phtree/v16/node.h b/include/phtree/v16/node.h index 4edc49d6..e52726b1 100644 --- a/include/phtree/v16/node.h +++ b/include/phtree/v16/node.h @@ -47,7 +47,8 @@ using EntryMap = typename std::conditional_t< detail::array_map, Entry, (size_t(1) << DIM)>, typename std::conditional_t< DIM <= 8, - detail::sparse_map, Entry>, + //detail::sparse_map, Entry>, + std::map, Entry>, ::phtree::bptree::b_plus_tree_map, Entry, (uint64_t(1) << DIM)>>>; template diff --git a/include/phtree/v16/phtree_v16.h b/include/phtree/v16/phtree_v16.h index 6b8dc90a..092acfcc 100644 --- a/include/phtree/v16/phtree_v16.h +++ b/include/phtree/v16/phtree_v16.h @@ -464,7 +464,7 @@ class PhTreeV16 { // It may happen that node_entry is not the immediate parent, but that is okay! if (entry != nullptr && entry->GetValue().empty()) { bool found = false; - while (node_entry != nullptr && node_entry->IsNode()) { + while (node_entry != nullptr) { found = false; node_entry = node_entry->GetNode().Erase(key, node_entry, node_entry != &root_, found); diff --git a/test/BUILD b/test/BUILD index ec0e1c33..b23c30f6 100644 --- a/test/BUILD +++ b/test/BUILD @@ -311,3 +311,16 @@ cc_test( "@gtest//:gtest_main", ], ) + +cc_test( + name = "phtree_test_issue_153", + timeout = "long", + srcs = [ + "phtree_test_issue_153.cc", + ], + linkstatic = True, + deps = [ + "//:phtree", + "@gtest//:gtest_main", + ], +) diff --git a/test/phtree_test_issue_153.cc b/test/phtree_test_issue_153.cc new file mode 100644 index 00000000..4e6a1a09 --- /dev/null +++ b/test/phtree_test_issue_153.cc @@ -0,0 +1,191 @@ +/* + * Copyright 2023 Tilmann Zäschke + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "phtree/phtree.h" +#include "phtree/phtree_multimap.h" +#include +#include +#include + +using namespace improbable::phtree; + +namespace phtree_test_issue_153 { + +// Number of entries that have the same coordinate +static const size_t NUM_DUPL = 1; +static const double WORLD_MIN = -100; +static const double WORLD_MAX = 100; +static const double BOX_LEN = 1; + +template +using TestKey = PhBoxD; + +class DoubleRng { + public: + DoubleRng(double minIncl, double maxExcl) : eng(), rnd{minIncl, maxExcl} {} + + double next() { + return rnd(eng); + } + + private: + std::default_random_engine eng; + std::uniform_real_distribution rnd; +}; + +template +void generateCube(std::vector>& points, size_t N) { + assert(N % NUM_DUPL == 0); + DoubleRng rng(WORLD_MIN, WORLD_MAX); + auto reference_set = std::unordered_map, size_t>(); + + points.reserve(N); + for (size_t i = 0; i < N / NUM_DUPL; i++) { + // create duplicates, i.e. entries with the same coordinates. However, avoid unintentional + // duplicates. + TestKey key{}; + for (dimension_t d = 0; d < DIM; ++d) { + key.min()[d] = rng.next(); + key.max()[d] = key.min()[d] + BOX_LEN; + } + if (reference_set.count(key) != 0) { + i--; + continue; + } + reference_set.emplace(key, i); + for (size_t dupl = 0; dupl < NUM_DUPL; dupl++) { + auto point = TestKey(key); + points.push_back(point); + } + } + ASSERT_EQ(reference_set.size(), N / NUM_DUPL); + ASSERT_EQ(points.size(), N); +} + +TEST(PhTreeTestIssue153, TestIssue153) { + /* + * This used to cause a heap-use-after-free problem in sparse_map + */ + auto points = std::vector>(); + points.emplace_back(PhBoxD<2>{{1, 1}, {1, 1}}); + points.emplace_back(PhBoxD<2>{{3, 3}, {3, 3}}); + points.emplace_back(PhBoxD<2>{{1, 3}, {1, 3}}); + points.emplace_back(PhBoxD<2>{{3, 1}, {3, 1}}); + points.emplace_back(PhBoxD<2>{{0, 0}, {0, 0}}); + + // The real test is ABOVE, see "#define CALLBACK" + auto tree = PhTreeMultiMapBoxD<2, size_t>(); + for (size_t i = 0; i < points.size(); ++i) { + tree.emplace(points[i], i); + } + + PhBoxD<2> b{{2, 2}, {2, 2}}; + for (size_t r = 0; r < 100; ++r) { + for (size_t i = 0; i < points.size(); ++i) { + PhBoxD<2>& bOld = points[i]; + double m0 = (bOld.min()[0] + b.min()[0]) / 2; + double m1 = (bOld.min()[1] + b.min()[1]) / 2; + double m2 = (bOld.max()[0] + b.max()[0]) / 2; + double m3 = (bOld.max()[1] + b.max()[1]) / 2; + PhBoxD<2> bNew{{m0, m1}, {m2, m3}}; + ASSERT_EQ(1, tree.relocate(bOld, bNew, i)); + points[i] = bNew; + } + } +} + +/* + * Try move in ever smaller steps. + */ +TEST(PhTreeTestIssue153, TestIssue153_2) { + int N = 10; + const dimension_t DIM = 2; + std::vector> points; + generateCube(points, N); + + auto tree = PhTreeMultiMapBoxD(); + for (size_t i = 0; i < points.size(); ++i) { + tree.emplace(points[i], i); + } + + TestKey x{}; + for (dimension_t d = 0; d < DIM; ++d) { + x.min()[d] = 2.; + x.max()[d] = 2.; + } + + for (size_t r = 0; r < 100; ++r) { + for (size_t i = 0; i < points.size(); ++i) { + TestKey& bOld = points[i]; + TestKey bNew{}; + for (dimension_t d = 0; d < DIM; ++d) { + double m0 = (bOld.min()[d] + x.min()[d]) / 2; + double m1 = (bOld.max()[d] + x.max()[d]) / 2; + bNew.min()[d] = m0; + bNew.max()[d] = m1; + } + ASSERT_EQ(1, tree.relocate(bOld, bNew, i)); + points[i] = bNew; + } + } +} + +/* + * Try moving in very small (but const-sized) steps. + */ +TEST(PhTreeTestIssue153, TestIssue153_Linear) { + int N = 10; + const int R_MAX = 1000000; + const dimension_t DIM = 2; + std::vector> points; + generateCube(points, N); + + TestKey x{}; + for (dimension_t d = 0; d < DIM; ++d) { + x.min()[d] = 2.; + x.max()[d] = 2. + BOX_LEN; + } + + std::vector> moves; + + auto tree = PhTreeMultiMapBoxD(); + for (size_t i = 0; i < points.size(); ++i) { + tree.emplace(points[i], i); + // distances.emplace_back(distance(points[i], x)); + PhPointD& m = moves.emplace_back(); + for (dimension_t d = 0; d < DIM; ++d) { + m[d] = (x.min()[d] - points[i].min()[d]) / R_MAX; + } + } + + for (size_t r = 0; r < R_MAX; ++r) { + for (size_t i = 0; i < points.size(); ++i) { + TestKey& bOld = points[i]; + if (i == 0) + std::cout << "r=" << r << " i=" << i << " " << bOld << std::endl; + TestKey bNew{}; + PhPointD mov = moves[i]; + for (dimension_t d = 0; d < DIM; ++d) { + bNew.min()[d] = bOld.min()[d] + mov[d]; + bNew.max()[d] = bOld.max()[d] + mov[d]; + } + ASSERT_EQ(1, tree.relocate(bOld, bNew, i)); + points[i] = bNew; + } + } +} + +} // namespace phtree_test_issue_153