From 6b5a688e0834c3ff94f15e9a202999894715766b Mon Sep 17 00:00:00 2001 From: Michael Date: Fri, 29 Mar 2019 22:14:22 +0000 Subject: [PATCH] Fix Catalog.Iterator test failure --- CMakeLists.txt | 6 ++-- llarp/util/object.hpp | 48 ++++++++++++++++------------ test/util/test_llarp_util_object.cpp | 44 ++++++++++++------------- 3 files changed, 53 insertions(+), 45 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index b5d49891c..542ef9ea2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -30,7 +30,7 @@ endif() if(${CMAKE_SYSTEM_NAME} MATCHES "SunOS") # check if we have the (saner) emulation of epoll here -# it's basically linux epoll but with a sane method of +# it's basically linux epoll but with a sane method of # dealing with closed file handles that still exist in the # epoll set # @@ -161,7 +161,7 @@ find_package(Threads REQUIRED) # not supported on Solaris - system libraries are not available as archives if(STATIC_LINK_RUNTIME) if (NOT SOLARIS) - add_compile_options(-static) + add_compile_options(-static) if (CMAKE_CXX_COMPILER_ID MATCHES "Clang") set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -static -static-libstdc++ -pthread" ) else() @@ -190,7 +190,7 @@ if(NOT DEBIAN) endif(NOT DEBIAN) if(ASAN) - set(DEBUG_FLAGS ${DEBUG_FLAGS} -fsanitize=address -fno-omit-frame-pointer) + set(DEBUG_FLAGS ${DEBUG_FLAGS} -fsanitize=undefined -fno-omit-frame-pointer) set(OPTIMIZE_FLAGS "-O0") endif(ASAN) diff --git a/llarp/util/object.hpp b/llarp/util/object.hpp index 1981da498..bd4d922ca 100644 --- a/llarp/util/object.hpp +++ b/llarp/util/object.hpp @@ -105,22 +105,25 @@ namespace llarp template < typename Value > class Catalog { - static constexpr int32_t INDEX_MASK = 0X007FFFFF; - static constexpr int32_t BUSY_INDICATOR = 0x00800000; - static constexpr int32_t GENERATION_INC = 0x01000000; - static constexpr int32_t GENERATION_MASK = 0XFF000000; + enum + { + INDEX_MASK = 0X007FFFFF, + BUSY_INDICATOR = 0x00800000, + GENERATION_INC = 0x01000000, + GENERATION_MASK = 0XFF000000 + }; struct Node { - typedef union { + union Payload { Buffer< Value > m_buffer; Node* m_next; - } Payload; + }; Payload m_payload; int32_t m_handle; }; - std::vector< Node* > m_nodes; + std::vector< Node* > m_nodes GUARDED_BY(m_mutex); Node* m_next; std::atomic_size_t m_size; @@ -146,11 +149,11 @@ namespace llarp } Node* - findNode(int32_t handle) const + findNode(int32_t handle) const SHARED_LOCKS_REQUIRED(m_mutex) { int32_t index = handle & INDEX_MASK; - if(0 > index || index >= (int32_t)m_nodes.size() + if(0 > index || index >= static_cast< int32_t >(m_nodes.size()) || !(handle & BUSY_INDICATOR)) { return nullptr; @@ -190,11 +193,11 @@ namespace llarp { assert(m_nodes.size() < BUSY_INDICATOR); - node = static_cast< Node* >(operator new(sizeof(Node))); + node = new Node; guard.manageNode(node, true); m_nodes.push_back(node); - node->m_handle = static_cast< int32_t >(m_nodes.size()) - 1; + node->m_handle = static_cast< int32_t >(m_nodes.size() - 1); guard.manageNode(node, false); } @@ -282,7 +285,7 @@ namespace llarp absl::optional< Value > find(int32_t handle) { - util::Lock l(&m_mutex); + absl::ReaderMutexLock l(&m_mutex); Node* node = findNode(handle); if(!node) @@ -305,43 +308,48 @@ namespace llarp }; template < typename Value > - class CatalogIterator + class SCOPED_LOCKABLE CatalogIterator { const Catalog< Value >* m_catalog; size_t m_index; + CatalogIterator(const CatalogIterator&) = delete; + CatalogIterator& + operator=(const CatalogIterator&) = delete; + public: - CatalogIterator(const Catalog< Value >& catalog) - : m_catalog(&catalog), m_index(-1) + explicit CatalogIterator(const Catalog< Value >* catalog) + SHARED_LOCK_FUNCTION(m_catalog->m_mutex) + : m_catalog(catalog), m_index(-1) { m_catalog->m_mutex.ReaderLock(); operator++(); } - ~CatalogIterator() + ~CatalogIterator() UNLOCK_FUNCTION() { m_catalog->m_mutex.ReaderUnlock(); } void - operator++() + operator++() NO_THREAD_SAFETY_ANALYSIS { m_index++; while(m_index < m_catalog->m_nodes.size() && !(m_catalog->m_nodes[m_index]->m_handle & Catalog< Value >::BUSY_INDICATOR)) { - ++m_index; + m_index++; } } - explicit operator bool() const + explicit operator bool() const NO_THREAD_SAFETY_ANALYSIS { return m_index < m_catalog->m_nodes.size(); } std::pair< int32_t, Value > - operator()() const + operator()() const NO_THREAD_SAFETY_ANALYSIS { auto* node = m_catalog->m_nodes[m_index]; return {node->m_handle, *Catalog< Value >::getValue(node)}; diff --git a/test/util/test_llarp_util_object.cpp b/test/util/test_llarp_util_object.cpp index f2557e987..ad188428f 100644 --- a/test/util/test_llarp_util_object.cpp +++ b/test/util/test_llarp_util_object.cpp @@ -28,8 +28,8 @@ TEST(Catalog, smoke) { const double value1 = 1.0; const double value2 = 2.0; - int handle1 = -1; - int handle2 = -1; + int32_t handle1 = -1; + int32_t handle2 = -1; Catalog< double > catalog; @@ -61,11 +61,12 @@ TEST(Catalog, Iterator) { static constexpr size_t THREAD_COUNT = 10; static constexpr size_t ITERATION_COUNT = 1000; + static constexpr int32_t MAX = std::numeric_limits< int32_t >::max(); std::array< std::thread, THREAD_COUNT + 3 > threads; using llarp::util::Barrier; - using Iterator = CatalogIterator< int >; - using Cat = Catalog< int >; + using Iterator = CatalogIterator< int32_t >; + using Cat = Catalog< int32_t >; Barrier barrier(THREAD_COUNT + 3); Cat catalog; @@ -74,22 +75,22 @@ TEST(Catalog, Iterator) for(size_t i = 0; i < THREAD_COUNT; ++i) { threads[i] = std::thread( - [](Barrier *barrier, Cat *catalog, int id) { + [](Barrier *barrier, Cat *catalog, int32_t id) { barrier->Block(); for(size_t i = 0; i < ITERATION_COUNT; ++i) { - int h = catalog->add(id); - absl::optional< int > res = catalog->find(h); + int32_t handle = catalog->add(id); + absl::optional< int32_t > res = catalog->find(handle); ASSERT_TRUE(res); ASSERT_EQ(res.value(), id); - ASSERT_TRUE(catalog->replace(-id - 1, h)); - res = catalog->find(h); + ASSERT_TRUE(catalog->replace(MAX - id, handle)); + res = catalog->find(handle); ASSERT_TRUE(res); - ASSERT_EQ(-id - 1, res.value()); - int removed = -1; - ASSERT_TRUE(catalog->remove(h, &removed)); - ASSERT_EQ(removed, -id - 1); - ASSERT_FALSE(catalog->find(h)); + ASSERT_EQ(MAX - id, res.value()); + int32_t removed = 0; + ASSERT_TRUE(catalog->remove(handle, &removed)); + ASSERT_EQ(removed, MAX - id); + ASSERT_FALSE(catalog->find(handle)); } }, &barrier, &catalog, i); @@ -113,30 +114,29 @@ TEST(Catalog, Iterator) barrier->Block(); for(size_t i = 0; i < ITERATION_COUNT; ++i) { - int arr[100]; + int32_t arr[100]; size_t size = 0; - for(Iterator it(*catalog); it; ++it) + for(Iterator it(catalog); it; ++it) { arr[size++] = it().second; } - for(int i = 0; i < 100; i++) + for(int32_t j = 0; j < 100; j++) { // value must be valid bool present = false; - for(int id = 0; id < static_cast< int >(THREAD_COUNT); id++) + for(int32_t id = 0; id < static_cast< int32_t >(THREAD_COUNT); id++) { - if(id == arr[i] || -id - 1 == arr[i]) + if(id == arr[j] || MAX - id == arr[j]) { present = true; break; } } - ASSERT_TRUE(present); // no duplicate should be there - for(size_t j = i + 1; j < size; j++) + for(size_t k = j + 1; k < size; k++) { - ASSERT_NE(arr[i], arr[j]); + ASSERT_NE(arr[j], arr[k]); } } }