From d1963aa4469647e597925298aa880e7d93ad96f0 Mon Sep 17 00:00:00 2001 From: Simon Farre Date: Thu, 11 Jul 2024 12:08:17 +0200 Subject: [PATCH 1/4] Added dynamic `TargetDescription` TargetDescription dynamically generates the "master" target description contents (target.xml), so that we only have to manage the individual concrete CPU features (like foo-sse.xml, for instance). TargetDescription will generate the xml contents that includes the used features. Removed combinatorial target.xml files These are now handled by `TargetDescription` instead. This unblocks the #3776 PR (AVX512 support) from getting pulled in, though that PR will need to take this PR into account. --- CMakeLists.txt | 7 +- src/GdbServer.cc | 1 + src/GdbServerConnection.cc | 42 +++------ src/GdbServerConnection.h | 5 +- src/TargetDescription.cc | 124 ++++++++++++++++++++++++++ src/TargetDescription.h | 27 ++++++ third-party/gdb/amd64-avx-linux.xml | 19 ---- third-party/gdb/amd64-linux.xml | 19 ---- third-party/gdb/amd64-pkeys-linux.xml | 20 ----- third-party/gdb/i386-avx-linux.xml | 18 ---- third-party/gdb/i386-linux.xml | 18 ---- third-party/gdb/i386-pkeys-linux.xml | 19 ---- 12 files changed, 170 insertions(+), 149 deletions(-) create mode 100644 src/TargetDescription.cc create mode 100644 src/TargetDescription.h delete mode 100644 third-party/gdb/amd64-avx-linux.xml delete mode 100644 third-party/gdb/amd64-linux.xml delete mode 100644 third-party/gdb/amd64-pkeys-linux.xml delete mode 100644 third-party/gdb/i386-avx-linux.xml delete mode 100644 third-party/gdb/i386-linux.xml delete mode 100644 third-party/gdb/i386-pkeys-linux.xml diff --git a/CMakeLists.txt b/CMakeLists.txt index 1c3a4780745..124fbf69c29 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -648,6 +648,7 @@ set(RR_SOURCES src/SourcesCommand.cc src/StdioMonitor.cc src/SysCpuMonitor.cc + src/TargetDescription.cc src/Task.cc src/ThreadGroup.cc src/TraceeAttentionSet.cc @@ -797,12 +798,6 @@ set(RR_GDB_RESOURCES 64bit-seg.xml 64bit-sse.xml 64bit-pkeys.xml - amd64-pkeys-linux.xml - amd64-avx-linux.xml - amd64-linux.xml - i386-pkeys-linux.xml - i386-avx-linux.xml - i386-linux.xml aarch64-core.xml aarch64-fpu.xml aarch64-pauth.xml diff --git a/src/GdbServer.cc b/src/GdbServer.cc index 18bbe46cdb8..86ff30e347e 100644 --- a/src/GdbServer.cc +++ b/src/GdbServer.cc @@ -206,6 +206,7 @@ void GdbServer::dispatch_regs_request(const Registers& regs, return; } vector rs; + rs.reserve(end); for (GdbServerRegister r = GdbServerRegister(0); r <= end; r = GdbServerRegister(r + 1)) { rs.push_back(get_reg(regs, extra_regs, r)); } diff --git a/src/GdbServerConnection.cc b/src/GdbServerConnection.cc index d551441f76c..ebda908395e 100644 --- a/src/GdbServerConnection.cc +++ b/src/GdbServerConnection.cc @@ -30,6 +30,7 @@ #include "DebuggerExtensionCommandHandler.h" #include "ReplaySession.h" #include "ScopedFd.h" +#include "TargetDescription.h" #include "core.h" #include "log.h" @@ -111,7 +112,8 @@ unique_ptr GdbServerConnection::await_connection( Task* t, ScopedFd& listen_fd, const GdbServerConnection::Features& features) { auto dbg = unique_ptr( new GdbServerConnection(t->thread_group()->tguid(), features)); - dbg->set_cpu_features(get_cpu_features(t->arch())); + const auto arch = t->arch(); + dbg->set_cpu_features(arch, get_cpu_features(arch)); dbg->await_debugger(listen_fd); return dbg; } @@ -503,29 +505,6 @@ static string read_target_desc(const char* file_name) { return ss.str(); } -static const char* target_description_name(uint32_t cpu_features) { - // This doesn't scale, but it's what gdb does... - switch (cpu_features) { - case 0: - return "i386-linux.xml"; - case GdbServerConnection::CPU_X86_64: - return "amd64-linux.xml"; - case GdbServerConnection::CPU_AVX: - return "i386-avx-linux.xml"; - case GdbServerConnection::CPU_X86_64 | GdbServerConnection::CPU_AVX: - return "amd64-avx-linux.xml"; - case GdbServerConnection::CPU_PKU | GdbServerConnection::CPU_AVX: - return "i386-pkeys-linux.xml"; - case GdbServerConnection::CPU_X86_64 | GdbServerConnection::CPU_PKU | GdbServerConnection::CPU_AVX: - return "amd64-pkeys-linux.xml"; - case GdbServerConnection::CPU_AARCH64: - return "aarch64-core.xml"; - default: - FATAL() << "Unknown features"; - return nullptr; - } -} - bool GdbServerConnection::xfer(const char* name, char* args) { const char* mode = args; args = strchr(args, ':'); @@ -609,11 +588,8 @@ bool GdbServerConnection::xfer(const char* name, char* args) { return false; } - string target_desc = - read_target_desc((strcmp(annex, "") && strcmp(annex, "target.xml")) - ? annex - : target_description_name(cpu_features_)); - write_xfer_response(target_desc.c_str(), target_desc.size(), offset, len); + const auto desc = (strcmp(annex, "") && strcmp(annex, "target.xml") ? read_target_desc(annex) : target_decription->to_xml()); + write_xfer_response(desc.c_str(), desc.size(), offset, len); return false; } @@ -2335,4 +2311,12 @@ bool GdbServerConnection::is_connection_alive() { return connection_alive_; } bool GdbServerConnection::is_pass_signal(int sig) { return pass_signals.find(to_gdb_signum(sig)) != pass_signals.end(); } +void GdbServerConnection::set_cpu_features(SupportedArch arch, + uint32_t features) { + cpu_features_ = features; + DEBUG_ASSERT(target_decription == nullptr && + "Target description already created"); + target_decription = std::make_unique(arch, cpu_features_); +} + } // namespace rr diff --git a/src/GdbServerConnection.h b/src/GdbServerConnection.h index a463eac6693..c9a7273052c 100644 --- a/src/GdbServerConnection.h +++ b/src/GdbServerConnection.h @@ -17,6 +17,7 @@ #include "ReplaySession.h" #include "ReplayTimeline.h" #include "ScopedFd.h" +#include "TargetDescription.h" #include "TaskishUid.h" #include "core.h" @@ -750,7 +751,8 @@ class GdbServerConnection { CPU_AARCH64 = 0x4, CPU_PKU = 0x8 }; - void set_cpu_features(uint32_t features) { cpu_features_ = features; } + + void set_cpu_features(SupportedArch arch, uint32_t features); uint32_t cpu_features() const { return cpu_features_; } GdbServerConnection(ThreadGroupUid tguid, const Features& features); @@ -877,6 +879,7 @@ class GdbServerConnection { bool hwbreak_supported_; // client supports hwbreak extension bool swbreak_supported_; // client supports swbreak extension bool list_threads_in_stop_reply_; // client requested threads: and thread-pcs: in stop replies + std::unique_ptr target_decription{nullptr}; }; } // namespace rr diff --git a/src/TargetDescription.cc b/src/TargetDescription.cc new file mode 100644 index 00000000000..ca6e0f58f03 --- /dev/null +++ b/src/TargetDescription.cc @@ -0,0 +1,124 @@ +#include "TargetDescription.h" +#include "GdbServerConnection.h" +#include "kernel_abi.h" +#include +namespace rr { + +class FeatureStream { + std::stringstream stream{}; + const char* arch_prefix{nullptr}; + +public: + std::string result() noexcept { return stream.str(); } + + template + friend FeatureStream& operator<<(FeatureStream& stream, Any any) noexcept; +}; + +template +FeatureStream& operator<<(FeatureStream& stream, Any any) noexcept { + stream.stream << any; + return stream; +} + +template <> +FeatureStream& operator<<(FeatureStream& stream, + rr::SupportedArch arch) noexcept { + stream << ""; + switch (arch) { + case rr::x86: + stream << "i386"; + stream.arch_prefix = "32bit-"; + break; + case rr::x86_64: + stream << "i386:x86-64"; + stream.arch_prefix = "64bit-"; + break; + case rr::aarch64: + stream << "aarch64"; + stream.arch_prefix = "aarch64-"; + break; + } + stream << "\n"; + return stream; +} + +template <> +FeatureStream& operator<<(FeatureStream& stream, + TargetFeature feature) noexcept { + DEBUG_ASSERT(stream.arch_prefix != nullptr && + "No architecture has been provided to description"); + stream << R"( )" << '\n'; + return stream; +} + +TargetDescription::TargetDescription(rr::SupportedArch arch, + u32 cpu_features) noexcept + : arch(arch), target_features() { + + // default-assumed registers per arch + switch (arch) { + case rr::x86: + target_features.push_back(TargetFeature::Core); + target_features.push_back(TargetFeature::SSE); + target_features.push_back(TargetFeature::Linux); + break; + case rr::x86_64: + target_features.push_back(TargetFeature::Core); + target_features.push_back(TargetFeature::SSE); + target_features.push_back(TargetFeature::Linux); + target_features.push_back(TargetFeature::Segment); + break; + case rr::aarch64: + target_features.push_back(TargetFeature::Core); + break; + } + + if (cpu_features & rr::GdbServerConnection::CPU_AVX) { + DEBUG_ASSERT((arch == rr::x86 || arch == rr::x86_64) && "unexpected arch"); + target_features.push_back(TargetFeature::AVX); + } + + if (cpu_features & rr::GdbServerConnection::CPU_PKU) { + DEBUG_ASSERT((arch == rr::x86 || arch == rr::x86_64) && "unexpected arch"); + target_features.push_back(TargetFeature::PKeys); + } +} + +static constexpr auto Header = R"( + + +)"; + +std::string TargetDescription::to_xml() const noexcept { + FeatureStream fs{}; + fs << Header << arch << "GNU/Linux\n"; + for (const auto feature : target_features) { + fs << feature; + } + fs << ""; + + return fs.result(); +} +} // namespace rr \ No newline at end of file diff --git a/src/TargetDescription.h b/src/TargetDescription.h new file mode 100644 index 00000000000..18c2d753b27 --- /dev/null +++ b/src/TargetDescription.h @@ -0,0 +1,27 @@ +#pragma once +#include "kernel_abi.h" +#include + +namespace rr { + +struct GdbServerRegisterValue; + +using u32 = std::uint32_t; + +enum class TargetFeature : u32 { + Core = 0, + SSE, + Linux, + Segment, + AVX, + PKeys +}; + +class TargetDescription { + SupportedArch arch; + std::vector target_features; +public: + explicit TargetDescription(rr::SupportedArch arch, u32 cpu_features) noexcept; + std::string to_xml() const noexcept; +}; +} \ No newline at end of file diff --git a/third-party/gdb/amd64-avx-linux.xml b/third-party/gdb/amd64-avx-linux.xml deleted file mode 100644 index d2dc3bc18f7..00000000000 --- a/third-party/gdb/amd64-avx-linux.xml +++ /dev/null @@ -1,19 +0,0 @@ - - - - - - - - i386:x86-64 - GNU/Linux - - - - - - diff --git a/third-party/gdb/amd64-linux.xml b/third-party/gdb/amd64-linux.xml deleted file mode 100644 index aad02a3ac6f..00000000000 --- a/third-party/gdb/amd64-linux.xml +++ /dev/null @@ -1,19 +0,0 @@ - - - - - - - - i386:x86-64 - GNU/Linux - - - - - - diff --git a/third-party/gdb/amd64-pkeys-linux.xml b/third-party/gdb/amd64-pkeys-linux.xml deleted file mode 100644 index 1fa5bde1161..00000000000 --- a/third-party/gdb/amd64-pkeys-linux.xml +++ /dev/null @@ -1,20 +0,0 @@ - - - - - - - - i386:x86-64 - GNU/Linux - - - - - - - diff --git a/third-party/gdb/i386-avx-linux.xml b/third-party/gdb/i386-avx-linux.xml deleted file mode 100644 index c957fabadd7..00000000000 --- a/third-party/gdb/i386-avx-linux.xml +++ /dev/null @@ -1,18 +0,0 @@ - - - - - - - - i386 - GNU/Linux - - - - - diff --git a/third-party/gdb/i386-linux.xml b/third-party/gdb/i386-linux.xml deleted file mode 100644 index 625984e6d83..00000000000 --- a/third-party/gdb/i386-linux.xml +++ /dev/null @@ -1,18 +0,0 @@ - - - - - - - - i386 - GNU/Linux - - - - - diff --git a/third-party/gdb/i386-pkeys-linux.xml b/third-party/gdb/i386-pkeys-linux.xml deleted file mode 100644 index 47f7b2f0093..00000000000 --- a/third-party/gdb/i386-pkeys-linux.xml +++ /dev/null @@ -1,19 +0,0 @@ - - - - - - - - i386 - GNU/Linux - - - - - - From d18f874ac44ca3f38ff9c17a7e1d4fb28bab3a11 Mon Sep 17 00:00:00 2001 From: Simon Farre Date: Thu, 11 Jul 2024 19:38:39 +0200 Subject: [PATCH 2/4] Actually provide aarch64-fpu.xml to gdb I don't even know how we have sent this before or why this hasn't been a bug prior to this. --- src/TargetDescription.cc | 4 ++++ src/TargetDescription.h | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/TargetDescription.cc b/src/TargetDescription.cc index ca6e0f58f03..469a006b13d 100644 --- a/src/TargetDescription.cc +++ b/src/TargetDescription.cc @@ -68,6 +68,9 @@ FeatureStream& operator<<(FeatureStream& stream, case TargetFeature::Segment: stream << "seg.xml"; break; + case TargetFeature::FPU: + stream << "fpu.xml"; + break; } stream << R"("/>)" << '\n'; return stream; @@ -92,6 +95,7 @@ TargetDescription::TargetDescription(rr::SupportedArch arch, break; case rr::aarch64: target_features.push_back(TargetFeature::Core); + target_features.push_back(TargetFeature::FPU); break; } diff --git a/src/TargetDescription.h b/src/TargetDescription.h index 18c2d753b27..78c148e810d 100644 --- a/src/TargetDescription.h +++ b/src/TargetDescription.h @@ -14,7 +14,8 @@ enum class TargetFeature : u32 { Linux, Segment, AVX, - PKeys + PKeys, + FPU, }; class TargetDescription { From 25c7506570d37a8da25db202575c5ad6b35949af Mon Sep 17 00:00:00 2001 From: Simon Farre Date: Mon, 15 Jul 2024 19:33:37 +0200 Subject: [PATCH 3/4] Fix formatting issues --- src/GdbServerConnection.cc | 17 ++++++++--------- src/GdbServerConnection.h | 4 ++-- src/TargetDescription.cc | 30 +++++++++++++++--------------- src/TargetDescription.h | 29 ++++++++++++++--------------- 4 files changed, 39 insertions(+), 41 deletions(-) diff --git a/src/GdbServerConnection.cc b/src/GdbServerConnection.cc index ebda908395e..eba06202da7 100644 --- a/src/GdbServerConnection.cc +++ b/src/GdbServerConnection.cc @@ -68,7 +68,8 @@ GdbServerConnection::GdbServerConnection(ThreadGroupUid tguid, const Features& f multiprocess_supported_(false), hwbreak_supported_(false), swbreak_supported_(false), - list_threads_in_stop_reply_(false) { + list_threads_in_stop_reply_(false), + target_description(nullptr) { #ifndef REVERSE_EXECUTION features_.reverse_execution = false; #endif @@ -112,8 +113,7 @@ unique_ptr GdbServerConnection::await_connection( Task* t, ScopedFd& listen_fd, const GdbServerConnection::Features& features) { auto dbg = unique_ptr( new GdbServerConnection(t->thread_group()->tguid(), features)); - const auto arch = t->arch(); - dbg->set_cpu_features(arch, get_cpu_features(arch)); + dbg->set_cpu_features(t->arch()); dbg->await_debugger(listen_fd); return dbg; } @@ -588,7 +588,7 @@ bool GdbServerConnection::xfer(const char* name, char* args) { return false; } - const auto desc = (strcmp(annex, "") && strcmp(annex, "target.xml") ? read_target_desc(annex) : target_decription->to_xml()); + const auto desc = strcmp(annex, "") && strcmp(annex, "target.xml") ? read_target_desc(annex) : target_description->to_xml(); write_xfer_response(desc.c_str(), desc.size(), offset, len); return false; } @@ -2311,12 +2311,11 @@ bool GdbServerConnection::is_connection_alive() { return connection_alive_; } bool GdbServerConnection::is_pass_signal(int sig) { return pass_signals.find(to_gdb_signum(sig)) != pass_signals.end(); } -void GdbServerConnection::set_cpu_features(SupportedArch arch, - uint32_t features) { - cpu_features_ = features; - DEBUG_ASSERT(target_decription == nullptr && +void GdbServerConnection::set_cpu_features(SupportedArch arch) { + cpu_features_ = get_cpu_features(arch); + DEBUG_ASSERT(target_description == nullptr && "Target description already created"); - target_decription = std::make_unique(arch, cpu_features_); + target_description = std::make_unique(arch, cpu_features_); } } // namespace rr diff --git a/src/GdbServerConnection.h b/src/GdbServerConnection.h index c9a7273052c..a8a9aa18fc6 100644 --- a/src/GdbServerConnection.h +++ b/src/GdbServerConnection.h @@ -752,7 +752,7 @@ class GdbServerConnection { CPU_PKU = 0x8 }; - void set_cpu_features(SupportedArch arch, uint32_t features); + void set_cpu_features(SupportedArch arch); uint32_t cpu_features() const { return cpu_features_; } GdbServerConnection(ThreadGroupUid tguid, const Features& features); @@ -879,7 +879,7 @@ class GdbServerConnection { bool hwbreak_supported_; // client supports hwbreak extension bool swbreak_supported_; // client supports swbreak extension bool list_threads_in_stop_reply_; // client requested threads: and thread-pcs: in stop replies - std::unique_ptr target_decription{nullptr}; + std::unique_ptr target_description; }; } // namespace rr diff --git a/src/TargetDescription.cc b/src/TargetDescription.cc index 469a006b13d..4b042579aba 100644 --- a/src/TargetDescription.cc +++ b/src/TargetDescription.cc @@ -2,28 +2,29 @@ #include "GdbServerConnection.h" #include "kernel_abi.h" #include + namespace rr { class FeatureStream { - std::stringstream stream{}; - const char* arch_prefix{nullptr}; - public: - std::string result() noexcept { return stream.str(); } + std::string result() { return stream.str(); } template - friend FeatureStream& operator<<(FeatureStream& stream, Any any) noexcept; + friend FeatureStream& operator<<(FeatureStream& stream, Any any); + +private: + std::stringstream stream; + const char* arch_prefix; }; template -FeatureStream& operator<<(FeatureStream& stream, Any any) noexcept { +FeatureStream& operator<<(FeatureStream& stream, Any any) { stream.stream << any; return stream; } template <> -FeatureStream& operator<<(FeatureStream& stream, - rr::SupportedArch arch) noexcept { +FeatureStream& operator<<(FeatureStream& stream, rr::SupportedArch arch) { stream << ""; switch (arch) { case rr::x86: @@ -44,8 +45,7 @@ FeatureStream& operator<<(FeatureStream& stream, } template <> -FeatureStream& operator<<(FeatureStream& stream, - TargetFeature feature) noexcept { +FeatureStream& operator<<(FeatureStream& stream, TargetFeature feature) { DEBUG_ASSERT(stream.arch_prefix != nullptr && "No architecture has been provided to description"); stream << R"( +static const char header[] = R"( )"; -std::string TargetDescription::to_xml() const noexcept { - FeatureStream fs{}; - fs << Header << arch << "GNU/Linux\n"; +std::string TargetDescription::to_xml() const { + FeatureStream fs; + fs << header << arch << "GNU/Linux\n"; for (const auto feature : target_features) { fs << feature; } diff --git a/src/TargetDescription.h b/src/TargetDescription.h index 78c148e810d..469ba8fa164 100644 --- a/src/TargetDescription.h +++ b/src/TargetDescription.h @@ -6,23 +6,22 @@ namespace rr { struct GdbServerRegisterValue; -using u32 = std::uint32_t; - -enum class TargetFeature : u32 { - Core = 0, - SSE, - Linux, - Segment, - AVX, - PKeys, - FPU, +enum class TargetFeature : uint32_t { + Core = 0, + SSE, + Linux, + Segment, + AVX, + PKeys, + FPU, }; class TargetDescription { - SupportedArch arch; - std::vector target_features; + SupportedArch arch; + std::vector target_features; + public: - explicit TargetDescription(rr::SupportedArch arch, u32 cpu_features) noexcept; - std::string to_xml() const noexcept; + explicit TargetDescription(rr::SupportedArch arch, uint32_t cpu_features); + std::string to_xml() const; }; -} \ No newline at end of file +} // namespace rr \ No newline at end of file From 1155d3407fda4205c4a64f18d5027907c9bf757d Mon Sep 17 00:00:00 2001 From: Simon Farre Date: Tue, 19 Nov 2024 18:40:42 +0100 Subject: [PATCH 4/4] Fixed nits --- src/TargetDescription.cc | 8 +++++--- src/TargetDescription.h | 23 +++++++++++++++-------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/TargetDescription.cc b/src/TargetDescription.cc index 4b042579aba..92214a069e7 100644 --- a/src/TargetDescription.cc +++ b/src/TargetDescription.cc @@ -1,3 +1,5 @@ +/* -*- Mode: C++; tab-width: 8; c-basic-offset: 2; indent-tabs-mode: nil; -*- */ + #include "TargetDescription.h" #include "GdbServerConnection.h" #include "kernel_abi.h" @@ -7,13 +9,13 @@ namespace rr { class FeatureStream { public: - std::string result() { return stream.str(); } + string result() { return stream.str(); } template friend FeatureStream& operator<<(FeatureStream& stream, Any any); private: - std::stringstream stream; + stringstream stream; const char* arch_prefix; }; @@ -115,7 +117,7 @@ static const char header[] = R"( )"; -std::string TargetDescription::to_xml() const { +string TargetDescription::to_xml() const { FeatureStream fs; fs << header << arch << "GNU/Linux\n"; for (const auto feature : target_features) { diff --git a/src/TargetDescription.h b/src/TargetDescription.h index 469ba8fa164..93569b47e40 100644 --- a/src/TargetDescription.h +++ b/src/TargetDescription.h @@ -1,10 +1,14 @@ -#pragma once +/* -*- Mode: C++; tab-width: 8; c-basic-offset: 2; indent-tabs-mode: nil; -*- */ + +#ifndef RR_TARGET_DESCRIPTION_H_ +#define RR_TARGET_DESCRIPTION_H_ + #include "kernel_abi.h" #include -namespace rr { +using namespace std; -struct GdbServerRegisterValue; +namespace rr { enum class TargetFeature : uint32_t { Core = 0, @@ -17,11 +21,14 @@ enum class TargetFeature : uint32_t { }; class TargetDescription { - SupportedArch arch; - std::vector target_features; - public: explicit TargetDescription(rr::SupportedArch arch, uint32_t cpu_features); - std::string to_xml() const; + string to_xml() const; + +private: + SupportedArch arch; + vector target_features; }; -} // namespace rr \ No newline at end of file +} // namespace rr + +#endif /* RR_TARGET_DESCRIPTION_H_ */ \ No newline at end of file