Added dynamic TargetDescription
#3780
Conversation
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 rr-debugger#3776 PR (AVX512 support) from getting pulled in, though that PR will need to take this PR into account.
theIDinside
force-pushed
the
dynamic-target-descriptions
branch
from
a571187
to
c276f0e
Compare
|
@rocallahan || @khuey |
I don't even know how we have sent this before or why this hasn't been a bug prior to this.
|
Prior to this patch, I have no idea how RR informed GDB on |
| @@ -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)); | |||
There was a problem hiding this comment.
You can just drop this hunk.
| ? 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()); |
There was a problem hiding this comment.
Remove outer parentheses here.
| @@ -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}; | |||
There was a problem hiding this comment.
Remove the initializer, it's the same as the default.
| using u32 = std::uint32_t; | ||
|
|
||
| enum class TargetFeature : u32 { | ||
| Core = 0, |
| }; | ||
|
|
||
| class TargetDescription { | ||
| SupportedArch arch; |
There was a problem hiding this comment.
2-space indent. Also, put the fields at the end of the class definition
| std::string result() noexcept { return stream.str(); } | ||
|
|
||
| template <typename Any> | ||
| friend FeatureStream& operator<<(FeatureStream& stream, Any any) noexcept; |
There was a problem hiding this comment.
Don't use noexcept anywhere.
| } | ||
|
|
||
| TargetDescription::TargetDescription(rr::SupportedArch arch, | ||
| u32 cpu_features) noexcept |
| )"; | ||
|
|
||
| std::string TargetDescription::to_xml() const noexcept { | ||
| FeatureStream fs{}; |
| } | ||
| } | ||
|
|
||
| static constexpr auto Header = R"(<?xml version="1.0"?> |
There was a problem hiding this comment.
Don't need to make this constexpr or auto. Also, name should be header or possibly HEADER. E.g. use
static const char header[] = ...
There was a problem hiding this comment.
... or else just write it inline, that's fine
|
|
||
| struct GdbServerRegisterValue; | ||
|
|
||
| using u32 = std::uint32_t; |
There was a problem hiding this comment.
Please don't do this. Just use uint32_t everywhere (no std::)
|
Also, it looks like tests are failing in CI on x86-64? |
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
TargetDescriptioninstead.This unblocks the #3776 PR (AVX512 support) from getting pulled in, though that PR will need to take this PR into account.
Note:
The
GdbServerRegistershould probably be rewritten/removed and then rely on aTargetDescriptionthat provides a "static" array ofGdbServerValuesince once we know the target description, it won't change during a replay, so dynamically allocating like we do now (indispatch_regs_request) is not really necessary. But these are just loose thoughts I have, I haven't really verified that this would be a better approach.