From bbaa7a2f1c6279739f13b818bcc6006ca2c510a6 Mon Sep 17 00:00:00 2001 From: Sajina Kandy Date: Fri, 7 Aug 2026 12:13:44 -0500 Subject: [PATCH 01/11] [tbb-removal] Remove tbbmalloc, concurrent_queue, and concurrent_unordered_set First step of removing the oneTBB dependency from Dyninst (the low-risk items): - DyninstTBB.cmake: drop the tbbmalloc and tbbmalloc_proxy components. These are a link-time global allocator override with no source-level API usage; Dyninst falls back to the system allocator. - concurrent.h: remove the unused dyn_c_queue alias and the include (no consumers anywhere in the tree). - indexed_modules.h: replace tbb::concurrent_unordered_set with a std::unordered_set guarded by a shared_mutex. The module index has small cardinality and is populated during parsing then read afterwards, so mutations take an exclusive lock and lookups take a shared lock. concurrent_hash_map and concurrent_vector are still provided by TBB and are handled in later commits. --- cmake/tpls/DyninstTBB.cmake | 9 +++---- common/h/concurrent.h | 4 --- symtabAPI/src/indexed_modules.h | 46 ++++++++++++++++++++++++++------- 3 files changed, 40 insertions(+), 19 deletions(-) diff --git a/cmake/tpls/DyninstTBB.cmake b/cmake/tpls/DyninstTBB.cmake index 357ca0f0d7..f05deae9e1 100644 --- a/cmake/tpls/DyninstTBB.cmake +++ b/cmake/tpls/DyninstTBB.cmake @@ -35,7 +35,7 @@ endif() find_package( TBB ${_min_version} - COMPONENTS tbb tbbmalloc tbbmalloc_proxy + COMPONENTS tbb REQUIRED ${_find_path_args}) # Don't let TBB variables seep through @@ -43,13 +43,10 @@ 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_link_libraries(Dyninst::TBB INTERFACE TBB::tbb) target_include_directories( Dyninst::TBB SYSTEM - INTERFACE $ - $ - $) + INTERFACE $) endif() message(STATUS "Found TBB ${TBB_VERSION}") diff --git a/common/h/concurrent.h b/common/h/concurrent.h index d4a1ace20c..b43048d7f9 100644 --- a/common/h/concurrent.h +++ b/common/h/concurrent.h @@ -41,7 +41,6 @@ #include #include #include -#include #include namespace Dyninst { @@ -199,9 +198,6 @@ class dyn_c_hash_map : protected tbb::concurrent_hash_map 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/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(); } }; }} From d4f52115d32e5c21ec83fdc61dd0ae4b2c53493c Mon Sep 17 00:00:00 2001 From: Sajina Kandy Date: Fri, 7 Aug 2026 12:21:50 -0500 Subject: [PATCH 02/11] [tbb-removal] Replace concurrent_vector with a std::deque-backed dyn_c_vector Replace tbb::concurrent_vector (the dyn_c_vector alias) with a small std::deque-derived class that preserves the two properties Dyninst relies on: - Concurrent push_back/emplace_back: serialized by an internal mutex. Parser.C appends to a shared dyn_c_vector from inside an OpenMP parallel loop, so the mutation entry points must be thread-safe. - Stable element addresses: std::deque never relocates elements on growth, so pointers/references handed out (e.g. Type::getComponents) stay valid, matching concurrent_vector's guarantee. Element access is inherited from std::deque and left unlocked; callers append during the parallel phase and read afterwards. No concurrent_vector-specific API (grow_by, range, reserve, capacity) is used and push_back's return value is never consumed, so std::deque covers the accessed surface. Also include explicitly (previously pulled in transitively via concurrent_vector.h) so dyn_c_hash_map's TBB_VERSION_MAJOR check still resolves. --- common/h/concurrent.h | 68 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 66 insertions(+), 2 deletions(-) diff --git a/common/h/concurrent.h b/common/h/concurrent.h index b43048d7f9..f75383ae07 100644 --- a/common/h/concurrent.h +++ b/common/h/concurrent.h @@ -32,15 +32,18 @@ #define _CONCURRENT_H_ #include "util.h" +#include #include +#include #include +#include #include #include #include #include #include +#include #include -#include #include namespace Dyninst { @@ -195,8 +198,69 @@ class dyn_c_hash_map : protected tbb::concurrent_hash_map -using dyn_c_vector = tbb::concurrent_vector>; +class dyn_c_vector : public std::deque { + using base = std::deque; + mutable dyncompat::mutex _mutex; + +public: + 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)...); + } +}; class dyn_mutex : public dyncompat::mutex { public: From d486aaa5724445048d0f84e69267cdbf2b106743 Mon Sep 17 00:00:00 2001 From: Sajina Kandy Date: Fri, 7 Aug 2026 13:01:33 -0500 Subject: [PATCH 03/11] [tbb-removal] Replace concurrent_hash_map with a sharded std::unordered_map Final step of removing the oneTBB dependency from Dyninst. Replaces tbb::concurrent_hash_map (the dyn_c_hash_map wrapper) with a sharded std::unordered_map, after which no Dyninst source uses TBB. - concurrent.h: dyn_c_hash_map now partitions keys across a fixed number of shards, each an independent std::unordered_map guarded by its own shared_mutex. The accessor/const_accessor interface is preserved -- an accessor holds its key's shard exclusively, a const_accessor holds it shared -- so the call sites are unchanged. Dyninst never holds two accessors into the same map instance simultaneously, so per-shard locking cannot self-deadlock. std::shared_mutex is understood natively by Valgrind DRD/Helgrind, so the explicit lock annotations are no longer needed. The last includes and the hash_compare shim are removed. - IBSTree-fast.h, emitElf.h, dwarfWalker.C: add / includes that were previously pulled in transitively through the TBB headers. - Build: drop the now-unused TBB dependency -- remove include(DyninstTBB) from CMakeLists.txt and DyninstConfig.cmake.in, drop Dyninst::TBB from common's PUBLIC_DEPS, delete cmake/tpls/DyninstTBB.cmake, and remove the TBB version check from the dependency-version workflow. --- .github/workflows/dependency-version.yaml | 7 - CMakeLists.txt | 1 - cmake/DyninstConfig.cmake.in | 1 - cmake/tpls/DyninstTBB.cmake | 56 ---- common/CMakeLists.txt | 1 - common/h/IBSTree-fast.h | 1 + common/h/concurrent.h | 376 ++++++++++++++++------ symtabAPI/src/dwarfWalker.C | 1 + symtabAPI/src/emitElf.h | 1 + 9 files changed, 281 insertions(+), 164 deletions(-) delete mode 100644 cmake/tpls/DyninstTBB.cmake 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 f05deae9e1..0000000000 --- a/cmake/tpls/DyninstTBB.cmake +++ /dev/null @@ -1,56 +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 - 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) - 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 f75383ae07..13f97efb5e 100644 --- a/common/h/concurrent.h +++ b/common/h/concurrent.h @@ -32,18 +32,20 @@ #define _CONCURRENT_H_ #include "util.h" +#include #include #include #include #include +#include +#include #include #include #include #include #include #include -#include -#include +#include #include namespace Dyninst { @@ -64,138 +66,316 @@ namespace concurrent { return dyncompat::hash{}(k); } }; +} - namespace detail { - template - class hash_compare; +// Thread-safe hash map backed by sharded std::unordered_map instances. +// +// 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. An `accessor` holds its key's shard locked exclusively for the +// accessor's lifetime; a `const_accessor` holds it shared. Dyninst never holds +// two accessors into the same map instance simultaneously, so per-shard locking +// cannot self-deadlock. +// +// 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 { + using map_type = std::unordered_map>; - // 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; - } + struct shard { + map_type map; + mutable dyncompat::shared_mutex mtx; }; - // 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; - } - }; - } -} + static constexpr std::size_t num_shards = 64; + std::unique_ptr shards_{new shard[num_shards]}; -template -class dyn_c_hash_map : protected tbb::concurrent_hash_map= 2021, K>> { - - using base = tbb::concurrent_hash_map= 2021, K>>; + static std::size_t shard_of(const K& k) { + return 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 = typename map_type::value_type; + using mapped_type = typename map_type::mapped_type; + using key_type = typename map_type::key_type; + + dyn_c_hash_map() = default; + ~dyn_c_hash_map() = default; - class const_accessor : public base::const_accessor { + dyn_c_hash_map(const dyn_c_hash_map& other) { + for(std::size_t i = 0; i < num_shards; ++i) { + dyncompat::shared_lock lock(other.shards_[i].mtx); + shards_[i].map = other.shards_[i].map; + } + } + + dyn_c_hash_map(dyn_c_hash_map&& other) noexcept + : shards_(std::move(other.shards_)) { + other.shards_.reset(new shard[num_shards]); + } + + dyn_c_hash_map& operator=(const dyn_c_hash_map& other) { + if(this != &other) { + for(std::size_t i = 0; i < num_shards; ++i) { + std::scoped_lock locks(shards_[i].mtx, other.shards_[i].mtx); + shards_[i].map = other.shards_[i].map; + } + } + return *this; + } + + dyn_c_hash_map& operator=(dyn_c_hash_map&& other) noexcept { + if(this != &other) { + shards_ = std::move(other.shards_); + other.shards_.reset(new shard[num_shards]); + } + return *this; + } + + // Holds a shared (read) lock on the target key's shard while alive. + class const_accessor { friend class dyn_c_hash_map; + protected: + dyncompat::shared_lock rlock_; + dyncompat::unique_lock wlock_; + typename map_type::const_iterator it_{}; + 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 &*it_; } + const value_type& operator*() const { return *it_; } + + void release() { + valid_ = false; + if(rlock_.owns_lock()) rlock_.unlock(); + if(wlock_.owns_lock()) wlock_.unlock(); } }; - class accessor : public base::accessor { + + // Holds an exclusive (write) lock on the target key's shard while alive. + class accessor { friend class dyn_c_hash_map; + protected: + dyncompat::unique_lock wlock_; + typename map_type::iterator it_{}; + 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 &*it_; } + value_type& operator*() const { return *it_; } + + void release() { + valid_ = false; + if(wlock_.owns_lock()) wlock_.unlock(); } }; bool find(const_accessor& ca, const K& k) const { - bool r = base::find(ca, k); - if(r) ca.acquire(); - return r; + ca.release(); + const shard& s = shard_for(k); + dyncompat::shared_lock lock(s.mtx); + auto it = s.map.find(k); + if(it == s.map.end()) return false; + ca.it_ = it; + ca.rlock_ = std::move(lock); + 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(); + shard& s = shard_for(k); + dyncompat::unique_lock lock(s.mtx); + auto it = s.map.find(k); + if(it == s.map.end()) return false; + a.it_ = it; + a.wlock_ = std::move(lock); + a.valid_ = true; + return true; } - int contains(const K& k) { return base::count(k) == 1; } - - 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; + int contains(const K& k) const { + const shard& s = shard_for(k); + dyncompat::shared_lock lock(s.mtx); + return s.map.count(k) == 1; } + 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; + a.release(); + shard& s = shard_for(k); + dyncompat::unique_lock lock(s.mtx); + auto res = s.map.try_emplace(k); + a.it_ = res.first; + a.wlock_ = std::move(lock); + a.valid_ = true; + return res.second; + } + + bool insert(accessor& a, const value_type& e) { + a.release(); + shard& s = shard_for(e.first); + dyncompat::unique_lock lock(s.mtx); + auto res = s.map.insert(e); + a.it_ = res.first; + a.wlock_ = std::move(lock); + a.valid_ = true; + return res.second; } + + bool insert(const_accessor& ca, const K& k) { + ca.release(); + shard& s = shard_for(k); + dyncompat::unique_lock lock(s.mtx); + auto res = s.map.try_emplace(k); + ca.it_ = res.first; + ca.wlock_ = std::move(lock); + ca.valid_ = true; + return res.second; + } + 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; + ca.release(); + shard& s = shard_for(e.first); + dyncompat::unique_lock lock(s.mtx); + auto res = s.map.insert(e); + ca.it_ = res.first; + ca.wlock_ = std::move(lock); + ca.valid_ = true; + return res.second; } - 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; + + bool insert(const value_type& e) { + shard& s = shard_for(e.first); + dyncompat::unique_lock lock(s.mtx); + return s.map.insert(e).second; + } + + bool erase(accessor& a) { + if(!a.valid_) return false; + shard& s = shard_for(a.it_->first); + s.map.erase(a.it_); + a.valid_ = false; + if(a.wlock_.owns_lock()) a.wlock_.unlock(); + return true; } - 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; + if(!ca.valid_) return false; + K key = ca->first; + ca.release(); + return erase(key); } - 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; + + bool erase(const K& k) { + shard& s = shard_for(k); + dyncompat::unique_lock lock(s.mtx); + return s.map.erase(k) != 0; } - 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) { + 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); + } + } + + void clear() { + for(std::size_t i = 0; i < num_shards; ++i) { + dyncompat::unique_lock lock(shards_[i].mtx); + shards_[i].map.clear(); + } + } - void rehash( int n = 0 ) { base::rehash(n); } + // 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; + using ref = std::conditional_t; + using ptr = std::conditional_t; + + 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; + + ref operator*() const { return *cur_; } + ptr operator->() const { return &*cur_; } + + 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 base::clear; + using iterator = iter_impl; + using const_iterator = iter_impl; - using typename base::iterator; - using typename base::const_iterator; - using base::begin; - using base::end; + 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, growable sequence container backed by std::deque. 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 From 6bbc5abd88a66ec56c007fccb1eed1c09fcb7896 Mon Sep 17 00:00:00 2001 From: Sajina Kandy Date: Fri, 7 Aug 2026 13:57:55 -0500 Subject: [PATCH 04/11] [tbb-removal] Address review on dyn_c_hash_map iterators and erase Combines the fixes for the concurrent_hash_map review comments: - Publish the std::iterator_traits member aliases on the sharded dyn_c_hash_map iterators (iterator_category, value_type, difference_type, pointer, reference), deriving iterator_category/difference_type from the wrapped std::unordered_map iterator via std::iterator_traits. Without them the iterators were unusable with std::iterator_traits and standard algorithms -- a regression from the TBB iterator. Also adds the include. - Remove the racy erase(const_accessor&) overload. It released the accessor's shared lock and re-acquired an exclusive lock by key, so the erase was no longer tied to the referenced element (another thread could erase/reinsert the key in the gap and the wrong element would be removed). It had no callers; callers use erase(accessor&) (atomic under the shard's exclusive lock) or erase(const K&). --- common/h/concurrent.h | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/common/h/concurrent.h b/common/h/concurrent.h index 13f97efb5e..82c3b9f5e6 100644 --- a/common/h/concurrent.h +++ b/common/h/concurrent.h @@ -34,6 +34,7 @@ #include "util.h" #include #include +#include #include #include #include @@ -277,13 +278,6 @@ class dyn_c_hash_map { return true; } - bool erase(const_accessor& ca) { - if(!ca.valid_) return false; - K key = ca->first; - ca.release(); - return erase(key); - } - bool erase(const K& k) { shard& s = shard_for(k); dyncompat::unique_lock lock(s.mtx); @@ -324,9 +318,15 @@ class dyn_c_hash_map { using shard_ptr = std::conditional_t; using inner = std::conditional_t; - using ref = std::conditional_t; - using ptr = std::conditional_t; + public: + using iterator_category = typename std::iterator_traits::iterator_category; + using value_type = typename map_type::value_type; + using difference_type = typename std::iterator_traits::difference_type; + using reference = std::conditional_t; + using pointer = std::conditional_t; + + private: shard_ptr shards_ = nullptr; std::size_t idx_ = num_shards; inner cur_{}; @@ -347,8 +347,8 @@ class dyn_c_hash_map { public: iter_impl() = default; - ref operator*() const { return *cur_; } - ptr operator->() const { return &*cur_; } + reference operator*() const { return *cur_; } + pointer operator->() const { return &*cur_; } iter_impl& operator++() { ++cur_; From 35589c2c87a94e5db73bb91a5a3bf6c61eb95584 Mon Sep 17 00:00:00 2001 From: Sajina Kandy Date: Mon, 10 Aug 2026 14:31:55 -0500 Subject: [PATCH 05/11] [tbb-removal] Fix dyn_c_hash_map self-deadlock with per-element locking The sharded dyn_c_hash_map held a shard lock for the accessor's whole lifetime, so a thread holding two accessors into one map (e.g. Parser::set_edge_parsing_status) self-deadlocked whenever two keys shared a shard ("Resource deadlock avoided", or a silent hang for read-then-write). Restore TBB's per-element locking: each element owns a shared_mutex held by the accessor, with the shard lock used only for the structural lookup and always released before the element lock (no nesting). Elements are held via shared_ptr so a concurrent erase cannot destroy a locked node. Newly-created nodes are locked while the shard lock is still held (atomic insert-and-lock), so no thread can observe an element before the inserter initializes it -- without this, a second thread read an uninitialized value and crashed in Symtab::addSymbolToAggregates. Co-authored-by: Cursor --- common/h/concurrent.h | 209 +++++++++++++++++++++++++++--------------- 1 file changed, 133 insertions(+), 76 deletions(-) diff --git a/common/h/concurrent.h b/common/h/concurrent.h index 82c3b9f5e6..bbd3570ca0 100644 --- a/common/h/concurrent.h +++ b/common/h/concurrent.h @@ -38,6 +38,7 @@ #include #include #include +#include #include #include #include @@ -69,15 +70,26 @@ namespace concurrent { }; } -// Thread-safe hash map backed by sharded std::unordered_map instances. +// 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. An `accessor` holds its key's shard locked exclusively for the -// accessor's lifetime; a `const_accessor` holds it shared. Dyninst never holds -// two accessors into the same map instance simultaneously, so per-shard locking -// cannot self-deadlock. +// 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 element lock is taken, so the two +// lock levels cannot form a cycle. // // 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. @@ -87,11 +99,22 @@ namespace concurrent { // original concurrent_hash_map usage. template class dyn_c_hash_map { - using map_type = std::unordered_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)...)) {} + }; + + using node_ptr = std::shared_ptr; + using map_type = std::unordered_map>; struct shard { map_type map; - mutable dyncompat::shared_mutex mtx; + mutable dyncompat::shared_mutex mtx; // guards map structure only }; static constexpr std::size_t num_shards = 64; @@ -104,9 +127,9 @@ class dyn_c_hash_map { const shard& shard_for(const K& k) const { return shards_[shard_of(k)]; } public: - using value_type = typename map_type::value_type; - using mapped_type = typename map_type::mapped_type; - using key_type = typename map_type::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; @@ -114,7 +137,12 @@ class dyn_c_hash_map { dyn_c_hash_map(const dyn_c_hash_map& other) { for(std::size_t i = 0; i < num_shards; ++i) { dyncompat::shared_lock lock(other.shards_[i].mtx); - shards_[i].map = other.shards_[i].map; + for(const auto& entry : other.shards_[i].map) { + dyncompat::shared_lock nlock(entry.second->mtx); + shards_[i].map.emplace( + entry.first, + std::make_shared(entry.first, entry.second->kv.second)); + } } } @@ -125,10 +153,8 @@ class dyn_c_hash_map { dyn_c_hash_map& operator=(const dyn_c_hash_map& other) { if(this != &other) { - for(std::size_t i = 0; i < num_shards; ++i) { - std::scoped_lock locks(shards_[i].mtx, other.shards_[i].mtx); - shards_[i].map = other.shards_[i].map; - } + dyn_c_hash_map tmp(other); + shards_ = std::move(tmp.shards_); } return *this; } @@ -141,13 +167,12 @@ class dyn_c_hash_map { return *this; } - // Holds a shared (read) lock on the target key's shard while alive. + // Holds a shared (read) lock on the target element while alive. class const_accessor { friend class dyn_c_hash_map; protected: - dyncompat::shared_lock rlock_; - dyncompat::unique_lock wlock_; - typename map_type::const_iterator it_{}; + node_ptr node_; + dyncompat::shared_lock lock_; bool valid_ = false; public: const_accessor() = default; @@ -156,22 +181,23 @@ class dyn_c_hash_map { ~const_accessor() { release(); } bool empty() const { return !valid_; } - const value_type* operator->() const { return &*it_; } - const value_type& operator*() const { return *it_; } + const value_type* operator->() const { return &node_->kv; } + const value_type& operator*() const { return node_->kv; } void release() { valid_ = false; - if(rlock_.owns_lock()) rlock_.unlock(); - if(wlock_.owns_lock()) wlock_.unlock(); + if(lock_.owns_lock()) lock_.unlock(); + lock_ = {}; + node_.reset(); } }; - // Holds an exclusive (write) lock on the target key's shard while alive. + // Holds an exclusive (write) lock on the target element while alive. class accessor { friend class dyn_c_hash_map; protected: - dyncompat::unique_lock wlock_; - typename map_type::iterator it_{}; + node_ptr node_; + dyncompat::unique_lock lock_; bool valid_ = false; public: accessor() = default; @@ -180,85 +206,113 @@ class dyn_c_hash_map { ~accessor() { release(); } bool empty() const { return !valid_; } - value_type* operator->() const { return &*it_; } - value_type& operator*() const { return *it_; } + value_type* operator->() const { return &node_->kv; } + value_type& operator*() const { return node_->kv; } void release() { valid_ = false; - if(wlock_.owns_lock()) wlock_.unlock(); + if(lock_.owns_lock()) lock_.unlock(); + lock_ = {}; + node_.reset(); } }; - bool find(const_accessor& ca, const K& k) const { - ca.release(); +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); - if(it == s.map.end()) return false; - ca.it_ = it; - ca.rlock_ = std::move(lock); + 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); + out_lock = LockT(np->mtx); + return {np, true}; + } + +public: + bool find(const_accessor& ca, const K& k) const { + ca.release(); + node_ptr np = find_node(k); + if(!np) return false; + ca.lock_ = dyncompat::shared_lock(np->mtx); + ca.node_ = std::move(np); ca.valid_ = true; return true; } bool find(accessor& a, const K& k) { a.release(); - shard& s = shard_for(k); - dyncompat::unique_lock lock(s.mtx); - auto it = s.map.find(k); - if(it == s.map.end()) return false; - a.it_ = it; - a.wlock_ = std::move(lock); + node_ptr np = find_node(k); + if(!np) return false; + a.lock_ = dyncompat::unique_lock(np->mtx); + a.node_ = std::move(np); a.valid_ = true; return true; } - int contains(const K& k) const { - const shard& s = shard_for(k); - dyncompat::shared_lock lock(s.mtx); - return s.map.count(k) == 1; - } + int contains(const K& k) const { return find_node(k) != nullptr; } bool insert(accessor& a, const K& k) { a.release(); - shard& s = shard_for(k); - dyncompat::unique_lock lock(s.mtx); - auto res = s.map.try_emplace(k); - a.it_ = res.first; - a.wlock_ = std::move(lock); + dyncompat::unique_lock new_lock; + auto res = emplace_locked(new_lock, k); + if(res.second) a.lock_ = std::move(new_lock); + else a.lock_ = dyncompat::unique_lock(res.first->mtx); + a.node_ = std::move(res.first); a.valid_ = true; return res.second; } bool insert(accessor& a, const value_type& e) { a.release(); - shard& s = shard_for(e.first); - dyncompat::unique_lock lock(s.mtx); - auto res = s.map.insert(e); - a.it_ = res.first; - a.wlock_ = std::move(lock); + dyncompat::unique_lock new_lock; + auto res = emplace_locked(new_lock, e.first, e.second); + if(res.second) a.lock_ = std::move(new_lock); + else a.lock_ = dyncompat::unique_lock(res.first->mtx); + a.node_ = std::move(res.first); a.valid_ = true; return res.second; } bool insert(const_accessor& ca, const K& k) { ca.release(); - shard& s = shard_for(k); - dyncompat::unique_lock lock(s.mtx); - auto res = s.map.try_emplace(k); - ca.it_ = res.first; - ca.wlock_ = std::move(lock); + dyncompat::shared_lock new_lock; + auto res = emplace_locked(new_lock, k); + if(res.second) ca.lock_ = std::move(new_lock); + else ca.lock_ = dyncompat::shared_lock(res.first->mtx); + ca.node_ = std::move(res.first); ca.valid_ = true; return res.second; } bool insert(const_accessor& ca, const value_type& e) { ca.release(); - shard& s = shard_for(e.first); - dyncompat::unique_lock lock(s.mtx); - auto res = s.map.insert(e); - ca.it_ = res.first; - ca.wlock_ = std::move(lock); + dyncompat::shared_lock new_lock; + auto res = emplace_locked(new_lock, e.first, e.second); + if(res.second) ca.lock_ = std::move(new_lock); + else ca.lock_ = dyncompat::shared_lock(res.first->mtx); + ca.node_ = std::move(res.first); ca.valid_ = true; return res.second; } @@ -266,16 +320,19 @@ class dyn_c_hash_map { bool insert(const value_type& e) { shard& s = shard_for(e.first); dyncompat::unique_lock lock(s.mtx); - return s.map.insert(e).second; + 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; } bool erase(accessor& a) { if(!a.valid_) return false; - shard& s = shard_for(a.it_->first); - s.map.erase(a.it_); - a.valid_ = false; - if(a.wlock_.owns_lock()) a.wlock_.unlock(); - return true; + K k = a.node_->kv.first; + a.release(); + shard& s = shard_for(k); + dyncompat::unique_lock lock(s.mtx); + return s.map.erase(k) != 0; } bool erase(const K& k) { @@ -320,9 +377,9 @@ class dyn_c_hash_map { typename map_type::iterator>; public: - using iterator_category = typename std::iterator_traits::iterator_category; - using value_type = typename map_type::value_type; - using difference_type = typename std::iterator_traits::difference_type; + 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; @@ -347,8 +404,8 @@ class dyn_c_hash_map { public: iter_impl() = default; - reference operator*() const { return *cur_; } - pointer operator->() const { return &*cur_; } + reference operator*() const { return cur_->second->kv; } + pointer operator->() const { return &cur_->second->kv; } iter_impl& operator++() { ++cur_; From c5af79a812502987420930ac2da3aae8069b5b1f Mon Sep 17 00:00:00 2001 From: Sajina Kandy Date: Mon, 10 Aug 2026 18:15:04 -0500 Subject: [PATCH 06/11] [tbb-removal] Harden dyn_c_hash_map/dyn_c_vector after container review Address residual issues found in a follow-up review of the TBB-container replacements: dyn_c_hash_map (Finding 1/4): restore tbb::concurrent_hash_map's atomic find/insert-and-lock and erase semantics that were lost when the element lock was taken after the shard lock was released. - find/insert now validate-after-lock: after acquiring an existing element's lock (shard lock already dropped), re-check under the shard lock that the key still maps to that same node; if it was erased/replaced, retry. Prevents binding an accessor to a stale node under concurrent erase+reinsert. - erase now acquires the element lock first (so it waits for outstanding accessors, like TBB) before removing under the shard lock; erase(accessor) removes by node identity rather than by key. Newly-created nodes are still locked under the shard lock (no gap), so no validation is needed there. dyn_c_vector (Finding 2/3): inherit std::deque privately and re-export only the used API, so a dyn_c_vector can no longer be sliced to or bound as a std::deque& (which would bypass the append lock) and cannot be deleted through a std::deque*. Document the concurrency contract: only concurrent append is synchronized; element access and non-append mutation must be phase-separated from appends. Verified: full dyninstAPI build; instrument ls/bash/python3 single- and multi-threaded (128 threads); ThreadSanitizer stress test of dyn_c_hash_map (0 data races, no deadlock). Co-authored-by: Cursor --- common/h/concurrent.h | 202 ++++++++++++++++++++++++++++++------------ 1 file changed, 145 insertions(+), 57 deletions(-) diff --git a/common/h/concurrent.h b/common/h/concurrent.h index bbd3570ca0..973c5593e1 100644 --- a/common/h/concurrent.h +++ b/common/h/concurrent.h @@ -250,71 +250,91 @@ class dyn_c_hash_map { 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 { ca.release(); - node_ptr np = find_node(k); - if(!np) return false; - ca.lock_ = dyncompat::shared_lock(np->mtx); - ca.node_ = std::move(np); - ca.valid_ = true; - return true; + 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) { a.release(); - node_ptr np = find_node(k); - if(!np) return false; - a.lock_ = dyncompat::unique_lock(np->mtx); - a.node_ = std::move(np); - a.valid_ = true; - return true; + 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) const { return find_node(k) != nullptr; } bool insert(accessor& a, const K& k) { - a.release(); - dyncompat::unique_lock new_lock; - auto res = emplace_locked(new_lock, k); - if(res.second) a.lock_ = std::move(new_lock); - else a.lock_ = dyncompat::unique_lock(res.first->mtx); - a.node_ = std::move(res.first); - a.valid_ = true; - return res.second; + return do_insert>(a, k); } bool insert(accessor& a, const value_type& e) { - a.release(); - dyncompat::unique_lock new_lock; - auto res = emplace_locked(new_lock, e.first, e.second); - if(res.second) a.lock_ = std::move(new_lock); - else a.lock_ = dyncompat::unique_lock(res.first->mtx); - a.node_ = std::move(res.first); - a.valid_ = true; - return res.second; + return do_insert>( + a, e.first, e.second); } bool insert(const_accessor& ca, const K& k) { - ca.release(); - dyncompat::shared_lock new_lock; - auto res = emplace_locked(new_lock, k); - if(res.second) ca.lock_ = std::move(new_lock); - else ca.lock_ = dyncompat::shared_lock(res.first->mtx); - ca.node_ = std::move(res.first); - ca.valid_ = true; - return res.second; + return do_insert>(ca, k); } bool insert(const_accessor& ca, const value_type& e) { - ca.release(); - dyncompat::shared_lock new_lock; - auto res = emplace_locked(new_lock, e.first, e.second); - if(res.second) ca.lock_ = std::move(new_lock); - else ca.lock_ = dyncompat::shared_lock(res.first->mtx); - ca.node_ = std::move(res.first); - ca.valid_ = true; - return res.second; + return do_insert>( + ca, e.first, e.second); } bool insert(const value_type& e) { @@ -326,19 +346,42 @@ class dyn_c_hash_map { 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) { if(!a.valid_) return false; - K k = a.node_->kv.first; - a.release(); + const K k = a.node_->kv.first; + node_ptr np = a.node_; shard& s = shard_for(k); - dyncompat::unique_lock lock(s.mtx); - return s.map.erase(k) != 0; + 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) { - shard& s = shard_for(k); - dyncompat::unique_lock lock(s.mtx); - return s.map.erase(k) != 0; + 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; + } } int size() const { @@ -435,23 +478,44 @@ class dyn_c_hash_map { const_iterator end() const { return const_iterator(shards_.get(), num_shards); } }; -// Thread-safe, growable sequence container backed by std::deque. +// 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). // -// Element access (operator[], iteration, size, ...) is inherited from std::deque -// and is NOT internally locked: callers append during a parallel phase and read -// afterwards, matching the original concurrent_vector usage. Only the concurrent -// mutation entry points take the lock. +// CONCURRENCY CONTRACT: only concurrent *append* (push_back/emplace_back) is +// synchronized. Element access (operator[], iteration, size, front/back) and the +// non-append mutators (clear, insert, erase, resize, ...) are NOT internally +// synchronized and must not run concurrently with an append to the same +// instance. Dyninst satisfies this by appending during the parallel phase and +// reading/modifying afterwards. NOTE: unlike tbb::concurrent_vector this type +// does not support simultaneous read + append; a segmented design would be +// required for that. +// +// 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 : public std::deque { +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; @@ -497,6 +561,30 @@ class dyn_c_vector : public std::deque { dyncompat::lock_guard lock(_mutex); return base::emplace_back(std::forward(args)...); } + + // Unsynchronized element access, iteration, and non-append mutation. Per the + // concurrency contract above, these must not run concurrently with an append + // to the same instance. + using base::operator[]; + using base::at; + using base::front; + using base::back; + using base::begin; + using base::end; + using base::cbegin; + using base::cend; + using base::rbegin; + using base::rend; + using base::size; + using base::max_size; + using base::empty; + using base::clear; + using base::resize; + using base::assign; + using base::insert; + using base::erase; + using base::pop_back; + using base::swap; }; class dyn_mutex : public dyncompat::mutex { From b68b6418ebb8c35ffa1e07c36739ff2686c5f37a Mon Sep 17 00:00:00 2001 From: Sajina Kandy Date: Tue, 11 Aug 2026 13:31:07 -0500 Subject: [PATCH 07/11] Code fixes for review comments --- common/h/concurrent.h | 63 +++++++++++++++++++++++++++++++++---------- 1 file changed, 49 insertions(+), 14 deletions(-) diff --git a/common/h/concurrent.h b/common/h/concurrent.h index 973c5593e1..8664f94702 100644 --- a/common/h/concurrent.h +++ b/common/h/concurrent.h @@ -88,8 +88,11 @@ namespace concurrent { // // 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 element lock is taken, so the two -// lock levels cannot form a cycle. +// 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. @@ -117,7 +120,14 @@ class dyn_c_hash_map { mutable dyncompat::shared_mutex mtx; // guards map structure only }; - static constexpr std::size_t num_shards = 64; + // Shard count trades lock contention against per-map memory. glibc's + // shared_mutex writes the lock word even for readers, so with too few shards + // those cache lines ping-pong between cores and throughput stops scaling: at + // 64 shards a mixed find/insert benchmark saturates from 16 threads upward. + // 256 keeps scaling out to 64 threads at 28 KB per map; 1024 is faster still + // but costs 112 KB, and Dyninst creates several of these maps per module in + // type-heavy workflows. + static constexpr std::size_t num_shards = 256; std::unique_ptr shards_{new shard[num_shards]}; static std::size_t shard_of(const K& k) { @@ -134,10 +144,21 @@ class dyn_c_hash_map { 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) { - dyncompat::shared_lock lock(other.shards_[i].mtx); - for(const auto& entry : other.shards_[i].map) { + 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, @@ -146,24 +167,31 @@ class dyn_c_hash_map { } } - dyn_c_hash_map(dyn_c_hash_map&& other) noexcept - : shards_(std::move(other.shards_)) { + // 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); - shards_ = std::move(tmp.shards_); + 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 { - if(this != &other) { - shards_ = std::move(other.shards_); - other.shards_.reset(new shard[num_shards]); - } + shards_.swap(other.shards_); return *this; } @@ -246,7 +274,14 @@ class dyn_c_hash_map { if(it != s.map.end()) return {it->second, false}; auto np = std::make_shared(k, std::forward(args)...); s.map.emplace(k, np); - out_lock = LockT(np->mtx); + // 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}; } From 84e2c4c69d0695add292a1f73f51b5f5a4b61958 Mon Sep 17 00:00:00 2001 From: Sajina Kandy Date: Tue, 11 Aug 2026 14:22:23 -0500 Subject: [PATCH 08/11] [tbb-removal] Avalanche the key hash before selecting a shard dyn_c_hash_map picked a shard with `std::hash(k) % num_shards`. In libstdc++ std::hash is the identity for pointers and integers, and Dyninst's keys are dominated by heap pointers (CodeRegion*, Block*, Function*, void*) and function entry addresses, all of which are 16-byte aligned. Their low four bits are therefore always zero, so a power-of-two modulo could only ever reach every 16th shard: 4 of 64, 16 of 256, 64 of 1024. The whole parallel parse was consequently funneling through a handful of shard mutexes, which is what made throughput fall as threads were added rather than rise. oneTBB did not have this problem because tbb_hash_compare multiplies the key by a hash multiplier before use. Run the hash through a MurmurHash3 finalizer before the modulo so every shard is reachable. Measured with a mixed find/insert benchmark on 16-byte-aligned keys (Mops/s at 16/32/64 threads): 256 shards, before: 5.1 / 4.0 / 4.0 1024 shards, before: 17.6 / 17.1 / 18.5 256 shards, after: 29.6 / 32.8 / 50.5 That is roughly 8x at 32 threads, and scaling is positive again. It also means 256 shards now outperforms 1024 unmixed while using a quarter of the memory. Verified: clean build with no new warnings; instrument ls/bash/python3 at 1 and 128 threads with unchanged results; ThreadSanitizer stress test of dyn_c_hash_map reports no data races or lock-order inversions. --- common/h/concurrent.h | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/common/h/concurrent.h b/common/h/concurrent.h index 8664f94702..4cc212ae69 100644 --- a/common/h/concurrent.h +++ b/common/h/concurrent.h @@ -130,8 +130,28 @@ class dyn_c_hash_map { static constexpr std::size_t num_shards = 256; 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; + } + static std::size_t shard_of(const K& k) { - return concurrent::hasher{}(k) % num_shards; + 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)]; } From c4a923c0e94acfb594da774e3b344b1aa97423b0 Mon Sep 17 00:00:00 2001 From: Sajina Kandy Date: Tue, 11 Aug 2026 14:22:44 -0500 Subject: [PATCH 09/11] [tbb-removal] Synchronize dyn_c_vector indexed reads against appends tbb::concurrent_vector allowed one thread to read elements while another appended. The std::deque-backed dyn_c_vector did not: push_back can reallocate the deque's internal map array out from under a reader that is walking it to locate an element, so a concurrent indexed read could follow a freed pointer. Dyninst relies on the old guarantee. fieldListType::operator== reads fieldList.size() and fieldList[i] with no lock, and addOrUpdateType calls it on a type that another OpenMP worker may still be filling in, because parseStructUnionClass publishes the type into typesByID before parsing its members. Take the append mutex in operator[], at, front, back, size and empty. Releasing it before the caller uses the returned reference is safe because std::deque never relocates existing elements: only the traversal that locates the element needs protecting, not the element itself. Iteration (begin/end/rbegin/rend) remains unsynchronized, since push_back invalidates every deque iterator and no caller iterates concurrently with an append. The class-level concurrency contract is updated to state exactly which operations are now safe against a concurrent append. Verified: clean build with no new warnings; instrument ls/bash/python3 at 1 and 128 threads, all succeeding with a stable instrumented-function set. --- common/h/concurrent.h | 80 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 63 insertions(+), 17 deletions(-) diff --git a/common/h/concurrent.h b/common/h/concurrent.h index 4cc212ae69..83ca963590 100644 --- a/common/h/concurrent.h +++ b/common/h/concurrent.h @@ -541,14 +541,14 @@ class dyn_c_hash_map { // 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: only concurrent *append* (push_back/emplace_back) is -// synchronized. Element access (operator[], iteration, size, front/back) and the -// non-append mutators (clear, insert, erase, resize, ...) are NOT internally -// synchronized and must not run concurrently with an append to the same -// instance. Dyninst satisfies this by appending during the parallel phase and -// reading/modifying afterwards. NOTE: unlike tbb::concurrent_vector this type -// does not support simultaneous read + append; a segmented design would be -// required for that. +// 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 @@ -617,22 +617,68 @@ class dyn_c_vector : private std::deque { return base::emplace_back(std::forward(args)...); } - // Unsynchronized element access, iteration, and non-append mutation. Per the - // concurrency contract above, these must not run concurrently with an append - // to the same instance. - using base::operator[]; - using base::at; - using base::front; - using base::back; + // 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::size; using base::max_size; - using base::empty; using base::clear; using base::resize; using base::assign; From 9ae404fb50edea3dd3274a43ef271be38a914f8e Mon Sep 17 00:00:00 2001 From: Sajina Kandy Date: Mon, 17 Aug 2026 15:46:15 -0500 Subject: [PATCH 10/11] [tbb-removal] Reduce dyn_c_hash_map shards from 256 to 64 The shard array is allocated eagerly, so every map instance pays 112 bytes per shard regardless of how many elements it holds. Dyninst keeps thousands of these maps alive during a parse -- roughly 5500 while instrumenting a 1 MB binary -- so at 256 shards the empty arrays alone account for ~158 MB, and they dominate the actual element data on small and medium targets. 256 was chosen because 64 scaled negatively past 16 threads, but 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 and `% 1024` only 64 -- which is why 1024 appeared to restore oneTBB performance. Since 84e2c4c69 avalanches the hash before the modulo, 64 shards yields 64 reachable shards, matching the effective parallelism of the earlier 1024 recommendation at a sixteenth of the memory. Measured on a full parse of libdyninstAPI.so (68 MB, 109/109 modules), 64 shards is both faster and smaller than 256 at every thread count from 1 to 128, with peak speedup unchanged at 1.40x on 8 threads. Peak RSS at 8 threads drops from 617 to 504 MB on a 1 MB target and from 3335 to 3208 MB on the 68 MB target. Function lists are unchanged. --- common/h/concurrent.h | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/common/h/concurrent.h b/common/h/concurrent.h index 83ca963590..8898fe614c 100644 --- a/common/h/concurrent.h +++ b/common/h/concurrent.h @@ -120,14 +120,20 @@ class dyn_c_hash_map { mutable dyncompat::shared_mutex mtx; // guards map structure only }; - // Shard count trades lock contention against per-map memory. glibc's - // shared_mutex writes the lock word even for readers, so with too few shards - // those cache lines ping-pong between cores and throughput stops scaling: at - // 64 shards a mixed find/insert benchmark saturates from 16 threads upward. - // 256 keeps scaling out to 64 threads at 28 KB per map; 1024 is faster still - // but costs 112 KB, and Dyninst creates several of these maps per module in - // type-heavy workflows. - static constexpr std::size_t num_shards = 256; + // 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 From 7d5b0891770dcbc1740efa69bef2573f9d3be162 Mon Sep 17 00:00:00 2001 From: Sajina Kandy Date: Mon, 17 Aug 2026 20:56:23 -0500 Subject: [PATCH 11/11] CI build failure fixes --- parseAPI/h/Location.h | 1 + parseAPI/src/IdiomModelDesc.C | 1 + symtabAPI/src/indexed_symbols.hpp | 1 + 3 files changed, 3 insertions(+) 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/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