This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc9a0754b443b: [lldb-vscode] Distinguish shadowed variables
in the scopes request (authored by wallace).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
Changed prior to commit:
https://reviews.llvm.org/D99989?vs=336945&id=339403#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99989/new/
https://reviews.llvm.org/D99989
Files:
lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py
lldb/test/API/tools/lldb-vscode/variables/main.cpp
lldb/tools/lldb-vscode/JSONUtils.cpp
lldb/tools/lldb-vscode/JSONUtils.h
lldb/tools/lldb-vscode/lldb-vscode.cpp
Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===================================================================
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -41,6 +41,7 @@
#include <set>
#include <sstream>
#include <thread>
+#include <unordered_map>
#include <vector>
#include "llvm/ADT/ArrayRef.h"
@@ -2716,7 +2717,9 @@
// 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 @@
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 @@
}
}
} 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 @@
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 @@
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<const char *, int> variable_name_counts;
+ for (auto i = start_idx; i < end_idx; ++i) {
+ lldb::SBValue variable = g_vsc.variables.GetValueAtIndex(i);
+ if (!variable.IsValid())
+ break;
+ 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));
+ 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
Index: lldb/tools/lldb-vscode/JSONUtils.h
===================================================================
--- lldb/tools/lldb-vscode/JSONUtils.h
+++ lldb/tools/lldb-vscode/JSONUtils.h
@@ -399,6 +399,14 @@
/// 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 @@
/// 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);
Index: lldb/tools/lldb-vscode/JSONUtils.cpp
===================================================================
--- lldb/tools/lldb-vscode/JSONUtils.cpp
+++ lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -17,6 +17,7 @@
#include "lldb/API/SBBreakpoint.h"
#include "lldb/API/SBBreakpointLocation.h"
+#include "lldb/API/SBDeclaration.h"
#include "lldb/API/SBValue.h"
#include "lldb/Host/PosixApi.h"
@@ -904,6 +905,25 @@
return llvm::json::Value(std::move(event));
}
+std::string CreateUniqueVariableNameForDisplay(lldb::SBValue v,
+ bool is_name_duplicated) {
+ lldb::SBStream name_builder;
+ const char *name = v.GetName();
+ name_builder.Print(name ? name : "<null>");
+ 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 @@
// "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 : "<null>");
+ EmplaceSafeString(object, "name",
+ CreateUniqueVariableNameForDisplay(v, is_name_duplicated));
+
if (format_hex)
v.SetFormat(lldb::eFormatHex);
SetValueForKey(v, object, "value");
Index: lldb/test/API/tools/lldb-vscode/variables/main.cpp
===================================================================
--- lldb/test/API/tools/lldb-vscode/variables/main.cpp
+++ lldb/test/API/tools/lldb-vscode/variables/main.cpp
@@ -14,5 +14,13 @@
PointType pt = { 11,22, {0}};
for (int i=0; i<BUFFER_SIZE; ++i)
pt.buffer[i] = i;
- return s_global - g_global - pt.y; // breakpoint 1
+ int x = s_global - g_global - pt.y; // breakpoint 1
+ {
+ int x = 42;
+ {
+ int x = 72;
+ s_global = x; // breakpoint 2
+ }
+ }
+ return 0; // breakpoint 3
}
Index: lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py
===================================================================
--- lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py
+++ lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py
@@ -110,6 +110,9 @@
'y': {'equals': {'type': 'int', 'value': '22'}},
'buffer': {'children': buffer_children}
}
+ },
+ 'x': {
+ 'equals': {'type': 'int'}
}
}
verify_globals = {
@@ -221,3 +224,61 @@
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)
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
===================================================================
--- lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
@@ -85,7 +85,6 @@
# 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:
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
