diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py index 5f5e4b402107..0a55fc0ead1e 100644 --- a/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py +++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py @@ -85,7 +85,6 @@ class VSCodeTestCaseBase(TestBase): # the right breakpoint matches and not worry about the actual # location. description = body['description'] - print("description: %s" % (description)) for breakpoint_id in breakpoint_ids: match_desc = 'breakpoint %s.' % (breakpoint_id) if match_desc in description: diff --git a/lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py b/lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py index 12fa3e1d3057..765cfbe6ed5a 100644 --- a/lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py +++ b/lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py @@ -110,6 +110,9 @@ class TestVSCode_variables(lldbvscode_testcase.VSCodeTestCaseBase): 'y': {'equals': {'type': 'int', 'value': '22'}}, 'buffer': {'children': buffer_children} } + }, + 'x': { + 'equals': {'type': 'int'} } } verify_globals = { @@ -221,3 +224,61 @@ class TestVSCode_variables(lldbvscode_testcase.VSCodeTestCaseBase): value = response['body']['variables'][0]['value'] self.assertEqual(value, '111', 'verify pt.x got set to 111 (111 != %s)' % (value)) + + # We check shadowed variables and that a new get_local_variables request + # gets the right data + breakpoint2_line = line_number(source, '// breakpoint 2') + lines = [breakpoint2_line] + breakpoint_ids = self.set_source_breakpoints(source, lines) + self.assertEqual(len(breakpoint_ids), len(lines), + "expect correct number of breakpoints") + self.continue_to_breakpoints(breakpoint_ids) + + verify_locals['argc']['equals']['value'] = '123' + verify_locals['pt']['children']['x']['equals']['value'] = '111' + verify_locals['x @ main.cpp:17'] = {'equals': {'type': 'int', 'value': '89'}} + verify_locals['x @ main.cpp:19'] = {'equals': {'type': 'int', 'value': '42'}} + verify_locals['x @ main.cpp:21'] = {'equals': {'type': 'int', 'value': '72'}} + + self.verify_variables(verify_locals, self.vscode.get_local_variables()) + + # Now we verify that we correctly change the name of a variable with and without differentiator suffix + self.assertFalse(self.vscode.request_setVariable(1, "x2", 9)['success']) + self.assertFalse(self.vscode.request_setVariable(1, "x @ main.cpp:0", 9)['success']) + + self.assertTrue(self.vscode.request_setVariable(1, "x @ main.cpp:17", 17)['success']) + self.assertTrue(self.vscode.request_setVariable(1, "x @ main.cpp:19", 19)['success']) + self.assertTrue(self.vscode.request_setVariable(1, "x @ main.cpp:21", 21)['success']) + + # The following should have no effect + self.assertFalse(self.vscode.request_setVariable(1, "x @ main.cpp:21", "invalid")['success']) + + verify_locals['x @ main.cpp:17']['equals']['value'] = '17' + verify_locals['x @ main.cpp:19']['equals']['value'] = '19' + verify_locals['x @ main.cpp:21']['equals']['value'] = '21' + + self.verify_variables(verify_locals, self.vscode.get_local_variables()) + + # The plain x variable shold refer to the innermost x + self.assertTrue(self.vscode.request_setVariable(1, "x", 22)['success']) + verify_locals['x @ main.cpp:21']['equals']['value'] = '22' + + self.verify_variables(verify_locals, self.vscode.get_local_variables()) + + # In breakpoint 3, there should be no shadowed variables + breakpoint3_line = line_number(source, '// breakpoint 3') + lines = [breakpoint3_line] + breakpoint_ids = self.set_source_breakpoints(source, lines) + self.assertEqual(len(breakpoint_ids), len(lines), + "expect correct number of breakpoints") + self.continue_to_breakpoints(breakpoint_ids) + + locals = self.vscode.get_local_variables() + names = [var['name'] for var in locals] + # The first shadowed x shouldn't have a suffix anymore + verify_locals['x'] = {'equals': {'type': 'int', 'value': '17'}} + self.assertNotIn('x @ main.cpp:17', names) + self.assertNotIn('x @ main.cpp:19', names) + self.assertNotIn('x @ main.cpp:21', names) + + self.verify_variables(verify_locals, locals) diff --git a/lldb/test/API/tools/lldb-vscode/variables/main.cpp b/lldb/test/API/tools/lldb-vscode/variables/main.cpp index 0223bd0a75ea..6ec5df906ba7 100644 --- a/lldb/test/API/tools/lldb-vscode/variables/main.cpp +++ b/lldb/test/API/tools/lldb-vscode/variables/main.cpp @@ -14,5 +14,13 @@ int main(int argc, char const *argv[]) { PointType pt = { 11,22, {0}}; for (int i=0; i"); + if (is_name_duplicated) { + name_builder.Print(" @ "); + lldb::SBDeclaration declaration = v.GetDeclaration(); + std::string file_name(declaration.GetFileSpec().GetFilename()); + const uint32_t line = declaration.GetLine(); + + if (!file_name.empty() && line > 0) + name_builder.Printf("%s:%u", file_name.c_str(), line); + else + name_builder.Print(v.GetLocation()); + } + return name_builder.GetData(); +} + // "Variable": { // "type": "object", // "description": "A Variable is a name/value pair. Optionally a variable @@ -967,10 +987,12 @@ llvm::json::Value CreateThreadStopped(lldb::SBThread &thread, // "required": [ "name", "value", "variablesReference" ] // } llvm::json::Value CreateVariable(lldb::SBValue v, int64_t variablesReference, - int64_t varID, bool format_hex) { + int64_t varID, bool format_hex, + bool is_name_duplicated) { llvm::json::Object object; - auto name = v.GetName(); - EmplaceSafeString(object, "name", name ? name : ""); + EmplaceSafeString(object, "name", + CreateUniqueVariableNameForDisplay(v, is_name_duplicated)); + if (format_hex) v.SetFormat(lldb::eFormatHex); SetValueForKey(v, object, "value"); diff --git a/lldb/tools/lldb-vscode/JSONUtils.h b/lldb/tools/lldb-vscode/JSONUtils.h index 6b150be8a4bf..fa640e7c5c91 100644 --- a/lldb/tools/lldb-vscode/JSONUtils.h +++ b/lldb/tools/lldb-vscode/JSONUtils.h @@ -399,6 +399,14 @@ llvm::json::Value CreateThread(lldb::SBThread &thread); /// definition outlined by Microsoft. llvm::json::Value CreateThreadStopped(lldb::SBThread &thread, uint32_t stop_id); +/// VSCode can't display two variables with the same name, so we need to +/// distinguish them by using a suffix. +/// +/// If the source and line information is present, we use it as the suffix. +/// Otherwise, we fallback to the variable address or register location. +std::string CreateUniqueVariableNameForDisplay(lldb::SBValue v, + bool is_name_duplicated); + /// Create a "Variable" object for a LLDB thread object. /// /// This function will fill in the following keys in the returned @@ -435,11 +443,20 @@ llvm::json::Value CreateThreadStopped(lldb::SBThread &thread, uint32_t stop_id); /// It set to true the variable will be formatted as hex in /// the "value" key value pair for the value of the variable. /// +/// \param[in] is_name_duplicated +/// Whether the same variable name appears multiple times within the same +/// context (e.g. locals). This can happen due to shadowed variables in +/// nested blocks. +/// +/// As VSCode doesn't render two of more variables with the same name, we +/// apply a suffix to distinguish duplicated variables. +/// /// \return /// A "Variable" JSON object with that follows the formal JSON /// definition outlined by Microsoft. llvm::json::Value CreateVariable(lldb::SBValue v, int64_t variablesReference, - int64_t varID, bool format_hex); + int64_t varID, bool format_hex, + bool is_name_duplicated = false); llvm::json::Value CreateCompileUnit(lldb::SBCompileUnit unit); diff --git a/lldb/tools/lldb-vscode/lldb-vscode.cpp b/lldb/tools/lldb-vscode/lldb-vscode.cpp index b28227429bed..b810bf738526 100644 --- a/lldb/tools/lldb-vscode/lldb-vscode.cpp +++ b/lldb/tools/lldb-vscode/lldb-vscode.cpp @@ -41,6 +41,7 @@ #include #include #include +#include #include #include "llvm/ADT/ArrayRef.h" @@ -2716,7 +2717,9 @@ void request_setVariable(const llvm::json::Object &request) { // This is a reference to the containing variable/scope const auto variablesReference = GetUnsigned(arguments, "variablesReference", 0); - const auto name = GetString(arguments, "name"); + llvm::StringRef name = GetString(arguments, "name"); + bool is_duplicated_variable_name = name.find(" @") != llvm::StringRef::npos; + const auto value = GetString(arguments, "value"); // Set success to false just in case we don't find the variable by name response.try_emplace("success", false); @@ -2758,14 +2761,10 @@ void request_setVariable(const llvm::json::Object &request) { break; } - // Find the variable by name in the correct scope and hope we don't have - // multiple variables with the same name. We search backwards because - // the list of variables has the top most variables first and variables - // in deeper scopes are last. This means we will catch the deepest - // variable whose name matches which is probably what the user wants. for (int64_t i = end_idx - 1; i >= start_idx; --i) { - auto curr_variable = g_vsc.variables.GetValueAtIndex(i); - llvm::StringRef variable_name(curr_variable.GetName()); + lldb::SBValue curr_variable = g_vsc.variables.GetValueAtIndex(i); + std::string variable_name = CreateUniqueVariableNameForDisplay( + curr_variable, is_duplicated_variable_name); if (variable_name == name) { variable = curr_variable; if (curr_variable.MightHaveChildren()) @@ -2774,6 +2773,9 @@ void request_setVariable(const llvm::json::Object &request) { } } } else { + // This is not under the globals or locals scope, so there are no duplicated + // names. + // We have a named item within an actual variable so we need to find it // withing the container variable by name. const int64_t var_idx = VARREF_TO_VARIDX(variablesReference); @@ -2810,6 +2812,8 @@ void request_setVariable(const llvm::json::Object &request) { EmplaceSafeString(body, "message", std::string(error.GetCString())); } response["success"] = llvm::json::Value(success); + } else { + response["success"] = llvm::json::Value(false); } response.try_emplace("body", std::move(body)); @@ -2925,12 +2929,26 @@ void request_variables(const llvm::json::Object &request) { break; } const int64_t end_idx = start_idx + ((count == 0) ? num_children : count); + + // We first find out which variable names are duplicated + std::unordered_map variable_name_counts; for (auto i = start_idx; i < end_idx; ++i) { lldb::SBValue variable = g_vsc.variables.GetValueAtIndex(i); if (!variable.IsValid()) break; - variables.emplace_back( - CreateVariable(variable, VARIDX_TO_VARREF(i), i, hex)); + variable_name_counts[variable.GetName()]++; + } + + // Now we construct the result with unique display variable names + for (auto i = start_idx; i < end_idx; ++i) { + lldb::SBValue variable = g_vsc.variables.GetValueAtIndex(i); + const char *name = variable.GetName(); + + if (!variable.IsValid()) + break; + variables.emplace_back(CreateVariable(variable, VARIDX_TO_VARREF(i), i, + hex, + variable_name_counts[name] > 1)); } } else { // We are expanding a variable that has children, so we will return its