[lldb] Retcon SBValue::GetChildAtIndex(synthetic=true) #140065
Merged
+63
−32
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesThe motivation here is being (un)able to treat pointer values as an array consistently. This works for pointers to simple/scalar values, but for aggregates, we get a very surprising result:
This patch reimagines this interface so that the value of This has the potential to break existing code, but I have a suspicion that code was already broken to begin with, which is why I think this would be better than introducing a new API and keeping the old (and surprising behavior). If our own test coverage is any indication, breakage should be minimal. Full diff: https://github.com/llvm/llvm-project/pull/140065.diff 4 Files Affected:
diff --git a/lldb/include/lldb/API/SBValue.h b/lldb/include/lldb/API/SBValue.h
index 75d20a4378f09..ad87639960fa0 100644
--- a/lldb/include/lldb/API/SBValue.h
+++ b/lldb/include/lldb/API/SBValue.h
@@ -160,31 +160,26 @@ class LLDB_API SBValue {
/// members (empty base classes are omitted), and all members of the
/// current class will then follow the base classes.
///
- /// Pointers differ depending on what they point to. If the pointer
- /// points to a simple type, the child at index zero
- /// is the only child value available, unless \a synthetic_allowed
- /// is \b true, in which case the pointer will be used as an array
- /// and can create 'synthetic' child values using positive or
- /// negative indexes. If the pointer points to an aggregate type
- /// (an array, class, union, struct), then the pointee is
- /// transparently skipped and any children are going to be the indexes
- /// of the child values within the aggregate type. For example if
- /// we have a 'Point' type and we have a SBValue that contains a
- /// pointer to a 'Point' type, then the child at index zero will be
- /// the 'x' member, and the child at index 1 will be the 'y' member
- /// (the child at index zero won't be a 'Point' instance).
+ /// For array and pointers the behavior of the function depends on the value
+ /// of the \a use_synthetic argument. If \b false, the function returns
+ /// members of the array as given by the array bounds. If the value is a
+ /// pointer to a simple type, the child at index zero is the only child
+ /// value available. If the pointer points to an aggregate type (an array,
+ /// class, union, etc.), then the pointee is transparently skipped and any
+ /// children are going to be the indexes of the child values within the
+ /// aggregate type. For example if we have a 'Point' type and we have a
+ /// SBValue that contains a pointer to a 'Point' type, then the child at
+ /// index zero will be the 'x' member, and the child at index 1 will be the
+ /// 'y' member (the child at index zero won't be a 'Point' instance). If \a
+ /// use_synthetic is \b true, pointer values will be used as a (C) array and
+ /// and the function will create 'synthetic' child values using positive or
+ /// negative indexes. In case of arrays, the function will return values
+ /// which are outside of the array bounds.
///
/// If you actually need an SBValue that represents the type pointed
/// to by a SBValue for which GetType().IsPointeeType() returns true,
/// regardless of the pointee type, you can do that with SBValue::Dereference.
///
- /// Arrays have a preset number of children that can be accessed by
- /// index and will returns invalid child values for indexes that are
- /// out of bounds unless the \a synthetic_allowed is \b true. In this
- /// case the array can create 'synthetic' child values for indexes
- /// that aren't in the array bounds using positive or negative
- /// indexes.
- ///
/// \param[in] idx
/// The index of the child value to get
///
@@ -193,7 +188,7 @@ class LLDB_API SBValue {
/// and also if the target can be run to figure out the dynamic
/// type of the child value.
///
- /// \param[in] can_create_synthetic
+ /// \param[in] use_synthetic
/// If \b true, then allow child values to be created by index
/// for pointers and arrays for indexes that normally wouldn't
/// be allowed.
@@ -202,7 +197,7 @@ class LLDB_API SBValue {
/// A new SBValue object that represents the child member value.
lldb::SBValue GetChildAtIndex(uint32_t idx,
lldb::DynamicValueType use_dynamic,
- bool can_create_synthetic);
+ bool use_synthetic);
// Matches children of this object only and will match base classes and
// member names if this is a clang typed object.
diff --git a/lldb/source/API/SBValue.cpp b/lldb/source/API/SBValue.cpp
index e5cdc8f311450..feb3f9eae6f33 100644
--- a/lldb/source/API/SBValue.cpp
+++ b/lldb/source/API/SBValue.cpp
@@ -680,19 +680,18 @@ SBValue SBValue::GetChildAtIndex(uint32_t idx) {
SBValue SBValue::GetChildAtIndex(uint32_t idx,
lldb::DynamicValueType use_dynamic,
- bool can_create_synthetic) {
- LLDB_INSTRUMENT_VA(this, idx, use_dynamic, can_create_synthetic);
-
- lldb::ValueObjectSP child_sp;
-
+ bool use_synthetic) {
+ LLDB_INSTRUMENT_VA(this, idx, use_dynamic, use_synthetic);
ValueLocker locker;
lldb::ValueObjectSP value_sp(GetSP(locker));
+
+ lldb::ValueObjectSP child_sp;
if (value_sp) {
const bool can_create = true;
- child_sp = value_sp->GetChildAtIndex(idx);
- if (can_create_synthetic && !child_sp) {
+ if (use_synthetic && (value_sp->IsPointerType() || value_sp->IsArrayType()))
child_sp = value_sp->GetSyntheticArrayMember(idx, can_create);
- }
+ else
+ child_sp = value_sp->GetChildAtIndex(idx);
}
SBValue sb_value;
diff --git a/lldb/test/API/python_api/sbvalue_synthetic/TestSBValueSynthetic.py b/lldb/test/API/python_api/sbvalue_synthetic/TestSBValueSynthetic.py
index 2fd1e0ce9c6a3..7c9b9b97e0a70 100644
--- a/lldb/test/API/python_api/sbvalue_synthetic/TestSBValueSynthetic.py
+++ b/lldb/test/API/python_api/sbvalue_synthetic/TestSBValueSynthetic.py
@@ -21,3 +21,24 @@ def test_str(self):
has_formatted = self.frame().FindVariable("has_foo")
self.expect(str(formatted), exe=False, substrs=["synth_child"])
self.expect(str(has_formatted), exe=False, substrs=["synth_child"])
+
+ def test_synth_arr(self):
+ self.build()
+ lldbutil.run_to_source_breakpoint(
+ self, "break here", lldb.SBFileSpec("main.cpp")
+ )
+ point_arr = self.frame().FindVariable("point_arr");
+ point_ptr = self.frame().FindVariable("point_ptr");
+ for v in [point_arr, point_ptr]:
+ for i in range(3):
+ child = v.GetChildAtIndex(i, lldb.eDynamicDontRunTarget, True)
+ check = ValueCheck(name=f"[{i}]", type="Point", children=[ValueCheck(name="x", value=str(2*i+1)), ValueCheck(name="y", value=str(2*i+2))])
+ check.check_value(self, child, f"{child}, child {i} of {v.GetName()}")
+
+ int_arr = self.frame().FindVariable("int_arr");
+ int_ptr = self.frame().FindVariable("int_ptr");
+ for v in [int_arr, int_ptr]:
+ for i in range(3):
+ child = v.GetChildAtIndex(i, lldb.eDynamicDontRunTarget, True)
+ check = ValueCheck(name=f"[{i}]", type="int", value=str(i+1))
+ check.check_value(self, child, f"{child}, child {i} of {v.GetName()}")
diff --git a/lldb/test/API/python_api/sbvalue_synthetic/main.cpp b/lldb/test/API/python_api/sbvalue_synthetic/main.cpp
index 52c6474d7a1b2..5ec272b7bc12e 100644
--- a/lldb/test/API/python_api/sbvalue_synthetic/main.cpp
+++ b/lldb/test/API/python_api/sbvalue_synthetic/main.cpp
@@ -6,8 +6,14 @@ struct HasFoo {
Foo f;
};
+struct Point { int x; int y; };
+
int main() {
Foo foo;
HasFoo has_foo;
+ Point point_arr[] = {{1,2},{3,4},{5,6}};
+ int int_arr[] = {1,2,3,4,5,6};
+ Point *point_ptr = point_arr;
+ int *int_ptr = int_arr;
return 0; // break here
}
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
The motivation here is being (un)able to treat pointer values as an array consistently. This works for pointers to simple/scalar values, but for aggregates, we get a very surprising result: - GetChildAtIndex(x, ??, true) returns the `x` child of the zeroth array member (the one you get by dereferencing the pointer/array) for all `x` which are smaller than the number of children of that value. - for other values of `x`, we get `v[x]`, where `v` is treated like a (C) pointer This patch reimagines this interface so that the value of `true` always treats (pointer and array) values as pointers. For `false`, we always dereference pointers, while in the case of arrays, we only return the values as far as the array bounds will allow. This has the potential to break existing code, but I have a suspicion that code was already broken to begin with, which is why I think this would be better than introducing a new API and keeping the old (and surprising behavior). If our own test coverage is any indication, breakage should be minimal.
jimingham
reviewed
May 15, 2025
lldb/include/lldb/API/SBValue.h
Outdated
| @@ -193,7 +188,7 @@ class LLDB_API SBValue { | |||
| /// and also if the target can be run to figure out the dynamic | |||
| /// type of the child value. | |||
| /// | |||
| /// \param[in] can_create_synthetic | |||
| /// \param[in] use_synthetic | |||
There was a problem hiding this comment.
I think this change makes sense, but there must be a better term for this. use_synthetic doesn't actually tell you anything about what you are going to use synthetic FOR, and is confusing because it does NOT mean "use the children produced by the synthetic child provider for this type", which is almost always what we mean by "using synthetic".
Seems to me this bool is forcing "treat_as_array" behavior for a pointer. Would that be an accurate name for what this parameter controls?
There was a problem hiding this comment.
Sounds good. I wasn't too happy with the name myself.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/15495 Here is the relevant piece of the build log for the reference |
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
The motivation here is being (un)able to treat pointer values as an array consistently. This works for pointers to simple/scalar values, but for aggregates, we get a very surprising result: - GetChildAtIndex(x, ??, true) returns the `x` child of the zeroth array member (the one you get by dereferencing the pointer/array) for all `x` which are smaller than the number of children of that value. - for other values of `x`, we get `v[x]`, where `v` is treated like a (C) pointer This patch reimagines this interface so that the value of `true` always treats (pointer and array) values as pointers. For `false`, we always dereference pointers, while in the case of arrays, we only return the values as far as the array bounds will allow. This has the potential to break existing code, but I have a suspicion that code was already broken to begin with, which is why I think this would be better than introducing a new API and keeping the old (and surprising) behavior. If our own test coverage is any indication, breakage should be minimal.
ajaden-codes
pushed a commit
to Jaddyen/llvm-project
that referenced
this pull request
Jun 6, 2025
The motivation here is being (un)able to treat pointer values as an array consistently. This works for pointers to simple/scalar values, but for aggregates, we get a very surprising result: - GetChildAtIndex(x, ??, true) returns the `x` child of the zeroth array member (the one you get by dereferencing the pointer/array) for all `x` which are smaller than the number of children of that value. - for other values of `x`, we get `v[x]`, where `v` is treated like a (C) pointer This patch reimagines this interface so that the value of `true` always treats (pointer and array) values as pointers. For `false`, we always dereference pointers, while in the case of arrays, we only return the values as far as the array bounds will allow. This has the potential to break existing code, but I have a suspicion that code was already broken to begin with, which is why I think this would be better than introducing a new API and keeping the old (and surprising) behavior. If our own test coverage is any indication, breakage should be minimal.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The motivation here is being (un)able to treat pointer values as an array consistently. This works for pointers to simple/scalar values, but for aggregates, we get a very surprising result:
xchild of the zeroth array member (the one you get by dereferencing the pointer/array) for allxwhich are smaller than the number of children of that value.x, we getv[x], wherevis treated like a (C) pointerThis patch reimagines this interface so that the value of
truealways treats (pointer and array) values as pointers. Forfalse, we always dereference pointers, while in the case of arrays, we only return the values as far as the array bounds will allow.This has the potential to break existing code, but I have a suspicion that code was already broken to begin with, which is why I think this would be better than introducing a new API and keeping the old (and surprising) behavior. If our own test coverage is any indication, breakage should be minimal.