From f0bedd4104bdaaf9c0b4293d162293367bd6f82c Mon Sep 17 00:00:00 2001 From: Jeremy <51220084+jeremy-rifkin@users.noreply.github.com> Date: Sat, 9 Mar 2024 17:48:09 -0600 Subject: [PATCH] Try error handling with a Result type --- src/binary/elf.hpp | 51 +++++-- src/binary/module_base.hpp | 12 +- src/binary/object.hpp | 39 ++++-- src/symbols/symbols_with_libdwarf.cpp | 4 +- src/utils/common.hpp | 8 ++ src/utils/error.hpp | 4 +- src/utils/utils.hpp | 191 ++++++++++++++++++++++---- 7 files changed, 258 insertions(+), 51 deletions(-) diff --git a/src/binary/elf.hpp b/src/binary/elf.hpp index a6532f1..54a7366 100644 --- a/src/binary/elf.hpp +++ b/src/binary/elf.hpp @@ -25,7 +25,7 @@ namespace detail { } template - static std::uintptr_t elf_get_module_image_base_from_program_table( + static Result elf_get_module_image_base_from_program_table( const std::string& object_path, std::FILE* file, bool is_little_endian @@ -33,33 +33,62 @@ namespace detail { static_assert(Bits == 32 || Bits == 64, "Unexpected Bits argument"); using Header = typename std::conditional::type; using PHeader = typename std::conditional::type; - Header file_header = load_bytes
(file, 0); - VERIFY(file_header.e_ehsize == sizeof(Header), "ELF file header size mismatch" + object_path); + auto loaded_header = load_bytes
(file, 0); + if(loaded_header.is_error()) { + return std::move(loaded_header).unwrap_error(); + } + const Header& file_header = loaded_header.unwrap_value(); + if(file_header.e_ehsize != sizeof(Header)) { + return internal_error("ELF file header size mismatch" + object_path); + } // PT_PHDR will occur at most once // Should be somewhat reliable https://stackoverflow.com/q/61568612/15675011 // It should occur at the beginning but may as well loop just in case for(int i = 0; i < file_header.e_phnum; i++) { - PHeader program_header = load_bytes(file, file_header.e_phoff + file_header.e_phentsize * i); + auto loaded_ph = load_bytes(file, file_header.e_phoff + file_header.e_phentsize * i); + if(loaded_ph.is_error()) { + return std::move(loaded_ph).unwrap_error(); + } + const PHeader& program_header = loaded_ph.unwrap_value(); if(elf_byteswap_if_needed(program_header.p_type, is_little_endian) == PT_PHDR) { return elf_byteswap_if_needed(program_header.p_vaddr, is_little_endian) - elf_byteswap_if_needed(program_header.p_offset, is_little_endian); } } - // Apparently some objects like shared objects can end up missing this file. 0 as a base seems correct. + // Apparently some objects like shared objects can end up missing this header. 0 as a base seems correct. return 0; } - static std::uintptr_t elf_get_module_image_base(const std::string& object_path) { + static Result elf_get_module_image_base(const std::string& object_path) { auto file = raii_wrap(std::fopen(object_path.c_str(), "rb"), file_deleter); if(file == nullptr) { - throw file_error("Unable to read object file " + object_path); + return internal_error("Unable to read object file " + object_path); } // Initial checks/metadata auto magic = load_bytes>(file, 0); - VERIFY(magic == (std::array{0x7F, 'E', 'L', 'F'}), "File is not ELF " + object_path); - bool is_64 = load_bytes(file, 4) == 2; - bool is_little_endian = load_bytes(file, 5) == 1; - VERIFY(load_bytes(file, 6) == 1, "Unexpected ELF endianness " + object_path); + if(magic.is_error()) { + return std::move(magic).unwrap_error(); + } + if(magic.unwrap_value() != (std::array{0x7F, 'E', 'L', 'F'})) { + return internal_error("File is not ELF " + object_path); + } + auto ei_class = load_bytes(file, 4); + if(ei_class.is_error()) { + return std::move(ei_class).unwrap_error(); + } + bool is_64 = ei_class.unwrap_value() == 2; + auto ei_data = load_bytes(file, 5); + if(ei_data.is_error()) { + return std::move(ei_data).unwrap_error(); + } + bool is_little_endian = ei_data.unwrap_value() == 1; + auto ei_version = load_bytes(file, 6); + if(ei_version.is_error()) { + return std::move(ei_version).unwrap_error(); + } + if(ei_version.unwrap_value() != 1) { + return internal_error("Unexpected ELF version " + object_path); + } // get image base if(is_64) { return elf_get_module_image_base_from_program_table<64>(object_path, file, is_little_endian); diff --git a/src/binary/module_base.hpp b/src/binary/module_base.hpp index 15c701b..294e33e 100644 --- a/src/binary/module_base.hpp +++ b/src/binary/module_base.hpp @@ -25,7 +25,7 @@ namespace cpptrace { namespace detail { #if IS_LINUX - inline std::uintptr_t get_module_image_base(const std::string& object_path) { + inline Result get_module_image_base(const std::string& object_path) { static std::mutex mutex; std::lock_guard lock(mutex); static std::unordered_map cache; @@ -34,14 +34,18 @@ namespace detail { // 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 = elf_get_module_image_base(object_path); - cache.insert(it, {object_path, base}); + // TODO: Cache the error + if(base.is_error()) { + return base.unwrap_error(); + } + cache.insert(it, {object_path, base.unwrap_value()}); return base; } else { return it->second; } } #elif IS_APPLE - inline std::uintptr_t get_module_image_base(const std::string& object_path) { + inline Result get_module_image_base(const std::string& object_path) { // 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. @@ -60,7 +64,7 @@ namespace detail { } } #else // Windows - inline std::uintptr_t get_module_image_base(const std::string& object_path) { + inline Result get_module_image_base(const std::string& object_path) { static std::mutex mutex; std::lock_guard lock(mutex); static std::unordered_map cache; diff --git a/src/binary/object.hpp b/src/binary/object.hpp index bbc59f3..3f71875 100644 --- a/src/binary/object.hpp +++ b/src/binary/object.hpp @@ -44,9 +44,14 @@ namespace detail { frame.object_path = buffer; } } - frame.object_address = address - - to_frame_ptr(result.dlfo_link_map->l_addr) - + get_module_image_base(frame.object_path); + auto base = get_module_image_base(frame.object_path); + if(base.has_value()) { + frame.object_address = address + - to_frame_ptr(result.dlfo_link_map->l_addr) + + base.unwrap_value(); + } else { + base.drop_error(); + } } return frame; } @@ -60,9 +65,14 @@ namespace detail { frame.object_address = 0; if(dladdr(reinterpret_cast(address), &info)) { // thread safe frame.object_path = info.dli_fname; - frame.object_address = address - - reinterpret_cast(info.dli_fbase) - + get_module_image_base(info.dli_fname); + auto base = get_module_image_base(info.dli_fname); + if(base.has_value()) { + frame.object_address = address + - reinterpret_cast(info.dli_fbase) + + base.unwrap_value(); + } else { + base.drop_error(); + } } return frame; } @@ -101,9 +111,14 @@ namespace detail { &handle )) { frame.object_path = get_module_name(handle); - frame.object_address = address - - reinterpret_cast(handle) - + get_module_image_base(frame.object_path); + auto base = get_module_image_base(frame.object_path); + if(base.has_value()) { + frame.object_address = address + - reinterpret_cast(handle) + + base.unwrap_value(); + } else { + base.drop_error(); + } } else { std::fprintf(stderr, "%s\n", std::system_error(GetLastError(), std::system_category()).what()); } @@ -121,9 +136,13 @@ namespace detail { } inline object_frame resolve_safe_object_frame(const safe_object_frame& frame) { + auto base = get_module_image_base(frame.object_path); + if(base.is_error()) { + throw base.unwrap_error(); + } return { frame.raw_address, - frame.address_relative_to_object_start + get_module_image_base(frame.object_path), + frame.address_relative_to_object_start + base.unwrap_value(), frame.object_path }; } diff --git a/src/symbols/symbols_with_libdwarf.cpp b/src/symbols/symbols_with_libdwarf.cpp index f768494..4635d56 100644 --- a/src/symbols/symbols_with_libdwarf.cpp +++ b/src/symbols/symbols_with_libdwarf.cpp @@ -901,7 +901,9 @@ namespace libdwarf { retrieve_symbol(vec_it->die, pc, vec_it->dwversion, frame, inlines); } } else { - ASSERT(cu_cache.size() == 0, "Vec should be empty?"); + // I've had this happen for _start, where there is a cached CU for the object but _start is outside + // of the CU's PC range + // ASSERT(cu_cache.size() == 0, "Vec should be empty?"); } } } diff --git a/src/utils/common.hpp b/src/utils/common.hpp index d091afa..c353e86 100644 --- a/src/utils/common.hpp +++ b/src/utils/common.hpp @@ -35,6 +35,14 @@ #error "Unsupported compiler" #endif +#if IS_GCC || IS_CLANG + #define NODISCARD __attribute__((warn_unused_result)) +// #elif IS_MSVC && _MSC_VER >= 1700 +// #define NODISCARD _Check_return_ +#else + #define NODISCARD +#endif + #include #include #include diff --git a/src/utils/error.hpp b/src/utils/error.hpp index d0533ab..5c52e8c 100644 --- a/src/utils/error.hpp +++ b/src/utils/error.hpp @@ -16,10 +16,10 @@ namespace cpptrace { namespace detail { - class file_error : public std::exception { + class internal_error : public std::exception { std::string msg; public: - file_error(std::string path) : msg("Unable to read file " + std::move(path)) {} + internal_error(std::string message) : msg(std::move(message)) {} const char* what() const noexcept override { return msg.c_str(); } diff --git a/src/utils/utils.hpp b/src/utils/utils.hpp index c61db30..696530f 100644 --- a/src/utils/utils.hpp +++ b/src/utils/utils.hpp @@ -191,15 +191,6 @@ namespace detail { static_assert(n_digits(11) == 2, "n_digits utility producing the wrong result"); static_assert(n_digits(1024) == 4, "n_digits utility producing the wrong result"); - // TODO: Re-evaluate use of off_t - template::value, int>::type = 0> - T load_bytes(std::FILE* object_file, off_t offset) { - T object; - VERIFY(std::fseek(object_file, offset, SEEK_SET) == 0, "fseek error"); - VERIFY(std::fread(&object, sizeof(T), 1, object_file) == 1, "fread error"); - return object; - } - struct nullopt_t {}; static constexpr nullopt_t nullopt; @@ -308,45 +299,199 @@ namespace detail { holds_value = false; } - T& unwrap() & { + NODISCARD T& unwrap() & { if(!holds_value) { - throw std::runtime_error{"Optional does not contain a value"}; + ASSERT(false, "Illegal unwrap: Optional does not contain a value"); } return uvalue; } - const T& unwrap() const & { + NODISCARD const T& unwrap() const & { if(!holds_value) { - throw std::runtime_error{"Optional does not contain a value"}; + ASSERT(false, "Illegal unwrap: Optional does not contain a value"); } return uvalue; } - T&& unwrap() && { + NODISCARD T unwrap() && { if(!holds_value) { - throw std::runtime_error{"Optional does not contain a value"}; + ASSERT(false, "Illegal unwrap: Optional does not contain a value"); } return std::move(uvalue); } - const T&& unwrap() const && { - if(!holds_value) { - throw std::runtime_error{"Optional does not contain a value"}; - } - return std::move(uvalue); - } + // NODISCARD const T unwrap() const && { + // if(!holds_value) { + // ASSERT(false, "Illegal unwrap: Optional does not contain a value"); + // } + // return std::move(uvalue); + // } template - T value_or(U&& default_value) const & { + NODISCARD T value_or(U&& default_value) const & { return holds_value ? uvalue : static_cast(std::forward(default_value)); } template - T value_or(U&& default_value) && { + NODISCARD T value_or(U&& default_value) && { return holds_value ? std::move(uvalue) : static_cast(std::forward(default_value)); } }; + template::value, int>::type = 0> + class Result { + // Not using a union because I don't want to have to deal with that + optional value_; + optional error_; + public: + Result(T value) : value_(std::move(value)) {} + Result(E error) : error_(std::move(error)) {} + + bool has_value() const { + ASSERT(value_.has_value() != error_.has_value(), "Illegal state for Result"); + return value_.has_value(); + } + + bool is_error() const { + ASSERT(value_.has_value() != error_.has_value(), "Illegal state for Result"); + return error_.has_value(); + } + + explicit operator bool() const { + return has_value(); + } + + NODISCARD optional value() const & { + return value_; + } + + NODISCARD optional error() const & { + return error_; + } + + NODISCARD optional value() && { + return std::move(value_); + } + + NODISCARD optional error() && { + return std::move(error_); + } + + NODISCARD T& unwrap_value() & { + return value_.unwrap(); + } + + NODISCARD const T& unwrap_value() const & { + return value_.unwrap(); + } + + NODISCARD T unwrap_value() && { + return std::move(value_).unwrap(); + } + + // NODISCARD const T unwrap() const && { + // return std::move(value_).unwrap(); + // } + + NODISCARD E& unwrap_error() & { + return error_.unwrap(); + } + + NODISCARD const E& unwrap_error() const & { + return error_.unwrap(); + } + + NODISCARD E unwrap_error() && { + return std::move(error_).unwrap(); + } + + // NODISCARD const E unwrap_error() const && { + // return std::move(error_).unwrap(); + // } + + + // NODISCARD const T& value() const & { + // ASSERT(has_value()); + // return value_.unwrap(); + // } + + // NODISCARD const E& error() const & { + // ASSERT(is_error()); + // return error_.unwrap(); + // } + + // NODISCARD T value() && { + // ASSERT(has_value()); + // return std::move(value_).unwrap(); + // } + + // NODISCARD E error() && { + // ASSERT(is_error()); + // return std::move(error_).unwrap(); + // } + + // NODISCARD T& unwrap() & { + // return value_.unwrap(); + // } + + // NODISCARD const T& unwrap() const & { + // return value_.unwrap(); + // } + + // NODISCARD T unwrap() && { + // return std::move(value_).unwrap(); + // } + + // NODISCARD const T unwrap() const && { + // return std::move(value_).unwrap(); + // } + + // NODISCARD E& unwrap_error() & { + // return error_.unwrap(); + // } + + // NODISCARD const E& unwrap_error() const & { + // return error_.unwrap(); + // } + + // NODISCARD E unwrap_error() && { + // return std::move(error_).unwrap(); + // } + + // NODISCARD const E unwrap_error() const && { + // return std::move(error_).unwrap(); + // } + + template + NODISCARD T value_or(U&& default_value) const & { + return value_.unwrap_or(std::forward(default_value)); + } + + template + NODISCARD T value_or(U&& default_value) && { + return std::move(value_).unwrap_or(std::forward(default_value)); + } + + void drop_error() const { + if(is_error()) { + std::fprintf(stderr, "%s", unwrap_error().what()); + } + } + }; + + // TODO: Re-evaluate use of off_t + template::value, int>::type = 0> + Result load_bytes(std::FILE* object_file, off_t offset) { + T object; + if(std::fseek(object_file, offset, SEEK_SET) != 0) { + return internal_error("fseek error"); + } + if(std::fread(&object, sizeof(T), 1, object_file) != 1) { + return internal_error("fread error"); + } + return object; + } + // shamelessly stolen from stackoverflow inline bool directory_exists(const std::string& path) { #if IS_WINDOWS