[lldb-vscode] improve modules request

lldb-vsdode was communicating the list of modules to the IDE with events, which in practice ended up having some drawbacks
- when debugging large targets, the number of these events were easily 10k, which polluted the messages being transmitted, which caused the following: a harder time debugging the messages, a lag after terminated the process because of these messages being processes (this could easily take several seconds). The latter was specially bad, as users were complaining about it even when they didn't check the modules view.
- these events were rarely used, as users only check the modules view when something is wrong and they try to debug things.

After getting some feedback from users, we realized that it's better to not used events but make this simply a request and is triggered by users whenever they needed.

This diff achieves that and does some small clean up in the existing code.

Differential Revision: https://reviews.llvm.org/D94033
This commit is contained in:
Walter Erquinigo 2021-01-04 14:05:42 -08:00
parent 39db5753f9
commit 39239f9b56
4 changed files with 78 additions and 73 deletions

View File

@ -113,7 +113,6 @@ class DebugCommunication(object):
self.initialize_body = None
self.thread_stop_reasons = {}
self.breakpoint_events = []
self.module_events = {}
self.sequence = 1
self.threads = None
self.recv_thread.start()
@ -134,8 +133,12 @@ class DebugCommunication(object):
if command['seq'] != response['request_seq']:
raise ValueError('seq mismatch in response')
def get_active_modules(self):
return self.module_events
def get_modules(self):
module_list = self.request_modules()['body']['modules']
modules = {}
for module in module_list:
modules[module['name']] = module
return modules
def get_output(self, category, timeout=0.0, clear=True):
self.output_condition.acquire()
@ -222,14 +225,6 @@ class DebugCommunication(object):
self.breakpoint_events.append(packet)
# no need to add 'breakpoint' event packets to our packets list
return keepGoing
elif event == 'module':
reason = body['reason']
if (reason == 'new' or reason == 'changed'):
self.module_events[body['module']['name']] = body['module']
elif reason == 'removed':
if body['module']['name'] in self.module_events:
self.module_events.pop(body['module']['name'])
return keepGoing
elif packet_type == 'response':
if packet['command'] == 'disconnect':
@ -782,10 +777,10 @@ class DebugCommunication(object):
}
return self.send_recv(command_dict)
def request_getCompileUnits(self, moduleId):
def request_compileUnits(self, moduleId):
args_dict = {'moduleId': moduleId}
command_dict = {
'command': 'getCompileUnits',
'command': 'compileUnits',
'type': 'request',
'arguments': args_dict
}
@ -804,6 +799,12 @@ class DebugCommunication(object):
}
return self.send_recv(command_dict)
def request_modules(self):
return self.send_recv({
'command': 'modules',
'type': 'request'
})
def request_stackTrace(self, threadId=None, startFrame=None, levels=None,
dump=False):
if threadId is None:

View File

