[lldb-vscode] Use auto summaries whenever variables don't have a summary

[walter-erquinigo]

Auto summaries were only being used when non-pointer/reference variables didn't have values nor summaries. Greg pointed out that it should be better to simply use auto summaries when the variable doesn't have a summary of its own, regardless of other conditions.

This led to code simplification and correct visualization of auto summaries for pointer/reference types, as seen in this screenshot.

[llvmbot]

@llvm/pr-subscribers-lldb

Changes

enableAutoVariableSummaries is a setting that enhances the summaries of variables in the IDE. A shortcoming of this feature is that it wasn't showing the addresses of pointers, which is valuable information. This fixes it by adding it whenever applicable.

An example of the proposal:
<img width="260" alt="Screenshot 2023-09-15 at 5 15 31 PM" src="https://github.com/llvm/llvm-project/assets/1613874/b55c8dbc-9b6b-40f3-8cba-3bee4a1237a9">

---

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

3 Files Affected:

• (modified) lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py (+4-1)
• (modified) lldb/test/API/tools/lldb-vscode/evaluate/main.cpp (+2-1)
• (modified) lldb/tools/lldb-vscode/JSONUtils.cpp (+30-21)

diff --git a/lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py b/lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py
index 3cfe02ef6aa1576..bf3b16067fed2fa 100644
--- a/lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py
+++ b/lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py
@@ -63,7 +63,10 @@ def run_test_evaluate_expressions(
             "struct1", "{foo:15}" if enableAutoVariableSummaries else "my_struct @ 0x"
         )
         self.assertEvaluate(
-            "struct2", "{foo:16}" if enableAutoVariableSummaries else "0x.*"
+            "struct2", "0x.* → {foo:16}" if enableAutoVariableSummaries else "0x.*"
+        )
+        self.assertEvaluate(
+            "struct3", "0x.*0"
         )
         self.assertEvaluate("struct1.foo", "15")
         self.assertEvaluate("struct2->foo", "16")
diff --git a/lldb/test/API/tools/lldb-vscode/evaluate/main.cpp b/lldb/test/API/tools/lldb-vscode/evaluate/main.cpp
index 3a541b21b220828..f09d00e6444bb79 100644
--- a/lldb/test/API/tools/lldb-vscode/evaluate/main.cpp
+++ b/lldb/test/API/tools/lldb-vscode/evaluate/main.cpp
@@ -18,6 +18,7 @@ struct my_struct {
 int main(int argc, char const *argv[]) {
   my_struct struct1 = {15};
   my_struct *struct2 = new my_struct{16};
+  my_struct *struct3 = nullptr;
   int var1 = 20;
   int var2 = 21;
   int var3 = static_int; // breakpoint 1
@@ -43,6 +44,6 @@ int main(int argc, char const *argv[]) {
   my_bool_vec.push_back(true);
   my_bool_vec.push_back(false); // breakpoint 6
   my_bool_vec.push_back(true); // breakpoint 7
-  
+
   return 0;
 }
diff --git a/lldb/tools/lldb-vscode/JSONUtils.cpp b/lldb/tools/lldb-vscode/JSONUtils.cpp
index c6b422e4d7a02e6..d475f45bec459bb 100644
--- a/lldb/tools/lldb-vscode/JSONUtils.cpp
+++ b/lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -203,10 +203,12 @@ static bool ShouldBeDereferencedForSummary(lldb::SBValue &v) {
   if (!v.GetType().IsPointerType() && !v.GetType().IsReferenceType())
     return false;
 
-  // If we are referencing a pointer, we don't dereference to avoid confusing
-  // the user with the addresses that could shown in the summary.
-  if (v.Dereference().GetType().IsPointerType())
-    return false;
+  // We don't want to dereference invalid data.
+  if (!v.IsSynthetic()) {
+    lldb::addr_t address = v.GetValueAsUnsigned(0);
+    if (address == 0 && address == LLDB_INVALID_ADDRESS)
+      return false;
+  }
 
   // If it's synthetic or a pointer to a basic type that provides a summary, we
   // don't dereference.
@@ -227,33 +229,40 @@ void SetValueForKey(lldb::SBValue &v, llvm::json::Object &object,
   if (!error.Success()) {
     strm << "<error: " << error.GetCString() << ">";
   } else {
-    auto tryDumpSummaryAndValue = [&strm](lldb::SBValue value) {
+    auto tryDumpSummaryAndValue =
+        [](lldb::SBValue value) -> std::optional<std::string> {
       llvm::StringRef val = value.GetValue();
       llvm::StringRef summary = value.GetSummary();
       if (!val.empty()) {
-        strm << val;
+        std::string dump;
+        llvm::raw_string_ostream os(dump);
+        os << val;
         if (!summary.empty())
-          strm << ' ' << summary;
-        return true;
+          os << ' ' << summary;
+        return dump;
       }
-      if (!summary.empty()) {
-        strm << ' ' << summary;
-        return true;
-      }
-      if (auto container_summary = GetSyntheticSummaryForContainer(value)) {
-        strm << *container_summary;
-        return true;
-      }
-      return false;
+      if (!summary.empty())
+        return summary.str();
+      return GetSyntheticSummaryForContainer(value);
     };
 
+    bool done = false;
     // We first try to get the summary from its dereferenced value.
-    bool done = ShouldBeDereferencedForSummary(v) &&
-                tryDumpSummaryAndValue(v.Dereference());
+    if (ShouldBeDereferencedForSummary(v)) {
+      if (std::optional<std::string> text =
+              tryDumpSummaryAndValue(v.Dereference())) {
+        strm << v.GetValue() << " → " << *text;
+        done = true;
+      }
+    }
 
     // We then try to get it from the current value itself.
-    if (!done)
-      done = tryDumpSummaryAndValue(v);
+    if (!done) {
+      if (std::optional<std::string> text = tryDumpSummaryAndValue(v)) {
+        strm << *text;
+        done = true;
+      }
+    }
 
     // As last resort, we print its type and address if available.
     if (!done) {

[clayborg]

This shouldn't be type specific. Any SBValue that reports a value when you call:

const char *SBValue::GetValue();

Should always show the value. In this case, a pointer always has a value to show. So it shouldn't matter that this is a pointer, it should be just "any SBValue that has a value should display the value and if it doesn't have a summary and we auto generate one, then we can append the generated summary string". If you look at the normal code it does this for any SBValue.

[walter-erquinigo]

if it doesn't have a summary and we auto generate one, then we can append the generated summary string

That makes a ton of sense. I'll try that approach. Thanks

[clayborg]

Much better solution that doesn't depend on having a pointer.