Skip to content

[lldb-vscode] Display a more descriptive summary for containers and pointers #65514

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 6, 2023
Merged

[lldb-vscode] Display a more descriptive summary for containers and pointers #65514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e7ed336
[lldb-vscode] Display a more descriptive summary for containers and p…
Sep 6, 2023
d9ac0d2
Update lldbvscode_testcase.py
walter-erquinigo Sep 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def run_test_evaluate_expressions(self, context=None):
self.assertEvaluate("var2", "21")
self.assertEvaluate("static_int", "42")
self.assertEvaluate("non_static_int", "43")
self.assertEvaluate("struct1", "my_struct @ 0x.*")
self.assertEvaluate("struct1", "{foo:15}")
self.assertEvaluate("struct1.foo", "15")
self.assertEvaluate("struct2->foo", "16")

Expand Down Expand Up @@ -85,7 +85,7 @@ def run_test_evaluate_expressions(self, context=None):
self.assertEvaluate(
"non_static_int", "10"
) # different variable with the same name
self.assertEvaluate("struct1", "my_struct @ 0x.*")
self.assertEvaluate("struct1", "{foo:15}")
self.assertEvaluate("struct1.foo", "15")
self.assertEvaluate("struct2->foo", "16")

Expand Down
13 changes: 7 additions & 6 deletions lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@
Test lldb-vscode setBreakpoints request
"""

import os

import lldbvscode_testcase
import vscode
from lldbsuite.test import lldbutil
from lldbsuite.test.decorators import *
from lldbsuite.test.lldbtest import *
from lldbsuite.test import lldbutil
import lldbvscode_testcase
import os


def make_buffer_verify_dict(start_idx, count, offset=0):
Expand Down Expand Up @@ -218,12 +219,12 @@ def test_scopes_variables_setVariable_evaluate(self):
},
"pt": {
"equals": {"type": "PointType"},
"startswith": {"result": "PointType @ 0x"},
"startswith": {"result": "{x:11, y:22}"},
"hasVariablesReference": True,
},
"pt.buffer": {
"equals": {"type": "int[32]"},
"startswith": {"result": "int[32] @ 0x"},
"startswith": {"result": "{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, ...}"},
"hasVariablesReference": True,
},
"argv": {
Expand Down Expand Up @@ -409,7 +410,7 @@ def test_scopes_and_evaluate_expansion(self):
"name": "pt",
"response": {
"equals": {"type": "PointType"},
"startswith": {"result": "PointType @ 0x"},
"startswith": {"result": "{x:11, y:22}"},
"missing": ["indexedVariables"],
"hasVariablesReference": True,
},
Expand Down
128 changes: 114 additions & 14 deletions lldb/tools/lldb-vscode/JSONUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,84 @@ std::vector<std::string> GetStrings(const llvm::json::Object *obj,
return strs;
}

/// 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.
static std::optional<std::string>
GetSyntheticSummaryForContainer(lldb::SBValue &v) {
if (v.TypeIsPointerType() || !v.MightHaveChildren())
return std::nullopt;
/// As this operation can be potentially slow, we limit the total time spent
/// fetching children to a few ms.
const auto max_evaluation_time = std::chrono::milliseconds(10);
/// We don't want to generate a extremely long summary string, so we limit its
/// length.
const size_t max_length = 32;

auto start = std::chrono::steady_clock::now();
std::string summary;
llvm::raw_string_ostream os(summary);
os << "{";

llvm::StringRef separator = "";

for (size_t i = 0, e = v.GetNumChildren(); i < e; ++i) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with this is say you have a pointer to a very very complex type that contains many of members variables and inherits from many classes, if you call "v.GetNumChildren()" is causes that type to have to be completed along with all contained types which can cause a lot of debug info to be parsed.

Previous to this change, if you have 10 class pointer variables, we wouldn't ever need to complete the types if the user didn't expand those types. Now with this change, all of these types have to be fully completed, due to the v.GetNumChildren() call as it will cause those types to be completed. So this can be huge performance regression.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do other debuggers do? (like Visual Studio itself)

& not sure that GetNumChildren should cause indefinite recursive expansion - could limit this feature to just going one level deep, for instance. But certainly worth comparing to other debuggers to see what sort of heuristics/rules/guidelines are good in terms of the amount of data expanded by default V unexpanded.

(& yeah, if everything's expanded all the way down, that's probably not good)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do other debuggers do? (like Visual Studio itself)

& not sure that GetNumChildren should cause indefinite recursive expansion - could limit this feature to just going one level deep, for instance. But certainly worth comparing to other debuggers to see what sort of heuristics/rules/guidelines are good in terms of the amount of data expanded by default V unexpanded.

(& yeah, if everything's expanded all the way down, that's probably not good)

It will cause the type to be completed, so if the type has instances of objects or it inherits from one or more classes, those need to be completed. We use clang AST types to answer these questions by counting fields and base classes that have ivars to show etc, so yes the type needs to be completed. We only need to know the layout of a type, so if we have ivars that are pointers or references, then we don't need the complete type. So it can and must complete the type.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand. v.GetNumChildren() wouldn't need to do any more work for a case with "10 class pointer variables", right? You can determine how many children this type has (10) without completing the types those pointers point to?

Or is it later code (like line 168, etc) that try to get the summary of those pointer member variables that are an issue? (though getting the summary of a pointer shouldn't automatically dereference the pointer/complete its pointed-to type, should it?)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand. v.GetNumChildren() wouldn't need to do any more work for a case with "10 class pointer variables", right?

correct. that is a problem. This API is asking any variable how many children it has. So if we have a CompilerType in LLDB that represents a class, it starts off as a forward declaration and it can complete itself through the external AST support in clang. As soon as we call GetNumChildren, we need to complete the type as some of the children of a type might come from base classes, but only if they have fields or other base classes that contain fields. Empty base classes do not show up in the variable view. And then we need to count the number of fields that the class actaully has as well so we can show them as children, so as soon as we call "v.GetNumChildren()" we must complete the type.

You can determine how many children this type has (10) without completing the types those pointers point to?

Correct yes, that is what I meant by "layout type". LLDB has the ability to say "get me a version of this type that is needed to layout this current class". So if a class has 10 ivars that are all pointers, we don't need to complete those classes. If we have 10 ivars that are class instances, then we do need to complete the type since we assist in the clang AST context in laying out the type with the help of the DWARF info. This is needed because of "-flimit-debug-info". If we have class A that inherits from class B, and there is no class B definition in the DWARF, then we have to have the DWARF help us laying out the type. What we do is start and end a definition for class B and we attach. metadata saying "this class should have been complete but wasn't", and the layout support that clang has allows us to specify the offsets of all fields and base classes so we can always show the variable correctly to people. The layout stuff is needed because DWARF doesn't encode many things, one being the current pragma pack settings and other things that can affect the layout of the type.

Or is it later code (like line 168, etc) that try to get the summary of those pointer member variables that are an issue? (though getting the summary of a pointer shouldn't automatically dereference the pointer/complete its pointed-to type, should it?)

Any summary code that needs to access children to make up the summary string can be expensive and cause the type to need to be completed due to above explanation. If we have a "Foo *", the value of the variable will show as the pointer value, but if the type that is being pointed to has a summary, we do end up showing that for pointers and for references. This is because users expect it from the GDB days and due to the way we expand variables. If we have a "Point *" and we expand it in a GUI, we don't expect it to expand to a "Point" and then for it to need to be expanded yet again to see the "x" and "y" values, we expect to directly see the "x" and "y" right away. If the pointee is a simple type, like an "int *", we exand and show the "int", but if we have a class pointer, then we immediately show the children of the class. Many users complained when we initially did it where a "Point *" would expand to a "Point" object, so we changed lldb.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the context - how's all this compare to other debuggers? (like Visual Studio, for example - what's it show by default for the types?)

// If we reached the time limit or exceeded the number of characters, we
// dump `...` to signal that there are more elements in the collection.
if (summary.size() > max_length ||
(std::chrono::steady_clock::now() - start) > max_evaluation_time) {
os << separator << "...";
break;
}
lldb::SBValue child = v.GetChildAtIndex(i);

if (llvm::StringRef name = child.GetName(); !name.empty()) {
llvm::StringRef value;
if (llvm::StringRef summary = child.GetSummary(); !summary.empty())
value = summary;
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 = ", ";
}
}
}
os << "}";

if (summary == "{...}" || summary == "{}")
return std::nullopt;
return summary;
}

/// Return whether we should dereference an SBValue in order to generate a more
/// meaningful summary string.
static bool ShouldBeDereferencedForSummary(lldb::SBValue &v) {
if (!v.GetType().IsPointerType() && !v.GetType().IsReferenceType())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have tests for this false cases?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, there's a test that includes a char**.

return false;

// If it's a pointer to a pointer, we don't dereference to avoid confusing
// the user with the addresses that could shown in the summary.
if (v.GetType().IsPointerType() &&
v.GetType().GetDereferencedType().IsPointerType())
return false;

// If it's synthetic or a pointer to a basic type that provides a summary, we
// don't dereference.
if ((v.IsSynthetic() || v.GetType().GetPointeeType().GetBasicType() !=
lldb::eBasicTypeInvalid) &&
!llvm::StringRef(v.GetSummary()).empty()) {
return false;
}
return true;
}

void SetValueForKey(lldb::SBValue &v, llvm::json::Object &object,
llvm::StringRef key) {
std::string result;
Expand All @@ -141,23 +219,45 @@ void SetValueForKey(lldb::SBValue &v, llvm::json::Object &object,
if (!error.Success()) {
strm << "<error: " << error.GetCString() << ">";
} else {
llvm::StringRef value = v.GetValue();
llvm::StringRef summary = v.GetSummary();
llvm::StringRef type_name = v.GetType().GetDisplayTypeName();
if (!value.empty()) {
strm << value;
if (!summary.empty())
auto tryDumpSummaryAndValue = [&strm](lldb::SBValue value) {
llvm::StringRef val = value.GetValue();
llvm::StringRef summary = value.GetSummary();
if (!val.empty()) {
strm << val;
if (!summary.empty())
strm << ' ' << summary;
return true;
}
if (!summary.empty()) {
strm << ' ' << summary;
} else if (!summary.empty()) {
strm << ' ' << summary;
} else if (!type_name.empty()) {
strm << type_name;
lldb::addr_t address = v.GetLoadAddress();
if (address != LLDB_INVALID_ADDRESS)
strm << " @ " << llvm::format_hex(address, 0);
return true;
}
if (auto container_summary = GetSyntheticSummaryForContainer(value)) {
strm << *container_summary;
return true;
}
return false;
};

// We first try to get the summary from its dereferenced value.
bool done = ShouldBeDereferencedForSummary(v) &&
tryDumpSummaryAndValue(v.Dereference());

// We then try to get it from the current value itself.
if (!done)
done = tryDumpSummaryAndValue(v);

// As last resort, we print its type and address if available.
if (!done) {
if (llvm::StringRef type_name = v.GetType().GetDisplayTypeName();
!type_name.empty()) {
strm << type_name;
lldb::addr_t address = v.GetLoadAddress();
if (address != LLDB_INVALID_ADDRESS)
strm << " @ " << llvm::format_hex(address, 0);
}
}
}
strm.flush();
EmplaceSafeString(object, key, result);
}

Expand Down