@ -24,7 +24,7 @@ class TestVSCode_module(lldbvscode_testcase.VSCodeTestCaseBase):
breakpoint_ids = self.set_function_breakpoints(functions)
self.assertEquals(len(breakpoint_ids), len(functions), 'expect one breakpoint')
self.continue_to_breakpoints(breakpoint_ids)
active_modules = self.vscode.get_active_modules()
active_modules = self.vscode.get_modules()
program_module = active_modules[program_basename]
self.assertIn(program_basename, active_modules, '%s module is in active modules' % (program_basename))
self.assertIn('name', program_module, 'make sure name is in module')
@ -36,7 +36,7 @@ class TestVSCode_module(lldbvscode_testcase.VSCodeTestCaseBase):
self.vscode.request_evaluate('`%s' % ('target symbols add -s "%s" "%s"' % (program, symbols_path)))
def checkSymbolsLoadedWithSize():
active_modules = self.vscode.get_active_modules()
active_modules = self.vscode.get_modules()
program_module = active_modules[program_basename]
self.assertIn('symbolFilePath', program_module)
self.assertIn(symbols_path, program_module['symbolFilePath'])
@ -45,17 +45,15 @@ class TestVSCode_module(lldbvscode_testcase.VSCodeTestCaseBase):
if expect_debug_info_size:
self.waitUntil(checkSymbolsLoadedWithSize)
active_modules = self.vscode.get_active_modules()
active_modules = self.vscode.get_modules()
program_module = active_modules[program_basename]
self.assertEqual(program_basename, program_module['name'])
self.assertEqual(program, program_module['path'])
self.assertIn('addressRange', program_module)
@skipIfWindows
@skipUnlessDarwin
@skipIfRemote
#TODO: Update the Makefile so that this test runs on Linux
def test_module_event(self):
def test_modules(self):
'''
Mac or linux.
@ -64,27 +62,21 @@ class TestVSCode_module(lldbvscode_testcase.VSCodeTestCaseBase):
sections are in .o files.
On other platforms, we expect a.out to have debug info, so we will expect a size.
expect_debug_info_size = platform.system() != 'Darwin'
return self.run_test("a.out", expect_debug_info_size)
'''
expect_debug_info_size = platform.system() != 'Darwin'
return self.run_test("a.out", expect_debug_info_size)
return self.run_test("a.out", expect_debug_info_size=platform.system() != 'Darwin')
@skipIfWindows
@skipUnlessDarwin
@skipIfRemote
def test_module_event_dsym(self):
def test_modules_dsym(self):
'''
Darwin only test with dSYM file.
On mac, if we load a.out.dSYM as our symbol file, we will have debug symbols and we
will have DWARF sections added to the module, so we will expect a size.
return self.run_test("a.out.dSYM", True)
'''
return self.run_test("a.out.dSYM", True)
return self.run_test("a.out.dSYM", expect_debug_info_size=True)
@skipIfWindows
@skipUnlessDarwin
@skipIfRemote
def test_compile_units(self):
program = self.getBuildArtifact("a.out")
@ -95,11 +87,10 @@ class TestVSCode_module(lldbvscode_testcase.VSCodeTestCaseBase):
lines = [breakpoint1_line]
breakpoint_ids = self.set_source_breakpoints(source, lines)
self.continue_to_breakpoints(breakpoint_ids)
moduleId = self.vscode.get_active_modules()['a.out']['id']
response = self.vscode.request_getCompileUnits(moduleId)
moduleId = self.vscode.get_modules()['a.out']['id']
response = self.vscode.request_compileUnits(moduleId)
self.assertTrue(response['body'])
self.assertTrue(len(response['body']['compileUnits']) == 1,
'Only one source file should exist')
self.assertTrue(response['body']['compileUnits'][0]['compileUnitPath'] == main_source_path,
'Real path to main.cpp matches')

View File

@ -355,11 +355,6 @@ void VSCode::SetTarget(const lldb::SBTarget target) {
lldb::SBTarget::eBroadcastBitBreakpointChanged);
listener.StartListeningForEvents(this->broadcaster,
eBroadcastBitStopEventThread);
listener.StartListeningForEvents(
this->target.GetBroadcaster(),
lldb::SBTarget::eBroadcastBitModulesLoaded |
lldb::SBTarget::eBroadcastBitModulesUnloaded |
lldb::SBTarget::eBroadcastBitSymbolsLoaded);
}
}

View File

