Skip to content
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.

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

Temporary fix for memory leak when reading variables #115

Merged
merged 1 commit into from
Dec 16, 2023

Conversation

sunshaoce
Copy link
Collaborator

This is a temporary solution, from @shiretu microsoft/vscode-cpptools#7240 (comment) .
I'm not sure if it really works, so I hope everyone can give more feedback, thank you!

Related issues:
microsoft/vscode-cpptools#1899
microsoft/vscode-cpptools#5805
microsoft/vscode-cpptools#7240
microsoft/vscode-cpptools#9488
microsoft/vscode-cpptools#9831
llvm/llvm-project#53634
#89
#101
vadimcn/codelldb#987
vadimcn/codelldb#998


Co-authored-by: Eugen-Andrei Gavriloaie andrei@gavriloaie.com

---------

Co-authored-by: Eugen-Andrei Gavriloaie <andrei@gavriloaie.com>
@sunshaoce
Copy link
Collaborator Author

It looks like no one has any objections to this PR.

@sunshaoce sunshaoce merged commit a6c8c66 into lldb-tools:main Dec 16, 2023
3 checks passed
@sunshaoce sunshaoce deleted the fix branch January 23, 2024 06:44
@@ -304,7 +304,8 @@ void CMICmdCmdVarCreate::CompleteSBValue(lldb::SBValue &vrwValue) {
// And update its children
Copy link

Choose a reason for hiding this comment

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

Any reason you are iterating over all of the children? This can go quite deep into structures of structures and could be quite costly since it is recursive. If any SBValue is asked for it values, it will update itself as needed. You don't need to iterate over all of its children.

Copy link

Choose a reason for hiding this comment

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

In the std::initializer_list case, you might have millions of children. I would suggest removing this entire loop as there is no need unless I am missing something.

And as an FYI: if you need to ask a SBValue if it can be expanded in a UI, it is best to use:

bool SBValue::MightHaveChildren();

This won't cause a type to complete itself just to answer if it can be expanded or not. LLDB lazily parses types and if you have a class, struct, union, we leave the type in forward declaration mode until someone asks something concrete from a type, like "SBValue::GetNumChildren()". Here you though you needed the number of children to be able to iterate over the children, but as stated above, I would suggest removing this loop unless it serves a purpose I can't see.

Comment on lines +601 to 605
const auto temp = vrwValue.GetNumChildren();
const MIuint nChildren = temp > 64 ? 64 : temp;
for (MIuint i = 0; i < nChildren; ++i) {
lldb::SBValue member = vrwValue.GetChildAtIndex(i);
if (!member.IsValid())
Copy link

Choose a reason for hiding this comment

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

I would remove this loop if possible? You really don't want to always iterate over all children to see if any children have been updated as it is expensive. Especially if there is a rogue synthetic child provider saying they have way too many children.

If you do need to do this to iterate over a finite amount of children, I would read the "target.max-children-count" setting. If you have a new enough LLDB you can do:

SBDebugger debugger = ...;
uint64_t max_children = debugger.GetSetting("target.max-children-count").GetUnsignedIntegerValue(256);

This defaults to 256 in LLDB, but if we fail to return the value from the settings, it will return 256 as the fail value (the argument to GetUnsignedIntegerValue(...)).

@sunshaoce
Copy link
Collaborator Author

@clayborg Thanks for your comments. Would you like to make a PR to fix this, considering there are fewer people developing this project?

@clayborg
Copy link

clayborg commented Aug 9, 2024

I don't work with the MI layer anymore.

If this is being used in Visual Studio Code, the LLDB project fully supports a native lldb-dap plug-in that interfaces directly with the IDE without going through the GDB MI layering. I would suggest switching cppdebug over to using this as we actively maintain this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants