[lldb-dap] Don't call GetNumChildren on non-indexed synthetic variables (#93534)

A synthetic child provider might need to do considerable amount of work
to compute the number of children. lldb-dap is currently calling that
for all synthethic variables, but it's only actually using the value for
values which it deems to be "indexed" (which is determined by looking at
the name of the first child). This patch reverses the logic so that
GetNumChildren is only called for variables with a suitable first child.

Author: labath

lldb/test/API/tools/lldb-dap/variables/children/Makefile

@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/test/API/tools/lldb-dap/variables/children/TestDAP_variables_children.py

@@ -0,0 +1,42 @@
+import os
+
+import dap_server
+import lldbdap_testcase
+from lldbsuite.test import lldbutil
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+
+class TestDAP_variables_children(lldbdap_testcase.DAPTestCaseBase):
+    def test_get_num_children(self):
+        """Test that GetNumChildren is not called for formatters not producing indexed children."""
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(
+            program,
+            preRunCommands=[
+                "command script import '%s'" % self.getSourcePath("formatter.py")
+            ],
+        )
+        source = "main.cpp"
+        breakpoint1_line = line_number(source, "// break here")
+        lines = [breakpoint1_line]
+
+        breakpoint_ids = self.set_source_breakpoints(
+            source, [line_number(source, "// break here")]
+        )
+        self.continue_to_breakpoints(breakpoint_ids)
+
+        local_vars = self.dap_server.get_local_variables()
+        print(local_vars)
+        indexed = next(filter(lambda x: x["name"] == "indexed", local_vars))
+        not_indexed = next(filter(lambda x: x["name"] == "not_indexed", local_vars))
+        self.assertIn("indexedVariables", indexed)
+        self.assertEquals(indexed["indexedVariables"], 1)
+        self.assertNotIn("indexedVariables", not_indexed)
+
+        self.assertIn(
+            "['Indexed']",
+            self.dap_server.request_evaluate(
+                "`script formatter.num_children_calls", context="repl"
+            )["body"]["result"],
+        )

lldb/test/API/tools/lldb-dap/variables/children/formatter.py

@@ -0,0 +1,42 @@
+import lldb
+
+
+num_children_calls = []
+
+
+class TestSyntheticProvider:
+    def __init__(self, valobj, dict):
+        target = valobj.GetTarget()
+        self._type = valobj.GetType()
+        data = lldb.SBData.CreateDataFromCString(lldb.eByteOrderLittle, 8, "S")
+        name = "child" if "Not" in self._type.GetName() else "[0]"
+        self._child = valobj.CreateValueFromData(
+            name, data, target.GetBasicType(lldb.eBasicTypeChar)
+        )
+
+    def num_children(self):
+        num_children_calls.append(self._type.GetName())
+        return 1
+
+    def get_child_at_index(self, index):
+        if index != 0:
+            return None
+        return self._child
+
+    def get_child_index(self, name):
+        if name == self._child.GetName():
+            return 0
+        return None
+
+
+def __lldb_init_module(debugger, dict):
+    cat = debugger.CreateCategory("TestCategory")
+    cat.AddTypeSynthetic(
+        lldb.SBTypeNameSpecifier("Indexed"),
+        lldb.SBTypeSynthetic.CreateWithClassName("formatter.TestSyntheticProvider"),
+    )
+    cat.AddTypeSynthetic(
+        lldb.SBTypeNameSpecifier("NotIndexed"),
+        lldb.SBTypeSynthetic.CreateWithClassName("formatter.TestSyntheticProvider"),
+    )
+    cat.SetEnabled(True)

lldb/test/API/tools/lldb-dap/variables/children/main.cpp

@@ -0,0 +1,8 @@
+struct Indexed {};
+struct NotIndexed {};
+
+int main() {
+  Indexed indexed;
+  NotIndexed not_indexed;
+  return 0; // break here
+}

lldb/tools/lldb-dap/JSONUtils.cpp

@@ -12,6 +12,7 @@
 #include <sstream>
 #include <string.h>
 
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/FormatAdapters.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
@@ -1211,34 +1212,34 @@ llvm::json::Value CreateVariable(lldb::SBValue v, int64_t variablesReference,
   // give a hint to the IDE that the type has indexed children so that the
   // request can be broken up in grabbing only a few children at a time. We
   // want to be careful and only call "v.GetNumChildren()" if we have an array
-  // type or if we have a synthetic child provider. We don't want to call
-  // "v.GetNumChildren()" on all objects as class, struct and union types
-  // don't need to be completed if they are never expanded. So we want to
-  // avoid calling this to only cases where we it makes sense to keep
+  // type or if we have a synthetic child provider producing indexed children.
+  // We don't want to call "v.GetNumChildren()" on all objects as class, struct
+  // and union types don't need to be completed if they are never expanded. So
+  // we want to avoid calling this to only cases where we it makes sense to keep
   // performance high during normal debugging.
 
   // If we have an array type, say that it is indexed and provide the number
   // of children in case we have a huge array. If we don't do this, then we
   // might take a while to produce all children at onces which can delay your
   // debug session.
-  const bool is_array = desc.type_obj.IsArrayType();
-  const bool is_synthetic = v.IsSynthetic();
-  if (is_array || is_synthetic) {
-    const auto num_children = v.GetNumChildren();
-    // We create a "[raw]" fake child for each synthetic type, so we have to
-    // account for it when returning indexed variables. We don't need to do
-    // this for non-indexed ones.
-    bool has_raw_child = is_synthetic && g_dap.enable_synthetic_child_debugging;
-    int actual_num_children = num_children + (has_raw_child ? 1 : 0);
-    if (is_array) {
-      object.try_emplace("indexedVariables", actual_num_children);
-    } else if (num_children > 0) {
-      // If a type has a synthetic child provider, then the SBType of "v"
-      // won't tell us anything about what might be displayed. So we can check
-      // if the first child's name is "[0]" and then we can say it is indexed.
-      const char *first_child_name = v.GetChildAtIndex(0).GetName();
-      if (first_child_name && strcmp(first_child_name, "[0]") == 0)
-        object.try_emplace("indexedVariables", actual_num_children);
+  if (desc.type_obj.IsArrayType()) {
+    object.try_emplace("indexedVariables", v.GetNumChildren());
+  } else if (v.IsSynthetic()) {
+    // For a type with a synthetic child provider, the SBType of "v" won't tell
+    // us anything about what might be displayed. Instead, we check if the first
+    // child's name is "[0]" and then say it is indexed. We call
+    // GetNumChildren() only if the child name matches to avoid a potentially
+    // expensive operation.
+    if (lldb::SBValue first_child = v.GetChildAtIndex(0)) {
+      llvm::StringRef first_child_name = first_child.GetName();
+      if (first_child_name == "[0]") {
+        size_t num_children = v.GetNumChildren();
+        // If we are creating a "[raw]" fake child for each synthetic type, we
+        // have to account for it when returning indexed variables.
+        if (g_dap.enable_synthetic_child_debugging)
+          ++num_children;
+        object.try_emplace("indexedVariables", num_children);
+      }
     }
   }
   EmplaceSafeString(object, "type", desc.display_type_name);