@ -440,30 +440,6 @@ void EventThreadFunction() {
g_vsc.SendJSON(llvm::json::Value(std::move(bp_event)));
}
}
} else if (lldb::SBTarget::EventIsTargetEvent(event)) {
if (event_mask & lldb::SBTarget::eBroadcastBitModulesLoaded ||
event_mask & lldb::SBTarget::eBroadcastBitModulesUnloaded ||
event_mask & lldb::SBTarget::eBroadcastBitSymbolsLoaded) {
int num_modules = lldb::SBTarget::GetNumModulesFromEvent(event);
for (int i = 0; i < num_modules; i++) {
auto module = lldb::SBTarget::GetModuleAtIndexFromEvent(i, event);
auto module_event = CreateEventObject("module");
llvm::json::Value module_value = CreateModule(module);
llvm::json::Object body;
if (event_mask & lldb::SBTarget::eBroadcastBitModulesLoaded) {
body.try_emplace("reason", "new");
} else if (event_mask &
lldb::SBTarget::eBroadcastBitModulesUnloaded) {
body.try_emplace("reason", "removed");
} else if (event_mask &
lldb::SBTarget::eBroadcastBitSymbolsLoaded) {
body.try_emplace("reason", "changed");
}
body.try_emplace("module", module_value);
module_event.try_emplace("body", std::move(body));
g_vsc.SendJSON(llvm::json::Value(std::move(module_event)));
}
}
} else if (event.BroadcasterMatchesRef(g_vsc.broadcaster)) {
if (event_mask & eBroadcastBitStopEventThread) {
done = true;
@ -1196,26 +1172,26 @@ void request_evaluate(const llvm::json::Object &request) {
g_vsc.SendJSON(llvm::json::Value(std::move(response)));
}
// "getCompileUnitsRequest": {
// "compileUnitsRequest": {
// "allOf": [ { "$ref": "#/definitions/Request" }, {
// "type": "object",
// "description": "Compile Unit request; value of command field is
// 'getCompileUnits'.",
// 'compileUnits'.",
// "properties": {
// "command": {
// "type": "string",
// "enum": [ "getCompileUnits" ]
// "enum": [ "compileUnits" ]
// },
// "arguments": {
// "$ref": "#/definitions/getCompileUnitRequestArguments"
// "$ref": "#/definitions/compileUnitRequestArguments"
// }
// },
// "required": [ "command", "arguments" ]
// }]
// },
// "getCompileUnitsRequestArguments": {
// "compileUnitsRequestArguments": {
// "type": "object",
// "description": "Arguments for 'getCompileUnits' request.",
// "description": "Arguments for 'compileUnits' request.",
// "properties": {
// "moduleId": {
// "type": "string",
@ -1224,23 +1200,21 @@ void request_evaluate(const llvm::json::Object &request) {
// },
// "required": [ "moduleId" ]
// },
// "getCompileUnitsResponse": {
// "compileUnitsResponse": {
// "allOf": [ { "$ref": "#/definitions/Response" }, {
// "type": "object",
// "description": "Response to 'getCompileUnits' request.",
// "description": "Response to 'compileUnits' request.",
// "properties": {
// "body": {
// "description": "Response to 'getCompileUnits' request. Array of
// "description": "Response to 'compileUnits' request. Array of
// paths of compile units."
// }
// }
// }]
// }
void request_getCompileUnits(const llvm::json::Object &request) {
void request_compileUnits(const llvm::json::Object &request) {
llvm::json::Object response;
FillResponse(request, response);
lldb::SBProcess process = g_vsc.target.GetProcess();
llvm::json::Object body;
llvm::json::Array units;
auto arguments = request.getObject("arguments");
@ -1262,6 +1236,48 @@ void request_getCompileUnits(const llvm::json::Object &request) {
g_vsc.SendJSON(llvm::json::Value(std::move(response)));
}
// "modulesRequest": {
// "allOf": [ { "$ref": "#/definitions/Request" }, {
// "type": "object",
// "description": "Modules request; value of command field is
// 'modules'.",
// "properties": {
// "command": {
// "type": "string",
// "enum": [ "modules" ]
// },
// },
// "required": [ "command" ]
// }]
// },
// "modulesResponse": {
// "allOf": [ { "$ref": "#/definitions/Response" }, {
// "type": "object",
// "description": "Response to 'modules' request.",
// "properties": {
// "body": {
// "description": "Response to 'modules' request. Array of
// module objects."
// }
// }
// }]
// }
void request_modules(const llvm::json::Object &request) {
llvm::json::Object response;
FillResponse(request, response);
llvm::json::Array modules;
for (size_t i = 0; i < g_vsc.target.GetNumModules(); i++) {
lldb::SBModule module = g_vsc.target.GetModuleAtIndex(i);
modules.emplace_back(CreateModule(module));
}
llvm::json::Object body;
body.try_emplace("modules", std::move(modules));
response.try_emplace("body", std::move(body));
g_vsc.SendJSON(llvm::json::Value(std::move(response)));
}
// "InitializeRequest": {
// "allOf": [ { "$ref": "#/definitions/Request" }, {
// "type": "object",
@ -2901,7 +2917,6 @@ void RegisterRequestCallbacks() {
g_vsc.RegisterRequestCallback("disconnect", request_disconnect);
g_vsc.RegisterRequestCallback("evaluate", request_evaluate);
g_vsc.RegisterRequestCallback("exceptionInfo", request_exceptionInfo);
g_vsc.RegisterRequestCallback("getCompileUnits", request_getCompileUnits);
g_vsc.RegisterRequestCallback("initialize", request_initialize);
g_vsc.RegisterRequestCallback("launch", request_launch);
g_vsc.RegisterRequestCallback("next", request_next);
@ -2919,6 +2934,9 @@ void RegisterRequestCallbacks() {
g_vsc.RegisterRequestCallback("stepOut", request_stepOut);
g_vsc.RegisterRequestCallback("threads", request_threads);
g_vsc.RegisterRequestCallback("variables", request_variables);
// Custom requests
g_vsc.RegisterRequestCallback("compileUnits", request_compileUnits);
g_vsc.RegisterRequestCallback("modules", request_modules);
// Testing requests
g_vsc.RegisterRequestCallback("_testGetTargetBreakpoints",
request__testGetTargetBreakpoints);