Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added dynamic TargetDescription #3780

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -629,6 +629,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
Expand Down Expand Up @@ -768,12 +769,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
Expand Down
1 change: 1 addition & 0 deletions src/GdbServer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ void GdbServer::dispatch_regs_request(const Registers& regs,
return;
}
vector<GdbServerRegisterValue> rs;
rs.reserve(end);
for (GdbServerRegister r = GdbServerRegister(0); r <= end; r = GdbServerRegister(r + 1)) {
rs.push_back(get_reg(regs, extra_regs, r));
}
Expand Down
42 changes: 13 additions & 29 deletions src/GdbServerConnection.cc
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "DebuggerExtensionCommandHandler.h"
#include "ReplaySession.h"
#include "ScopedFd.h"
#include "TargetDescription.h"
#include "core.h"
#include "log.h"

Expand Down Expand Up @@ -111,7 +112,8 @@ unique_ptr<GdbServerConnection> GdbServerConnection::await_connection(
Task* t, ScopedFd& listen_fd, const GdbServerConnection::Features& features) {
auto dbg = unique_ptr<GdbServerConnection>(
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));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just drop this hunk.

dbg->await_debugger(listen_fd);
return dbg;
}
Expand Down Expand Up @@ -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, ':');
Expand Down Expand Up @@ -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());
theIDinside marked this conversation as resolved.
Show resolved Hide resolved
write_xfer_response(desc.c_str(), desc.size(), offset, len);
return false;
}

Expand Down Expand Up @@ -2341,4 +2317,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<TargetDescription>(arch, cpu_features_);
}

} // namespace rr
5 changes: 4 additions & 1 deletion src/GdbServerConnection.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "ReplaySession.h"
#include "ReplayTimeline.h"
#include "ScopedFd.h"
#include "TargetDescription.h"
#include "TaskishUid.h"
#include "core.h"

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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<TargetDescription> target_decription{nullptr};
theIDinside marked this conversation as resolved.
Show resolved Hide resolved
};

} // namespace rr
Expand Down
124 changes: 124 additions & 0 deletions src/TargetDescription.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
#include "TargetDescription.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put

/* -*- Mode: C++; tab-width: 8; c-basic-offset: 2; indent-tabs-mode: nil; -*- */

on the first line like our other .cc files.

#include "GdbServerConnection.h"
#include "kernel_abi.h"
#include <sstream>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Order #includes like our other files: first TargetDescription.h, then the C++ library headers, then the rr headers.

namespace rr {
theIDinside marked this conversation as resolved.
Show resolved Hide resolved

class FeatureStream {
std::stringstream stream{};
theIDinside marked this conversation as resolved.
Show resolved Hide resolved
const char* arch_prefix{nullptr};
theIDinside marked this conversation as resolved.
Show resolved Hide resolved

public:
std::string result() noexcept { return stream.str(); }

template <typename Any>
friend FeatureStream& operator<<(FeatureStream& stream, Any any) noexcept;
theIDinside marked this conversation as resolved.
Show resolved Hide resolved
};

template <typename Any>
FeatureStream& operator<<(FeatureStream& stream, Any any) noexcept {
stream.stream << any;
return stream;
}

template <>
FeatureStream& operator<<(FeatureStream& stream,
rr::SupportedArch arch) noexcept {
stream << "<architecture>";
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 << "</architecture>\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"( <xi:include href=")" << stream.arch_prefix;
switch (feature) {
case TargetFeature::Core:
stream << "core.xml";
break;
case TargetFeature::Linux:
stream << "linux.xml";
break;
case TargetFeature::SSE:
stream << "sse.xml";
break;
case TargetFeature::AVX:
stream << "avx.xml";
break;
case TargetFeature::PKeys:
stream << "pkeys.xml";
break;
case TargetFeature::Segment:
stream << "seg.xml";
break;
}
stream << R"("/>)" << '\n';
return stream;
}

TargetDescription::TargetDescription(rr::SupportedArch arch,
u32 cpu_features) noexcept
theIDinside marked this conversation as resolved.
Show resolved Hide resolved
: 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"(<?xml version="1.0"?>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to make this constexpr or auto. Also, name should be header or possibly HEADER. E.g. use
static const char header[] = ...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... or else just write it inline, that's fine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to header, but refrained from inlining it directly as raw string literals make code look weird, though that's a personal taste though, and I'll gladly change if it you like.

<!DOCTYPE target SYSTEM "gdb-target.dtd">
<target>
)";

std::string TargetDescription::to_xml() const noexcept {
FeatureStream fs{};
theIDinside marked this conversation as resolved.
Show resolved Hide resolved
fs << Header << arch << "<osabi>GNU/Linux</osabi>\n";
for (const auto feature : target_features) {
fs << feature;
}
fs << "</target>";

return fs.result();
}
} // namespace rr
27 changes: 27 additions & 0 deletions src/TargetDescription.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#pragma once
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use this. Use #define guards instead like our other .h files.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, put

/* -*- Mode: C++; tab-width: 8; c-basic-offset: 2; indent-tabs-mode: nil; -*- */

on the first line like our other source files.

#include "kernel_abi.h"
#include <cstdint>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, reorder these includes.


namespace rr {

struct GdbServerRegisterValue;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove this since it's not used.


using u32 = std::uint32_t;
theIDinside marked this conversation as resolved.
Show resolved Hide resolved

enum class TargetFeature : u32 {
Core = 0,
theIDinside marked this conversation as resolved.
Show resolved Hide resolved
SSE,
Linux,
Segment,
AVX,
PKeys
};

class TargetDescription {
SupportedArch arch;
theIDinside marked this conversation as resolved.
Show resolved Hide resolved
std::vector<TargetFeature> target_features;
public:
explicit TargetDescription(rr::SupportedArch arch, u32 cpu_features) noexcept;
theIDinside marked this conversation as resolved.
Show resolved Hide resolved
std::string to_xml() const noexcept;
};
}
19 changes: 0 additions & 19 deletions third-party/gdb/amd64-avx-linux.xml

This file was deleted.

19 changes: 0 additions & 19 deletions third-party/gdb/amd64-linux.xml

This file was deleted.

20 changes: 0 additions & 20 deletions third-party/gdb/amd64-pkeys-linux.xml

This file was deleted.

18 changes: 0 additions & 18 deletions third-party/gdb/i386-avx-linux.xml

This file was deleted.

18 changes: 0 additions & 18 deletions third-party/gdb/i386-linux.xml

This file was deleted.

Loading
Loading