[lldb-dap] Add readOnly attribute for variables

[DrSergei]

This patch adds readOnly attribute for variables. When this attribute is returned for a variable, VS Code prevents editing (and grays out the Set Value button). Without this, users might be confused if the UI allows edits but the debug adapter often returns an error. I checked SetValueFromCString function implementation and found that it doesn't support aggregate data types, so I added simple check for them. Also, I found that I can update value of registers sets (but without any effects). So I also added checks for those as well.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Druzhkov Sergei (DrSergei)

Changes

This patch adds readOnly attribute for variables. When this attribute is returned for a variable, VS Code prevents editing (and grays out the Set Value button). Without this, users might be confused if the UI allows edits but the debug adapter often returns an error. I checked SetValueFromCString function implementation and found that it doesn't support aggregate data types, so I added simple check for them. Also, I found that I can update value of registers sets (but without any effects). So I also added checks for those as well.

---

Full diff: https://github.com/llvm/llvm-project/pull/151884.diff

2 Files Affected:

• (modified) lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py (+26-9)
• (modified) lldb/tools/lldb-dap/ProtocolUtils.cpp (+8)

diff --git a/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py b/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
index a3a4bdaaf40a6..a55f080c3b71b 100644
--- a/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
+++ b/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
@@ -65,6 +65,11 @@ def verify_values(self, verify_dict, actual, varref_dict=None, expression=None):
                 self.assertNotIn(
                     key, actual, 'key "%s" is not expected in %s' % (key, actual)
                 )
+        isReadOnly = verify_dict.get("readOnly", False)
+        attributes = actual.get("presentationHint", {}).get("attributes", [])
+        self.assertEqual(
+            isReadOnly, "readOnly" in attributes, "%s %s" % (verify_dict, actual)
+        )
         hasVariablesReference = "variablesReference" in actual
         varRef = None
         if hasVariablesReference:
@@ -179,8 +184,9 @@ def do_test_scopes_variables_setVariable_evaluate(
                 "children": {
                     "x": {"equals": {"type": "int", "value": "11"}},
                     "y": {"equals": {"type": "int", "value": "22"}},
-                    "buffer": {"children": buffer_children},
+                    "buffer": {"children": buffer_children, "readOnly": True},
                 },
+                "readOnly": True,
             },
             "x": {"equals": {"type": "int"}},
         }
@@ -456,8 +462,10 @@ def do_test_scopes_and_evaluate_expansion(self, enableAutoVariableSummaries: boo
                     "buffer": {
                         "children": buffer_children,
                         "equals": {"indexedVariables": 16},
+                        "readOnly": True,
                     },
                 },
+                "readOnly": True,
             },
             "x": {
                 "equals": {"type": "int"},
@@ -540,7 +548,7 @@ def do_test_scopes_and_evaluate_expansion(self, enableAutoVariableSummaries: boo
             "children": {
                 "x": {"equals": {"type": "int", "value": "11"}},
                 "y": {"equals": {"type": "int", "value": "22"}},
-                "buffer": {"children": buffer_children},
+                "buffer": {"children": buffer_children, "readOnly": True},
             },
         }
 
@@ -634,11 +642,17 @@ def do_test_indexedVariables(self, enableSyntheticChildDebugging: bool):
         # "[raw]" child.
         raw_child_count = 1 if enableSyntheticChildDebugging else 0
         verify_locals = {
-            "small_array": {"equals": {"indexedVariables": 5}},
-            "large_array": {"equals": {"indexedVariables": 200}},
-            "small_vector": {"equals": {"indexedVariables": 5 + raw_child_count}},
-            "large_vector": {"equals": {"indexedVariables": 200 + raw_child_count}},
-            "pt": {"missing": ["indexedVariables"]},
+            "small_array": {"equals": {"indexedVariables": 5}, "readOnly": True},
+            "large_array": {"equals": {"indexedVariables": 200}, "readOnly": True},
+            "small_vector": {
+                "equals": {"indexedVariables": 5 + raw_child_count},
+                "readOnly": True,
+            },
+            "large_vector": {
+                "equals": {"indexedVariables": 200 + raw_child_count},
+                "readOnly": True,
+            },
+            "pt": {"missing": ["indexedVariables"], "readOnly": True},
         }
         self.verify_variables(verify_locals, locals)
 
@@ -652,7 +666,10 @@ def do_test_indexedVariables(self, enableSyntheticChildDebugging: bool):
             "[4]": {"equals": {"type": "int", "value": "0"}},
         }
         if enableSyntheticChildDebugging:
-            verify_children["[raw]"] = ({"contains": {"type": ["vector"]}},)
+            verify_children["[raw]"] = {
+                "contains": {"type": ["vector"]},
+                "readOnly": True,
+            }
 
         children = self.dap_server.request_variables(locals[2]["variablesReference"])[
             "body"
@@ -672,7 +689,7 @@ def test_return_variables(self):
             return_name: {"equals": {"type": "int", "value": "300"}},
             "argc": {},
             "argv": {},
-            "pt": {},
+            "pt": {"readOnly": True},
             "x": {},
             "return_result": {"equals": {"type": "int"}},
         }
diff --git a/lldb/tools/lldb-dap/ProtocolUtils.cpp b/lldb/tools/lldb-dap/ProtocolUtils.cpp
index 775c82fbb7716..868c67ca72986 100644
--- a/lldb/tools/lldb-dap/ProtocolUtils.cpp
+++ b/lldb/tools/lldb-dap/ProtocolUtils.cpp
@@ -301,6 +301,14 @@ Variable CreateVariable(lldb::SBValue v, int64_t var_ref, bool format_hex,
   if (lldb::addr_t addr = v.GetLoadAddress(); addr != LLDB_INVALID_ADDRESS)
     var.memoryReference = addr;
 
+  bool is_readonly = v.GetType().IsAggregateType() ||
+                     v.GetValueType() == lldb::eValueTypeRegisterSet;
+  if (is_readonly) {
+    if (!var.presentationHint)
+      var.presentationHint = {VariablePresentationHint()};
+    var.presentationHint->attributes.push_back("readOnly");
+  }
+
   return var;
 }
 

[walter-erquinigo]

This is not the right place for this check. Instead, add a method in the SBType class that does it. A possible name is IsWritable, and invoke it from here.

[labath]

If we're going to be implementing this in the SB API, then I don't think SBType is the right place for it. It should probably be a property of SBValue, as one can easily imagine situations where a value is not modifiable, even though another value of the same type is.

[DrSergei]

I agree with you, for example, a const global integer variable and a const local integer variable. I don't want to add anything to the SB API because there are a lot of corner cases (and also it looks like SetData has less preconditions than SetValueFromCString, so we need two different versions of IsWritable for both functions). I added simple heuristics at the lldb-dap level to avoid confusing users in most cases. CodeLLDB uses different heuristics for that. I think the better solution is to place the check here.

[walter-erquinigo]

yeah, i'm fine with this

[walter-erquinigo]

yeah, i'm fine with this

[DrSergei]

@JDevlieghere friendly ping

[walter-erquinigo]

lgtm!

[DrSergei]

@JDevlieghere Please take a look If you have time