Skip to content
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ def __init__(
self.terminated: bool = False
self.events: List[Event] = []
self.progress_events: List[Event] = []
self.invalidated_event: Event = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.invalidated_event: Event = None
self.invalidated_event: Optional[Event] = None

self.reverse_requests: List[Request] = []
self.module_events: List[Dict] = []
self.sequence: int = 1
Expand Down Expand Up @@ -440,6 +441,8 @@ def _handle_event(self, packet: Event) -> None:
elif event == "capabilities" and body:
# Update the capabilities with new ones from the event.
self.capabilities.update(body["capabilities"])
elif event == "invalidated":
self.invalidated_event = packet
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a list in case we get more invalidate events? Or when does this reset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We check invalidated event in verify_invalidated_event function and reset this field. This function is called inside wrappers around setVariable and writeMemory requests.


def _handle_reverse_request(self, request: Request) -> None:
if request in self.reverse_requests:
Expand Down Expand Up @@ -1014,6 +1017,7 @@ def request_initialize(self, sourceInitFile=False):
"supportsVariableType": True,
"supportsStartDebuggingRequest": True,
"supportsProgressReporting": True,
"supportsInvalidatedEvent": True,
"$__lldb_sourceInitFile": sourceInitFile,
},
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,13 @@ def verify_commands(self, flavor: str, output: str, commands: list[str]):
f"Command '{flavor}' - '{cmd}' not found in output: {output}",
)

def verify_invalidated_event(self, expected_areas):
event = self.dap_server.invalidated_event
self.dap_server.invalidated_event = None
self.assertIsNotNone(event)
areas = event["body"].get("areas", [])
self.assertEqual(set(expected_areas), set(areas))

