From 8cbd629010b22eb735c7bb95f4c4cd396dae1039 Mon Sep 17 00:00:00 2001 From: sfan5 Date: Sat, 20 Jan 2024 15:37:30 +0100 Subject: [PATCH] Fix bugs in ModifySafeMap (#14276) --- src/unittest/CMakeLists.txt | 1 + src/unittest/test_datastructures.cpp | 181 +++++++++++++++++++++++++++ src/util/container.h | 34 +++-- 3 files changed, 206 insertions(+), 10 deletions(-) create mode 100644 src/unittest/test_datastructures.cpp diff --git a/src/unittest/CMakeLists.txt b/src/unittest/CMakeLists.txt index 84c545459..8466902df 100644 --- a/src/unittest/CMakeLists.txt +++ b/src/unittest/CMakeLists.txt @@ -9,6 +9,7 @@ set (UNITTEST_SRCS ${CMAKE_CURRENT_SOURCE_DIR}/test_compression.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_connection.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_craft.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/test_datastructures.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_filesys.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_inventory.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_irrptr.cpp diff --git a/src/unittest/test_datastructures.cpp b/src/unittest/test_datastructures.cpp new file mode 100644 index 000000000..a7e24b989 --- /dev/null +++ b/src/unittest/test_datastructures.cpp @@ -0,0 +1,181 @@ +/* +Minetest +Copyright (C) 2024 sfan5 + +This program is free software; you can redistribute it and/or modify +it under the terms of the GNU Lesser General Public License as published by +the Free Software Foundation; either version 2.1 of the License, or +(at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU Lesser General Public License for more details. + +You should have received a copy of the GNU Lesser General Public License along +with this program; if not, write to the Free Software Foundation, Inc., +51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +*/ + +#include "test.h" + +#include "util/container.h" + +class TestDataStructures : public TestBase +{ +public: + TestDataStructures() { TestManager::registerTestModule(this); } + const char *getName() { return "TestDataStructures"; } + + void runTests(IGameDef *gamedef); + + void testMap1(); + void testMap2(); + void testMap3(); + void testMap4(); + void testMap5(); +}; + +static TestDataStructures g_test_instance; + +void TestDataStructures::runTests(IGameDef *gamedef) +{ + rawstream << "-------- ModifySafeMap" << std::endl; + TEST(testMap1); + TEST(testMap2); + TEST(testMap3); + TEST(testMap4); + TEST(testMap5); +} + +namespace { + +struct TrackerState { + bool copied = false; + bool deleted = false; +}; + +class Tracker { + TrackerState *res = nullptr; + + inline void trackDeletion() { res && (res->deleted = true); } + +public: + Tracker() {} + Tracker(TrackerState &res) : res(&res) {} + + operator bool() const { return !!res; } + + Tracker(const Tracker &other) { *this = other; } + Tracker &operator=(const Tracker &other) { + trackDeletion(); + res = other.res; + res && (res->copied = true); + return *this; + } + Tracker(Tracker &&other) { *this = std::move(other); } + Tracker &operator=(Tracker &&other) { + trackDeletion(); + res = other.res; + other.res = nullptr; + return *this; + } + + ~Tracker() { trackDeletion(); } +}; + +} + +void TestDataStructures::testMap1() +{ + ModifySafeMap map; + TrackerState t0, t1; + + // no-copy put + map.put(1, Tracker(t0)); + UASSERT(!t0.copied); + UASSERT(!t0.deleted); + + // overwrite during iter + bool once = false; + for (auto &it : map.iter()) { + (void)it; + map.put(1, Tracker(t1)); + UASSERT(t0.deleted); + UASSERT(!t1.copied); + UASSERT(!t1.deleted); + if (once |= 1) + break; + } + UASSERT(once); +} + +void TestDataStructures::testMap2() +{ + ModifySafeMap map; + TrackerState t0, t1; + + // overwrite + map.put(1, Tracker(t0)); + map.put(1, Tracker(t1)); + UASSERT(t0.deleted); + UASSERT(!t1.copied); + UASSERT(!t1.deleted); +} + +void TestDataStructures::testMap3() +{ + ModifySafeMap map; + TrackerState t0, t1; + + // take + map.put(1, Tracker(t0)); + { + auto v = map.take(1); + UASSERT(!t0.copied); + UASSERT(!t0.deleted); + } + UASSERT(t0.deleted); + + // remove during iter + map.put(1, Tracker(t1)); + for (auto &it : map.iter()) { + (void)it; + map.remove(1); + UASSERT(t1.deleted); + break; + } +} + +void TestDataStructures::testMap4() +{ + ModifySafeMap map; + + // overwrite + take during iter + map.put(1, 100); + for (auto &it : map.iter()) { + (void)it; + map.put(1, 200); + u32 taken = map.take(1); + UASSERTEQ(u32, taken, 200); + break; + } + + UASSERT(map.get(1) == u32()); + UASSERTEQ(size_t, map.size(), 0); +} + +void TestDataStructures::testMap5() +{ + ModifySafeMap map; + + // overwrite 2x during iter + map.put(9001, 9001); + for (auto &it : map.iter()) { + (void)it; + map.put(1, 100); + map.put(1, 200); + UASSERTEQ(u32, map.get(1), 200); + break; + } +} diff --git a/src/util/container.h b/src/util/container.h index 985a9a447..9a52778d4 100644 --- a/src/util/container.h +++ b/src/util/container.h @@ -376,10 +376,16 @@ public: assert(false); return; } - if (m_iterating) - m_new.emplace(key, value); - else - m_values.emplace(key, value); + if (m_iterating) { + auto it = m_values.find(key); + if (it != m_values.end()) { + it->second = V(); + m_garbage++; + } + m_new[key] = value; + } else { + m_values[key] = value; + } } void put(const K &key, V &&value) { @@ -387,10 +393,16 @@ public: assert(false); return; } - if (m_iterating) - m_new.emplace(key, std::move(value)); - else - m_values.emplace(key, std::move(value)); + if (m_iterating) { + auto it = m_values.find(key); + if (it != m_values.end()) { + it->second = V(); + m_garbage++; + } + m_new[key] = std::move(value); + } else { + m_values[key] = std::move(value); + } } V take(const K &key) { @@ -405,7 +417,8 @@ public: auto it = m_values.find(key); if (it == m_values.end()) return ret; - ret = std::move(it->second); + if (!ret) + ret = std::move(it->second); if (m_iterating) { it->second = V(); m_garbage++; @@ -516,7 +529,8 @@ private: std::map m_values; std::map m_new; unsigned int m_iterating = 0; - size_t m_garbage = 0; // upper bound for null-placeholders in m_values + // approximate amount of null-placeholders in m_values, reliable for != 0 tests + size_t m_garbage = 0; static constexpr size_t GC_MIN_SIZE = 30; };