Added dynamic `TargetDescription` #3780

theIDinside · 2024-07-11T11:59:00Z

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.

Note:
The GdbServerRegister should probably be rewritten/removed and then rely on a TargetDescription that provides a "static" array of GdbServerValue since once we know the target description, it won't change during a replay, so dynamically allocating like we do now (in dispatch_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.

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 · 2024-07-11T16:58:11Z

@rocallahan || @khuey
Does the tests run on actual hardware or some emulation? The errors are confusing; I just can't see how that can possibly be true without there actually existing RR bugs, and that now, with this patch we actually provide aarch64 target descriptions and that's why gdb asserts saying it expects 268 bytes (which is the total size of all registers defined in aarch64-core.xml size) but got 788 (which I think is just x86 or some variant, possibly without a few registers)

I don't even know how we have sent this before or why this hasn't been a bug prior to this.

theIDinside · 2024-07-11T17:49:43Z

Prior to this patch, I have no idea how RR informed GDB on aarch64 that the g packet contains the aarch64-fpu.xml because the only file it ever should have transferred using xfer was aarch64-core.xml. But with this patch, I forgot to add the fpu.xml in the dynamically generated target description and the tests for aarch64 failed. Now with the latest commit it works. But it does puzzle me why it hasn't failed before (since fpu.xml was never actually sent before).

rocallahan · 2024-07-14T10:15:18Z

src/GdbServerConnection.cc

@@ -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));


You can just drop this hunk.

src/GdbServerConnection.cc

src/GdbServerConnection.h

src/TargetDescription.h

src/TargetDescription.cc

rocallahan · 2024-07-14T10:24:45Z

src/TargetDescription.cc

+  }
+}
+
+static constexpr auto Header = R"(<?xml version="1.0"?>


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

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

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.

src/TargetDescription.h

rocallahan · 2024-07-14T10:29:22Z

Also, it looks like tests are failing in CI on x86-64?

theIDinside · 2024-07-15T17:44:15Z

Also, it looks like tests are failing in CI on x86-64?

Where can I see these tests? It says "All checks have passed" here.

rocallahan · 2024-07-18T11:32:35Z

src/TargetDescription.h

@@ -0,0 +1,27 @@
+#pragma once


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

Also, put

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

on the first line like our other source files.

rocallahan · 2024-07-18T11:33:40Z

src/TargetDescription.cc

@@ -0,0 +1,128 @@
+#include "TargetDescription.h"


Put

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

on the first line like our other .cc files.

rocallahan · 2024-07-18T11:34:20Z

src/TargetDescription.cc

+#include "TargetDescription.h"
+#include "GdbServerConnection.h"
+#include "kernel_abi.h"
+#include <sstream>


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

rocallahan · 2024-07-18T11:34:33Z

src/TargetDescription.h

@@ -0,0 +1,27 @@
+#pragma once
+#include "kernel_abi.h"
+#include <cstdint>


As above, reorder these includes.

rocallahan · 2024-07-18T11:35:15Z

src/TargetDescription.h

+
+namespace rr {
+
+struct GdbServerRegisterValue;


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

rocallahan · 2024-07-18T11:35:52Z

src/TargetDescription.h

+};
+
+class TargetDescription {
+  SupportedArch arch;


Put this below the public members in a private: section.

rocallahan · 2024-07-18T11:36:37Z

src/TargetDescription.cc

+
+class FeatureStream {
+public:
+  std::string result() { return stream.str(); }


Add using namespace std; after the includes and don't use the std:: prefix (except for std::move where clang requires it...)

rocallahan · 2024-07-18T11:40:09Z

Where can I see these tests? It says "All checks have passed" here.

Yep, that looks good!

theIDinside force-pushed the dynamic-target-descriptions branch from a571187 to c276f0e Compare July 11, 2024 12:04

Actually provide aarch64-fpu.xml to gdb

56cc86c

I don't even know how we have sent this before or why this hasn't been a bug prior to this.

rocallahan requested changes Jul 14, 2024

View reviewed changes

Fix formatting issues

b194918

rocallahan requested changes Jul 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added dynamic `TargetDescription` #3780

Added dynamic `TargetDescription` #3780

theIDinside commented Jul 11, 2024

theIDinside commented Jul 11, 2024 •

edited

Loading

theIDinside commented Jul 11, 2024 •

edited

Loading

rocallahan Jul 14, 2024

rocallahan Jul 14, 2024

rocallahan Jul 14, 2024

theIDinside Jul 15, 2024

rocallahan commented Jul 14, 2024

theIDinside commented Jul 15, 2024

rocallahan Jul 18, 2024

rocallahan Jul 18, 2024

rocallahan Jul 18, 2024

rocallahan Jul 18, 2024

rocallahan Jul 18, 2024

rocallahan Jul 18, 2024

rocallahan Jul 18, 2024

rocallahan Jul 18, 2024

rocallahan commented Jul 18, 2024

Added dynamic TargetDescription #3780

Are you sure you want to change the base?

Added dynamic TargetDescription #3780

Conversation

theIDinside commented Jul 11, 2024

theIDinside commented Jul 11, 2024 • edited Loading

theIDinside commented Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocallahan commented Jul 14, 2024

theIDinside commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocallahan commented Jul 18, 2024

Added dynamic `TargetDescription` #3780

Added dynamic `TargetDescription` #3780

theIDinside commented Jul 11, 2024 •

edited

Loading

theIDinside commented Jul 11, 2024 •

edited

Loading