[lldb-vscode] Distinguish shadowed variables in the scopes request
authorWalter Erquinigo <a20012251@gmail.com>
Wed, 21 Apr 2021 22:06:44 +0000 (15:06 -0700)
committerWalter Erquinigo <a20012251@gmail.com>
Wed, 21 Apr 2021 22:09:39 +0000 (15:09 -0700)
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
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 5f5e4b4..0a55fc0 100644 (file)
@@ -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:
index 12fa3e1..765cfbe 100644 (file)
@@ -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)
index 0223bd0..6ec5df9 100644 (file)
@@ -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
 }
index 3a92407..6894ec0 100644 (file)
@@ -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");
index 6b150be..fa640e7 100644 (file)
@@ -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);
 
index b282274..b810bf7 100644 (file)
@@ -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