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.

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

Temporary fix for memory leak when reading variables #115

Merged
merged 1 commit into from
Dec 16, 2023
+4 −2
Merged

Temporary fix for memory leak when reading variables #115

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions src/MICmdCmdVar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

lldb::SBType valueType = vrwValue.GetType();
if (!valueType.IsPointerType() && !valueType.IsReferenceType()) {
const MIuint nChildren = vrwValue.GetNumChildren();
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())
Expand Down Expand Up @@ -597,7 +598,8 @@ bool CMICmdCmdVarUpdate::ExamineSBValueForChange(lldb::SBValue &vrwValue,
return MIstatus::success;
}

const MIuint nChildren = vrwValue.GetNumChildren();
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())
Comment on lines +601 to 605
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(...)).

Expand Down