Skip to content

[lldb] Add lookup by name to SBValue through new member property #118814

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 5 commits into from
Dec 9, 2024
Merged

[lldb] Add lookup by name to SBValue through new member property #118814

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
639fc6d
[lldb] Add lookup by name to SBValue.child
kastiglione Dec 4, 2024
c6f926b
Support base class children
kastiglione Dec 5, 2024
7e2c554
Add missing colon
kastiglione Dec 5, 2024
174b562
Introduce member property which looks up members by name
kastiglione Dec 6, 2024
d1df3ce
Use a more helpful TypeError message
kastiglione Dec 7, 2024
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
11 changes: 9 additions & 2 deletions lldb/bindings/interface/SBValueExtensions.i
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ STRING_EXTENSION_OUTSIDE(SBValue)
return self.GetDynamicValue (eDynamicCanRunTarget)

class children_access(object):
'''A helper object that will lazily hand out thread for a process when supplied an index.'''
'''A helper object that will lazily hand out child values when supplied an index or name.'''

def __init__(self, sbvalue):
self.sbvalue = sbvalue
Expand All @@ -23,6 +23,13 @@ STRING_EXTENSION_OUTSIDE(SBValue)
if -count <= key < count:
key %= count
return self.sbvalue.GetChildAtIndex(key)
elif isinstance(key, str):
if child := self.sbvalue.GetChildMemberWithName(key):
return child
# Support base classes, which are children but not members.
for child in self.sbvalue:
if child.name == key:
return child
Copy link
Collaborator

@labath labath Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this is a good idea, as it means every negative lookup could end up enumerating all children of the object -- and there could be millions of them. If you really want to support looking up base classes (*), I think it should be done by iterating through the base base classes.

(*) I actually think we shouldn't be providing extra functionality here -- at least unless GetChildMemberWithName supports that as well. I think the fact that operator[](string) is shorthand for GetChildMemberWithName(string) makes things easy to understand. I think it'd be confusing if one of them provided functionality which is not available in the other one. You'd also have to be very careful about handling data formatters -- they can provide synthetic children, but not "synthetic base classes".

Copy link
Contributor Author

@kastiglione kastiglione Dec 6, 2024

Choose a reason for hiding this comment

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

I think it should be done by iterating through the base base classes.

Is there even an API for this? Maybe it'd have to be derived, something like:

def base_classes(self):
    for child in self:
        if self.GetChildMemberWithName(child.name):
            # child is a member, stop iteration.
            return
        yield child

Copy link
Contributor Author

@kastiglione kastiglione Dec 6, 2024

Choose a reason for hiding this comment

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

I agree with your opinions. I think I'd like to avoid introducing complexities in interface/consistency.

Should I change this PR to introduce a new member property? Or should value.child[name] be documented as meaning only value.GetChildMemberWithName(name)? cc @jimingham

Copy link
Collaborator

@jimingham jimingham Dec 6, 2024

Choose a reason for hiding this comment

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

If you have a class with a base class, SBValue::GetChildAtIndex(0) and var.child[0] return the base class. So the "child" accessor currently returns both base classes and ivars. Moreover, GetChildMemberWithName actually looks into base classes to pull out ivars (it doesn't however handle duplicate names, it only returns the first one it finds with the matching name which isn't great...)

So GetChildMemberWithName will pull out children that aren't even in the indexed child array.

That does argue to me that child is the wrong property to use for this.

Copy link
Collaborator

@jimingham jimingham Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Might be nice to have both "members" and "bases" so you could easily pull out base class values by name as well.

Copy link
Contributor Author

@kastiglione kastiglione Dec 6, 2024

Choose a reason for hiding this comment

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

I'll introduce a member property. I haven't need to access a child base class, by name or otherwise. I'll leave a bases property for another time.

Copy link
Collaborator

@clayborg clayborg Dec 6, 2024

Choose a reason for hiding this comment

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

NOTE: Base classes will only be returned by GetChildAtIndex(N) if they have something to show the user, i.e. they have instance variables or have base classes that have instance variables. If a base class has no ivars, it will not be returned. So if we are looking for a reliable way to get base classes, it is better to use the exsting APIs on lldb.SBType.

The other thing is, do we want to add a SBValue.__get_item__(self, arg) method to easily access base classes or members where we just return the value of SBValue.GetChildAtIndex(...)? Then we can do:

var = lldb.frame.FindVariable('my_struct')
first_child = var[0]

Instead of:

first_child = None
if (len(value.bases) > 0)
  first_child = value.bases[0]
else:
  first_child = value.members[0]

Copy link
Contributor Author

@kastiglione kastiglione Dec 6, 2024

Choose a reason for hiding this comment

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

I've updated the PR to leave child alone, and introduced member. Does this work for everyone? It doesn't provide any means for accessing base classes by name, but that was not my primary intention.

Copy link
Collaborator

@jimingham jimingham Dec 6, 2024

Choose a reason for hiding this comment

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

This is fine by me, though at some point it would be convenient (and symmetric) to come back and add base class accessors. My only objection to the original patch was that using the child property for member access was confusing. member is clear.

Copy link
Collaborator

@labath labath Dec 9, 2024

Choose a reason for hiding this comment

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

Is there even an API for this?

I don't think so, but that was sort of my point. If that's what we want to do, then we should have an API for that -- instead of trying to work around its absence in python.

return None

def get_child_access_object(self):
Expand All @@ -49,7 +56,7 @@ STRING_EXTENSION_OUTSIDE(SBValue)
return self.GetNumChildren()

children = property(get_value_child_list, None, doc='''A read only property that returns a list() of lldb.SBValue objects for the children of the value.''')
child = property(get_child_access_object, None, doc='''A read only property that returns an object that can access children of a variable by index (child_value = value.children[12]).''')
child = property(get_child_access_object, None, doc='''A read only property that returns an object that can access children of a variable by index or by name.''')
name = property(GetName, None, doc='''A read only property that returns the name of this value as a string.''')
type = property(GetType, None, doc='''A read only property that returns a lldb.SBType object that represents the type for this value.''')
size = property(GetByteSize, None, doc='''A read only property that returns the size in bytes of this value.''')
Expand Down
8 changes: 3 additions & 5 deletions lldb/test/API/python_api/value/TestValueAPI.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,8 @@ def test(self):
val_i = target.EvaluateExpression("i")
val_s = target.EvaluateExpression("s")
val_a = target.EvaluateExpression("a")
self.assertTrue(
val_s.GetChildMemberWithName("a").GetAddress().IsValid(), VALID_VARIABLE
)
self.assertTrue(val_s.GetChildMemberWithName("a").AddressOf(), VALID_VARIABLE)
self.assertTrue(val_s.child["a"].GetAddress().IsValid(), VALID_VARIABLE)
self.assertTrue(val_s.child["a"].AddressOf(), VALID_VARIABLE)
self.assertTrue(val_a.Cast(val_i.GetType()).AddressOf(), VALID_VARIABLE)

# Test some other cases of the Cast API. We allow casts from one struct type
Expand Down Expand Up @@ -210,7 +208,7 @@ def test(self):
weird_cast = f_var.Cast(val_s.GetType())
self.assertSuccess(weird_cast.GetError(), "Can cast from a larger to a smaller")
self.assertEqual(
weird_cast.GetChildMemberWithName("a").GetValueAsSigned(0),
weird_cast.child["a"].GetValueAsSigned(0),
33,
"Got the right value",
)
Expand Down
Loading