[lldb-vscode] Distinguish shadowed variables in the scopes request

[wallace]

Summary

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

Without shadowed variables

Modifying these variables through the UI works

[clayborg]

I think this really should be fixed in the VS code variable view. It allows debugging of languages that support having multiple variables with the same name, so their debugger should support it.

But that won't happen right away, so I added some suggestions to make this better using SBDeclaration

lldb/tools/lldb-vscode/JSONUtils.cpp

994–998

We could use the data in the SBValue's declaration:

lldb::SBDeclaration SBValue::GetDeclaration();

Something like:

lldb::SBStream name_builder;
name_builder.Print(name ? name : "<null>");
if (name_repeats > 1) {
  name_builder.Print(" @ ")
  v.GetDeclaration().GetDescription(name_builder);
}
EmplaceSafeString(object, "name", name_builder.GetData());

Then the name would look something like "i @ main.cpp:12"

997–998

Can we just add spaces after the variable name here? So the first would be "i", the next would be "i " and the next would be "i ". We need to make sure that we use the real name for the "evaluateName" key:

[
  {"evaluateName":"i","id":1,"name":"i","type":"int","value":"0","variablesReference":0},
  {"evaluateName":"i","id":2,"name":"i ","type":"int","value":"0","variablesReference":0},
  {"evaluateName":"i","id":3,"name":"i  ","type":"int","value":"0","variablesReference":0},
]

[wallace]

Some changes:

• Now adding the source + line information to distinguish variables, and using the address as fallback, which should happen pretty rarely.
• Now also supporting modifying these variables via request_setVariable. Before we could only modify the innermost variable, now we can change any variable due to uniquing.
• Added more comprehensive tests

[clayborg]

lldb/tools/lldb-vscode/JSONUtils.cpp

922

variables don't always have a load address, as they can be in a register. SBValue objects can return their location to you as a string already:

const char *SBValue::GetLocation();

lldb/tools/lldb-vscode/lldb-vscode.cpp

2934

This variable name can be improved to accurately describe what it is doing. "variable_name_counts" would be my suggestion since that is what it is really doing.

You can also just make the key of this map "const char *" since the variable names come from ConstString values inside LLDB the pointers will be unique

[wallace]

Address comments

[clayborg]

Just one inline fix switch from int to uint32_t and this is good to go.

lldb/tools/lldb-vscode/JSONUtils.cpp

917

This should be uint32_t instead of int, and probably add "const uint32_t" as well.

[teemperor]

lldb/tools/lldb-vscode/lldb-vscode.cpp

2934

Let's not. The last time someone did this whole "This const char * is actually coming from a ConstString so assume ConstString semantics" it just caused us a bunch of pain (see D88490 and the related revisions).

Either add an assert that the strings are coming from a ConstString (assuming we really care about the performance benefit here) or use some StringMap/std::string-key approach.

[clayborg]

lldb/tools/lldb-vscode/lldb-vscode.cpp

2934

We are getting a variable name via the public API which returns a "const char *". The only way this works is if the string comes from a ConstString otherwise there are lifetime issues. So this is and should be ok. The diff you are talking about was from internal LLDB code. For internal LLDB code we can enforce this by ensuring that people use ConstString, but externally through the API, we assume any "const char *" that comes out of it is uniqued and comes form a ConstString.

[teemperor]

lldb/tools/lldb-vscode/lldb-vscode.cpp

2934

I don't think there is any promise that every const char * coming from the SB API is generated from a ConstString. If there was such a promise a good chunk of the SB API would already violate it.

That the const char * needs to have a reasonable life time (e.g., the same life time as the SB API object that returns it) seems like a basic foundation to have the API work at all. But promising that they are uniquified really serves essentially no purpose? I sure hope that simply returning string literals doesn't suddenly become an API violation (or that we need to forever store every generated error message that we return from the SB API).

Also I think my example is spot on: An unverified assumption that doesn't hold true. And once we change the underlying code someone has to spend a week tracking down bogus string comparisons in the year 2021.

[wallace]

lldb/tools/lldb-vscode/lldb-vscode.cpp

2934

The number of variables shouldn't be massive, so I'll change it to std::string just for safety. I don't want to spend a lot of time debugging this if at some point the assumption doensn't hold.

lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py

