[Lldb-commits] [lldb] c9a0754 - [lldb-vscode] Distinguish shadowed variables in the scopes request
Walter Erquinigo via lldb-commits
lldb-commits at lists.llvm.org
Wed Apr 21 15:09:45 PDT 2021
Author: Walter Erquinigo
Date: 2021-04-21T15:09:39-07:00
New Revision: c9a0754b443ba73623574db880a7b0b14826fedb
URL: https://github.com/llvm/llvm-project/commit/c9a0754b443ba73623574db880a7b0b14826fedb
DIFF: https://github.com/llvm/llvm-project/commit/c9a0754b443ba73623574db880a7b0b14826fedb.diff
LOG: [lldb-vscode] Distinguish shadowed variables in the scopes request
VSCode doesn't render multiple variables with the same name in the variables view. It only renders one of them. This is a situation that happens often when there are shadowed variables.
The nodejs debugger solves this by adding a number suffix to the variable, e.g. "x", "x2", "x3" are the different x variables in nested blocks.
In this patch I'm doing something similar, but the suffix is " @ <file_name:line>), e.g. "x @ main.cpp:17", "x @ main.cpp:21". The fallback would be an address if the source and line information is not present, which should be rare.
This fix is only needed for globals and locals. Children of variables don't suffer of this problem.
When there are shadowed variables
{F16182150}
Without shadowed variables
{F16182152}
Modifying these variables through the UI works
Reviewed By: clayborg
Differential Revision: https://reviews.llvm.org/D99989
Added:
Modified:
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
Removed:
################################################################################
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 5f5e4b4021073..0a55fc0ead1e4 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 @@ def verify_breakpoint_hit(self, breakpoint_ids):
# 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 12fa3e1d30572..765cfbe6ed5a1 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 @@ def test_scopes_variables_setVariable_evaluate(self):
'y': {'equals': {'type': 'int', 'value': '22'}},
'buffer': {'children': buffer_children}
}
+ },
+ 'x': {
+ 'equals': {'type': 'int'}
}
}
verify_globals = {
@@ -221,3 +224,61 @@ def test_scopes_variables_setVariable_evaluate(self):
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
diff erentiator 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 0223bd0a75ea8..6ec5df906ba7c 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<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
}
diff --git a/lldb/tools/lldb-vscode/JSONUtils.cpp b/lldb/tools/lldb-vscode/JSONUtils.cpp
index 3a9240749a7ff..6894ec0fff839 100644
--- a/lldb/tools/lldb-vscode/JSONUtils.cpp
+++ b/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 @@ llvm::json::Value CreateThreadStopped(lldb::SBThread &thread,
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 @@ 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 : "<null>");
+ 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 6b150be8a4bf7..fa640e7c5c91c 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 b28227429bed3..b810bf7385267 100644
--- a/lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ b/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 @@ 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<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
More information about the lldb-commits
mailing list