def get_dict_value(self, d: dict, key_path: list[str]) -> Any:
"""Verify each key in the key_path array is in contained in each
dictionary within "d". Assert if any key isn't in the
Expand Down Expand Up @@ -352,13 +359,20 @@ def get_local_as_int(self, name, threadId=None):
else:
return int(value)

def set_variable(self, varRef, name, value, id=None):
"""Set a variable."""
response = self.dap_server.request_setVariable(varRef, name, str(value), id=id)
if response["success"]:
self.verify_invalidated_event(["variables"])
return response

def set_local(self, name, value, id=None):
"""Set a top level local variable only."""
return self.dap_server.request_setVariable(1, name, str(value), id=id)
return self.set_variable(1, name, str(value), id=id)

def set_global(self, name, value, id=None):
"""Set a top level global variable only."""
return self.dap_server.request_setVariable(2, name, str(value), id=id)
return self.set_variable(2, name, str(value), id=id)

def stepIn(
self,
Expand Down Expand Up @@ -577,4 +591,6 @@ def writeMemory(self, memoryReference, data=None, offset=0, allowPartial=False):
response = self.dap_server.request_writeMemory(
memoryReference, encodedData, offset=offset, allowPartial=allowPartial
)
if response["success"]:
self.verify_invalidated_event(["all"])
return response
4 changes: 1 addition & 3 deletions lldb/test/API/tools/lldb-dap/memory/TestDAP_memory.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,7 @@ def test_memory_refs_set_variable(self):
ptr_value = self.get_local_as_int("rawptr")
self.assertIn(
"memoryReference",
self.dap_server.request_setVariable(1, "rawptr", ptr_value + 2)[
"body"
].keys(),
self.set_local("rawptr", ptr_value + 2)["body"].keys(),
)

@skipIfWindows
Expand Down
34 changes: 10 additions & 24 deletions lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ def do_test_scopes_variables_setVariable_evaluate(
# Set a variable value whose name is synthetic, like a variable index
# and verify the value by reading it
variable_value = 100
response = self.dap_server.request_setVariable(varRef, "[0]", variable_value)
response = self.set_variable(varRef, "[0]", variable_value)
# Verify dap sent the correct response
verify_response = {
"type": "int",
Expand All @@ -315,7 +315,7 @@ def do_test_scopes_variables_setVariable_evaluate(
# Set a variable value whose name is a real child value, like "pt.x"
# and verify the value by reading it
varRef = varref_dict["pt"]
self.dap_server.request_setVariable(varRef, "x", 111)
self.set_variable(varRef, "x", 111)
response = self.dap_server.request_variables(varRef, start=0, count=1)
value = response["body"]["variables"][0]["value"]
self.assertEqual(
Expand All @@ -341,27 +341,15 @@ def do_test_scopes_variables_setVariable_evaluate(
self.verify_variables(verify_locals, self.dap_server.get_local_variables())

# Now we verify that we correctly change the name of a variable with and without differentiator suffix
self.assertFalse(self.dap_server.request_setVariable(1, "x2", 9)["success"])
self.assertFalse(
self.dap_server.request_setVariable(1, "x @ main.cpp:0", 9)["success"]
)
self.assertFalse(self.set_local("x2", 9)["success"])
self.assertFalse(self.set_local("x @ main.cpp:0", 9)["success"])

self.assertTrue(
self.dap_server.request_setVariable(1, "x @ main.cpp:19", 19)["success"]
)
self.assertTrue(
self.dap_server.request_setVariable(1, "x @ main.cpp:21", 21)["success"]
)
self.assertTrue(
self.dap_server.request_setVariable(1, "x @ main.cpp:23", 23)["success"]
)
self.assertTrue(self.set_local("x @ main.cpp:19", 19)["success"])
self.assertTrue(self.set_local("x @ main.cpp:21", 21)["success"])
self.assertTrue(self.set_local("x @ main.cpp:23", 23)["success"])

# The following should have no effect
self.assertFalse(
self.dap_server.request_setVariable(1, "x @ main.cpp:23", "invalid")[
"success"
]
)
self.assertFalse(self.set_local("x @ main.cpp:23", "invalid")["success"])

verify_locals["x @ main.cpp:19"]["equals"]["value"] = "19"
verify_locals["x @ main.cpp:21"]["equals"]["value"] = "21"
Expand All @@ -370,7 +358,7 @@ def do_test_scopes_variables_setVariable_evaluate(
self.verify_variables(verify_locals, self.dap_server.get_local_variables())

# The plain x variable shold refer to the innermost x
self.assertTrue(self.dap_server.request_setVariable(1, "x", 22)["success"])
self.assertTrue(self.set_local("x", 22)["success"])
verify_locals["x @ main.cpp:23"]["equals"]["value"] = "22"

self.verify_variables(verify_locals, self.dap_server.get_local_variables())
Expand Down Expand Up @@ -708,9 +696,7 @@ def test_return_variables(self):
self.verify_variables(verify_locals, local_variables, varref_dict)
break

self.assertFalse(
self.dap_server.request_setVariable(1, "(Return Value)", 20)["success"]
)
self.assertFalse(self.set_local("(Return Value)", 20)["success"])

@skipIfWindows
def test_indexedVariables(self):
Expand Down
11 changes: 11 additions & 0 deletions lldb/tools/lldb-dap/EventHelper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@
#include "JSONUtils.h"
#include "LLDBUtils.h"
#include "Protocol/ProtocolEvents.h"
#include "Protocol/ProtocolRequests.h"
#include "Protocol/ProtocolTypes.h"
#include "lldb/API/SBFileSpec.h"
#include "llvm/Support/Error.h"
#include <utility>

#if defined(_WIN32)
#define NOMINMAX
Expand Down Expand Up @@ -273,4 +275,13 @@ void SendProcessExitedEvent(DAP &dap, lldb::SBProcess &process) {
dap.SendJSON(llvm::json::Value(std::move(event)));
}

void SendInvalidatedEvent(
DAP &dap, std::vector<protocol::InvalidatedEventBody::Area> &&areas) {
if (!dap.clientFeatures.contains(protocol::eClientFeatureInvalidatedEvent))
return;
protocol::InvalidatedEventBody body;
body.areas = std::move(areas);
dap.Send(protocol::Event{"invalidated", std::move(body)});
}

} // namespace lldb_dap
5 changes: 5 additions & 0 deletions lldb/tools/lldb-dap/EventHelper.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
#define LLDB_TOOLS_LLDB_DAP_EVENTHELPER_H

#include "DAPForward.h"
#include "Protocol/ProtocolEvents.h"
#include "llvm/Support/Error.h"
#include <vector>

namespace lldb_dap {
struct DAP;
Expand All @@ -32,6 +34,9 @@ void SendContinuedEvent(DAP &dap);

void SendProcessExitedEvent(DAP &dap, lldb::SBProcess &process);

void SendInvalidatedEvent(
Copy link
Member

Choose a reason for hiding this comment

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

Add documention and mention that areas is moved within the function.
I also think it might be fine not to use &&, get a llvm::ArrayRef as argument and create a copy of it inside the implementation. We don't need to over optimize this part and instead it might be better to reduce the number of side effects (i.e. moving areas).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed

DAP &dap, std::vector<protocol::InvalidatedEventBody::Area> &&areas);

} // namespace lldb_dap

#endif
5 changes: 5 additions & 0 deletions lldb/tools/lldb-dap/Handler/SetVariableRequestHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "DAP.h"
#include "EventHelper.h"
#include "JSONUtils.h"
#include "Protocol/ProtocolEvents.h"
#include "RequestHandler.h"

using namespace lldb_dap::protocol;
Expand Down Expand Up @@ -77,6 +78,10 @@ SetVariableRequestHandler::Run(const SetVariableArguments &args) const {
if (ValuePointsToCode(variable))
body.valueLocationReference = new_var_ref;

// Also send invalidated event to signal client that some variables
// (e.g. references) can be changed.
SendInvalidatedEvent(dap, {InvalidatedEventBody::eAreaVariables});

return body;
}

Expand Down
16 changes: 12 additions & 4 deletions lldb/tools/lldb-dap/Handler/WriteMemoryRequestHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,24 @@
//===----------------------------------------------------------------------===//

#include "DAP.h"
#include "EventHelper.h"
#include "JSONUtils.h"
#include "Protocol/ProtocolEvents.h"
#include "RequestHandler.h"
#include "lldb/API/SBMemoryRegionInfo.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/Support/Base64.h"

using namespace lldb_dap::protocol;

namespace lldb_dap {

// Writes bytes to memory at the provided location.
//
// Clients should only call this request if the corresponding capability
// supportsWriteMemoryRequest is true.
llvm::Expected<protocol::WriteMemoryResponseBody>
WriteMemoryRequestHandler::Run(
const protocol::WriteMemoryArguments &args) const {
llvm::Expected<WriteMemoryResponseBody>
WriteMemoryRequestHandler::Run(const WriteMemoryArguments &args) const {
const lldb::addr_t address = args.memoryReference + args.offset;

lldb::SBProcess process = dap.target.GetProcess();
Expand Down Expand Up @@ -91,8 +94,13 @@ WriteMemoryRequestHandler::Run(
if (bytes_written == 0) {
return llvm::make_error<DAPError>(write_error.GetCString());
}
protocol::WriteMemoryResponseBody response;
WriteMemoryResponseBody response;
response.bytesWritten = bytes_written;

// Also send invalidated event to signal client that some things
// (e.g. variables) can be changed.
SendInvalidatedEvent(dap, {InvalidatedEventBody::eAreaAll});
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this change everything? Or just variables? I am not sure if writing into memory like this prevents changing the current stack or PC counter or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can change return address on stack using writeMemory request


return response;
}

Expand Down
18 changes: 18 additions & 0 deletions lldb/tools/lldb-dap/Protocol/ProtocolEvents.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,22 @@ json::Value toJSON(const ModuleEventBody &MEB) {
return json::Object{{"reason", MEB.reason}, {"module", MEB.module}};
}

llvm::json::Value toJSON(const InvalidatedEventBody::Area &IEBA) {
switch (IEBA) {
case InvalidatedEventBody::eAreaAll:
return "all";
case InvalidatedEventBody::eAreaStacks:
return "stacks";
case InvalidatedEventBody::eAreaThreads:
return "threads";
case InvalidatedEventBody::eAreaVariables:
return "variables";
}
llvm_unreachable("unhandled invalidated event area!.");
}

llvm::json::Value toJSON(const InvalidatedEventBody &IEB) {
return json::Object{{"areas", IEB.areas}};
}

} // namespace lldb_dap::protocol
9 changes: 9 additions & 0 deletions lldb/tools/lldb-dap/Protocol/ProtocolEvents.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

#include "Protocol/ProtocolTypes.h"
#include "llvm/Support/JSON.h"
#include <vector>

namespace lldb_dap::protocol {

Expand Down Expand Up @@ -56,6 +57,14 @@ struct ModuleEventBody {
llvm::json::Value toJSON(const ModuleEventBody::Reason &);
llvm::json::Value toJSON(const ModuleEventBody &);

struct InvalidatedEventBody {
enum Area : unsigned { eAreaAll, eAreaStacks, eAreaThreads, eAreaVariables };

std::vector<Area> areas;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have the fields for threadId and stackFrameId in case we add that in the future?

};
llvm::json::Value toJSON(const InvalidatedEventBody::Area &);
llvm::json::Value toJSON(const InvalidatedEventBody &);

} // end namespace lldb_dap::protocol

#endif
14 changes: 14 additions & 0 deletions lldb/unittests/DAP/ProtocolTypesTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1073,3 +1073,17 @@ TEST(ProtocolTypesTest, CompletionsResponseBody) {
ASSERT_THAT_EXPECTED(expected, llvm::Succeeded());
EXPECT_EQ(pp(*expected), pp(response));
}

TEST(ProtocolTypesTest, InvalidatedEventBody) {
InvalidatedEventBody body;
body.areas = {InvalidatedEventBody::eAreaStacks,
InvalidatedEventBody::eAreaThreads};
StringRef json = R"({
"areas": [
"stacks",
"threads"
]
})";
// Validate toJSON
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Validate toJSON

EXPECT_EQ(json, pp(body));
}
Loading