[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

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:

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 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)

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
 }

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");

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);
 

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