From d12cd313d33deb7b2302588e30cf7f33917942e0 Mon Sep 17 00:00:00 2001 From: Jeremy Rifkin <51220084+jeremy-rifkin@users.noreply.github.com> Date: Sun, 23 Jul 2023 19:05:11 -0400 Subject: [PATCH] Cache expensive computations (#18) --- src/full/full_trace_with_libbacktrace.cpp | 5 +- src/platform/program_name.hpp | 18 +-- src/symbols/symbols_with_addr2line.cpp | 134 +++++++++++++++------- src/symbols/symbols_with_libbacktrace.cpp | 5 +- 4 files changed, 105 insertions(+), 57 deletions(-) diff --git a/src/full/full_trace_with_libbacktrace.cpp b/src/full/full_trace_with_libbacktrace.cpp index 508d699..16c6e5f 100644 --- a/src/full/full_trace_with_libbacktrace.cpp +++ b/src/full/full_trace_with_libbacktrace.cpp @@ -53,10 +53,9 @@ namespace cpptrace { fprintf(stderr, "Libbacktrace error: %s, code %d\n", msg, errnum); } - std::mutex state_mutex; - backtrace_state* get_backtrace_state() { - const std::lock_guard lock(state_mutex); + static std::mutex mutex; + const std::lock_guard lock(mutex); // backtrace_create_state must be called only one time per program // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) static backtrace_state* state = nullptr; diff --git a/src/platform/program_name.hpp b/src/platform/program_name.hpp index 18913eb..46145ce 100644 --- a/src/platform/program_name.hpp +++ b/src/platform/program_name.hpp @@ -4,22 +4,14 @@ #include #include -namespace cpptrace { - namespace detail { - inline std::mutex& get_program_name_mutex() { // stupid workaround for an inline variable - static std::mutex mutex; - return mutex; - } - } -} - #if defined(_WIN32) #include namespace cpptrace { namespace detail { inline std::string program_name() { - const std::lock_guard lock(get_program_name_mutex()); + static std::mutex mutex; + const std::lock_guard lock(mutex); static std::string name; static bool did_init = false; static bool valid = false; @@ -46,7 +38,8 @@ namespace cpptrace { namespace cpptrace { namespace detail { inline const char* program_name() { - const std::lock_guard lock(get_program_name_mutex()); + static std::mutex mutex; + const std::lock_guard lock(mutex); static std::string name; static bool did_init = false; static bool valid = false; @@ -73,7 +66,8 @@ namespace cpptrace { namespace cpptrace { namespace detail { inline const char* program_name() { - const std::lock_guard lock(get_program_name_mutex()); + static std::mutex mutex; + const std::lock_guard lock(mutex); static std::string name; static bool did_init = false; static bool valid = false; diff --git a/src/symbols/symbols_with_addr2line.cpp b/src/symbols/symbols_with_addr2line.cpp index 80727af..7eb84a5 100644 --- a/src/symbols/symbols_with_addr2line.cpp +++ b/src/symbols/symbols_with_addr2line.cpp @@ -6,10 +6,11 @@ #include #include +#include +#include #include #include #include -#include #include #if IS_LINUX || IS_APPLE @@ -57,26 +58,34 @@ namespace cpptrace { } bool has_addr2line() { - // Detects if addr2line exists by trying to invoke addr2line --help - constexpr int magic = 42; - // NOLINTNEXTLINE(misc-include-cleaner) - const pid_t pid = fork(); - if(pid == -1) { return false; } - if(pid == 0) { // child - close(STDOUT_FILENO); - close(STDERR_FILENO); // atos --help writes to stderr - // TODO: path - #if !IS_APPLE - execlp("addr2line", "addr2line", "--help", nullptr); - #else - execlp("atos", "atos", "--help", nullptr); - #endif - _exit(magic); + static std::mutex mutex; + static bool has_addr2line = false; + static bool checked = false; + std::lock_guard lock(mutex); + if(!checked) { + checked = true; + // Detects if addr2line exists by trying to invoke addr2line --help + constexpr int magic = 42; + // NOLINTNEXTLINE(misc-include-cleaner) + const pid_t pid = fork(); + if(pid == -1) { return false; } + if(pid == 0) { // child + close(STDOUT_FILENO); + close(STDERR_FILENO); // atos --help writes to stderr + // TODO: path + #if !IS_APPLE + execlp("addr2line", "addr2line", "--help", nullptr); + #else + execlp("atos", "atos", "--help", nullptr); + #endif + _exit(magic); + } + int status; + waitpid(pid, &status, 0); + // NOLINTNEXTLINE(misc-include-cleaner) + has_addr2line = WEXITSTATUS(status) == 0; } - int status; - waitpid(pid, &status, 0); - // NOLINTNEXTLINE(misc-include-cleaner) - return WEXITSTATUS(status) == 0; + return has_addr2line; } struct pipe_t { @@ -140,10 +149,42 @@ namespace cpptrace { // We have to parse the Mach-O to find the offset of the text section..... // I don't know how addresses are handled if there is more than one __TEXT load command. I'm assuming for // now that there is only one, and I'm using only the first section entry within that load command. - return get_text_vmaddr(entry.obj_path.c_str()); + static std::mutex mutex; + std::lock_guard lock(mutex); + static std::unordered_map cache; + auto it = cache.find(entry.obj_path); + if(it == cache.end()) { + // arguably it'd be better to release the lock while computing this, but also arguably it's good to not + // have two threads try to do the same computation + auto base = get_text_vmaddr(entry.obj_path.c_str()); + cache.insert(it, {entry.obj_path, base}); + return base; + } else { + return it->second; + } } #endif #elif IS_WINDOWS + std::string get_module_name(HMODULE handle) { + static std::mutex mutex; + std::lock_guard lock(mutex); + static std::unordered_map cache; + auto it = cache.find(handle); + if(it == cache.end()) { + char path[MAX_PATH]; + if(GetModuleFileNameA(handle, path, sizeof(path))) { + ///fprintf(stderr, "path: %s base: %p\n", path, handle); + cache.insert(it, {handle, path}); + return path; + } else { + fprintf(stderr, "%s\n", std::system_error(GetLastError(), std::system_category()).what()); + cache.insert(it, {handle, ""}); + return ""; + } + } else { + return it->second; + } + } // aladdr queries are needed to get pre-ASLR addresses and targets to run addr2line on std::vector backtrace_frames(const std::vector& addrs) { // reference: https://github.com/bminor/glibc/blob/master/debug/backtracesyms.c @@ -159,16 +200,8 @@ namespace cpptrace { static_cast(addr), &handle )) { - char path[MAX_PATH]; - // TODO: Memoize - if(GetModuleFileNameA(handle, path, sizeof(path))) { - ///fprintf(stderr, "path: %s base: %p\n", path, handle); - frame.obj_path = path; - frame.obj_base = reinterpret_cast(handle); - frame.symbol = ""; - } else { - fprintf(stderr, "%s\n", std::system_error(GetLastError(), std::system_category()).what()); - } + frame.obj_base = reinterpret_cast(handle); + frame.obj_path = get_module_name(handle); } else { fprintf(stderr, "%s\n", std::system_error(GetLastError(), std::system_category()).what()); } @@ -178,10 +211,19 @@ namespace cpptrace { } bool has_addr2line() { - // TODO: Memoize - // TODO: Popen is a hack. Implement properly with CreateProcess and pipes later. - FILE* p = popen("addr2line --version", "r"); - return pclose(p) == 0; + static std::mutex mutex; + static bool has_addr2line = false; + static bool checked = false; + std::lock_guard lock(mutex); + if(!checked) { + // TODO: Popen is a hack. Implement properly with CreateProcess and pipes later. + checked = true; + FILE* p = popen("addr2line --version", "r"); + if(p) { + has_addr2line = pclose(p) == 0; + } + } + return has_addr2line; } std::string resolve_addresses(const std::string& addresses, const std::string& executable) { @@ -200,12 +242,10 @@ namespace cpptrace { return output; } - // TODO: Refactor into backtrace_frames... - // TODO: Memoize - uintptr_t get_module_image_base(const dlframe &entry) { + uintptr_t pe_get_module_image_base(const std::string& obj_path) { // PE header values are little endian bool do_swap = !is_little_endian(); - FILE* file = fopen(entry.obj_path.c_str(), "rb"); + FILE* file = fopen(obj_path.c_str(), "rb"); char magic[2]; internal_verify(fread(magic, 1, 2, file) == 2); // file + 0x0 internal_verify(memcmp(magic, "MZ", 2) == 0); @@ -251,6 +291,22 @@ namespace cpptrace { fclose(file); return image_base; } + + uintptr_t get_module_image_base(const dlframe &entry) { + static std::mutex mutex; + std::lock_guard lock(mutex); + static std::unordered_map cache; + auto it = cache.find(entry.obj_path); + if(it == cache.end()) { + // arguably it'd be better to release the lock while computing this, but also arguably it's good to not + // have two threads try to do the same computation + auto base = pe_get_module_image_base(entry.obj_path); + cache.insert(it, {entry.obj_path, base}); + return base; + } else { + return it->second; + } + } #endif struct symbolizer::impl { diff --git a/src/symbols/symbols_with_libbacktrace.cpp b/src/symbols/symbols_with_libbacktrace.cpp index fc25e51..06895e6 100644 --- a/src/symbols/symbols_with_libbacktrace.cpp +++ b/src/symbols/symbols_with_libbacktrace.cpp @@ -42,10 +42,9 @@ namespace cpptrace { fprintf(stderr, "Libbacktrace error: %s, code %d\n", msg, errnum); } - std::mutex state_mutex; - backtrace_state* get_backtrace_state() { - const std::lock_guard lock(state_mutex); + static std::mutex mutex; + const std::lock_guard lock(mutex); // backtrace_create_state must be called only one time per program // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) static backtrace_state* state = nullptr;