def verify_breakpoint_hit(self, breakpoint_ids):
                 # "breakpoint 1.1", so look for any description that matches
                 # ("breakpoint 1.") in the description field as verification
                 # that one of the breakpoint locations was hit. VSCode doesn't
                 # allow breakpoints to have multiple locations, but LLDB does.
                 # So when looking at the description we just want to make sure
                 # 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:
                         return
         self.assertTrue(False, "breakpoint not hit")
 
     def verify_exception_breakpoint_hit(self, filter_label):
         '''Wait for the process we are debugging to stop, and verify the stop

lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py

def test_scopes_variables_setVariable_evaluate(self):
             'pt': {
                 'equals': {'type': 'PointType'},
                 'hasVariablesReference': True,
                 'children': {
                     'x': {'equals': {'type': 'int', 'value': '11'}},
                     'y': {'equals': {'type': 'int', 'value': '22'}},
                     'buffer': {'children': buffer_children}
                 }
+            },
+            'x': {
+                'equals': {'type': 'int'}
             }
         }
         verify_globals = {
             's_local': {
                 'equals': {'type': 'float', 'value': '2.25'}
             },
             '::g_global': {
                 'equals': {'type': 'int', 'value': '123'}
def test_scopes_variables_setVariable_evaluate(self):
         # Set a variable value whose name is a real child value, like "pt.x"
         # and verify the value by reading it
         varRef = varref_dict['pt']
         self.vscode.request_setVariable(varRef, "x", 111)
         response = self.vscode.request_variables(varRef, start=0, count=1)
         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

 
 #define BUFFER_SIZE 32
 struct PointType {
   int x;
   int y;
   int buffer[BUFFER_SIZE];
 };
 
 int g_global = 123;
 static int s_global = 234;
 
 int main(int argc, char const *argv[]) {
   static float s_local = 2.25;
   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.h

 ///     The LLDB thread to use when populating out the "StoppedEvent"
 ///     object.
 ///
 /// \return
 ///     A "StoppedEvent" JSON object with that follows the formal JSON
 ///     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
 /// object:
 ///   "name" - the name of the variable
 ///   "value" - the value of the variable as a string
 ///   "type" - the typename of the variable as a string
 ///   "id" - a unique identifier for a value in case there are multiple

 ///     A unique variable identifier to help in properly identifying
 ///     variables with the same name. This is an extension to the
 ///     VS protocol.
 ///
 /// \param[in] format_hex
 ///     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);
 
 /// Create a runInTerminal reverse request object
 ///
 /// \param[in] launch_request
 ///     The original launch_request object whose fields are used to construct
 ///     the reverse request object.

lldb/tools/lldb-vscode/JSONUtils.cpp

 
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/FormatAdapters.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
 
 #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"
 
 #include "ExceptionBreakpoint.h"
 #include "JSONUtils.h"
 #include "LLDBUtils.h"
 #include "VSCode.h"
 
if (tid == g_vsc.focus_tid) {
     body.try_emplace("threadCausedFocus", true);
   }
   body.try_emplace("preserveFocusHint", tid != g_vsc.focus_tid);
   body.try_emplace("allThreadsStopped", true);
   event.try_emplace("body", std::move(body));
   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
 //                   can have a 'type' that is shown if space permits or when
 //                   hovering over the variable's name. An optional 'kind' is
 //                   used to render additional properties of the variable,
 //                   e.g. different icons can be used to indicate that a
 //                   variable is public or private. If the value is

 //       "description": "The number of indexed child variables. The client
 //                       can use this optional information to present the
 //                       children in a paged UI and fetch them in chunks."
 //     }
 //   },
 //   "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");
   auto type_cstr = v.GetType().GetDisplayTypeName();
   EmplaceSafeString(object, "type", type_cstr ? type_cstr : NO_TYPENAME);
   if (varID != INT64_MAX)
     object.try_emplace("id", varID);
   if (v.MightHaveChildren())
     object.try_emplace("variablesReference", variablesReference);
   else
     object.try_emplace("variablesReference", (int64_t)0);

lldb/tools/lldb-vscode/lldb-vscode.cpp

 #include <chrono>
 #include <fstream>
 #include <map>
 #include <memory>
 #include <mutex>
 #include <set>
 #include <sstream>
 #include <thread>
+#include <unordered_map>
 #include <vector>
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/Errno.h"
void request_setVariable(const llvm::json::Object &request) {
   llvm::json::Object response;
   FillResponse(request, response);
   llvm::json::Array variables;
   llvm::json::Object body;
   auto arguments = request.getObject("arguments");
   // 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);
 
   lldb::SBValue variable;
   int64_t newVariablesReference = 0;
 
   // The "id" is the unique integer ID that is unique within the enclosing
if (id_value != UINT64_MAX) {
     case VARREF_REGS:
       start_idx = g_vsc.num_locals + g_vsc.num_globals;
       end_idx = start_idx + g_vsc.num_regs;
       break;
     default:
       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())
           newVariablesReference = i;
         break;
       }
     }
   } 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);
     lldb::SBValue container = g_vsc.variables.GetValueAtIndex(var_idx);
     variable = container.GetChildMemberWithName(name.data());
     if (!variable.IsValid()) {
       if (name.startswith("[")) {
         llvm::StringRef index_str(name.drop_front(1));
if (variable.IsValid()) {
     if (success) {
       SetValueForKey(variable, body, "value");
       EmplaceSafeString(body, "type", variable.GetType().GetDisplayTypeName());
       body.try_emplace("variablesReference", newVariablesReference);
     } else {
       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));
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
 }
 
 // "VariablesRequest": {
 //   "allOf": [ { "$ref": "#/definitions/Request" }, {
if (VARREF_IS_SCOPE(variablesReference)) {
     case VARREF_REGS:
       start_idx = start + g_vsc.num_locals + g_vsc.num_globals;
       num_children = g_vsc.num_regs;
       break;
     default:
       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;
-      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
     // children.
     const int64_t var_idx = VARREF_TO_VARIDX(variablesReference);
     lldb::SBValue variable = g_vsc.variables.GetValueAtIndex(var_idx);
     if (variable.IsValid()) {
       const auto num_children = variable.GetNumChildren();