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

walter-erquinigo · 2023-09-06T18:43:08Z

We've been displaying types and addresses for containers, but that's not very useful information. A better approach is to compose the summary of containers with the summary of a few of its children.

Not only that, we can dereference simple pointers and references to get the summary of the pointer variable, which is also better than just showing an anddress.

And in the rare case where the user wants to inspect the raw address, they can always use the debug console for that.

For the record, this is very similar to what the CodeLLDB extension does, and it seems to give a better experience.

An example of the new output:

And this is the

old output:

…ointers We've been displaying types and addresses for containers, but that's not very useful information. A better approach is to compose the summary of containers with the summary of a few of its children. Not only that, we can dereference simple pointers and references to get the summary of the pointer variable, which is also better than just showing an anddress. And in the rare case where the user wants to inspect the raw address, they can always use the debug console for that.

River707 · 2023-09-06T21:04:22Z

lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py

 import time

+import vscode


Are the changes in this file necessary?

River707 · 2023-09-06T21:07:00Z

lldb/tools/lldb-vscode/JSONUtils.cpp

+/// 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())


Do we have tests for this false cases?

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

clayborg

This will cause large performance regressions when debugging large complex codebases due to the extra type completions that will now happen every time you stop somewhere and view variables.

clayborg · 2023-09-07T17:23:23Z

lldb/tools/lldb-vscode/JSONUtils.cpp

+
+  llvm::StringRef separator = "";
+
+  for (size_t i = 0, e = v.GetNumChildren(); i < e; ++i) {


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.

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)

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.

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?)

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.

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?)

…ointers (llvm#65514) We've been displaying types and addresses for containers, but that's not very useful information. A better approach is to compose the summary of containers with the summary of a few of its children. Not only that, we can dereference simple pointers and references to get the summary of the pointer variable, which is also better than just showing an anddress. And in the rare case where the user wants to inspect the raw address, they can always use the debug console for that. For the record, this is very similar to what the CodeLLDB extension does, and it seems to give a better experience. An example of the new output: <img width="494" alt="Screenshot 2023-09-06 at 2 24 27 PM" src="https://github.com/llvm/llvm-project/assets/1613874/588659b8-421a-4865-8d67-ce4b6182c4f9"> And this is the <img width="476" alt="Screenshot 2023-09-06 at 2 46 30 PM" src="https://github.com/llvm/llvm-project/assets/1613874/5768a52e-a773-449d-9aab-1b2fb2a98035"> old output:

…ointers (llvm#65514) We've been displaying types and addresses for containers, but that's not very useful information. A better approach is to compose the summary of containers with the summary of a few of its children. Not only that, we can dereference simple pointers and references to get the summary of the pointer variable, which is also better than just showing an anddress. And in the rare case where the user wants to inspect the raw address, they can always use the debug console for that. For the record, this is very similar to what the CodeLLDB extension does, and it seems to give a better experience. An example of the new output: <img width="494" alt="Screenshot 2023-09-06 at 2 24 27 PM" src="https://github.com/llvm/llvm-project/assets/1613874/588659b8-421a-4865-8d67-ce4b6182c4f9"> And this is the <img width="476" alt="Screenshot 2023-09-06 at 2 46 30 PM" src="https://github.com/llvm/llvm-project/assets/1613874/5768a52e-a773-449d-9aab-1b2fb2a98035"> old output: (cherry picked from commit 89a81ec)

github-actions bot added the lldb label Sep 6, 2023

walter-erquinigo requested review from River707 and clayborg September 6, 2023 18:43

River707 approved these changes Sep 6, 2023

View reviewed changes

Update lldbvscode_testcase.py

d9ac0d2

walter-erquinigo merged commit 89a81ec into llvm:main Sep 6, 2023

walter-erquinigo deleted the walter/variables branch September 6, 2023 21:13

clayborg reviewed Sep 7, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

walter-erquinigo commented Sep 6, 2023 •

edited

Loading

Uh oh!

River707 Sep 6, 2023

Uh oh!

River707 Sep 6, 2023

Uh oh!

walter-erquinigo Sep 6, 2023

Uh oh!

clayborg left a comment

Uh oh!

clayborg Sep 7, 2023

Uh oh!

dwblaikie Sep 20, 2023

Uh oh!

clayborg Sep 22, 2023

Uh oh!

dwblaikie Sep 25, 2023

Uh oh!

clayborg Sep 26, 2023

Uh oh!

dwblaikie Sep 26, 2023

Uh oh!

Uh oh!


		llvm::StringRef separator = "";

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

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

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

Uh oh!

Conversation

walter-erquinigo commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clayborg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

walter-erquinigo commented Sep 6, 2023 •

edited

Loading