diff --git a/.github/workflows/dependency-version.yaml b/.github/workflows/dependency-version.yaml index bb2e3d4bb3..342b693ba3 100644 --- a/.github/workflows/dependency-version.yaml +++ b/.github/workflows/dependency-version.yaml @@ -30,13 +30,6 @@ jobs: res=1 fi - current=$(awk 'match($0,/set\(_min_version (.+)\)/,a){print a[1]}' cmake/tpls/DyninstTBB.cmake) - expected=$(awk 'match($0,/tbb:(.+)/,a){print a[1]}' docker/dependencies.versions) - if test "$current" != "$expected"; then - echo "TBB mismatch: Found $current, expected $expected" >/dev/stderr - res=1 - fi - current=$(awk 'match($0,/set\(_min_version (.+)\)/,a){print a[1]}' cmake/tpls/DyninstElfUtils.cmake) expected=$(awk 'match($0,/elfutils:(.+)/,a){print a[1]}' docker/dependencies.versions) if test "$current" != "$expected"; then diff --git a/CMakeLists.txt b/CMakeLists.txt index 5174c479f0..9ba01cd17a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -43,7 +43,6 @@ include(DyninstOptimization) # Locate third-party libraries include(DyninstThreads) -include(DyninstTBB) include(DyninstElfUtils) include(DyninstLibIberty) include(DyninstThread_DB) diff --git a/cmake/DyninstConfig.cmake.in b/cmake/DyninstConfig.cmake.in index 9939e42755..6ffe6c824e 100644 --- a/cmake/DyninstConfig.cmake.in +++ b/cmake/DyninstConfig.cmake.in @@ -4,7 +4,6 @@ list(INSERT CMAKE_MODULE_PATH 0 "${CMAKE_CURRENT_LIST_DIR}/Modules" "${CMAKE_CURRENT_LIST_DIR}/tpls") include(DyninstElfUtils) -include(DyninstTBB) set(CMAKE_MODULE_PATH ${_DYNINST_module_path_save}) unset(_DYNINST_module_path_save) diff --git a/cmake/tpls/DyninstTBB.cmake b/cmake/tpls/DyninstTBB.cmake deleted file mode 100644 index 357ca0f0d7..0000000000 --- a/cmake/tpls/DyninstTBB.cmake +++ /dev/null @@ -1,59 +0,0 @@ -#===================================================== -# -# Configure Intel's Threading Building Blocks -# -# ---------------------------------------- -# -# TBB_ROOT_DIR - Directory hint for TBB installation -# -# The individual find-modules use the _ROOT convention -# as the first location to search for the package. If the user -# specifies TBB_ROOT_DIR, we override the _ROOT -# values and require that each package ignores system directories. -# In effect, this forces the package search to find only -# candidates in _ROOT or CMAKE_PREFIX_PATH. -# -#===================================================== - -include_guard(GLOBAL) - -# Minimum supported version -set(_min_version 2019.9) - -if(TBB_ROOT_DIR) - set(TBB_ROOT ${TBB_ROOT_DIR}) - mark_as_advanced(TBB_ROOT) - set(_find_path_args NO_CMAKE_SYSTEM_PATH NO_SYSTEM_ENVIRONMENT_PATH) -endif() - -# If Dyninst::TBB target already exists (created by rocprofiler-systems build), -# skip find_package since dependencies are being built from source -if(TARGET Dyninst::TBB) - message(STATUS "Using pre-configured Dyninst::TBB target (building from source)") - return() -endif() - -find_package( - TBB ${_min_version} - COMPONENTS tbb tbbmalloc tbbmalloc_proxy - REQUIRED ${_find_path_args}) - -# Don't let TBB variables seep through -mark_as_advanced(TBB_DIR) - -if(NOT TARGET Dyninst::TBB) - add_library(Dyninst::TBB INTERFACE IMPORTED) - target_link_libraries(Dyninst::TBB INTERFACE TBB::tbb TBB::tbbmalloc - TBB::tbbmalloc_proxy) - target_include_directories( - Dyninst::TBB SYSTEM - INTERFACE $ - $ - $) -endif() - -message(STATUS "Found TBB ${TBB_VERSION}") -get_target_property(_tmp TBB::tbb INTERFACE_INCLUDE_DIRECTORIES) -message(STATUS "TBB include directories: ${_tmp}") - -unset(_find_path_args) diff --git a/common/CMakeLists.txt b/common/CMakeLists.txt index 5301d07bec..d3e6059a01 100644 --- a/common/CMakeLists.txt +++ b/common/CMakeLists.txt @@ -165,7 +165,6 @@ dyninst_library( PRIVATE_HEADER_FILES ${_private_headers} SOURCE_FILES ${_sources} DEFINES COMMON_LIB - PUBLIC_DEPS Dyninst::TBB PRIVATE_DEPS Dyninst::LibIberty OpenMP::OpenMP_CXX Dyninst::Valgrind Threads::Threads ) # cmake-format: on diff --git a/common/h/IBSTree-fast.h b/common/h/IBSTree-fast.h index 54723cf847..a8ff7a0684 100644 --- a/common/h/IBSTree-fast.h +++ b/common/h/IBSTree-fast.h @@ -32,6 +32,7 @@ #define IBSTREE_FAST_H #include "IBSTree.h" #include +#include #include #include diff --git a/common/h/concurrent.h b/common/h/concurrent.h index d4a1ace20c..8898fe614c 100644 --- a/common/h/concurrent.h +++ b/common/h/concurrent.h @@ -32,16 +32,22 @@ #define _CONCURRENT_H_ #include "util.h" +#include +#include +#include #include +#include #include +#include +#include +#include +#include #include #include #include #include #include -#include -#include -#include +#include #include namespace Dyninst { @@ -62,146 +68,632 @@ namespace concurrent { return dyncompat::hash{}(k); } }; +} - namespace detail { - template - class hash_compare; - - // New style tbb_hash_compare concept (TBB_VERSION_MAJOR >= 2021) - template - class hash_compare { - hasher my_hasher; - public: - size_t hash(Key const& k) const { - return my_hasher(k); - } - bool equal(Key const& k1, Key const& k2) const { - return k1 == k2; - } +// Thread-safe hash map backed by sharded std::unordered_map instances with +// per-element locking. +// +// Replaces tbb::concurrent_hash_map while preserving the accessor/const_accessor +// interface Dyninst relies on. Keys are partitioned across a fixed number of +// shards; each shard is an independent std::unordered_map guarded by its own +// shared_mutex that protects only the map *structure*. In addition, every stored +// element owns its own shared_mutex, and an accessor holds *that element's* lock +// (exclusive for `accessor`, shared for `const_accessor`) for its lifetime -- +// matching tbb::concurrent_hash_map's per-element locking contract. +// +// Per-element (rather than per-shard) locking is required because several call +// sites -- e.g. Parser::set_edge_parsing_status -- hold multiple accessors into +// the same map instance at once. Per-shard locking self-deadlocks as soon as two +// of those keys hash to the same shard. +// +// Elements are held through shared_ptr so a concurrent erase cannot destroy a +// node (and its mutex) out from under a thread that is acquiring or holding it. +// The shard lock is always released before an *existing* element's lock is taken, +// so the two lock levels cannot form a cycle. The sole exception is a newly +// created node, which is locked while the shard lock is still held but is not yet +// reachable by any other thread, so it can never be contended (see +// emplace_locked). +// +// std::shared_mutex is understood natively by Valgrind's DRD/Helgrind tools, so +// the explicit lock annotations of the old TBB-based wrapper are unnecessary. +// +// Element access via begin()/end() is not internally synchronized: callers +// populate the map during a parallel phase and iterate afterwards, matching the +// original concurrent_hash_map usage. +template +class dyn_c_hash_map { + struct node { + std::pair kv; + mutable dyncompat::shared_mutex mtx; + + template + explicit node(const K& k, Args&&... args) + : kv(std::piecewise_construct, std::forward_as_tuple(k), + std::forward_as_tuple(std::forward(args)...)) {} }; - // Old style tbb_hash_compare concept - template - class hash_compare { - public: - static size_t hash(Key const& k) { - return hasher{}(k); - } - static bool equal(Key const& k1, Key const& k2) { - return k1 == k2; - } + using node_ptr = std::shared_ptr; + using map_type = std::unordered_map>; + + struct shard { + map_type map; + mutable dyncompat::shared_mutex mtx; // guards map structure only }; - } -} -template -class dyn_c_hash_map : protected tbb::concurrent_hash_map= 2021, K>> { + // Shard count trades lock contention against per-map memory. The array below + // is allocated eagerly, so every map instance pays 112 bytes per shard (a 56 + // byte empty unordered_map plus a 56 byte shared_mutex) whether or not it ever + // holds an element -- and Dyninst keeps thousands of these alive at once: + // roughly 5500 while instrumenting a 1 MB binary, so the fixed cost dominates + // the element data on small and medium targets. + // + // An earlier revision raised this to 256 because 64 scaled negatively past 16 + // threads. That was the unmixed hash rather than the shard count: keys are + // dominated by 16-byte-aligned addresses, whose low four bits are constant, so + // `% 64` reached only 4 distinct shards. With mix() applied (see shard_of) + // every shard is reachable, and 64 then measures faster than 256 at every + // thread count from 1 to 128 on a full parse while using ~120 MB less. + static constexpr std::size_t num_shards = 64; + std::unique_ptr shards_{new shard[num_shards]}; + + // Avalanche the hash before selecting a shard. std::hash is the identity for + // pointers and integers, and Dyninst's keys are dominated by heap pointers and + // function entry addresses, which are 16-byte aligned -- so their low bits are + // constant. Feeding those straight into `% num_shards` would leave only every + // 16th shard reachable (4 of 64, 16 of 256) and funnel the whole parallel + // parse through a handful of mutexes. tbb_hash_compare avoided this by + // multiplying the key by a hash multiplier; this is the same idea. + static std::size_t mix(std::size_t h) { + if constexpr(sizeof(std::size_t) == 8) { + h ^= h >> 33; + h *= 0xff51afd7ed558ccdULL; // MurmurHash3 64-bit finalizer + h ^= h >> 33; + } else { + h ^= h >> 16; + h *= 0x85ebca6bUL; + h ^= h >> 13; + } + return h; + } - using base = tbb::concurrent_hash_map= 2021, K>>; + static std::size_t shard_of(const K& k) { + return mix(concurrent::hasher{}(k)) % num_shards; + } + shard& shard_for(const K& k) { return shards_[shard_of(k)]; } + const shard& shard_for(const K& k) const { return shards_[shard_of(k)]; } public: - using typename base::value_type; - using typename base::mapped_type; - using typename base::key_type; + using value_type = std::pair; + using mapped_type = V; + using key_type = K; + + dyn_c_hash_map() = default; + ~dyn_c_hash_map() = default; + + // Copies element values without ever holding a shard lock and an element lock + // at the same time: snapshot the (key, node) pairs under the shard lock, drop + // it, then lock each element in turn. The snapshot holds shared_ptrs, so the + // nodes stay alive even if the source erases them in the meantime. + dyn_c_hash_map(const dyn_c_hash_map& other) { + std::vector> entries; + for(std::size_t i = 0; i < num_shards; ++i) { + entries.clear(); + { + dyncompat::shared_lock lock(other.shards_[i].mtx); + entries.reserve(other.shards_[i].map.size()); + for(const auto& entry : other.shards_[i].map) + entries.emplace_back(entry.first, entry.second); + } + for(const auto& entry : entries) { + dyncompat::shared_lock nlock(entry.second->mtx); + shards_[i].map.emplace( + entry.first, + std::make_shared(entry.first, entry.second->kv.second)); + } + } + } - class const_accessor : public base::const_accessor { + // Deliberately not noexcept: the moved-from map is left with a fresh (empty) + // shard array so it remains usable, and that allocation can throw. + dyn_c_hash_map(dyn_c_hash_map&& other) : shards_(std::move(other.shards_)) { + other.shards_.reset(new shard[num_shards]); + } + + // Keep this map's shard array in place and assign per shard under its own + // lock. Replacing the array wholesale would free it while a concurrent reader + // may still hold a `shard&` obtained from shard_for(), leaving a dangling + // reference -- the shared_ptr nodes do not protect the shard array itself. + dyn_c_hash_map& operator=(const dyn_c_hash_map& other) { + if(this != &other) { + dyn_c_hash_map tmp(other); // snapshot without holding our locks + for(std::size_t i = 0; i < num_shards; ++i) { + dyncompat::unique_lock lock(shards_[i].mtx); + shards_[i].map = std::move(tmp.shards_[i].map); + } + } + return *this; + } + + // Swap rather than reallocate: both objects already own a shard array, so this + // needs no allocation and is genuinely nothrow. + dyn_c_hash_map& operator=(dyn_c_hash_map&& other) noexcept { + shards_.swap(other.shards_); + return *this; + } + + // Holds a shared (read) lock on the target element while alive. + class const_accessor { friend class dyn_c_hash_map; + protected: + node_ptr node_; + dyncompat::shared_lock lock_; + bool valid_ = false; public: - ~const_accessor() { release_ann(); } - void acquire() { dyn_c_annotations::rlock(this->my_node); } - void release() { release_ann(); base::const_accessor::release(); } - private: - void release_ann() { - if(this->my_node) dyn_c_annotations::runlock(this->my_node); + const_accessor() = default; + const_accessor(const const_accessor&) = delete; + const_accessor& operator=(const const_accessor&) = delete; + ~const_accessor() { release(); } + + bool empty() const { return !valid_; } + const value_type* operator->() const { return &node_->kv; } + const value_type& operator*() const { return node_->kv; } + + void release() { + valid_ = false; + if(lock_.owns_lock()) lock_.unlock(); + lock_ = {}; + node_.reset(); } }; - class accessor : public base::accessor { + + // Holds an exclusive (write) lock on the target element while alive. + class accessor { friend class dyn_c_hash_map; + protected: + node_ptr node_; + dyncompat::unique_lock lock_; + bool valid_ = false; public: - ~accessor() { release_ann(); } - void acquire() { dyn_c_annotations::wlock(this->my_node); } - void release() { release_ann(); base::accessor::release(); } - private: - void release_ann() { - if(this->my_node) dyn_c_annotations::wunlock(this->my_node); + accessor() = default; + accessor(const accessor&) = delete; + accessor& operator=(const accessor&) = delete; + ~accessor() { release(); } + + bool empty() const { return !valid_; } + value_type* operator->() const { return &node_->kv; } + value_type& operator*() const { return node_->kv; } + + void release() { + valid_ = false; + if(lock_.owns_lock()) lock_.unlock(); + lock_ = {}; + node_.reset(); } }; +private: + // Look up k under the shard's shared lock and return its node (or null). The + // shard lock is released on return, before the caller takes the element lock. + node_ptr find_node(const K& k) const { + const shard& s = shard_for(k); + dyncompat::shared_lock lock(s.mtx); + auto it = s.map.find(k); + return (it == s.map.end()) ? node_ptr{} : it->second; + } + + // Find-or-create the node for k under the shard's exclusive lock. Returns the + // node and whether it was newly inserted. + // + // When a node is newly created it is locked (into out_lock) *before* the shard + // lock is dropped. The node is not yet reachable by any other thread, so this + // is uncontended (cannot deadlock) and it guarantees that no other thread can + // observe the element before the inserting caller has initialized it -- this + // matches tbb::concurrent_hash_map's atomic insert-and-lock semantics. + // + // Existing nodes are returned unlocked; the caller takes their lock only after + // the shard lock is released, so shard and element locks never nest. + template + std::pair emplace_locked(LockT& out_lock, const K& k, Args&&... args) { + shard& s = shard_for(k); + dyncompat::unique_lock lock(s.mtx); + auto it = s.map.find(k); + if(it != s.map.end()) return {it->second, false}; + auto np = std::make_shared(k, std::forward(args)...); + s.map.emplace(k, np); + // Acquire non-blocking first: the node is unreachable, so this always + // succeeds in practice, and a try_lock is excluded from ThreadSanitizer's + // lock-order graph (it cannot participate in a cycle), which suppresses the + // shard->element inversions this one nesting would otherwise report. + // try_lock may fail spuriously, so fall back to a blocking -- still + // uncontended -- acquisition rather than return an unlocked accessor. + out_lock = LockT(np->mtx, std::try_to_lock); + if(!out_lock.owns_lock()) out_lock.lock(); + return {np, true}; + } + + // True iff k still maps to exactly this node. Confirms that a node obtained + // after the shard lock was released was not erased or replaced before its + // element lock was taken -- restoring the atomic find/insert-and-lock + // guarantee of tbb::concurrent_hash_map. Takes only the shard lock (shared), + // while the caller holds the element lock, so it never holds a shard lock + // while waiting for a contended element lock. + bool still_current(const K& k, const node_ptr& np) const { + const shard& s = shard_for(k); + dyncompat::shared_lock lock(s.mtx); + auto it = s.map.find(k); + return it != s.map.end() && it->second == np; + } + + // Shared implementation of the accessor/const_accessor insert overloads. + // A freshly created node is already locked under the shard lock (no gap). An + // existing node is locked after the shard lock is dropped, then validated + // with still_current(); if it was erased/replaced in between, retry. + template + bool do_insert(Acc& acc, const K& k, Args&&... args) { + acc.release(); + for(;;) { + LockT new_lock; + auto res = emplace_locked(new_lock, k, std::forward(args)...); + if(res.second) { + acc.lock_ = std::move(new_lock); + acc.node_ = std::move(res.first); + acc.valid_ = true; + return true; + } + LockT lk(res.first->mtx); + if(!still_current(k, res.first)) continue; + acc.lock_ = std::move(lk); + acc.node_ = std::move(res.first); + acc.valid_ = true; + return false; + } + } + +public: bool find(const_accessor& ca, const K& k) const { - bool r = base::find(ca, k); - if(r) ca.acquire(); - return r; + ca.release(); + for(;;) { + node_ptr np = find_node(k); + if(!np) return false; + dyncompat::shared_lock lk(np->mtx); + if(!still_current(k, np)) continue; // erased/replaced after lookup; retry + ca.lock_ = std::move(lk); + ca.node_ = std::move(np); + ca.valid_ = true; + return true; + } } + bool find(accessor& a, const K& k) { - bool r = base::find(a, k); - if(r) a.acquire(); - return r; + a.release(); + for(;;) { + node_ptr np = find_node(k); + if(!np) return false; + dyncompat::unique_lock lk(np->mtx); + if(!still_current(k, np)) continue; // erased/replaced after lookup; retry + a.lock_ = std::move(lk); + a.node_ = std::move(np); + a.valid_ = true; + return true; + } } - int contains(const K& k) { return base::count(k) == 1; } + int contains(const K& k) const { return find_node(k) != nullptr; } - bool insert(const_accessor& ca, const K& k) { - bool r = base::insert(ca, k); - if(r) dyn_c_annotations::rwinit(ca.my_node); - ca.acquire(); - return r; - } bool insert(accessor& a, const K& k) { - bool r = base::insert(a, k); - if(r) dyn_c_annotations::rwinit(a.my_node); - a.acquire(); - return r; - } - bool insert(const_accessor& ca, const value_type& e) { - bool r = base::insert(ca, e); - if(r) dyn_c_annotations::rwinit(ca.my_node); - ca.acquire(); - return r; + return do_insert>(a, k); } + bool insert(accessor& a, const value_type& e) { - bool r = base::insert(a, e); - if(r) dyn_c_annotations::rwinit(a.my_node); - a.acquire(); - return r; + return do_insert>( + a, e.first, e.second); + } + + bool insert(const_accessor& ca, const K& k) { + return do_insert>(ca, k); + } + + bool insert(const_accessor& ca, const value_type& e) { + return do_insert>( + ca, e.first, e.second); } - bool insert(const value_type& e) { return base::insert(e); } - bool erase(const_accessor& ca) { - void* n = ca.my_node; - ca.release_ann(); - bool r = base::erase(ca); - if(r) dyn_c_annotations::rwdeinit(n); - return r; + bool insert(const value_type& e) { + shard& s = shard_for(e.first); + dyncompat::unique_lock lock(s.mtx); + auto it = s.map.find(e.first); + if(it != s.map.end()) return false; + s.map.emplace(e.first, std::make_shared(e.first, e.second)); + return true; } + + // Erase the exact element the accessor holds. The accessor already owns the + // element lock, so taking the shard lock here is node -> shard ordering and + // never nests a shard lock while waiting for a contended element lock. bool erase(accessor& a) { - void* n = a.my_node; - a.release_ann(); - bool r = base::erase(a); - if(r) dyn_c_annotations::rwdeinit(n); - return r; + if(!a.valid_) return false; + const K k = a.node_->kv.first; + node_ptr np = a.node_; + shard& s = shard_for(k); + dyncompat::unique_lock slock(s.mtx); + bool removed = false; + auto it = s.map.find(k); + if(it != s.map.end() && it->second == np) { // erase by identity, not by key + s.map.erase(it); + removed = true; + } + a.release(); + return removed; + } + + bool erase(const K& k) { + for(;;) { + node_ptr np = find_node(k); + if(!np) return false; + // Acquire the element lock first, so erase waits for outstanding + // accessors (as tbb::concurrent_hash_map does), then remove under the + // shard lock. node -> shard ordering; no shard lock is held while + // waiting for the element lock. + dyncompat::unique_lock elock(np->mtx); + shard& s = shard_for(k); + dyncompat::unique_lock slock(s.mtx); + auto it = s.map.find(k); + if(it == s.map.end()) return false; + if(it->second != np) continue; // replaced after lookup; retry + s.map.erase(it); + return true; + } } - bool erase(const K& k) { return base::erase(k); } - int size() const { return base::size(); } + int size() const { + std::size_t n = 0; + for(std::size_t i = 0; i < num_shards; ++i) { + dyncompat::shared_lock lock(shards_[i].mtx); + n += shards_[i].map.size(); + } + return static_cast(n); + } - void rehash( int n = 0 ) { base::rehash(n); } + void rehash(int n = 0) { + const std::size_t per = + (n > 0) ? static_cast(n) / num_shards + 1 : 0; + for(std::size_t i = 0; i < num_shards; ++i) { + dyncompat::unique_lock lock(shards_[i].mtx); + shards_[i].map.rehash(per); + } + } - using base::clear; + void clear() { + for(std::size_t i = 0; i < num_shards; ++i) { + dyncompat::unique_lock lock(shards_[i].mtx); + shards_[i].map.clear(); + } + } + // Forward iterator that walks every shard in turn. Not synchronized; use + // only after the concurrent insertion phase has completed. + template + class iter_impl { + friend class dyn_c_hash_map; + + using shard_ptr = std::conditional_t; + using inner = std::conditional_t; + + public: + using iterator_category = std::forward_iterator_tag; + using value_type = std::pair; + using difference_type = std::ptrdiff_t; + using reference = std::conditional_t; + using pointer = std::conditional_t; + + private: + shard_ptr shards_ = nullptr; + std::size_t idx_ = num_shards; + inner cur_{}; + + void advance_to_valid() { + while(idx_ < num_shards && cur_ == shards_[idx_].map.end()) { + if(++idx_ < num_shards) cur_ = shards_[idx_].map.begin(); + } + } + + iter_impl(shard_ptr s, std::size_t idx) : shards_(s), idx_(idx) { + if(idx_ < num_shards) { + cur_ = shards_[idx_].map.begin(); + advance_to_valid(); + } + } + + public: + iter_impl() = default; + + reference operator*() const { return cur_->second->kv; } + pointer operator->() const { return &cur_->second->kv; } + + iter_impl& operator++() { + ++cur_; + advance_to_valid(); + return *this; + } + iter_impl operator++(int) { + iter_impl tmp = *this; + ++(*this); + return tmp; + } + + bool operator==(const iter_impl& o) const { + if(idx_ != o.idx_) return false; + if(idx_ == num_shards) return true; + return cur_ == o.cur_; + } + bool operator!=(const iter_impl& o) const { return !(*this == o); } + }; + + using iterator = iter_impl; + using const_iterator = iter_impl; + + iterator begin() { return iterator(shards_.get(), 0); } + iterator end() { return iterator(shards_.get(), num_shards); } + const_iterator begin() const { return const_iterator(shards_.get(), 0); } + const_iterator end() const { return const_iterator(shards_.get(), num_shards); } +}; + +// Thread-safe, append-during-parallel-phase sequence container backed by +// std::deque. +// +// Replaces tbb::concurrent_vector, preserving the two properties Dyninst relies +// on: (1) push_back/emplace_back may be called concurrently (serialized here by +// an internal mutex), and (2) pointers and references to existing elements stay +// valid as the container grows (std::deque never relocates its elements). +// +// CONCURRENCY CONTRACT: appends (push_back/emplace_back) and indexed reads +// (operator[], at, front, back, size, empty) are synchronized, so a reader that +// addresses elements by index may run alongside an appender, as it could with +// tbb::concurrent_vector. Iteration (begin/end/rbegin/rend) and the non-append +// mutators (clear, insert, erase, resize, ...) are NOT synchronized and must not +// run concurrently with an append, because push_back invalidates every deque +// iterator. Dyninst satisfies that restriction by appending during the parallel +// phase and iterating afterwards. +// +// std::deque is inherited privately so a dyn_c_vector cannot be sliced to, or +// bound as, a std::deque& -- which would silently bypass the append lock. The +// subset of the std::deque API that Dyninst uses is re-exported below. +template +class dyn_c_vector : private std::deque { + using base = std::deque; + mutable dyncompat::mutex _mutex; + +public: + using typename base::value_type; + using typename base::size_type; + using typename base::difference_type; + using typename base::reference; + using typename base::const_reference; + using typename base::pointer; + using typename base::const_pointer; using typename base::iterator; using typename base::const_iterator; + using typename base::reverse_iterator; + using typename base::const_reverse_iterator; + + using base::base; + + dyn_c_vector() = default; + + dyn_c_vector(const dyn_c_vector& other) : base() { + dyncompat::lock_guard lock(other._mutex); + base::operator=(static_cast(other)); + } + + dyn_c_vector(dyn_c_vector&& other) : base() { + dyncompat::lock_guard lock(other._mutex); + base::operator=(std::move(static_cast(other))); + } + + dyn_c_vector& operator=(const dyn_c_vector& other) { + if(this != &other) { + std::scoped_lock locks(_mutex, other._mutex); + base::operator=(static_cast(other)); + } + return *this; + } + + dyn_c_vector& operator=(dyn_c_vector&& other) { + if(this != &other) { + std::scoped_lock locks(_mutex, other._mutex); + base::operator=(std::move(static_cast(other))); + } + return *this; + } + + void push_back(const T& value) { + dyncompat::lock_guard lock(_mutex); + base::push_back(value); + } + + void push_back(T&& value) { + dyncompat::lock_guard lock(_mutex); + base::push_back(std::move(value)); + } + + template + typename base::reference emplace_back(Args&&... args) { + dyncompat::lock_guard lock(_mutex); + return base::emplace_back(std::forward(args)...); + } + + // Synchronized element access. tbb::concurrent_vector let one thread read + // while another appended; std::deque does not, because push_back can + // reallocate the internal map array out from under a reader that is walking + // it to locate an element. Dyninst depends on that guarantee in + // fieldListType::operator==, which compares a type's fields while another + // OpenMP worker may still be adding fields to it (a type is published into + // typesByID before its members are parsed). + // + // Releasing the lock before the caller uses the returned reference is safe: + // std::deque never relocates existing elements, so only the traversal that + // locates the element needs protecting, not the element itself. + reference operator[](size_type n) { + dyncompat::lock_guard lock(_mutex); + return base::operator[](n); + } + const_reference operator[](size_type n) const { + dyncompat::lock_guard lock(_mutex); + return base::operator[](n); + } + reference at(size_type n) { + dyncompat::lock_guard lock(_mutex); + return base::at(n); + } + const_reference at(size_type n) const { + dyncompat::lock_guard lock(_mutex); + return base::at(n); + } + reference front() { + dyncompat::lock_guard lock(_mutex); + return base::front(); + } + const_reference front() const { + dyncompat::lock_guard lock(_mutex); + return base::front(); + } + reference back() { + dyncompat::lock_guard lock(_mutex); + return base::back(); + } + const_reference back() const { + dyncompat::lock_guard lock(_mutex); + return base::back(); + } + size_type size() const { + dyncompat::lock_guard lock(_mutex); + return base::size(); + } + bool empty() const { + dyncompat::lock_guard lock(_mutex); + return base::empty(); + } + + // Unsynchronized iteration and non-append mutation. Per the concurrency + // contract above, these must not run concurrently with an append to the same + // instance: push_back invalidates all deque iterators. using base::begin; using base::end; + using base::cbegin; + using base::cend; + using base::rbegin; + using base::rend; + using base::max_size; + using base::clear; + using base::resize; + using base::assign; + using base::insert; + using base::erase; + using base::pop_back; + using base::swap; }; -template -using dyn_c_vector = tbb::concurrent_vector>; - -template -using dyn_c_queue = tbb::concurrent_queue>; - class dyn_mutex : public dyncompat::mutex { public: using unique_lock = dyncompat::unique_lock; diff --git a/parseAPI/h/Location.h b/parseAPI/h/Location.h index 81b8ea2ce4..a5aaeffd24 100644 --- a/parseAPI/h/Location.h +++ b/parseAPI/h/Location.h @@ -38,6 +38,7 @@ #include "InstructionDecoder.h" #include "Instruction.h" +#include #include #include #include diff --git a/parseAPI/src/IdiomModelDesc.C b/parseAPI/src/IdiomModelDesc.C index 09a5554507..85d508d737 100644 --- a/parseAPI/src/IdiomModelDesc.C +++ b/parseAPI/src/IdiomModelDesc.C @@ -7,6 +7,7 @@ #include "registers/x86_regs.h" #include "registers/x86_64_regs.h" +#include #include #include diff --git a/symtabAPI/src/dwarfWalker.C b/symtabAPI/src/dwarfWalker.C index d7c26e4dde..3c99d01326 100644 --- a/symtabAPI/src/dwarfWalker.C +++ b/symtabAPI/src/dwarfWalker.C @@ -29,6 +29,7 @@ */ #include +#include #include "common/src/vgannotations.h" #include "compiler_diagnostics.h" #include "dwarfWalker.h" diff --git a/symtabAPI/src/emitElf.h b/symtabAPI/src/emitElf.h index ef3f4acc85..292a7e4c26 100644 --- a/symtabAPI/src/emitElf.h +++ b/symtabAPI/src/emitElf.h @@ -34,6 +34,7 @@ #include "Object.h" #include "debug.h" #include "Elf_X.h" +#include #include #include diff --git a/symtabAPI/src/indexed_modules.h b/symtabAPI/src/indexed_modules.h index d960e0553d..d0dc979d62 100644 --- a/symtabAPI/src/indexed_modules.h +++ b/symtabAPI/src/indexed_modules.h @@ -34,7 +34,14 @@ #include "Module.h" #include -#include +#include +#include + +#include +#include +#include +#include +#include namespace Dyninst { namespace SymtabAPI { @@ -56,15 +63,30 @@ namespace Dyninst { namespace SymtabAPI { }; } + // Thread-safe index of Modules keyed by (file name, offset). + // + // Replaces tbb::concurrent_unordered_set: mutations take an exclusive lock + // and lookups take a shared lock. Modules are inserted during object parsing + // and iterated afterwards; the cardinality is small (one entry per module), + // so lock contention is negligible. class indexed_modules { - tbb::concurrent_unordered_set index; + using set_type = std::unordered_set; + set_type index; + mutable dyncompat::shared_mutex mtx; public: - void insert(Module *m) { index.insert(m); } + void insert(Module *m) { + dyncompat::unique_lock l(mtx); + index.insert(m); + } - bool contains(Module *m) const { return index.count(m) != 0UL; } + bool contains(Module *m) const { + dyncompat::shared_lock l(mtx); + return index.count(m) != 0UL; + } std::vector find(std::string const& name) const { + dyncompat::shared_lock l(mtx); std::vector mods; std::copy_if(index.begin(), index.end(), std::back_inserter(mods), [&name](Module *m) { return m->fileName() == name; }); @@ -72,6 +94,7 @@ namespace Dyninst { namespace SymtabAPI { } Module *find(Dyninst::Offset offset) const { + dyncompat::shared_lock l(mtx); for (auto *m : index) { if (m->addr() == offset) return m; @@ -79,15 +102,20 @@ namespace Dyninst { namespace SymtabAPI { return nullptr; } - bool empty() const { return index.empty(); } + bool empty() const { + dyncompat::shared_lock l(mtx); + return index.empty(); + } - decltype(index)::iterator begin() { return index.begin(); } + // NOTE: iteration is not internally locked. Callers iterate after the + // parallel parsing/insertion phase has completed (see class comment). + set_type::iterator begin() { return index.begin(); } - decltype(index)::iterator end() { return index.end(); } + set_type::iterator end() { return index.end(); } - decltype(index)::const_iterator cbegin() const { return index.cbegin(); } + set_type::const_iterator cbegin() const { return index.cbegin(); } - decltype(index)::const_iterator cend() const { return index.cend(); } + set_type::const_iterator cend() const { return index.cend(); } }; }} diff --git a/symtabAPI/src/indexed_symbols.hpp b/symtabAPI/src/indexed_symbols.hpp index 80b0c85540..351e4c70d0 100644 --- a/symtabAPI/src/indexed_symbols.hpp +++ b/symtabAPI/src/indexed_symbols.hpp @@ -3,6 +3,7 @@ #include "Symbol.h" #include "concurrent.h" +#include #include #include #include