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

kastiglione · 2024-12-05T14:38:17Z

Introduces a member property to SBValue. This property provides pythonic access to a value's members, by name. The expression value.member["name"] will be an alternate form form of writing value.GetChildMemberWithName("name").

llvmbot · 2024-12-05T14:38:51Z

@llvm/pr-subscribers-lldb

Author: Dave Lee (kastiglione)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/118814.diff

2 Files Affected:

(modified) lldb/bindings/interface/SBValueExtensions.i (+4-2)
(modified) lldb/test/API/python_api/value/TestValueAPI.py (+3-5)

diff --git a/lldb/bindings/interface/SBValueExtensions.i b/lldb/bindings/interface/SBValueExtensions.i
index bee9c27775d453..f743b8b9bc786f 100644
--- a/lldb/bindings/interface/SBValueExtensions.i
+++ b/lldb/bindings/interface/SBValueExtensions.i
@@ -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
@@ -23,6 +23,8 @@ STRING_EXTENSION_OUTSIDE(SBValue)
                     if -count <= key < count:
                         key %= count
                         return self.sbvalue.GetChildAtIndex(key)
+                elif isinstance(key, str):
+                    return self.sbvalue.GetChildMemberWithName(key)
                 return None
 
         def get_child_access_object(self):
@@ -49,7 +51,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.''')
diff --git a/lldb/test/API/python_api/value/TestValueAPI.py b/lldb/test/API/python_api/value/TestValueAPI.py
index 512100912d6fe7..eb6dbfe6362a43 100644
--- a/lldb/test/API/python_api/value/TestValueAPI.py
+++ b/lldb/test/API/python_api/value/TestValueAPI.py
@@ -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
@@ -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",
         )

medismailben

Nice! LGTM!

jimingham · 2024-12-05T17:45:04Z

I wonder if "member" might be a better name than "child". The base classes of an SBValue representing a C++ object are also children of the value, but you won't be able to look them up in this array.

kastiglione · 2024-12-05T18:03:08Z

@jimingham are you proposing a new property named member? In which case child would be used for index based access, and member would be used for name based access?

kastiglione · 2024-12-05T18:04:32Z

or, should we allow base classes to be looked up by name as well?

medismailben · 2024-12-05T18:15:57Z

or, should we allow base classes to be looked up by name as well?

I'd prefer the latter so they can used interchangeably. I like the fact that the subscript can be either an int or a string.

kastiglione · 2024-12-05T22:00:01Z

I updated the PR to support look up of base class children by name.

jimingham · 2024-12-06T00:06:40Z

Unfortunately, this is legal C++:

class Foo {
public:
  int m_intvar = 10;
};

class Bar : Foo {
public:
  int Foo = 100;
};

int
main()
{
  Bar myBar;
  return myBar.Foo;
}

So the lookup by name of Foo wouldn't be unique here.

jimingham · 2024-12-06T00:09:11Z

I think you have to do lookup by name just of the members (and maybe a separate lookup by name of the base classes?)

jimingham · 2024-12-06T00:17:29Z

The other way to do it would be to use "::Foo" to refer to the base class, and "Foo" the member. Maybe even nicer would be to allow just "Foo" if there's no ambiguity, and only require the :: if there is.

jimingham · 2024-12-06T00:21:14Z

If we ever come across a language that allows numbers for ivar names, overloading child for by name access will make us sad.
But if we can tell the difference between value.child[1] and value.child["1"] we could even work around that.

labath · 2024-12-06T09:06:01Z

lldb/bindings/interface/SBValueExtensions.i

+                    for child in self.sbvalue:
+                        if child.name == key:
+                            return child


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".

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

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

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.

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

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.

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]

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.

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.

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.

jimingham

LGTM

clayborg · 2024-12-06T23:48:54Z

lldb/bindings/interface/SBValueExtensions.i

+                def __getitem__(self, key):
+                    if isinstance(key, str):
+                        return self.valobj.GetChildMemberWithName(key)
+                    raise TypeError("invalid subscript key")


Do we want to be more clear here and say something other than "invalid subscript key"? Do we want to say "member key values must be string values that specify the name of a member variable to search for"?

I changed it to member key must be a string. Since the error is only raised when the key is not a string (a dynamic type check), I left out the semantic part of your suggested message.

labath

I'm fine with this.

…m#118814) Introduces a `member` property to `SBValue`. This property provides pythonic access to a value's members, by name. The expression `value.member["name"]` will be an alternate form form of writing `value.GetChildMemberWithName("name")`. (cherry-picked from commit 53fd724)

[lldb] Add lookup by name to SBValue.child

639fc6d

kastiglione requested a review from JDevlieghere as a code owner December 5, 2024 14:38

llvmbot added the lldb label Dec 5, 2024

kastiglione requested review from medismailben and jimingham December 5, 2024 14:44

medismailben approved these changes Dec 5, 2024

View reviewed changes

kastiglione added 2 commits December 5, 2024 13:04

Support base class children

c6f926b

Add missing colon

7e2c554

labath reviewed Dec 6, 2024

View reviewed changes

Introduce member property which looks up members by name

174b562

kastiglione changed the title ~~[lldb] Add lookup by name to SBValue.child~~ [lldb] Add lookup by name to SBValue through new member property Dec 6, 2024

kastiglione requested a review from medismailben December 6, 2024 23:17

jimingham approved these changes Dec 6, 2024

View reviewed changes

clayborg reviewed Dec 6, 2024

View reviewed changes

Use a more helpful TypeError message

d1df3ce

labath approved these changes Dec 9, 2024

View reviewed changes

kastiglione merged commit 53fd724 into llvm:main Dec 9, 2024
7 checks passed

kastiglione deleted the lldb-Add-lookup-by-name-to-SBValue.child branch December 9, 2024 18:48

kastiglione mentioned this pull request Apr 3, 2025

[lldb] Upstream bindings improvements swiftlang/llvm-project#10416

Merged

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

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

Uh oh!

Conversation

kastiglione commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 5, 2024

Uh oh!

medismailben left a comment

Choose a reason for hiding this comment

Uh oh!

jimingham commented Dec 5, 2024

Uh oh!

kastiglione commented Dec 5, 2024

Uh oh!

kastiglione commented Dec 5, 2024

Uh oh!

medismailben commented Dec 5, 2024

Uh oh!

kastiglione commented Dec 5, 2024

Uh oh!

jimingham commented Dec 6, 2024

Uh oh!

jimingham commented Dec 6, 2024

Uh oh!

jimingham commented Dec 6, 2024

Uh oh!

jimingham commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

labath Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jimingham Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jimingham left a comment

Choose a reason for hiding this comment

Uh oh!

clayborg Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labath left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kastiglione commented Dec 5, 2024 •

edited

Loading

jimingham commented Dec 6, 2024 •

edited

Loading

labath Dec 6, 2024 •

edited

Loading

jimingham Dec 6, 2024 •

edited

Loading

clayborg Dec 6, 2024 •

edited

Loading