Skip to content

Commit c9f3a68

Browse files
JDevlieghereJDevlieghere
committed
Address review feedback
1 parent 74cc459 commit c9f3a68

File tree

8 files changed

+106
-55
lines changed

8 files changed

+106
-55
lines changed

‎lldb/include/lldb/Core/Debugger.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -367,7 +367,7 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
367367

368368
bool GetNotifyVoid() const;
369369

370-
const std::string &GetInstanceName() { return m_instance_name; }
370+
const std::string &GetInstanceName() const { return m_instance_name; }
371371

372372
bool GetShowInlineDiagnostics() const;
373373

‎lldb/include/lldb/Target/Target.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1093,7 +1093,7 @@ class Target : public std::enable_shared_from_this<Target>,
10931093

10941094
Architecture *GetArchitecturePlugin() const { return m_arch.GetPlugin(); }
10951095

1096-
Debugger &GetDebugger() { return m_debugger; }
1096+
Debugger &GetDebugger() const { return m_debugger; }
10971097

10981098
size_t ReadMemoryFromFileCache(const Address &addr, void *dst, size_t dst_len,
10991099
Status &error);

‎lldb/source/Plugins/Protocol/MCP/MCPError.h‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ class MCPError : public llvm::ErrorInfo<MCPError> {
1717
public:
1818
static char ID;
1919

20-
MCPError(std::string message, int64_t error_code);
20+
MCPError(std::string message, int64_t error_code = kInternalError);
2121

2222
void log(llvm::raw_ostream &OS) const override;
2323
std::error_code convertToErrorCode() const override;
@@ -26,6 +26,9 @@ class MCPError : public llvm::ErrorInfo<MCPError> {
2626

2727
protocol::Error toProtcolError() const;
2828

29+
static constexpr int64_t kResourceNotFound = -32002;
30+
static constexpr int64_t kInternalError = -32603;
31+
2932
private:
3033
std::string m_message;
3134
int64_t m_error_code;

‎lldb/source/Plugins/Protocol/MCP/ProtocolServerMCP.cpp‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ ProtocolServerMCP::Handle(protocol::Request request) {
8484
}
8585

8686
return make_error<MCPError>(
87-
llvm::formatv("no handler for request: {0}", request.method).str(), 1);
87+
llvm::formatv("no handler for request: {0}", request.method).str());
8888
}
8989

9090
void ProtocolServerMCP::Handle(protocol::Notification notification) {
@@ -225,7 +225,7 @@ ProtocolServerMCP::HandleData(llvm::StringRef data) {
225225
response.takeError(),
226226
[&](const MCPError &err) { protocol_error = err.toProtcolError(); },
227227
[&](const llvm::ErrorInfoBase &err) {
228-
protocol_error.error.code = -1;
228+
protocol_error.error.code = MCPError::kInternalError;
229229
protocol_error.error.message = err.message();
230230
});
231231
protocol_error.id = request->id;
@@ -253,6 +253,8 @@ ProtocolServerMCP::HandleData(llvm::StringRef data) {
253253
protocol::Capabilities ProtocolServerMCP::GetCapabilities() {
254254
protocol::Capabilities capabilities;
255255
capabilities.tools.listChanged = true;
256+
// FIXME: Support sending notifications when a debugger/target are
257+
// added/removed.
256258
capabilities.resources.listChanged = false;
257259
return capabilities;
258260
}
@@ -403,5 +405,6 @@ ProtocolServerMCP::ResourcesReadHandler(const protocol::Request &request) {
403405
}
404406

405407
return make_error<MCPError>(
406-
llvm::formatv("no resource handler for uri: {0}", uri_str).str(), 1);
408+
llvm::formatv("no resource handler for uri: {0}", uri_str).str(),
409+
MCPError::kResourceNotFound);
407410
}

‎lldb/source/Plugins/Protocol/MCP/Resource.cpp‎

Lines changed: 87 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,48 @@
88
#include "MCPError.h"
99
#include "lldb/Core/Debugger.h"
1010
#include "lldb/Core/Module.h"
11+
#include "lldb/Target/Platform.h"
1112

1213
using namespace lldb_private::mcp;
1314

15+
namespace {
16+
struct DebuggerResource {
17+
uint64_t debugger_id;
18+
std::string name;
19+
uint64_t num_targets;
20+
};
21+
22+
llvm::json::Value toJSON(const DebuggerResource &DR) {
23+
llvm::json::Object Result{{"debugger_id", DR.debugger_id},
24+
{"num_targets", DR.num_targets}};
25+
if (!DR.name.empty())
26+
Result.insert({"name", DR.name});
27+
return Result;
28+
}
29+
30+
struct TargetResource {
31+
size_t debugger_id;
32+
size_t target_idx;
33+
std::string arch;
34+
std::string path;
35+
std::string platform;
36+
};
37+
38+
llvm::json::Value toJSON(const TargetResource &TR) {
39+
llvm::json::Object Result{{"debugger_id", TR.debugger_id},
40+
{"target_idx", TR.target_idx}};
41+
if (!TR.arch.empty())
42+
Result.insert({"arch", TR.arch});
43+
if (!TR.path.empty())
44+
Result.insert({"path", TR.path});
45+
if (!TR.platform.empty())
46+
Result.insert({"platform", TR.platform});
47+
return Result;
48+
}
49+
} // namespace
50+
51+
static constexpr llvm::StringLiteral kMimeTypeJSON = "application/json";
52+
1453
template <typename... Args>
1554
static llvm::Error createStringError(const char *format, Args &&...args) {
1655
return llvm::createStringError(
@@ -22,27 +61,36 @@ static llvm::Error createUnsupportedURIError(llvm::StringRef uri) {
2261
}
2362

2463
protocol::Resource
25-
DebuggerResourceProvider::GetDebuggerResource(lldb::user_id_t debugger_id) {
64+
DebuggerResourceProvider::GetDebuggerResource(Debugger &debugger) {
65+
const lldb::user_id_t debugger_id = debugger.GetID();
66+
2667
protocol::Resource resource;
2768
resource.uri = llvm::formatv("lldb://debugger/{0}", debugger_id);
28-
resource.name = llvm::formatv("debugger {0}", debugger_id);
69+
resource.name = debugger.GetInstanceName();
2970
resource.description =
30-
llvm::formatv("Information about debugger instance {0}", debugger_id);
31-
resource.mimeType = "application/json";
71+
llvm::formatv("Information about debugger instance {0}: {1}", debugger_id,
72+
debugger.GetInstanceName());
73+
resource.mimeType = kMimeTypeJSON;
3274
return resource;
3375
}
3476

3577
protocol::Resource
36-
DebuggerResourceProvider::GetTargetResource(lldb::user_id_t debugger_id,
37-
lldb::user_id_t target_id) {
78+
DebuggerResourceProvider::GetTargetResource(size_t target_idx, Target &target) {
79+
const size_t debugger_id = target.GetDebugger().GetID();
80+
81+
std::string target_name = llvm::formatv("target {0}", target_idx);
82+
83+
if (Module *exe_module = target.GetExecutableModulePointer())
84+
target_name = exe_module->GetFileSpec().GetFilename().GetString();
85+
3886
protocol::Resource resource;
3987
resource.uri =
40-
llvm::formatv("lldb://debugger/{0}/target/{1}", debugger_id, target_id);
41-
resource.name = llvm::formatv("target {0}", target_id);
88+
llvm::formatv("lldb://debugger/{0}/target/{1}", debugger_id, target_idx);
89+
resource.name = target_name;
4290
resource.description =
4391
llvm::formatv("Information about target {0} in debugger instance {1}",
44-
target_id, debugger_id);
45-
resource.mimeType = "application/json";
92+
target_idx, debugger_id);
93+
resource.mimeType = kMimeTypeJSON;
4694
return resource;
4795
}
4896

@@ -54,15 +102,15 @@ std::vector<protocol::Resource> DebuggerResourceProvider::GetResources() const {
54102
lldb::DebuggerSP debugger_sp = Debugger::GetDebuggerAtIndex(i);
55103
if (!debugger_sp)
56104
continue;
57-
resources.emplace_back(GetDebuggerResource(i));
105+
resources.emplace_back(GetDebuggerResource(*debugger_sp));
58106

59107
TargetList &target_list = debugger_sp->GetTargetList();
60108
const size_t num_targets = target_list.GetNumTargets();
61109
for (size_t j = 0; j < num_targets; ++j) {
62110
lldb::TargetSP target_sp = target_list.GetTargetAtIndex(j);
63111
if (!target_sp)
64112
continue;
65-
resources.emplace_back(GetTargetResource(i, j));
113+
resources.emplace_back(GetTargetResource(j, *target_sp));
66114
}
67115
}
68116

@@ -71,6 +119,7 @@ std::vector<protocol::Resource> DebuggerResourceProvider::GetResources() const {
71119

72120
llvm::Expected<protocol::ResourceResult>
73121
DebuggerResourceProvider::ReadResource(llvm::StringRef uri) const {
122+
74123
auto [protocol, path] = uri.split("://");
75124

76125
if (protocol != "lldb")
@@ -85,45 +134,42 @@ DebuggerResourceProvider::ReadResource(llvm::StringRef uri) const {
85134
if (components[0] != "debugger")
86135
return createUnsupportedURIError(uri);
87136

88-
lldb::user_id_t debugger_id;
89-
if (components[1].getAsInteger(0, debugger_id))
137+
size_t debugger_idx;
138+
if (components[1].getAsInteger(0, debugger_idx))
90139
return createStringError("invalid debugger id '{0}': {1}", components[1],
91140
path);
92141

93142
if (components.size() > 3) {
94143
if (components[2] != "target")
95144
return createUnsupportedURIError(uri);
96145

97-
lldb::user_id_t target_id;
98-
if (components[3].getAsInteger(0, target_id))
146+
size_t target_idx;
147+
if (components[3].getAsInteger(0, target_idx))
99148
return createStringError("invalid target id '{0}': {1}", components[3],
100149
path);
101150

102-
return ReadTargetResource(uri, debugger_id, target_id);
151+
return ReadTargetResource(uri, debugger_idx, target_idx);
103152
}
104153

105-
return ReadDebuggerResource(uri, debugger_id);
154+
return ReadDebuggerResource(uri, debugger_idx);
106155
}
107156

108157
llvm::Expected<protocol::ResourceResult>
109158
DebuggerResourceProvider::ReadDebuggerResource(llvm::StringRef uri,
110159
lldb::user_id_t debugger_id) {
111-
lldb::DebuggerSP debugger_sp = Debugger::GetDebuggerAtIndex(debugger_id);
160+
lldb::DebuggerSP debugger_sp = Debugger::FindDebuggerWithID(debugger_id);
112161
if (!debugger_sp)
113162
return createStringError("invalid debugger id: {0}", debugger_id);
114163

115-
TargetList &target_list = debugger_sp->GetTargetList();
116-
const size_t num_targets = target_list.GetNumTargets();
117-
118-
llvm::json::Value value = llvm::json::Object{{"debugger_id", debugger_id},
119-
{"num_targets", num_targets}};
120-
121-
std::string json = llvm::formatv("{0}", value);
164+
DebuggerResource debugger_resource;
165+
debugger_resource.debugger_id = debugger_id;
166+
debugger_resource.name = debugger_sp->GetInstanceName();
167+
debugger_resource.num_targets = debugger_sp->GetTargetList().GetNumTargets();
122168

123169
protocol::ResourceContents contents;
124170
contents.uri = uri;
125-
contents.mimeType = "application/json";
126-
contents.text = json;
171+
contents.mimeType = kMimeTypeJSON;
172+
contents.text = llvm::formatv("{0}", toJSON(debugger_resource));
127173

128174
protocol::ResourceResult result;
129175
result.contents.push_back(contents);
@@ -133,32 +179,31 @@ DebuggerResourceProvider::ReadDebuggerResource(llvm::StringRef uri,
133179
llvm::Expected<protocol::ResourceResult>
134180
DebuggerResourceProvider::ReadTargetResource(llvm::StringRef uri,
135181
lldb::user_id_t debugger_id,
136-
lldb::user_id_t target_id) {
182+
size_t target_idx) {
137183

138-
lldb::DebuggerSP debugger_sp = Debugger::GetDebuggerAtIndex(debugger_id);
184+
lldb::DebuggerSP debugger_sp = Debugger::FindDebuggerWithID(debugger_id);
139185
if (!debugger_sp)
140186
return createStringError("invalid debugger id: {0}", debugger_id);
141187

142188
TargetList &target_list = debugger_sp->GetTargetList();
143-
lldb::TargetSP target_sp = target_list.GetTargetAtIndex(target_id);
189+
lldb::TargetSP target_sp = target_list.GetTargetAtIndex(target_idx);
144190
if (!target_sp)
145-
return createStringError("invalid target id: {0}", target_id);
191+
return createStringError("invalid target idx: {0}", target_idx);
146192

147-
llvm::json::Object object{
148-
{"debugger_id", debugger_id},
149-
{"target_id", target_id},
150-
{"arch", target_sp->GetArchitecture().GetTriple().str()}};
193+
TargetResource target_resource;
194+
target_resource.debugger_id = debugger_id;
195+
target_resource.target_idx = target_idx;
196+
target_resource.arch = target_sp->GetArchitecture().GetTriple().str();
151197

152198
if (Module *exe_module = target_sp->GetExecutableModulePointer())
153-
object.insert({"path", exe_module->GetFileSpec().GetPath()});
154-
155-
llvm::json::Value value = std::move(object);
156-
std::string json = llvm::formatv("{0}", value);
199+
target_resource.path = exe_module->GetFileSpec().GetPath();
200+
if (lldb::PlatformSP platform_sp = target_sp->GetPlatform())
201+
target_resource.platform = platform_sp->GetName();
157202

158203
protocol::ResourceContents contents;
159204
contents.uri = uri;
160-
contents.mimeType = "application/json";
161-
contents.text = json;
205+
contents.mimeType = kMimeTypeJSON;
206+
contents.text = llvm::formatv("{0}", toJSON(target_resource));
162207

163208
protocol::ResourceResult result;
164209
result.contents.push_back(contents);

‎lldb/source/Plugins/Protocol/MCP/Resource.h‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,15 +35,15 @@ class DebuggerResourceProvider : public ResourceProvider {
3535
ReadResource(llvm::StringRef uri) const override;
3636

3737
private:
38-
static protocol::Resource GetDebuggerResource(lldb::user_id_t target_id);
39-
static protocol::Resource GetTargetResource(lldb::user_id_t debugger_id,
40-
lldb::user_id_t target_id);
38+
static protocol::Resource GetDebuggerResource(Debugger &debugger);
39+
static protocol::Resource GetTargetResource(size_t target_idx,
40+
Target &target);
4141

4242
static llvm::Expected<protocol::ResourceResult>
43-
ReadDebuggerResource(llvm::StringRef uri, lldb::user_id_t target_id);
43+
ReadDebuggerResource(llvm::StringRef uri, lldb::user_id_t debugger_id);
4444
static llvm::Expected<protocol::ResourceResult>
4545
ReadTargetResource(llvm::StringRef uri, lldb::user_id_t debugger_id,
46-
lldb::user_id_t target_id);
46+
size_t target_idx);
4747
};
4848

4949
} // namespace lldb_private::mcp

‎lldb/source/Plugins/Protocol/MCP/Tool.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ CommandTool::Call(const protocol::ToolArguments &args) {
6565
return root.getError();
6666

6767
lldb::DebuggerSP debugger_sp =
68-
Debugger::GetDebuggerAtIndex(arguments.debugger_id);
68+
Debugger::FindDebuggerWithID(arguments.debugger_id);
6969
if (!debugger_sp)
7070
return createStringError(
7171
llvm::formatv("no debugger with id {0}", arguments.debugger_id));

‎lldb/unittests/Protocol/ProtocolMCPServerTest.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -265,7 +265,7 @@ TEST_F(ProtocolServerMCPTest, ToolsCallError) {
265265
llvm::StringLiteral request =
266266
R"json({"method":"tools/call","params":{"name":"error","arguments":{"arguments":"foo","debugger_id":0}},"jsonrpc":"2.0","id":11})json";
267267
llvm::StringLiteral response =
268-
R"json({"error":{"code":-1,"message":"error"},"id":11,"jsonrpc":"2.0"})json";
268+
R"json({"error":{"code":-32603,"message":"error"},"id":11,"jsonrpc":"2.0"})json";
269269

270270
ASSERT_THAT_ERROR(Write(request), llvm::Succeeded());
271271

0 commit comments

Comments
 (0)