[Lldb-commits] [lldb] [lldb-dap] Don't call GetNumChildren on non-indexed synthetic variables (PR #93534)
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Tue May 28 04:41:11 PDT 2024
https://github.com/labath created https://github.com/llvm/llvm-project/pull/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.
>From 1e96acf526eb00e470ce620e5ab523cc21939390 Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Tue, 28 May 2024 11:11:27 +0000
Subject: [PATCH] [lldb-dap] Don't call GetNumChildren on non-indexed synthetic
variables
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.
---
.../lldb-dap/variables/children/Makefile | 3 ++
.../children/TestDAP_variables_children.py | 42 +++++++++++++++++
.../lldb-dap/variables/children/formatter.py | 42 +++++++++++++++++
.../lldb-dap/variables/children/main.cpp | 8 ++++
lldb/tools/lldb-dap/JSONUtils.cpp | 45 ++++++++++---------
5 files changed, 118 insertions(+), 22 deletions(-)
create mode 100644 lldb/test/API/tools/lldb-dap/variables/children/Makefile
create mode 100644 lldb/test/API/tools/lldb-dap/variables/children/TestDAP_variables_children.py
create mode 100644 lldb/test/API/tools/lldb-dap/variables/children/formatter.py
create mode 100644 lldb/test/API/tools/lldb-dap/variables/children/main.cpp
diff --git a/lldb/test/API/tools/lldb-dap/variables/children/Makefile b/lldb/test/API/tools/lldb-dap/variables/children/Makefile
new file mode 100644
index 0000000000000..99998b20bcb05
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/variables/children/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/tools/lldb-dap/variables/children/TestDAP_variables_children.py b/lldb/test/API/tools/lldb-dap/variables/children/TestDAP_variables_children.py
new file mode 100644
index 0000000000000..54fb318289aec
--- /dev/null
+++ b/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"],
+ )
diff --git a/lldb/test/API/tools/lldb-dap/variables/children/formatter.py b/lldb/test/API/tools/lldb-dap/variables/children/formatter.py
new file mode 100644
index 0000000000000..b578faf4f1d3d
--- /dev/null
+++ b/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)
diff --git a/lldb/test/API/tools/lldb-dap/variables/children/main.cpp b/lldb/test/API/tools/lldb-dap/variables/children/main.cpp
new file mode 100644
index 0000000000000..5d625fe1903a3
--- /dev/null
+++ b/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
+}
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index 069877dbab339..544e9ffb172bf 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/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);
More information about the lldb-commits
mailing list