From ca8416ea1e2e2b159068bb13df93469b296374bc Mon Sep 17 00:00:00 2001 From: Jeremy Rifkin <51220084+jeremy-rifkin@users.noreply.github.com> Date: Wed, 19 Feb 2025 20:40:58 -0600 Subject: [PATCH] Abstract out an de-duplicate syminit and symcleanup logic for dbghelp --- src/platform/dbghelp_utils.cpp | 118 +++++++++++++++++++++------ src/platform/dbghelp_utils.hpp | 32 ++++++-- src/symbols/symbols_with_dbghelp.cpp | 23 +----- src/unwind/unwind_with_dbghelp.cpp | 23 +----- 4 files changed, 121 insertions(+), 75 deletions(-) diff --git a/src/platform/dbghelp_utils.cpp b/src/platform/dbghelp_utils.cpp index b99c9aa..8f5b8bc 100644 --- a/src/platform/dbghelp_utils.cpp +++ b/src/platform/dbghelp_utils.cpp @@ -10,6 +10,7 @@ #include "utils/error.hpp" #include "utils/microfmt.hpp" +#include "utils/utils.hpp" #include @@ -21,40 +22,105 @@ namespace cpptrace { namespace detail { + dbghelp_syminit_info::dbghelp_syminit_info(void* handle, bool should_sym_cleanup, bool should_close_handle) + : handle(handle), should_sym_cleanup(should_sym_cleanup), should_close_handle(should_close_handle) {} - dbghelp_syminit_manager::~dbghelp_syminit_manager() { - for(auto kvp : cache) { - if(!SymCleanup(kvp.second)) { - ASSERT(false, microfmt::format("Cpptrace SymCleanup failed with code {}\n", GetLastError()).c_str()); + dbghelp_syminit_info::~dbghelp_syminit_info() { + release(); + } + + void dbghelp_syminit_info::release() { + if(!handle) { + return; + } + if(should_sym_cleanup) { + if(!SymCleanup(handle)) { + throw internal_error("SymCleanup failed with code {}\n", GetLastError()); } - if(!CloseHandle(kvp.second)) { - ASSERT(false, microfmt::format("Cpptrace CloseHandle failed with code {}\n", GetLastError()).c_str()); + } + if(should_close_handle) { + if(!CloseHandle(handle)) { + throw internal_error("CloseHandle failed with code {}\n", GetLastError()); } } } - HANDLE dbghelp_syminit_manager::init(HANDLE proc) { - auto itr = cache.find(proc); - - if(itr != cache.end()) { - return itr->second; - } - HANDLE duplicated_handle = nullptr; - if(!DuplicateHandle(proc, proc, proc, &duplicated_handle, 0, FALSE, DUPLICATE_SAME_ACCESS)) { - throw internal_error("DuplicateHandle failed {}", GetLastError()); - } - - if(!SymInitialize(duplicated_handle, NULL, TRUE)) { - throw internal_error("SymInitialize failed {}", GetLastError()); - } - cache[proc] = duplicated_handle; - return duplicated_handle; + dbghelp_syminit_info dbghelp_syminit_info::make_not_owned(void* handle) { + return dbghelp_syminit_info(handle, false, false); } - // Thread-safety: Must only be called from symbols_with_dbghelp while the dbghelp_lock lock is held - dbghelp_syminit_manager& get_syminit_manager() { - static dbghelp_syminit_manager syminit_manager; - return syminit_manager; + dbghelp_syminit_info dbghelp_syminit_info::make_owned(void* handle, bool should_close_handle) { + return dbghelp_syminit_info(handle, true, should_close_handle); + } + + dbghelp_syminit_info::dbghelp_syminit_info(dbghelp_syminit_info&& other) { + handle = exchange(other.handle, nullptr); + should_sym_cleanup = other.should_sym_cleanup; + should_close_handle = other.should_close_handle; + } + + dbghelp_syminit_info& dbghelp_syminit_info::operator=(dbghelp_syminit_info&& other) { + release(); + handle = exchange(other.handle, nullptr); + should_sym_cleanup = other.should_sym_cleanup; + should_close_handle = other.should_close_handle; + return *this; + } + + void* dbghelp_syminit_info::get_process_handle() const { + return handle; + } + + dbghelp_syminit_info dbghelp_syminit_info::make_non_owning_view() const { + return make_not_owned(handle); + } + + std::unordered_map& get_syminit_cache() { + static std::unordered_map syminit_cache; + return syminit_cache; + } + + dbghelp_syminit_info ensure_syminit() { + auto lock = get_dbghelp_lock(); // locking around the entire access of the cache unordered_map + HANDLE proc = GetCurrentProcess(); + if(get_cache_mode() == cache_mode::prioritize_speed) { + auto& syminit_cache = get_syminit_cache(); + auto it = syminit_cache.find(proc); + if(it != syminit_cache.end()) { + return it->second.make_non_owning_view(); + } + } + + auto duplicated_handle = raii_wrap(nullptr, [] (void* handle) { + if(handle) { + if(!CloseHandle(handle)) { + throw internal_error("CloseHandle failed with code {}\n", GetLastError()); + } + } + }); + // https://github.com/jeremy-rifkin/cpptrace/issues/204 + // https://github.com/jeremy-rifkin/cpptrace/pull/206 + // https://learn.microsoft.com/en-us/windows/win32/debug/initializing-the-symbol-handler + // Apparently duplicating the process handle is the idiomatic thing to do and this avoids issues of + // SymInitialize being called twice. + // TODO: Fallback on failure + if(!DuplicateHandle(proc, proc, proc, &duplicated_handle.get(), 0, FALSE, DUPLICATE_SAME_ACCESS)) { + throw internal_error("DuplicateHandle failed{}", GetLastError()); + } + if(!SymInitialize(duplicated_handle.get(), NULL, TRUE)) { + throw internal_error("SymInitialize failed{}", GetLastError()); + } + + auto info = dbghelp_syminit_info::make_owned(exchange(duplicated_handle.get(), nullptr), true); + // either cache and return a view or return the owning wrapper + if(get_cache_mode() == cache_mode::prioritize_speed) { + auto& syminit_cache = get_syminit_cache(); + auto pair = syminit_cache.insert({proc, std::move(info)}); + VERIFY(pair.second); + return pair.first->second.make_non_owning_view(); + } else { + return info; + } } std::recursive_mutex dbghelp_lock; diff --git a/src/platform/dbghelp_utils.hpp b/src/platform/dbghelp_utils.hpp index daa5623..57571aa 100644 --- a/src/platform/dbghelp_utils.hpp +++ b/src/platform/dbghelp_utils.hpp @@ -10,16 +10,34 @@ namespace cpptrace { namespace detail { - struct dbghelp_syminit_manager { - // The set below contains Windows `HANDLE` objects, `void*` is used to avoid - // including the (expensive) Windows header here - std::unordered_map cache; + class dbghelp_syminit_info { + // `void*` is used to avoid including the (expensive) windows.h header here + void* handle = nullptr; + bool should_sym_cleanup; // true if cleanup is not managed by the syminit cache + bool should_close_handle; // true if cleanup is not managed by the syminit cache and the handle was duplicated + dbghelp_syminit_info(void* handle, bool should_sym_cleanup, bool should_close_handle); + public: + ~dbghelp_syminit_info(); + void release(); - ~dbghelp_syminit_manager(); - void* init(void* proc); + static dbghelp_syminit_info make_not_owned(void* handle); + static dbghelp_syminit_info make_owned(void* handle, bool should_close_handle); + + dbghelp_syminit_info(const dbghelp_syminit_info&) = delete; + dbghelp_syminit_info(dbghelp_syminit_info&&); + dbghelp_syminit_info& operator=(const dbghelp_syminit_info&) = delete; + dbghelp_syminit_info& operator=(dbghelp_syminit_info&&); + + void* get_process_handle() const; + dbghelp_syminit_info make_non_owning_view() const; }; - dbghelp_syminit_manager& get_syminit_manager(); + // Ensure SymInitialize is called on the process. This function either + // - Finds that SymInitialize has been called for a handle to the current process already, in which case it returns + // a non-owning dbghelp_syminit_info instance holding the handle + // - Calls SymInitialize a handle to the current process, caches it, and returns a non-owning dbghelp_syminit_info + // - Calls SymInitialize and returns an owning dbghelp_syminit_info which will handle cleanup + dbghelp_syminit_info ensure_syminit(); std::unique_lock get_dbghelp_lock(); } diff --git a/src/symbols/symbols_with_dbghelp.cpp b/src/symbols/symbols_with_dbghelp.cpp index 593ad71..b691db1 100644 --- a/src/symbols/symbols_with_dbghelp.cpp +++ b/src/symbols/symbols_with_dbghelp.cpp @@ -427,21 +427,10 @@ namespace dbghelp { // TODO: When does this need to be called? Can it be moved to the symbolizer? SymSetOptions(SYMOPT_ALLOW_ABSOLUTE_SYMBOLS); - HANDLE duplicated_handle = nullptr; - HANDLE proc = GetCurrentProcess(); - if(get_cache_mode() == cache_mode::prioritize_speed) { - duplicated_handle = get_syminit_manager().init(proc); - } else { - if(!DuplicateHandle(proc, proc, proc, &duplicated_handle, 0, FALSE, DUPLICATE_SAME_ACCESS)) { - throw internal_error("DuplicateHandle failed {}", GetLastError()); - } - if(!SymInitialize(duplicated_handle, NULL, TRUE)) { - throw internal_error("SymInitialize failed {}", GetLastError()); - } - } + auto syminit_info = ensure_syminit(); for(const auto frame : frames) { try { - trace.push_back(resolve_frame(duplicated_handle , frame)); + trace.push_back(resolve_frame(syminit_info.get_process_handle() , frame)); } catch(...) { // NOSONAR if(!detail::should_absorb_trace_exceptions()) { throw; @@ -451,14 +440,6 @@ namespace dbghelp { trace.push_back(entry); } } - if(get_cache_mode() != cache_mode::prioritize_speed) { - if(!SymCleanup(duplicated_handle)) { - throw internal_error("SymCleanup failed"); - } - if(!CloseHandle(duplicated_handle)) { - throw internal_error("CloseHandle failed"); - } - } return trace; } } diff --git a/src/unwind/unwind_with_dbghelp.cpp b/src/unwind/unwind_with_dbghelp.cpp index 6e77b5f..f223c46 100644 --- a/src/unwind/unwind_with_dbghelp.cpp +++ b/src/unwind/unwind_with_dbghelp.cpp @@ -101,24 +101,13 @@ namespace detail { // SymInitialize( GetCurrentProcess(), NULL, TRUE ) has // already been called. // - HANDLE duplicated_handle = nullptr; - HANDLE proc = GetCurrentProcess(); + auto syminit_info = ensure_syminit(); HANDLE thread = GetCurrentThread(); - if(get_cache_mode() == cache_mode::prioritize_speed) { - duplicated_handle = get_syminit_manager().init(proc); - } else { - if(!DuplicateHandle(proc, proc, proc, &duplicated_handle, 0, FALSE, DUPLICATE_SAME_ACCESS)) { - throw internal_error("DuplicateHandle failed{}", GetLastError()); - } - if(!SymInitialize(duplicated_handle, NULL, TRUE)) { - throw internal_error("SymInitialize failed{}", GetLastError()); - } - } while(trace.size() < max_depth) { if( !StackWalk64( machine_type, - duplicated_handle, + syminit_info.get_process_handle(), thread, &frame, machine_type == IMAGE_FILE_MACHINE_I386 ? NULL : &context, @@ -146,14 +135,6 @@ namespace detail { break; } } - if(get_cache_mode() != cache_mode::prioritize_speed) { - if(!SymCleanup(duplicated_handle)) { - throw internal_error("SymCleanup failed"); - } - if(!CloseHandle(duplicated_handle)) { - throw internal_error("CloseHandle failed"); - } - } return trace; }