[Lldb-commits] [lldb] [lldb-dap] Updating VariableDescription to use GetDescription() as a fallback. (PR #77026)

John Harrison via lldb-commits lldb-commits at lists.llvm.org
Tue Jan 9 13:30:27 PST 2024


https://github.com/ashgti updated https://github.com/llvm/llvm-project/pull/77026

>From 7656af47e058aa7101504cb31aaa067178110351 Mon Sep 17 00:00:00 2001
From: John Harrison <harjohn at google.com>
Date: Thu, 4 Jan 2024 15:42:35 -0800
Subject: [PATCH 1/4] [lldb-dap] Updating VariableDescription to use
 GetDescription() as a fallback.

When generating a `display_value` for a variable the current approach calls
`SBValue::GetValue()` and `SBValue::GetSummary()` to generate a `display_value`
for the `SBValue`. However, there are cases where both of these return an empty
string and the fallback is to print a pointer and type name instead (e.g.
"FooBarType @ 0x00321").

For swift types, lldb includes a langauge runtime plugin that can generate a
user description of the object but this is only used with
`SBValue::GetDescription()`.

For example:
```
$ lldb swift-binary
... stop at breakpoint ...
lldb> script
>>> event = lldb.frame.GetValueForVariablePath("event")
>>> print("Value", event.GetValue())
Value None
>>> print("Summary", event.GetSummary())
Summary None
>>> print("Description", event)
Description (main.Event) event = (name = "Greetings", time = 2024-01-04 23:38:06 UTC)
```

With this change, if GetValue and GetSummary return empty then we try
`SBValue::GetDescription()` as a fallback before using the previous logic of
printing "<type> @ <addr>".
---
 lldb/tools/lldb-dap/JSONUtils.cpp | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index df17ac9d849176..f8ac53ef809e6e 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -1042,10 +1042,14 @@ VariableDescription::VariableDescription(lldb::SBValue v, bool format_hex,
         os_display_value << " " << *effective_summary;
     } else if (effective_summary) {
       os_display_value << *effective_summary;
-
-      // As last resort, we print its type and address if available.
     } else {
-      if (!raw_display_type_name.empty()) {
+      lldb::SBStream description;
+      // Try letting lldb generate a description.
+      if (v.GetDescription(description) && description.GetSize()) {
+        os_display_value << description.GetData();
+
+        // As last resort, we print its type and address if available.
+      } else if (!raw_display_type_name.empty()) {
         os_display_value << raw_display_type_name;
         lldb::addr_t address = v.GetLoadAddress();
         if (address != LLDB_INVALID_ADDRESS)

>From 4c3ad4b1c9363dc9fbaebe7aa005889e87ca5039 Mon Sep 17 00:00:00 2001
From: John Harrison <harjohn at google.com>
Date: Fri, 5 Jan 2024 16:48:27 -0800
Subject: [PATCH 2/4] Tweaking the format to remove trailing newlines and
 updating tests to cover the adjusted printing.

---
 .../lldb-dap/evaluate/TestDAP_evaluate.py     | 17 +++-
 .../lldb-dap/variables/TestDAP_variables.py   | 93 ++++++++++++++++---
 .../API/tools/lldb-dap/variables/main.cpp     |  2 +-
 lldb/tools/lldb-dap/JSONUtils.cpp             | 47 ++++++----
 4 files changed, 127 insertions(+), 32 deletions(-)

diff --git a/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py b/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py
index de9d2c93a1109d..d9e96b8219af34 100644
--- a/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py
+++ b/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py
@@ -2,6 +2,7 @@
 Test lldb-dap completions request
 """
 
+import re
 
 import lldbdap_testcase
 import dap_server
@@ -10,7 +11,7 @@
 from lldbsuite.test.lldbtest import *
 
 
-class TestDAP_variables(lldbdap_testcase.DAPTestCaseBase):
+class TestDAP_evaluate(lldbdap_testcase.DAPTestCaseBase):
     def assertEvaluate(self, expression, regex):
         self.assertRegexpMatches(
             self.dap_server.request_evaluate(expression, context=self.context)["body"][
@@ -60,7 +61,12 @@ def run_test_evaluate_expressions(
         self.assertEvaluate("static_int", "42")
         self.assertEvaluate("non_static_int", "43")
         self.assertEvaluate(
-            "struct1", "{foo:15}" if enableAutoVariableSummaries else "my_struct @ 0x"
+            "struct1",
+            re.escape(
+                "{foo:15}"
+                if enableAutoVariableSummaries
+                else "(my_struct) struct1 = (foo = 15)"
+            ),
         )
         self.assertEvaluate(
             "struct2", "0x.* {foo:16}" if enableAutoVariableSummaries else "0x.*"
@@ -96,7 +102,12 @@ def run_test_evaluate_expressions(
             "non_static_int", "10"
         )  # different variable with the same name
         self.assertEvaluate(
-            "struct1", "{foo:15}" if enableAutoVariableSummaries else "my_struct @ 0x"
+            "struct1",
+            re.escape(
+                "{foo:15}"
+                if enableAutoVariableSummaries
+                else "(my_struct) struct1 = (foo = 15)"
+            ),
         )
         self.assertEvaluate("struct1.foo", "15")
         self.assertEvaluate("struct2->foo", "16")
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 58d6b941ac81a7..16824827d14d91 100644
--- a/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
+++ b/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
@@ -3,6 +3,7 @@
 """
 
 import os
+import re
 
 import dap_server
 import lldbdap_testcase
@@ -39,7 +40,18 @@ def verify_values(self, verify_dict, actual, varref_dict=None, expression=None):
                 startswith = actual_value.startswith(verify_value)
                 self.assertTrue(
                     startswith,
-                    ('"%s" value "%s" doesn\'t start with' ' "%s")')
+                    ('"%s" value "%s" doesn\'t start with "%s")')
+                    % (key, actual_value, verify_value),
+                )
+        if "matches" in verify_dict:
+            verify = verify_dict["matches"]
+            for key in verify:
+                verify_value = verify[key]
+                actual_value = actual[key]
+                self.assertRegex(
+                    actual_value,
+                    verify_value,
+                    ('"%s" value "%s" doesn\'t match pattern "%s")')
                     % (key, actual_value, verify_value),
                 )
         if "contains" in verify_dict:
@@ -150,7 +162,7 @@ def do_test_scopes_variables_setVariable_evaluate(
         self.continue_to_breakpoints(breakpoint_ids)
         locals = self.dap_server.get_local_variables()
         globals = self.dap_server.get_global_variables()
-        buffer_children = make_buffer_verify_dict(0, 32)
+        buffer_children = make_buffer_verify_dict(0, 16)
         verify_locals = {
             "argc": {
                 "equals": {
@@ -242,19 +254,57 @@ def do_test_scopes_variables_setVariable_evaluate(
             },
             "pt": {
                 "equals": {"type": "PointType"},
-                "startswith": {
-                    "result": "{x:11, y:22}"
+                "equals": {
+                    "result": "{x:11, y:22, buffer:{...}}"
                     if enableAutoVariableSummaries
-                    else "PointType @ 0x"
+                    else """(PointType) pt = {
+  x = 11
+  y = 22
+  buffer = {
+    [0] = 0
+    [1] = 1
+    [2] = 2
+    [3] = 3
+    [4] = 4
+    [5] = 5
+    [6] = 6
+    [7] = 7
+    [8] = 8
+    [9] = 9
+    [10] = 10
+    [11] = 11
+    [12] = 12
+    [13] = 13
+    [14] = 14
+    [15] = 15
+  }
+}"""
                 },
                 "hasVariablesReference": True,
             },
             "pt.buffer": {
                 "equals": {"type": "int[32]"},
-                "startswith": {
+                "equals": {
                     "result": "{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, ...}"
                     if enableAutoVariableSummaries
-                    else "int[32] @ 0x"
+                    else """(int[16]) buffer = {
+  [0] = 0
+  [1] = 1
+  [2] = 2
+  [3] = 3
+  [4] = 4
+  [5] = 5
+  [6] = 6
+  [7] = 7
+  [8] = 8
+  [9] = 9
+  [10] = 10
+  [11] = 11
+  [12] = 12
+  [13] = 13
+  [14] = 14
+  [15] = 15
+}"""
                 },
                 "hasVariablesReference": True,
             },
@@ -440,7 +490,7 @@ def do_test_scopes_and_evaluate_expansion(self, enableAutoVariableSummaries: boo
                     },
                     "buffer": {
                         "children": buffer_children,
-                        "equals": {"indexedVariables": 32},
+                        "equals": {"indexedVariables": 16},
                     },
                 },
             },
@@ -457,10 +507,31 @@ def do_test_scopes_and_evaluate_expansion(self, enableAutoVariableSummaries: boo
             "name": "pt",
             "response": {
                 "equals": {"type": "PointType"},
-                "startswith": {
-                    "result": "{x:11, y:22}"
+                "matches": {
+                    "result": re.escape("{x:11, y:22, buffer:{...}}")
                     if enableAutoVariableSummaries
-                    else "PointType @ 0x"
+                    else r"""\(PointType\) (\$0|pt) = \{
+  x = 11
+  y = 22
+  buffer = \{
+    \[0\] = 0
+    \[1\] = 1
+    \[2\] = 2
+    \[3\] = 3
+    \[4\] = 4
+    \[5\] = 5
+    \[6\] = 6
+    \[7\] = 7
+    \[8\] = 8
+    \[9\] = 9
+    \[10\] = 10
+    \[11\] = 11
+    \[12\] = 12
+    \[13\] = 13
+    \[14\] = 14
+    \[15\] = 15
+  \}
+\}"""
                 },
                 "missing": ["indexedVariables"],
                 "hasVariablesReference": True,
diff --git a/lldb/test/API/tools/lldb-dap/variables/main.cpp b/lldb/test/API/tools/lldb-dap/variables/main.cpp
index da09ecb474f3b2..3557067ed9ce13 100644
--- a/lldb/test/API/tools/lldb-dap/variables/main.cpp
+++ b/lldb/test/API/tools/lldb-dap/variables/main.cpp
@@ -1,5 +1,5 @@
 
-#define BUFFER_SIZE 32
+#define BUFFER_SIZE 16
 struct PointType {
   int x;
   int y;
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index f8ac53ef809e6e..88f9df20b40409 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -135,6 +135,18 @@ std::vector<std::string> GetStrings(const llvm::json::Object *obj,
   return strs;
 }
 
+static std::string GetDescriptionTrimmed(lldb::SBValue &value) {
+  lldb::SBStream stream;
+  value.GetDescription(stream);
+  const char *description = stream.GetData();
+  size_t length = stream.GetSize();
+  if (length > 0 &&
+      (description[length - 1] == '\n' || description[length - 1] == '\r')) {
+    --length;
+  }
+  return std::string(description, length);
+}
+
 /// Create a short summary for a container that contains the summary of its
 /// first children, so that the user can get a glimpse of its contents at a
 /// glance.
@@ -173,21 +185,21 @@ TryCreateAutoSummaryForContainer(lldb::SBValue &v) {
     lldb::SBValue child = v.GetChildAtIndex(i);
 
     if (llvm::StringRef name = child.GetName(); !name.empty()) {
-      llvm::StringRef value;
+      llvm::StringRef desc;
       if (llvm::StringRef summary = child.GetSummary(); !summary.empty())
-        value = summary;
+        desc = summary;
+      else if (llvm::StringRef value = child.GetValue(); !value.empty())
+        desc = value;
       else
-        value = child.GetValue();
-
-      if (!value.empty()) {
-        // If the child is an indexed entry, we don't show its index to save
-        // characters.
-        if (name.starts_with("["))
-          os << separator << value;
-        else
-          os << separator << name << ":" << value;
-        separator = ", ";
-      }
+        desc = "{...}"; // Fallback for nested types.
+
+      // If the child is an indexed entry, we don't show its index to save
+      // characters.
+      if (name.starts_with("["))
+        os << separator << desc;
+      else
+        os << separator << name << ":" << desc;
+      separator = ", ";
     }
   }
   os << "}";
@@ -1043,10 +1055,11 @@ VariableDescription::VariableDescription(lldb::SBValue v, bool format_hex,
     } else if (effective_summary) {
       os_display_value << *effective_summary;
     } else {
-      lldb::SBStream description;
-      // Try letting lldb generate a description.
-      if (v.GetDescription(description) && description.GetSize()) {
-        os_display_value << description.GetData();
+      // Try the value description, which may call into language runtime
+      // specific formatters, see ValueObjectPrinter.
+      std::string description = GetDescriptionTrimmed(v);
+      if (!description.empty()) {
+        os_display_value << description;
 
         // As last resort, we print its type and address if available.
       } else if (!raw_display_type_name.empty()) {

>From 86b8fd1493c7a0df9f34613cfcf078609074313c Mon Sep 17 00:00:00 2001
From: John Harrison <harjohn at google.com>
Date: Tue, 9 Jan 2024 11:40:28 -0800
Subject: [PATCH 3/4] Ensuring the SBValue::GetDescription returns the correct
 value to inidicate if the description was actually detected.

---
 lldb/source/API/SBValue.cpp       | 13 +++++++------
 lldb/tools/lldb-dap/JSONUtils.cpp |  5 ++++-
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/lldb/source/API/SBValue.cpp b/lldb/source/API/SBValue.cpp
index 89d26a1fbe2824..b3e896dedabc0a 100644
--- a/lldb/source/API/SBValue.cpp
+++ b/lldb/source/API/SBValue.cpp
@@ -1210,15 +1210,16 @@ bool SBValue::GetDescription(SBStream &description) {
 
   ValueLocker locker;
   lldb::ValueObjectSP value_sp(GetSP(locker));
-  if (value_sp) {
-    DumpValueObjectOptions options;
-    options.SetUseDynamicType(m_opaque_sp->GetUseDynamic());
-    options.SetUseSyntheticValue(m_opaque_sp->GetUseSynthetic());
-    value_sp->Dump(strm, options);
-  } else {
+  if (!value_sp) {
     strm.PutCString("No value");
+    return false;
   }
 
+  DumpValueObjectOptions options;
+  options.SetUseDynamicType(m_opaque_sp->GetUseDynamic());
+  options.SetUseSyntheticValue(m_opaque_sp->GetUseSynthetic());
+  value_sp->Dump(strm, options);
+
   return true;
 }
 
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index 88f9df20b40409..b45d737cceb58a 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -137,7 +137,10 @@ std::vector<std::string> GetStrings(const llvm::json::Object *obj,
 
 static std::string GetDescriptionTrimmed(lldb::SBValue &value) {
   lldb::SBStream stream;
-  value.GetDescription(stream);
+  if (!value.GetDescription(stream)) {
+    return "";
+  }
+
   const char *description = stream.GetData();
   size_t length = stream.GetSize();
   if (length > 0 &&

>From f3d1412fe1f4b6641d100a628844db1a13996baf Mon Sep 17 00:00:00 2001
From: John Harrison <harjohn at google.com>
Date: Tue, 9 Jan 2024 13:30:11 -0800
Subject: [PATCH 4/4] Rename GetDescriptionTrimmed>TryCreateDescription and
 simplify the impl.

---
 lldb/tools/lldb-dap/JSONUtils.cpp | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index b45d737cceb58a..1d0790ff6cc113 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -135,19 +135,16 @@ std::vector<std::string> GetStrings(const llvm::json::Object *obj,
   return strs;
 }
 
-static std::string GetDescriptionTrimmed(lldb::SBValue &value) {
+/// Creates a description of the specified value. The description may be
+/// generated by the language runtime of the value, if one is detected
+/// and configured.
+static std::optional<std::string> TryCreateDescription(lldb::SBValue &value) {
   lldb::SBStream stream;
-  if (!value.GetDescription(stream)) {
-    return "";
-  }
+  if (!value.IsValid() || !value.GetDescription(stream))
+    return std::nullopt;
 
-  const char *description = stream.GetData();
-  size_t length = stream.GetSize();
-  if (length > 0 &&
-      (description[length - 1] == '\n' || description[length - 1] == '\r')) {
-    --length;
-  }
-  return std::string(description, length);
+  llvm::StringRef description = stream.GetData();
+  return description.trim().str();
 }
 
 /// Create a short summary for a container that contains the summary of its
@@ -1060,9 +1057,9 @@ VariableDescription::VariableDescription(lldb::SBValue v, bool format_hex,
     } else {
       // Try the value description, which may call into language runtime
       // specific formatters, see ValueObjectPrinter.
-      std::string description = GetDescriptionTrimmed(v);
-      if (!description.empty()) {
-        os_display_value << description;
+      std::optional<std::string> description = TryCreateDescription(v);
+      if (description) {
+        os_display_value << *description;
 
         // As last resort, we print its type and address if available.
       } else if (!raw_display_type_name.empty()) {



More information about the lldb-commits mailing list